[Marchhill]

Changes

InclusionListValidator excused every frame transaction (if (il[i].SupportsFrames) continue;), so a builder could omit any of them. This implements EIP-7805's Profile 2 omission claims and per-IL VERIFY budget, and the Bogota engine API that carries them:

• EIP-7805: Update EIP-7805: Profile 2 omission claims and per-list VERIFY budget ethereum/EIPs#12394 (Profile 2 omission claims and per-list VERIFY budget)
• EIP-8369: Update EIP-8369: deterministic Profile 2 budgets and replay ethereum/EIPs#12396 (deterministic Profile 2 budgets and replay)
• Engine API: engine: separate inclusion lists and omission claims in Bogota ethereum/execution-apis#911 (separate inclusion lists and omission claims in Bogota)
• Consensus: Add omission claims to Heze (EIP-7805) ethereum/consensus-specs#5698 (omission claims in Heze)
• Reference and fixtures: feat(specs,test-specs,tests): support FOCIL frame omission claims ethereum/execution-specs#3777 (FOCIL frame omission claims)

Activation

Profile 2 is active when EIP-7805, EIP-8141, EIP-8250 and EIP-8272 are all enabled (Eip8369Profile2.IsActive). Before that, omitted frame transactions are excused and claims have no effect. Once it is active, an omission outside both profiles is excused, blob transactions included. Nothing changes before EIP-7805.

Profile 2

• Candidates (Eip8369Profile2.Classify): no blobs; a declared prefix of an optional expiry_verify, then an optional recent_root_verify, then self_verify, deploy | self_verify, only_verify | pay or deploy | only_verify | pay; no ATOMIC_BATCH_FLAG in the prefix; no VERIFY frame after it; verify_cost ≤ MAX_VERIFY_GAS_PER_TX, counting the signature cost and the prefix's execution limits.
• VERIFY budget: filled separately for each of the 16 inclusion lists, each deduplicated and taken in (hash, envelope) order from MAX_VERIFY_GAS_PER_IL. Entries failing the noncryptographic candidate checks are skipped without a debit; one costlier than what remains is skipped without a debit; otherwise the signature cost is debited, and the prefix cost too once the signatures verify. An entry is admitted if any list admits it, and its signatures are verified once however many lists carry it. The builder runs the same fill.
• Evaluation index: min(claim, n) for an omitted candidate whose hash is claimed exactly once, otherwise n. Each candidate is replayed once, at that index alone.
• Replay (Profile2EligibilityReplayer): parent state plus the EIP-7928 changes with block_access_index ≤ k, under MAX_VERIFY_GAS_PER_TX, the AA-VOPS access rules and the replay code bounds. The deploy frame's factory must be an undelegated contract.
• Gas fit at k uses the EIP-8037 execution and state gas used before k, recorded from the execution receipts.

Engine API (execution-apis#911)

• engine_newPayloadV6 and engine_newPayloadWithWitnessV6 take inclusionLists (exactly 16 lists, each within 8 KiB before deduplication, no empty entry) and omissionClaims ({transactionHash, evaluationIndex}, at most 4096, the index a canonical QUANTITY whatever JsonRpc.StrictHexFormat is). Malformed inputs answer -32602, after the -38005 fork check.
• Inclusion is checked apart from execution. Once execution is VALID, newPayloadV6 schedules the check and answers at once with inclusionCheckId (keccak256("engine/inclusion-check/v1" || RLP([blockHash, inclusionLists, claims]))) and inclusionListSatisfied, which is null until the check completes. The pipeline no longer judges the list, so execution validity is never held for it; the flat-list check it ran (Block.InclusionListTransactions, InclusionListSatisfactionChecker, ProcessingResult.InclusionListUnsatisfied) is removed, and BranchProcessor records the per-transaction gas the check needs.
• engine_getInclusionListVerdictV1(inclusionCheckId) returns the verdict, null while pending, or -38007 Unknown inclusion check. Identical checks share one run, a completed verdict never changes, and polling starts nothing. At most 256 checks are kept; past that a check for a block at or below the finalized one is evicted first, then a pending one, and a completed verdict only when nothing else is left. At most four run at once. A check that cannot be decided (state or gas counters unavailable) is evicted rather than completed, and a resubmission runs it again.
• engine_forkchoiceUpdatedV5 takes PayloadAttributesV5.inclusionLists (-38003 when malformed) and returns PayloadStatusV1; it no longer reports inclusion. The retained-list machinery behind that, and the queued-copy tracking the handler kept for per-call lists, are removed.
• engine_getPayloadV7 returns omissionClaims. The builder claims the payload length at the first failed attempt to append an admitted candidate, only when the candidate is invalid there.
• engine_getInclusionListV1 also lists ready Profile 2 candidate frame transactions once Profile 2 is active, keeping their total VERIFY cost within MAX_VERIFY_GAS_PER_IL and leaving out candidates already expired at the parent.
• SSZ-REST carries the same fields; the verdict query is JSON-RPC only.

Constants

MAX_VERIFY_GAS_PER_IL = 2**20, MAX_VERIFY_GAS_PER_TX = 2**18 (also Blocks.FocilProfile2MaxVerifyGas, where 0 lifts it), AA_VOPS_SLOT_COUNT = 4, MAX_OMISSION_CLAIMS = 2**12, MAX_REPLAY_CODE_BODIES = 16, MAX_REPLAY_CODE_BYTES = 2**17.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

• Unit tests: Eip8369Profile2Tests, InclusionListValidatorTests, Profile2EligibilityReplayerTests, FrameTxValidationPrefixSimulationTests, TransactionsExecutorTests, InclusionListTxSourceTests, PayloadAttributes*Tests, InclusionCheckIdTests, InclusionChecksTests, EngineModuleTests (V7 and Eip8369), SszCodecTests and SszMiddlewareTests.
• All 96 engine fixtures filled from execution-specs#3777 (tests/bogota/eip8369_focil_frames and tests/bogota/eip7805_focil) pass with nethtest -e --forkAlias Bogota=Eip8369Prototype; the runner polls engine_getInclusionListVerdictV1 for each VALID payload. Eip8369Prototype is a test-only fork: Bogota with EIP-8141, EIP-8250 and EIP-8272.
• The focilTest CI job still runs tests-focil-devnet@v0.3.0, whose engine_newPayloadV6 carries a flat list, and fails until that release is refilled against execution-apis#911.

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[Marchhill]

Builder claims only replay-invalid candidates (0a540ac); replay code bounds MAX_REPLAY_CODE_BODIES/MAX_REPLAY_CODE_BYTES enforced (cfe5bf1). All 39 FOCIL/frame fixtures pass.

@claude review

[claude]

Claude finished @Marchhill's task in 1m 43s —— View job

---

PR review

• Gather context: the commits since the last review (e655f869, 0a540acd, cfe5bf17)
• Review builder claims (only replay-invalid candidates)
• Review replay code bounds (MAX_REPLAY_CODE_BODIES / MAX_REPLAY_CODE_BYTES)
• Post findings

Verdict: not mergeable yet. There is one High finding. The new code-body counting in cfe5bf17 lets an includer drive gigabytes of Keccak on the attester's replay path. Builder claims (0a540acd) and the per-tx gas gate (e655f869) are fine.

High

LoadCode re-hashes the whole code body every time a loop jumps back to pc 0. Inline comment: FrameTxValidationTracer.cs:166.

• The branch pc == 0 && _codeBodies is not null runs on every instruction at offset 0, not just when a frame starts. LoadCode computes ValueKeccak.Compute(code) before the dedup Add.
• Take a sender whose code is a JUMPDEST PUSH0 JUMP loop (11 gas per iteration), padded to Bogota's 64 KiB MaxCodeSize, which is under MAX_REPLAY_CODE_BYTES. Under the 2**18 cap that is about 23.8k iterations, or about 1.5 GB of Keccak per replay.
• Up to about 4 such candidates per position fit in the budget, across 16 positions. All of it runs under the replayer's lock, synchronously in BranchProcessor and NewPayloadHandler.
• The includer pays nothing: the prefix runs out of gas, so the candidate is ineligible and never goes on chain.
• Fix: skip CodeInfo instances already counted (reference comparer) before hashing. Use the stamped CodeInfo.CodeHash, or the codeHash the EXTCODE* path already has, instead of recomputing it. Add a loop regression test.

Builder claims only where the candidate is invalid (0a540acd): checked and fine

• SetInvalid covers exactly the reasons the attester re-checks at k:
  • the execution and state reservation fit, which the attester evaluates against gasUsedBefore[k] in each dimension;
  • the nonce and the keyed nonce set, both checked in the replay;
  • an execution rejection, which comes from the transaction processor.
• Policy skips are left without a claim, so the attester judges them at n. These are: too large for the CL, already in the block, init code too large, an EIP-3607 contract sender, a sender-balance skip (non-frame only), the gas reservation not being computable, and any AddingTransaction handler. A claim binds the builder, so not claiming a policy skip is the safe direction.
• Set resets IsInvalidHere, so a handler that overrides the action later cannot leave a stale flag.
• invalidHere is read only together with claimOnSkip, which still requires the first attempt and a claimable hash.
• TransactionsExecutorTests covers both sides.

Per-tx gas recorded only once Profile 2 is active (e655f869): fine

The last review's optional Low is addressed. Before activation GasUsedBeforeTransactions is null, and nothing reads it there, because claims are cleared before activation.

Replay code bounds: the rest is fine

• Bodies are deduplicated by hash across execution and EXTCODE* reads. Empty code counts as nothing. The body-count bound and the byte bound are each checked with >, so a prefix exactly at either limit is allowed. EXTCODEHASH is correctly not counted.
• GetCode on a 7702-delegated target returns the 23-byte designator, which is what EXTCODESIZE/EXTCODECOPY read, so that is what gets counted.
• Only the replayer passes the bounds. The mempool's defaults of 0 leave its behaviour unchanged.

Still acknowledged in the PR body

• The tests-focil-devnet@v0.2.0 engine fixtures are rejected for missing membership. That is correct under execution-apis#911, and they are deliberately left out of KNOWN_FAILURES.

I only read the code and didn't build or run tests. I reviewed fa72b5e8..cfe5bf17. The "39 fixtures pass" result is your figure; I didn't check it.

[claude]

High (DoS on the attester path, new in cfe5bf1): pc == 0 fires on every opcode at pc 0, not only when a frame starts, so a loop that jumps back to 0 re-hashes the whole code body on every iteration.

StartOperation runs for each instruction, so a JUMP to a JUMPDEST at offset 0 re-enters this branch. LoadCode runs ValueKeccak.Compute(code) before the HashSet.Add dedup. The dedup stops the count from growing, but not the hashing.

• A sender whose code starts JUMPDEST PUSH0 JUMP costs 11 gas per iteration. Under FocilProfile2MaxVerifyGas = 2**18 that is about 23,800 iterations.
• Bogota inherits Amsterdam's 64 KiB MaxCodeSize, which is under MAX_REPLAY_CODE_BYTES. That is about 1.5 GB of Keccak per replay, roughly 2–3 s.
• The per-IL budget caps replays at about 4 such candidates per position, ×16 positions. That is minutes of hashing per block, all inside AreEligible's lock, synchronously in BranchProcessor and NewPayloadHandler.
• The includer pays nothing. The prefix just runs out of gas, the candidate is ineligible, and it never lands on chain. Many senders can share one 64 KiB body through 7702 delegation, so the deploy happens once.
• Before this commit the tracer did no hashing here, so this commit introduces the cost.

Two fixes, and both together are cheap:

1. Remember the CodeInfo instances already counted (HashSet<CodeInfo>(ReferenceEqualityComparer.Instance)) and return before hashing on a repeat. (CodeInfo overrides Equals, so the reference comparer matters.)
2. Use env.CodeInfo.CodeHash when it is stamped (!= default), and fall back to ValueKeccak.Compute only for initcode. The EXTCODESIZE/EXTCODECOPY path already has codeHash from state.GetCodeHash(target), so it can pass that in rather than rehashing the body it just loaded.

Please add a regression test: a self-verify prefix whose code is a JUMPDEST PUSH0 JUMP loop on a large body. Assert the hash count (for example through a small seam) or the elapsed time, plus a case that re-enters the same body through a self-CALL.

Fix this →

[Marchhill]

Fixed in 9c9a98a: bodies counted only on entry into new code, deduped by instance before hashing, keyed on the known code hash; a loop to pc 0 or self-call never rehashes. Covered by Simulate_ReplayCodeLimits_HashEachBodyOnce. EELS had the same issue, fixed in execution-specs#3777 (5969c74bd).

[Marchhill]

@claude review

[claude]

Claude finished @Marchhill's task in 51s —— View job

---

PR review

• Gather context: one new commit since the last review (9c9a98a7, "count a replay code body on entry only, without rehashing")
• Review the FrameTxValidationTracer change and the new regression test
• Post findings

Verdict: mergeable. The High finding from the last review (a loop back to pc 0 re-hashed the whole code body on every iteration) is fixed. I found nothing new at Critical, High or Medium.

Last review's High: fixed

• Change detection replaces pc == 0. The tracer now enters the counting branch only when env.CodeInfo differs by reference from _executingCode. A JUMP back to offset 0 inside the same body costs one reference comparison.
• Instances are deduplicated before any hashing. _enteredCode uses ReferenceEqualityComparer, which matters because CodeInfo overrides Equals. The pc == 0 condition is still required when the code changes, so returning to a caller at pc != 0 updates _executingCode without counting the caller again. That is correct, because the caller was counted when it was entered.
• The stamped hash is used. CodeInfo.CodeHash is stamped whenever an ICodeCache stores the instance. The replayer's IReadOnlyTxProcessingEnvFactory scope uses the normal cache; NoopCodeCache appears only in the witness envs. So deployed code is never hashed, and only initcode falls back to ValueKeccak. Initcode costs at least one CREATE each time, so the gas bounds that hashing.
• Re-entry through a self-call. If the code cache returns the same instance, nothing happens. If it returns a new instance, the stamped hash makes _codeBodies.Add a no-op. Either way there is no rehash, and the body is not counted twice.
• EXTCODESIZE/EXTCODECOPY. The hash already read through GetCodeHash is reused, and _codeBodies.Contains short-circuits before GetCode on a repeat read. The byte count still uses the designator length for a 7702-delegated target, as before.
• Semantics are unchanged. Empty code still counts as nothing, the two bounds are still checked with >, and the mempool defaults (0/0) still leave both sets null.
• Regression test. Simulate_ReplayCodeLimits_HashEachBodyOnce covers a JUMPDEST PUSH0 JUMP loop and a self-STATICCALL loop on a 48 KiB body, and asserts at most one hash. It covers both paths I asked for.

Low (optional)

• CodeHashComputations is a counter only the tests read, kept on the production path (inline). This is the same pattern as StateReconstructions, which you removed in 478be65. The cost is one increment on the initcode path only, so this doesn't block merge.
• HashEachBodyOnce catches OperationCanceledException when (tracer.Violated) and then asserts Violated is false, so that catch can never let the test pass. It is harmless. You could drop the try, or keep it as a guard so that a violation shows up as a clear assertion failure.

Still acknowledged in the PR body

• The tests-focil-devnet@v0.2.0 engine fixtures are rejected for missing membership. That is correct under execution-apis#911, and they are deliberately left out of KNOWN_FAILURES.

I only read the code and didn't build or run tests. I reviewed cfe5bf17..9c9a98a7.

[claude]

Low (optional): CodeHashComputations is a counter only the tests read, kept on the production path. This is the same pattern as StateReconstructions, which was removed in 478be65. The cost is one increment, and only on the unstamped (initcode) path, so this doesn't block. If you want the class free of test-only state, the test could assert on elapsed time or a hashing seam instead.

[Marchhill]

Kept the counter: the code-entry path reads no state and ValueKeccak/CodeInfo are static/sealed, so there is no seam a test double could observe, and a timing assert would be flaky. Dropped the unreachable catch in 6c74323.

[Marchhill]

@claude review

[claude]

Claude finished @Marchhill's task in 1m 59s —— View job

---

PR review

• Gather context: commits since the last review (6c74323d, the master merge 72cbe904, and b086f5a9)
• Review the deferred inclusion check (InclusionChecks, InclusionCheckId, engine_getInclusionListVerdictV1)
• Review separated inclusion lists / omission claims and the builder side
• Post findings

Verdict: mergeable. b086f5a9 replaces the retained-list design with execution-apis#911's deferred, ID-keyed check. That design is simpler, and I found nothing at Critical, High or Medium. Two optional Lows are below.

Deferred inclusion check: checked and fine

• Execution validity is never held for the IL. NewPayloadWithInclusionCheck schedules a check only on VALID, when both inputs are present, and ValidateInitialParams guarantees both are present under 7805. The canonical-behind-head shortcut also answers VALID without an IL arm now, so the old SYNCING-on-resend case is gone.
• InclusionChecks behaves as the spec describes:
  • Start is idempotent per ID.
  • TryGetVerdict never starts a check.
  • A completed verdict is never mutated.
  • The oldest of 256 checks is evicted.
  • An undecidable or faulted check is removed, and only if it is still the entry for its ID (the ReferenceEquals guard), so the CL can resubmit.
  • A check that is evicted while still pending completes on an orphaned object and does no harm.
  • Writing Verdict outside the lock is benign: a reader that runs early just sees "pending".
• EvaluateAsync reads the right state and gas:
  • It waits for the executed copy to leave the queue before reading state, because VALID is answered before commit.
  • It stamps gas dimensions and per-tx gas onto the freshly rebuilt block, not the tree's shared header.
  • It returns null instead of guessing when state is missing. The existing two-bound fallback still covers a cache miss.
• InclusionCheckId: the streaming KeccakRlpWriter lengths match the encoder (LengthOfKeccakRlp, LengthOf(byte[]), LengthOf(ulong)). Matches_the_rlp_the_engine_api_defines checks it against an independently built Rlp.Encode of the same structure.
• Separate lists:
  • InclusionLists.Validate covers exactly 16 lists, no empty entry, and the 8 KiB cap before dedup. It runs on newPayload (-32602) and on FCU attributes (-38003).
  • DecodeLists shares one Transaction per hash, so sender recovery and signature checks run once per envelope.
  • InFillOrder deduplicates each list and sorts it by hash, which gives the (hash, envelope) fill order. A hash collision is the only exception, as the remark notes.
  • Distinct keeps first-occurrence order for ilByHash, so the "first occurrence answers for every copy" rule is unchanged.
• Builder: InclusionListTxSource keys its cache on the attributes' byte[][][], fills each position on its own, and IsEmpty treats 16 empty lists as no IL.
• Claims: OmissionClaim requires both fields, a null hash is -32602, and the 4096 cap matches MAX_OMISSION_CLAIMS. ClaimedIndices semantics are unchanged; only the type was renamed.

Low (optional)

• The pipeline's IL verdict path is now dead in production (inline). Block.InclusionListTransactions is no longer set by the engine API, so these are reachable only from tests:

  • InclusionListSatisfactionChecker's judging half and the flat IsSatisfied overloads;
  • Block.IsInclusionListSatisfied and ProcessingResult.InclusionListUnsatisfied;
  • NotifyInclusionListUnsatisfied;
  • the IL handling in the prewarmer, in RecoverSignatures and in BlockchainProcessor.

  AGENTS.md asks for code a change makes unused to be removed. A follow-up PR is fine, but please note it in the PR body.

• Checks are not throttled. Each new check ID gets its own Task.Run, and evicted checks keep running. Replays are serialised by the replayer's lock, but decoding, sender recovery and the budget fill all run concurrently. The CL is trusted and the work per check is bounded, so this doesn't block merge. A small SemaphoreSlim would cap it if CLs ever send many IL variants per slot.

Still acknowledged in the PR body

• The focilTest CI job (tests-focil-devnet@v0.3.0) will fail until those fixtures are refilled for execution-apis#911.

I only read the code and didn't build it or run the tests. I reviewed 6c74323d..b086f5a9, skipping the master merge. "All 96 engine fixtures pass" is your result, not one I checked.

[claude]

Low: production code no longer reaches anything after this line. ExecutionPayload no longer sets Block.InclusionListTransactions; only the Block constructor and test builders do. So outside tests this method always returns true here, and nothing downstream ever fires:

• the flat InclusionListValidator.IsSatisfied(Block, Transaction[]?, …) overloads;
• Block.IsInclusionListSatisfied;
• ProcessingResult.InclusionListUnsatisfied and NotifyInclusionListUnsatisfied in BlockchainProcessor;
• the IL arm in NewPayloadHandler's verdict mapping (L882/L913);
• the IL sender warm-up in BlockCachePreWarmer, the IL recovery in RecoverSignatures, and the BlockRef guard in BlockchainProcessor.

The checker now only records GasUsedBeforeTransactions. AGENTS.md says to remove code a change makes unused. That can be a follow-up so this diff doesn't grow, but please say so in the PR body. Otherwise a reader will assume the pipeline still reaches a verdict.

[Marchhill]

Removed in 7603394: the flat-list checker and everything only it reached are gone.

[Marchhill]

Removed the dead flat-list pipeline check and throttled/cancelled inclusion checks (both Lows from the last review); engine_getInclusionListV1 now lists Profile 2 candidate frame txs within the per-IL VERIFY budget; completed verdicts are kept until finalized; evaluationIndex must be a canonical QUANTITY. All 96 FOCIL/frame engine fixtures pass.

@claude review

[claude]

Claude finished @Marchhill's task in 1m 32s —— View job

---

PR review

• Gather context: one new commit since the last review (7603394c)
• Review removal of the flat-list pipeline check
• Review InclusionChecks eviction, throttling and cancellation
• Review Profile 2 candidates in engine_getInclusionListV1
• Review CanonicalQuantityConverter
• Post findings

Verdict: mergeable. Both Lows from the last review are fixed. I found nothing at Critical, High or Medium. There is one new Low, in eviction.

Last review's findings: resolved

• The dead flat-list pipeline check is removed. I grepped the tree for InclusionListUnsatisfied, Block.InclusionListTransactions, IInclusionListSatisfactionChecker and NotifyInclusionListUnsatisfied. None are left, apart from NewPayloadHandler's private IsInclusionListSatisfied helper, which is a different thing.
  • BranchProcessor now records GasUsedBeforeTransactions itself, under the same NoValidation and IsActive gates the checker used.
  • The verdict mapping in NewPayloadHandler is back to Success-only.
  • The single remaining IsSatisfied overload always gets the committee lists, so Profile2Inputs.InclusionLists is now correctly non-nullable.
• Checks are throttled and cancelled.
  • _running allows 4 checks at a time.
  • entered makes sure the semaphore is released only by a check that actually acquired it.
  • An evicted check is cancelled through a token linked to _disposed, so it stops whether it is still waiting for a slot or already evaluating.
  • The when (cancellation.IsCancellationRequested) filter covers both cancellation sources.
  • Removing a cancelled check is still guarded by ReferenceEquals, so a newer check with the same ID is never removed.

New since last review: checked and fine

• Retaining unfinalized verdicts. The eviction order is: a check at or below the finalized block, then the oldest pending check other than added, then the oldest check. This matches the PR body. ulong <= ulong? is false when nothing is finalized, so with no finalized block the first rule never fires. The finalized number is read only when the store is near capacity, and outside the lock. The Count read there is racy, but a wrong read only changes which entry is evicted, so it is harmless.

• Profile 2 candidates in engine_getInclusionListV1 (InclusionListBuilder):

  • Each candidate is its own reservoir entry. Blob transactions are excluded by Classify.
  • Readiness uses the keyed nonce set or the account nonce at head, the same state AppendableRun uses.
  • The expiry cut-off deadline <= parentTimestamp is conservative under either reading of the deadline, because the target timestamp is always after the parent's.
  • In EncodeTransactionsUpToLimit, the budget is debited only when an entry is actually listed. The total stays within MAX_VERIFY_GAS_PER_IL, so the attester's fill admits every listed candidate, whatever order it processes them in.
  • A frame candidate that doesn't fit is skipped without putting its sender in droppedSenders, which is right, because it is a run of one.
  • Scanning GetPendingTransactions() is a whole-pool pass, but it runs once per slot and only on IL committee duty, so it is off the hot path.

• CanonicalQuantityConverter rejects:

  • non-string tokens;
  • a missing or uppercase 0X prefix;
  • empty digits;
  • leading zeros (0x0 is still accepted);
  • more than 16 digits;
  • whitespace, because AllowHexSpecifier alone doesn't accept it.

  It accepts either case of hex digit. Its namespace matches the other converters in Nethermind.Core/JsonConverters.

• Wiring. InclusionChecks gets IBlockFinder through the existing Bind<IBlockFinder, IBlockTree>(). GetInclusionListTransactionsHandler passes FocilProfile2MaxVerifyGas through to the builder.

Low

• A newly added check whose block is at or below the finalized one can evict itself (inline, InclusionChecks.cs:106).
  • The finalized rule doesn't exclude added, although the remark and the commit message say the added check is never evicted.
  • With a full store, newPayloadV6 for an already-finalized block returns an ID that polling immediately reports as -38007, and each resubmission does the same.
  • A verdict on a finalized block has little value, so this doesn't block merge. The fix is one condition, plus a test case.

Still acknowledged in the PR body

• The focilTest CI job (tests-focil-devnet@v0.3.0) will fail until those fixtures are refilled for execution-apis#911.

I only read the code; I didn't build it or run tests. I reviewed b086f5a9..7603394c. "All 96 fixtures pass" is your result, not one I checked.

[claude]

Low: the finalized rule can evict the check that is being added. The remark says added is "never the one evicted", and the commit message says "never the check being added". The pending branch excludes added, but this line doesn't.

How it happens:

• The store is full (256 checks).
• No older check is at or below the finalized block.
• The CL calls newPayloadV6 for a block at or below the finalized one, for example after a restart or a resend.

Then added is the first match here. It is removed, and its Evicted token is cancelled. newPayloadV6 still returns its inclusionCheckId, but polling gets -38007 straight away. The CL resubmits, and the same eviction happens on every resubmission for as long as the store stays full. A verdict for a finalized block is not worth much, so this doesn't block merge. Still, the code doesn't do what the doc says.

Suggested change

	if (check.BlockNumber <= finalizedNumber) return check;
	if (check.BlockNumber <= finalizedNumber && !ReferenceEquals(check, added)) return check;

Please add a case to Eviction_keeps_unfinalized_verdicts where the new check's block is at or below the finalized one.

[Marchhill]

Fixed in 34cd32b: added is skipped before every eviction rule. Regression case added to Eviction_keeps_unfinalized_verdicts (added check at the finalized block); it fails without the fix.

[Marchhill]

@claude review

[claude]

Claude finished @Marchhill's task in 20s —— View job

---

PR review

• Gather context: one new commit since the last review (34cd32ba, "never evict the inclusion check being added under the finalized rule")
• Review the InclusionChecks.EvictionCandidate change and the new test case
• Post findings

Verdict: mergeable. The Low from the last review is fixed. I found nothing at Critical, High or Medium.

Last review's finding: resolved

A newly added check could evict itself under the finalized rule.

• The fix. EvictionCandidate now skips added before it applies any rule (InclusionChecks.cs:106), so the remark "never the one evicted" is now true for all three rules. The pending rule no longer needs its own ReferenceEquals guard, and the fix removes it.
• The _order.First!.Value fallback can't return added. Eviction only runs when Count > Capacity, so the store holds at least 257 entries. added is the last of them, appended by AddLast.
• The new test case exercises the bug. With (0UL, 0UL, "pending") the store already holds blocks 1, 3 and 2, so none of them is at or below finalized block 0. Only added, at block 0, matches the finalized rule. Without the fix it would remove itself, and the assertion TryGetVerdict(KeccakA) is True would fail. With the fix the pending check at block 3 is evicted, which matches the documented order.
• The assertion isn't racy. For block 0 the evaluator returns true, and only undecided (null) verdicts are removed, so added stays in the store whether or not its run has finished by the time the test polls.

Still acknowledged in the PR body

• The focilTest CI job (tests-focil-devnet@v0.3.0) will fail until those fixtures are refilled for execution-apis#911.

I only read the code and didn't build or run tests. I reviewed 7603394c..34cd32ba.