Implement EIP4788: Beacon block root in EVM #2810
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Referenced PR is merged. This is ready for review. |
|
Am on mobile, cannot see if this can be merged without conflicts. (how do I see this on mobile?) |
|
Yes, no, so this needs a branch update. |
|
Have also dropped a short question on the readiness-state of the EIP on https://discord.com/channels/595666850260713488/856943705209569291/1127869874354524240. |
| @@ -935,6 +936,7 @@ export class EVM { | |||
| common: this.common, | |||
| _EVM: this, | |||
| _debug: this.DEBUG ? debugPrecompiles : undefined, | |||
| stateManager: this.stateManager, | |||
There was a problem hiding this comment.
Out of interest because I'm totally not getting the whole picture: what's the "super" part here? 😋 What does this do/change?
There was a problem hiding this comment.
Update: just only getting this now, is this whole thing a precompile and not an opcode??
| } | ||
| } | ||
| const timestampExtended = timestampIndex + historicalRootsLength | ||
| const returnData = setLengthLeft( |
There was a problem hiding this comment.
does the output of getContractStorage needs setLengthLeft?
There was a problem hiding this comment.
Note that this writes to CALLs out memory, so we need to write 32 bytes. This is not explicit in getContractStorage;
const value = await trie.get(key)
if (!this._storageCacheSettings.deactivate) {
this._storageCache?.put(address, key, value ?? hexToBytes('0x80'))
}
const decoded = RLP.decode(value ?? new Uint8Array(0)) as Uint8Array
return decoded
```There was a problem hiding this comment.
ahh makes sense, thank you!
Co-authored-by: g11tech <[email protected]>
jochem-brouwer
force-pushed
the
eip4788-beaconroot
branch
from
e3698c3
to
4a0c19a
Compare
| "comment": "Beacon block root in the EVM", | ||
| "url": "https://eips.ethereum.org/EIPS/eip-4788", | ||
| "status": "Draft", | ||
| "minimumHardfork": "cancun", |
There was a problem hiding this comment.
I know it's somewhat counter intuitive and I wonder if we ever "get this in" reliably 😋, but the minimum HF here is not the inclusion HF but St least one lower (do here Shanghai would be a good choice), otherwise this can't be used in a Shanghai+EIP way.
So this is to read: the state of the network where all preconditions are fulfilled so that this EIP can be activated.
There was a problem hiding this comment.
So just to re-iterate on this and make this very concrete: this is the situation which breaks otherwise (and currently does) if the minimum HF is set to Cancun here:
➜ vm git:(master) ts-node
> import { Chain, Common, Hardfork } from '@ethereumjs/common'
undefined
> let c = new Common({ chain: Chain.Mainnet, eips: [ 4788 ] })
/ethereumjs-monorepo/packages/common/dist/cjs/common.js:361
throw new Error(`${eip} cannot be activated on hardfork ${this.hardfork()}, minimumHardfork: ${minHF}`);
^
Uncaught Error: 4788 cannot be activated on hardfork shanghai, minimumHardfork: false
at Common.setEIPs (/ethereumjs-monorepo/packages/common/src/common.ts:455:15)
at new Common (/ethereumjs-monorepo/packages/common/src/common.ts:241:12)
at /ethereumjs-monorepo/packages/vm/<repl>.ts:2:9
at Script.runInThisContext (node:vm:129:12)
at runInContext (/node/v18.14.2/lib/node_modules/ts-node/src/repl.ts:673:19)
at Object.execCommand (/node/v18.14.2/lib/node_modules/ts-node/src/repl.ts:639:28)
at /node/v18.14.2/lib/node_modules/ts-node/src/repl.ts:661:47
at Array.reduce (<anonymous>)
at appendCompileAndEvalInput (/node/v18.14.2/lib/node_modules/ts-node/src/repl.ts:661:23)
at evalCodeInternal (/node/v18.14.2/lib/node_modules/ts-node/src/repl.ts:222:12)
>There was a problem hiding this comment.
Not sure, since this is such a re-occuring place for stumbling over it might be really worth to rename to something like the following? 🤔
minimumHardforkForIsolatedEIP
Even if the name is a bit bulky.
There was a problem hiding this comment.
Will take over this discussion to the chat for a quick exchange.
There was a problem hiding this comment.
For EL it should be Merge (instead of Shanghai), I agree here though we should review whatever the lowest hardfork should be (I should have set this to Merge)
Implements https://eips.ethereum.org/EIPS/eip-4788
WIP
Note: keep eyes on ethereum/EIPs#7178
From the EIP (specifically ethereum/EIPs#7178:)
This will need a PR on the CL side for the execution API, so will not implement here