allforks: Hive withdrawal fixes #2529
Conversation
g11tech
added
type: test all hardforks
g11tech
force-pushed
the
hive-withdrawals-fx
branch
from
408660d to
369b642
Compare
g11tech
changed the title
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Some (more) fixes might be in this PR: #2507 (I honestly thought this was merged already) |
thanks @jochem-brouwer , yea we had incorporated fixes from there into this 👍 |
| @@ -94,6 +94,8 @@ export class EEI extends VmState implements EEIInterface { | |||
| } | |||
|
|
|||
| public copy() { | |||
| return new EEI(this._stateManager.copy(), this._common.copy(), this._blockchain.copy()) | |||
| const common = this._common.copy() | |||
| common.setHardfork(this._common.hardfork()) | |||
There was a problem hiding this comment.
Valid solution here, but I guess this hardfork setting should be functionality directly integrated into the Common copy() function? 🤔
There was a problem hiding this comment.
I wasn't sure on this one. It might warrant further research on whether or not there is other "state" inside of common that needs to be copied along with the current hardfork. That is the most impactful one (as identified by our failing tests) but I'm not sure if there's more to be thought through here.
There was a problem hiding this comment.
This got me a bit curious and a had another look, since on second thought it seemed extremely unlikely to me that our libraries would work at all if in the former setup Common.copy() wouldn't preserve the hardfork state. Tested this out and it turns out that it does, since it does copy property values as well in Common.copy().
You can test out the following code:
import { Common, Chain, Hardfork } from '@ethereumjs/common'
const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.Shanghai, eips: [4844] })
const common2 = common.copy()
common2.hardfork() // 'shanghai'So seems we can delete these additional lines here again, the fixes seem to come from some other code changes in this PR. 🙂
EIPs are actually preserved as well, common2.eips() // [ 4844 ].
There was a problem hiding this comment.
Ah, okay. That makes sense. I wasn't sure if the EEI was impacted or not. We certainly had a bug where copying VM resulted in separate common instances being created for the internal evm and EEI but maybe that was the limit of the issue.
|
Had another look here, thanks🙏, this all looks great! |
Fixes regarding failing hive testvectors
Closes #2498