core/vm: deep copy jumptable when enabling extra eips #26137
Conversation
yihuang
requested review from
karalabe,
holiman and
rjl493456442
as code owners
When there's extra-eips, concurrent query could cause non-deterministic evm execution result. It could affect even if exists in historical states. Ref: ethereum/go-ethereum#26137
When there's extra-eips, concurrent query could cause non-deterministic evm execution result. It could affect even if exists in historical states. Ref: ethereum/go-ethereum#26137
|
The go-ethereum/core/vm/jump_table.go Line 111 in d30e39b All the instruction tables are separate, and defined here: go-ethereum/core/vm/jump_table.go Line 47 in d30e39b I'm interested in the root issue reported in #26136, what the actual background is where this was found to be a problem @yihuang ? |
| @@ -83,14 +83,14 @@ func NewEVMInterpreter(evm *EVM, cfg Config) *EVMInterpreter { | |||
| } | |||
| var extraEips []int | |||
| for _, eip := range cfg.ExtraEips { | |||
There was a problem hiding this comment.
if len(cfg.ExtraEips) > 0 {
// Deep-copy jumptable to prevent modification of opcodes in other tables
cfg.JumpTable := copyJumpTable(cfg.JumpTable)
}
for _, eip := range cfg.ExtraEips {
if err := EnableEIP(eip, &cfg.JumpTable); err != nil {
// Disable it, so caller can check if it's activated or not
log.Error("EIP activation failed", "eip", eip, "error", err)
} else {
extraEips = append(extraEips, eip)
}
}
Would this work? No need to deep-copy it multiple times
There was a problem hiding this comment.
I think the original one also prevents a partial write if EnableEIP returns an error in the middle.
There was a problem hiding this comment.
Ok, the original one set cfg.JumpTable unconditionally, so it don't prevent a partial write neither.
There was a problem hiding this comment.
Ok. Doesn't matter a whole lot, this is not a hot path (for us)
There was a problem hiding this comment.
And there's no partial write to prevent anyway, I just changed to your version.
holiman
changed the title
Co-authored-by: Martin Holst Swende <[email protected]>
| @@ -83,14 +83,14 @@ func NewEVMInterpreter(evm *EVM, cfg Config) *EVMInterpreter { | |||
| } | |||
| var extraEips []int | |||
| for _, eip := range cfg.ExtraEips { | |||
There was a problem hiding this comment.
Ok. Doesn't matter a whole lot, this is not a hot path (for us)
Co-authored-by: Martin Holst Swende <[email protected]>
Co-authored-by: Martin Holst Swende <[email protected]>
When there's extra-eips, concurrent query could cause non-deterministic evm execution result. It could affect even if exists in historical states. Ref: ethereum/go-ethereum#26137
When the interpreter is configured to use extra-eips, this change makes it so that all the opcodes are deep-copied, to prevent accidental modification of the 'base' jumptable. Closes: ethereum#26136 Co-authored-by: Martin Holst Swende <[email protected]>
When the interpreter is configured to use extra-eips, this change makes it so that all the opcodes are deep-copied, to prevent accidental modification of the 'base' jumptable. Closes: ethereum#26136 Co-authored-by: Martin Holst Swende <[email protected]>
When the interpreter is configured to use extra-eips, this change makes it so that all the opcodes are deep-copied, to prevent accidental modification of the 'base' jumptable. Closes: ethereum#26136 Co-authored-by: Martin Holst Swende <[email protected]>
Closes: #26136
Thanks: @mmsqe