Implement EIP-7805 (FOCIL)#8003
Conversation
|
@claude review |
|
Claude finished @Marchhill's task in 4m 10s —— View job EIP-7805 (FOCIL) PR Review — Round 16Tasks:
Progress since Round 15Two commits have landed since the last Claude review (
LukaszRozmej Round 16 findings
Verdict: mergeable. All Critical, High, and Medium findings across all review rounds are resolved. The sole remaining item — reservoir over-allocation in |
ArrayPoolList<Transaction> for the reservoir — Count tracks actual non-blob fill, so the prior alloc + Array.Resize + copy pair when the mempool yielded fewer than capacity non-blob txs is gone. Buffer is rented from the shared ArrayPool and disposed via the using in GetInclusionList. Addresses Lukasz's #4 round-5 comment.
A tx whose GasLimit is below its intrinsic gas can never execute, so the block legitimately omitting it must not trip INCLUSION_LIST_UNSATISFIED. Caught by EEST `test_block_with_intrinsic_gas_too_low_pending_il_tx_is_valid` (see ethereum/execution-specs tests-focil@v0.0.1 after the eip_7805 / Bogota patch that bumps fixtures to engine_newPayloadV6).
Locks in the behaviour added in 3e7ef38 — a tx whose GasLimit is below the intrinsic gas cost must be treated as not-appendable so the block omitting it does not trip INCLUSION_LIST_UNSATISFIED. Mirrors EEST `test_block_with_intrinsic_gas_too_low_pending_il_tx_is_valid`.
# Conflicts: # src/Nethermind/Nethermind.Consensus/Producers/PayloadAttributes.cs # src/Nethermind/Nethermind.Merge.Plugin/MergeErrorCodes.cs # src/Nethermind/Nethermind.Merge.Plugin/SszRest/Handlers/SszRestPaths.cs
Post-merge build needs the namespace for EngineApiVersions, which moved under Nethermind.Core. Local build was green only because the file had been edited but never staged into the merge commit.
* fix: build failure due to missing 'using Nethermind.Core' * Update src/Nethermind/Nethermind.Merge.Plugin/EngineRpcModule.Bogota.cs --------- Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
# Conflicts: # src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayload.cs
Adds a BogotaEngineBlockchainTests Pyspec fixture sourced from the tests-focil@v0.1.0 EELS release archive (separate from the BAL archive), plus the plumbing needed for FOCIL-shaped engine fixtures: - SpecNameParser recognises "Bogota" so JsonToEthereumTest.LoadSpec doesn't throw NotSupportedException when parsing focil fixture network. - TestEngineNewPayloadsJson grows InclusionListTransactions and Status fields; the V6 path appends IL as the 5th positional arg to engine_newPayloadV6, and AssertPayloadStatus honours the fixture's explicit Status (covers INCLUSION_LIST_UNSATISFIED). - Constants.cs gains FOCIL_ARCHIVE_VERSION / NAME for the separate release archive. 13 of the 24 collected tests pass against the existing IL pipeline; the remaining 11 (the "unsatisfied_*" scenarios) expose a postState question — EELS expects the block to be on-chain when engine returns INCLUSION_LIST_UNSATISFIED, but our flow rolls the block back. Tracked as a follow-up rather than blocking this CI bump.
EIP-7805 treats an IL-unsatisfied block as a *valid, executable* block that just didn't honour the inclusion list — the CL gets the INCLUSION_LIST_UNSATISFIED status and decides canonical inclusion via fork-choice. Throwing InclusionListUnsatisfiedException after execution rolled the block back even though its txs had already mutated state, which diverged from EELS fixtures whose postState reflects the committed block. - Add Block.IsInclusionListSatisfied (default true). - BlockProcessor.ProcessOne sets the flag instead of throwing; the block is committed normally and receipts are stored. - BlockchainProcessor.ProcessBlocks checks the flag and emits ProcessingResult.InclusionListUnsatisfied via BlockRemoved (so NewPayloadHandler still returns the same engine status to the CL). - InclusionListUnsatisfiedException removed (no callers left). - Test harness: send FCU after INCLUSION_LIST_UNSATISFIED too, so the chain head advances to the committed block and the EELS fixture's lastblockhash + postState assertions match. Verified end-to-end: all 24 BogotaEngineBlockchainTests pass against tests-focil@v0.1.0 (was 13/24).
|
@claude review |
|
Claude finished @Marchhill's task in 7m 7s —— View job EIP-7805 (FOCIL) PR Review — Round 17Tasks:
Progress since Round 16Round 16 reviewed up to
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| void NotifyInclusionListUnsatisfied(BlockRef blockRef, Block block) | ||
| { | ||
| if (_logger.IsTrace) _logger.Trace($"Invalid inclusion list for block {block.ToString(Block.Format.Full)}"); |
There was a problem hiding this comment.
Low — log message says "Invalid" but the spec calls this a valid, executable block
EIP-7805 treats IL-unsatisfied blocks as structurally valid — the CL receives INCLUSION_LIST_UNSATISFIED and decides canonical inclusion via fork-choice. Logging "Invalid inclusion list" may confuse operators into thinking the block itself is invalid.
| if (_logger.IsTrace) _logger.Trace($"Invalid inclusion list for block {block.ToString(Block.Format.Full)}"); | |
| if (_logger.IsTrace) _logger.Trace($"Inclusion list unsatisfied for block {block.ToString(Block.Format.Full)}"); |
|
Claude finished @Marchhill's task in 3m 12s —— View job EIP-7805 (FOCIL) PR Review — Round 18Tasks:
Progress since Round 17Round 17 reviewed up to
New commits reviewed
The
Remaining issuesLow (carried from Round 17)
Low (new)
The Verdict: mergeable. All Critical, High, and Medium findings across all 18 review rounds are resolved. The two remaining items are cosmetic log-message wording (Low) and do not affect correctness, consensus, or security. |
…ler.cs Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
- InclusionListTxSource.GetTransactions: long → ulong gasLimit to match the new ITxSource signature from upstream. - InclusionListValidator: (long) → (ulong) cast on IntrinsicGasCalculator result; EthereumIntrinsicGas now exposes ulong via its explicit conversion. - Test sites: WithNonce/AccountStruct ctors + GetTransactions args updated to ulong; UInt256 Nonce comparisons in tests switched to ulong literals (the older comparisons silently fail because of the type mismatch under the NUnit2021 analyzer). - BlockchainProcessor.NotifyInclusionListUnsatisfied: log wording from "Invalid inclusion list" → "Inclusion list unsatisfied" (Round 18 Low review point) — the block is valid + committed; only the IL signal failed.
| if (payloadStatus.Status == PayloadStatus.Valid) | ||
| // FCU after VALID — and also after INCLUSION_LIST_UNSATISFIED so the chain head | ||
| // advances to the committed block, matching the EELS fixture's lastblockhash/postState. | ||
| if (payloadStatus.Status == PayloadStatus.Valid || payloadStatus.Status == PayloadStatus.InclusionListUnsatisfied) |
| public string? ValidationError { get; set; } | ||
| // EIP-7805: emitted by the test fixture out-of-band from `params` (rather than as a 5th | ||
| // positional arg) so we append it explicitly when V6 is the target. | ||
| public JsonElement? InclusionListTransactions { get; set; } |
There was a problem hiding this comment.
Exactly why not proper type structure?
|
|
||
| namespace Nethermind.Blockchain.Test.Validators; | ||
|
|
||
| public class InclusionListValidatorTests |
There was a problem hiding this comment.
Can it be TestCaseSource based?
| WarmingState<Block>.FinallyAction); | ||
|
|
||
| // Prewarm inclusion list transaction senders too — they may be promoted into the block. | ||
| if (block.InclusionListTransactions is { Length: > 0 } inclusionList) |
There was a problem hiding this comment.
Potentially can be united with the top statement, where i above state.Payload.Transactions count means InclusionListTransactions
| _ => null | ||
| }; | ||
|
|
||
| string? validationMessage = e.ProcessingResult switch |
There was a problem hiding this comment.
Does not account for InclusionListUnsatisfied seems like
| // FOCIL is conditional: no gas left for a base-cost transfer → nothing is appendable. | ||
| if (block.GasUsed + Transaction.BaseTxGasCost > block.GasLimit) return true; | ||
|
|
||
| Span<bool> included = il.Length <= Eip7805Constants.MaxTransactionsPerInclusionList |
There was a problem hiding this comment.
Should we decline instead if tx count is too big?
| /// <inheritdoc/> | ||
| public bool ValidateInclusionList(Block processedBlock, Block suggestedBlock, IWorldState worldState, ProcessingOptions options) | ||
| { | ||
| if (options.ContainsFlag(ProcessingOptions.NoValidation)) return true; |
There was a problem hiding this comment.
Is not it's something that should happen in some external code?
| public const int MaxBytesPerInclusionList = 8192; | ||
| // 32 bytes as conservative lower bound for transaction size | ||
| public const int MinTransactionSizeBytesLower = 32; | ||
| public const int MinTransactionSizeBytesUpper = 100; |
There was a problem hiding this comment.
What is MinTransactionSizeBytesUpper for?
| return true; | ||
| } | ||
|
|
||
| private static bool CouldIncludeTx(Transaction tx, Block block, IReadOnlyStateProvider state, IReleaseSpec spec, ref Dictionary<AddressAsKey, AccountStruct>? senderCache) |
There was a problem hiding this comment.
Probably does not account for several txs from one account(I don't see nonce, balance counting)
| IRlpDecoder<Transaction>? rlpDecoder = Rlp.GetDecoder<Transaction>(); | ||
| if (rlpDecoder is null) return new TransactionDecodingResult($"{nameof(Transaction)} decoder is not registered"); | ||
|
|
||
| return TryDecodeParallel(txData, rlpDecoder, skipErrors, out TransactionDecodingResult result) |
There was a problem hiding this comment.
Can be simplified a bit if you just use txData.Length < ParallelDecodeThreshold for branching into parallel or seq, so you can omit try pattern
|
From codex
|
|
@claude review the code and review comments(confirm their relevance) |
|
Claude finished @flcl42's task in 4m 19s —— View job EIP-7805 (FOCIL) — Code review + relevance check of the Codex findingsI read the current code for each of the 7 Codex points (and spot-checked flcl42's inline notes). Verdict on each below, with my own severity. Codex findings — confirmed relevance1. 2. Bogota SSZ routes advertised but not wired — VALID (Medium, Bogota-devnet only).
So a CL selecting the advertised SSZ endpoint for any Bogota method will 404 / fail to dispatch, and a Bogota block returning 3. 4. Production prepends IL once; validation is post-execution appendability — VALID (Medium). 5. Unchecked arithmetic in 6. Enforce 7. Already-known + IL requeue isn't forced → spurious error — VALID (Medium).
So the Round-16 cache-bypass is incomplete: it forces re-validation but the requeue path can't actually re-run. Fix: add flcl42 inline comments (2026-06-29) — quick relevance pass
Summary
Findings 1, 2, 4, 7 (Medium) are real and newly raised — not yet acknowledged with rationale in prior rounds. Recommend addressing (or explicitly documenting the rationale for) these before merge. Finding 6 should be dismissed with a reference to the |
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
Excessive comments
More TestCase/Source?
Update SSZ REST to newest format
| Assert.That(validationError, Is.Not.Null, $"engine_newPayloadV{payloadVersion} RPC error: {errorCode} {errorMessage}"); | ||
|
|
||
| private static void AssertPayloadStatus(PayloadStatusV1 payloadStatus, string? expectedValidationError, int payloadVersion) | ||
| private static void AssertPayloadStatus(PayloadStatusV1 payloadStatus, string? fixtureStatus, string? expectedValidationError, int payloadVersion) |
There was a problem hiding this comment.
should fixtureStatus be default null? Also maybe call it explicitStatus?
| public string? ValidationError { get; set; } | ||
| // EIP-7805: emitted by the test fixture out-of-band from `params` (rather than as a 5th | ||
| // positional arg) so we append it explicitly when V6 is the target. | ||
| public JsonElement? InclusionListTransactions { get; set; } |
There was a problem hiding this comment.
Exactly why not proper type structure?
5 participants
Changes
engine_getInclusionListPayloadAttributesandnewPayloadparametersTypes of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes