Feature/eip 8282#12092
Conversation
glamsterdam-devnet-6 adds two builder-request predeploys that are system-called at the end of every block, after the EIP-7002 withdrawal and EIP-7251 consolidation requests: - builder deposit (type 0x03) at 0x0000884d2A...008282 - builder exit (type 0x04) at 0x000014574A...008282 Their per-block system calls read storage that must appear in the EIP-7928 block access list, so without them every Amsterdam block's BAL hash mismatches. Mirrors the existing 7002/7251 request handling: IsEip8282Enabled wired through the spec + chainspec, ExecutionRequestType BuilderDeposit/BuilderExit, two SystemCall reads in the requests processor (flat-encoded in request-type order so the requests hash is correct), and both predeploys registered as BAL system contracts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @Marchhill's task in 6m 6s —— View job EIP-8282 PR ReviewTasks:
SummaryThe PR adds EIP-8282 builder execution requests (builder deposit + builder exit) following the same system-call pattern as EIP-7002 and EIP-7251. The scaffolding (spec flag, chain spec plumbing, fork assignment, BAL validation, error messages) is correct and well-structured. However, there is one critical omission and several medium/low issues: Critical
= ExecutionRequestExtensions.GetFlatDecodedRequests(block.ExecutionRequests);Once Amsterdam activates and a block contains builder requests, the stateless executor path will throw. Fix: add explicit HighNo tests for EIP-8282 code paths —
MediumContract addresses not configurable via spec (see inline comment) — EIP-7002 and EIP-7251 store their predeploy addresses in Low
|
| WithdrawalRequest = 1, | ||
| ConsolidationRequest = 2 | ||
| ConsolidationRequest = 2, | ||
| BuilderDepositRequest = 3, // eip-8282 |
There was a problem hiding this comment.
Medium — Inline comments on enum members add noise here; the type names are already self-documenting. If the EIP reference is useful, prefer XML doc on the enum member or at minimum use // EIP-8282 (uppercase) for consistency with how EIP references appear elsewhere in the codebase. Also note BuilderExitRequest = 4 is missing a trailing comma (style inconsistency with the other members).
| // withdrawal/consolidation requests so the flat encoding stays in request-type order. | ||
| if (spec.BuilderRequestsEnabled) | ||
| { | ||
| ReadRequests(block, state, Eip8282Constants.BuilderDepositRequestPredeployAddress, ref requests, _builderDepositTransaction, |
There was a problem hiding this comment.
Medium — EIP-7002 and EIP-7251 read their contract addresses through spec.Eip7002ContractAddress / spec.Eip7251ContractAddress, which are set from constants by default in 00_Olympic.cs but can be overridden per-chain via chain spec JSON (Eip7002ContractAddress field in ChainSpecParamsJson). EIP-8282 hard-codes the address from constants here and doesn't add a spec property, breaking that pattern. If any devnet or test network ever needs a different predeploy address, there's no way to configure it. Consider adding Eip8282BuilderDepositContractAddress and Eip8282BuilderExitContractAddress properties to IReleaseSpec / ChainSpecParamsJson, following the existing pattern.
| _withdrawalTransaction.Hash = _withdrawalTransaction.CalculateHash(); | ||
| _consolidationTransaction.Hash = _consolidationTransaction.CalculateHash(); | ||
| _builderDepositTransaction.Hash = _builderDepositTransaction.CalculateHash(); | ||
| _builderExitTransaction.Hash = _builderExitTransaction.CalculateHash(); |
There was a problem hiding this comment.
Low — ArrayPoolListRef<byte[]> requests = new(3) on line 85 (not in diff) now has an incorrect initial capacity. With EIP-8282, there are 5 possible request-type buckets (deposits, withdrawals, consolidations, builder deposits, builder exits). The buffer will auto-resize if needed, so this is not a correctness issue, but it causes an extra allocation on Amsterdam blocks.
Sending a positive balance to an empty SELFDESTRUCT beneficiary charges ACCOUNT_WRITE (8000) regular gas in addition to the NEW_ACCOUNT state gas, per EELS amsterdam selfdestruct. Nethermind charged only the state gas. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes Closes Resolves #
Please choose one of the keywords above to refer to the issue this PR solves followed by the issue number (e.g. Fixes #000). If no issue number, remove the line. Also, remove everything marked optional that is not applicable. Remove this note after reading.
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Optional. Remove if not applicable.
Documentation
Requires documentation update
If yes, link the PR to the docs update or the issue with the details labeled
docs. Remove if not applicable.Requires explanation in Release Notes
If yes, fill in the details here. Remove if not applicable.
Remarks
Optional. Remove if not applicable.