-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EIP4844 support & Other misc issues fix #2
Conversation
ae954d0
to
cd8e84f
Compare
metis/clt_rand.go
Outdated
|
||
// GenerateRandomInt generates a random integer from min to max (inclusive), based on a provided seed. | ||
// This simulates a normal distribution by summing multiple uniform random variables. | ||
func (g *CLTGenerator) GenerateRandomInt(seed int64, min, max uint64) (int64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any difference from randomRangeInclusive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aiden069 told me that the numbers generated by randomRangeInclusive
are not in a normal distribution manner, so I rewrote the random generation part with CLT (since this is the only stateless random generation method I can find that will most likely provide a normal distribution result).
Here is the test result:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to discuss it first.
It's still unfair for a validator with higher power.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, shall we setup a quick meeting for this? And @aiden069 what's your opinion here?
Please move the selection change to a new PR. |
Removed, will create a separate PR for selection soon. |
@ericlee42 PR for new selection is here |
@@ -0,0 +1,167 @@ | |||
package types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some differences in RLP decoding in go-ethereum 1.14 and 1.10. The binary data of unsigned transaction does not have r, s and v encoded, if we use tx.UnmarshalBinary
provided in go-ethereum 1.14, it will cause an error (too many elements) when decoding. So I copied over the data structures from go-ethereum and set the r, s, v to optional to avoid this error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so.
Please give an example for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Create a dynamic fee tx
- Decode the transaction with
tx.UnmarshalBinary
Also that one thing to notice, I played with different transaction types, it seems that this error will only happens on typed transactions (access list / dynamic fee / blob), not on legacy tx. I guess that's why the previous themis code works, since the txs submitted to MPC were only legacy txs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked the code of the ether.js, it doesn't add the sig fields to the rlp bytes
But it should work for geth
package main
import (
"math/big"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
)
func main() {
var a common.Address
tx1 := types.NewTx(&types.DynamicFeeTx{
ChainID: big.NewInt(1),
GasTipCap: big.NewInt(1e9),
GasFeeCap: big.NewInt(1e9),
Gas: 1e5,
To: &a,
Value: big.NewInt(0),
})
tx1Raw, err := tx1.MarshalBinary()
if err != nil {
panic(err)
}
var tx2 types.Transaction
if err := tx2.UnmarshalBinary(tx1Raw); err != nil {
panic(err)
}
}
Fair enough, we can accept the change.
go-ethereum
to v1.14.11 to support EIP4844 transaction signingThis is still under testing, do not merge for now.