-
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
Replay for Byzantium data #436
base: jsign-replay-kaustinen6
Are you sure you want to change the base?
Conversation
if transfersValue && !evm.chainRules.IsEIP4762 { | ||
if transfersValue && (!evm.chainRules.IsEIP4762 || replayMode) { |
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.
Other case that we should disable the effects.
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.
but isn't just deactivating eip 4762 enough?
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.
note to self: no it isn't, because if you do then eip2929 is still active and we don't want to handle that case. I guess eip2929 becomes the new final boss.
bc.stateCache.InitTransitionStatus(true, true) | ||
bc.stateCache.EndVerkleTransition() | ||
bc.stateCache.InitTransitionStatus(false, false) | ||
// bc.stateCache.EndVerkleTransition() |
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 a required change to avoid an error when processing the second block.
Before merging we have to figure out a coherent way to do this for both replay and non-replay setups to work correctly.
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.
indeed. but if it's the only change that we have to keep in a separate branch, that's ok too.
for i := prevNumber; i > 0 && i >= prevNumber-params.Eip2935BlockHashHistorySize; i-- { | ||
for i := prevNumber; i > 0 && i > prevNumber-params.Eip2935BlockHashHistorySize; i-- { |
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 be removed when #438 is merged and the base branch change bubbles up when we rebase branches. For now leaving the fix since it's needed.
core/vm/interpreter.go
Outdated
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.
note to self: these changes are why this can't be merged, it's only to replay very old stuff.
6d11543
to
e796a78
Compare
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
b524731
to
cd92db3
Compare
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
This PR is basically the branch from #431 but with further adjustments to allow trying to replay the old (smaller) replay data.
This PR isn't intended to be merged, but was to be able to fix anything that was a bug for replay to work in general. #431 configuration reg precompiles/instructions shouldn't be changed. At some point we need to decide to merge bug-related/adjustements to that branch, see comments.