Skip to content
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

Draft
wants to merge 2 commits into
base: kaustinen-with-shapella
Choose a base branch
from
Draft

Conversation

jsign
Copy link
Collaborator

@jsign jsign commented May 6, 2024

This is a redo of the previous #391 but for a replay configuration after Shanghai.

cmd/utils/cmd.go Outdated Show resolved Hide resolved
@@ -1535,30 +1531,6 @@ func (bc *BlockChain) InsertChain(chain types.Blocks) (int, error) {
return bc.insertChain(chain, true)
}

func findVerkleConversionBlock() (uint64, error) {
Copy link
Collaborator Author

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.

Comment on lines +1745 to 1748
stateRoot := parent.Root
if block.Header().Number.Uint64() == 4702178 {
stateRoot = common.HexToHash("0x00")
}
Copy link
Collaborator Author

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.

Comment on lines +154 to 155
// Note: this config should be correctly set depending on replay starting point.
return PrecompiledAddressesBerlin
Copy link
Collaborator Author

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.

Comment on lines -44 to +47
case evm.chainRules.IsPrague:
precompiles = PrecompiledContractsBerlin
case evm.chainRules.IsCancun:
precompiles = PrecompiledContractsCancun
case evm.chainRules.IsPrague:
precompiles = PrecompiledContractsBerlin
Copy link
Collaborator Author

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.

Comment on lines -59 to +63
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
Copy link
Collaborator Author

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)
Copy link
Collaborator Author

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?

Comment on lines +216 to +243
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)
}
Copy link
Collaborator Author

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.

jsign added 2 commits June 7, 2024 11:17
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants