-
Notifications
You must be signed in to change notification settings - Fork 13
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
Post-Shapella Replay #431
base: kaustinen-with-shapella
Are you sure you want to change the base?
Post-Shapella Replay #431
Conversation
@@ -1535,30 +1531,6 @@ func (bc *BlockChain) InsertChain(chain types.Blocks) (int, error) { | |||
return bc.insertChain(chain, true) | |||
} | |||
|
|||
func findVerkleConversionBlock() (uint64, 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.
This was removed since we don't rely on the old .txt
anymore but in timestamps.
stateRoot := parent.Root | ||
if block.Header().Number.Uint64() == 4702178 { | ||
stateRoot = common.HexToHash("0x00") | ||
} |
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.
So this was a very ugly rabbit hole due to a bug in the way our previous replay data was exported.
In the previous setup the first block to be replayed was 4702177 and unfortunately that block had a snapshot in the state. We need to force any second (i.e: previously 4702178) or further block to always read from the tree and not use snapshots.
Maybe the new replay data wasn't corrupted in that way and we can remove this. But it's worth checking that state.New(stateRoot, ...)
in the line below never finds a snapshot for the second-and-further replayed blocks, it must always read from the tree. If that isn't the case, we might be replaying using snapshots prepopulated by the original execution that created the exported chain and not the replayed blocks.
// Note: this config should be correctly set depending on replay starting point. | ||
return PrecompiledAddressesBerlin |
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.
In our previous replay branch, we had to override this to be PrecompiledAddressByzantium
. I think now the current configuration since Shanghai precompiles are the same as Berlin, correct?
Left a comment here to remember to keep updating this as we keep rebasing Verkle further down the forks. I can remove it though, since maybe from now forward the default branch and replay data will always match.
case evm.chainRules.IsPrague: | ||
precompiles = PrecompiledContractsBerlin | ||
case evm.chainRules.IsCancun: | ||
precompiles = PrecompiledContractsCancun | ||
case evm.chainRules.IsPrague: | ||
precompiles = PrecompiledContractsBerlin |
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.
Just to express the correct order since we're rebased on top of shapella, not cancun.
case evm.chainRules.IsPrague: | ||
// TODO replace with prooper instruction set when fork is specified | ||
table = &pragueInstructionSet | ||
case evm.chainRules.IsCancun: | ||
table = &cancunInstructionSet | ||
case evm.chainRules.IsPrague: | ||
// TODO replace with proper instruction set when fork is specified | ||
table = &shanghaiInstructionSet |
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.
This is an important change for replay since we're replaying history without many gas changes for Verkle.
Before potentially merging this branch into kaustinen-with-shapella
we should find a reasonable way to configure this depending if the branch is used for Kaustinen or replay.
return c.IsLondon(num) && isTimestampForked(c.PragueTime, time) | ||
return c.IsShanghai(num, time) && isTimestampForked(c.PragueTime, time) |
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 think we might need to do a similar change in kaustinen-with-shapella
since I Think using IsLondon
isn't entirely correct?
var ( | ||
err error | ||
values = make([][]byte, verkle.NodeWidth) | ||
stem = t.pointCache.GetTreeKeyVersionCached(addr[:]) | ||
) | ||
|
||
for i := 0; i < verkle.NodeWidth; i++ { | ||
values[i] = zero[:] | ||
} | ||
switch root := t.root.(type) { | ||
case *verkle.InternalNode: | ||
err = root.InsertValuesAtStem(stem, values, t.FlatdbNodeResolver) | ||
default: | ||
return errInvalidRootType | ||
} | ||
if err != nil { | ||
return fmt.Errorf("DeleteAccount (%x) error: %v", addr, err) | ||
} |
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.
Another thing that we should make dynamic if we merge replay in kaustinen-with-shapella
. For running Kaustinen this should be skipped, for replay it should be enabled.
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
6d11543
to
e796a78
Compare
This is a redo of the previous #391 but for a replay configuration after Shanghai.