Implement EIP 2935 #3268
Implement EIP 2935 #3268
Conversation
…d (no state inspection)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
jochem-brouwer
added
PR state: needs review
and removed
PR state: WIP
PR state: do-not-merge
PR/Issue state: blocked
labels
packages/common/src/common.ts
Outdated
| * @returns Block timestamp or null if unscheduled | ||
| */ | ||
| eipTimestamp(eip: number): bigint | null { | ||
| // NOTE: this is not elegant, what if the eip got activated on a block and not timestamp? |
There was a problem hiding this comment.
Don't the older hardfork block headers still have a timestamp in them that we can report here? I'm not sure if this is really a concern. Or, maybe better yet, we just return null for any EIP that's not scheduled or pre-Shanghai.
There was a problem hiding this comment.
Oh right, this is indeed an interesting line of thought. However this would make the method async and we would need to get the block from blockchain in order to retrieve the timestamp 🤔 Not sure I like that. Especially gets a bit hairy if it reorgs (you save the timestamp, chain reorgs, and now the timestamp differs). I will update the docs to remove the "block" part and just write down that it returns the timestamp when this EIP is scheduled (therefore, on missed slots the first block might actually be later than the scheduled timestamp)
There was a problem hiding this comment.
Ahh! Wait! I now remember the point I wanted to make here. The same applies to eipBlock actually. If eipTimestamp returns null (for instance, for Dao), then we know it is either (1) not scheduled at a timestamp (but at a block number) or (2) it is not even scheduled at all (so the fork will never happen). I think this is beyond the scope of this PR though and it is also an edge case.
packages/common/src/eips.ts
Outdated
| @@ -190,6 +191,23 @@ export const EIPs: EIPsDict = { | |||
| }, | |||
| }, | |||
| }, | |||
| 2935: { | |||
| comment: 'Save historical block hashes in state', | |||
There was a problem hiding this comment.
Can we add some addition like ...state (Verkle related usage, likely not stable yet) or so?
(let me know if the comment does not make sense "as is")
There was a problem hiding this comment.
It does, I normally just copy the EIP title but you are right this definitely should get an unstable warning.
There was a problem hiding this comment.
👍
Take whatever text you like, just that people know this is not "one of the normal EIPs they can integrate" (and DRAFT status is not that much of a warning, even EIPs close to hardfork often still in DRAFT 😂 ).
| const forkTime = this.common.eipTimestamp(2935) | ||
| if (forkTime === null) { | ||
| throw new Error('EIP 2935 should be activated by timestamp') | ||
| } |
There was a problem hiding this comment.
I don't think that we need this extra eipTimestamp() method. Isn't this the normal isActivatedEIP() call??
Also: such a check should likely be at the beginning of the method (?).
There was a problem hiding this comment.
I have added this because I need to detect we are on the first fork block. The reason is that on the first fork block, we have to store all the MIN_HISTORY_SERVE_WINDOW hashes. I don't know if there is another way to detect if we are on the fork block? I think we need to fork timestamp and then the exact condition if (parentBlock.header.timestamp < forkTime) { (this only happens once and is on the fork block)
There was a problem hiding this comment.
Ah, you are right, sorry, please dismiss! 🙂
Should then the whole method get a check at the beginning if 2935 is activated and throw otherwise?
There was a problem hiding this comment.
I am comparing it with accumulateParentBeaconBlockRoot, this also does not have a check for common.isActivatedEIP(4788), but for consistency I can add (but then I will add it to both methods)
| if (parentBlock.header.timestamp < forkTime) { | ||
| let ancestor = parentBlock | ||
| const range = this.common.param('vm', 'minHistoryServeWindow') | ||
| for (let i = 0; i < Number(range) - 1; i++) { |
There was a problem hiding this comment.
I would cautiously think that this < usage together with - 1 might produce an "off by one" error?
Since the code from the EIP states for i in range(MIN_HISTORY_SERVE_WINDOW - 1)?
(maybe confirm with edge case tests?)
There was a problem hiding this comment.
I think this is correct (but this is definitely off-by-one error prone! 😄 )
From EIP: for i in range(MIN_HISTORY_SERVE_WINDOW - 1): this is python code. This loops the for loop MIN_HISTORY_SERVE_WINDOW - 1 time. Note that MIN_HISTORY_SERVE_WINDOW is 256 so we loop this 255 times. In the current TS code we loop i < 256 - 1 = 255 times also. Note that for each loop, we store the ancestor block. Also note that previously we have already stored the parent block hash of the current block. So, in total we have stored 256 blocks and therefore the history contract has access to 256 slots.
| ) | ||
| assert.ok(equalsBytes(storage, genesis.hash())) | ||
| }) | ||
| it('should ensure blocks older than 256 blocks can be retrieved from the history contract', async () => { |
There was a problem hiding this comment.
@holgerd77 had to re-check, but this test should actually test for the off-by-one error.
This PR is based upon the EIP 2935 version of: ethereum/EIPs#8166, which is currently not merged.
Note: this also adds functionality to Common which adds the ability to specify new hardforks, where users can describe when they want these forks to be activated (handy for EIP transitioning tests which have not yet been assigned to a hardfork)