Repository navigation
Conversation
Gate a new IsEip8198Enabled flag behind Eip8198TransitionTimestamp; no named fork enables it. When active, the EIP-1559 base fee update uses target_fee_gas_delta * 5 / 48 instead of / BaseFeeMaxChangeDenominator, per execution-specs eips/bogota/eip-8198. The flag lives on IEip1559Spec because the base fee calculator only sees that interface. eth_baseFee, eth_feeHistory and eth_fillTransaction looked up the next block's 1559 spec by block number only, which ignores timestamp forks; use the parent timestamp as ChainHeadInfoProvider does.
|
Claude finished @Marchhill's task in 1m 9s —— View job Review of PR #14606: EIP-8198 (Quick Slots), max base fee change of 5/48
Verdict: I found no Critical, High or Medium issues. There are two Low notes. I did not build the code or run the tests here. What I checked
FindingsLow: simplify
Low / informational: fork-boundary prediction in the RPC,
Comment quality is good. The added comments cite the EIP or explain why the flag sits on |
Stateless guest cost
Variable cost by bucket, summed over shared blocks (excludes BASE)
Compared against |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| private static UInt256 MaxChange(in UInt256 targetFeeGasDelta, in UInt256 denominator, bool isEip8198) => | ||
| isEip8198 | ||
| ? targetFeeGasDelta * Eip8198Constants.BaseFeeMaxChangeNumerator / denominator | ||
| : targetFeeGasDelta / denominator; |
There was a problem hiding this comment.
Low (simplification): MaxChange takes both the denominator and the isEip8198 flag, so the fork check happens in two places. If you choose the numerator next to the denominator (UInt256 maxChangeNumerator = isEip8198 ? Eip8198Constants.BaseFeeMaxChangeNumerator : UInt256.One;), both branches become targetFeeGasDelta * maxChangeNumerator / maxChangeDenominator and the helper can go. Multiplying by 1 gives the same result as before, so pre-fork behaviour is unchanged. The extra UInt256 multiply costs nothing measurable here.
There was a problem hiding this comment.
Done in e9b3caf: a numerator (5 or 1) now sits beside the denominator, both branches compute delta * num / den, and MaxChange is gone. Flag-off results are unchanged (×1), and the vectors and the 7×7 matrix still pass.
Pick a numerator (5 or 1) beside the denominator so both branches compute delta * num / den, and drop the MaxChange helper. Flag-off results are unchanged. eth_feeHistory's next base fee for the newest block now takes the spec from the canonical child's timestamp when the child exists, so a block just before a timestamp fork reports the post-fork rule. The head keeps the parent-timestamp estimate.
|
@claude review Fork-boundary Low: |
|
Claude finished @Marchhill's task in 1m 8s —— View job Re-review of PR #14606 (after e9b3caf)
Verdict: I found no Critical, High or Medium issues, and one new Low. Both earlier findings are resolved. I did not build the code or run the tests here. Earlier findings
New findingLow: simplify
Comment qualityThe added comments explain the reason or cite the EIP; the only issue is the |
| /// <summary> | ||
| /// The cached estimate picks the next block's spec from the block's own timestamp, so it is one block | ||
| /// off at a timestamp fork. When the canonical child already exists, its timestamp picks the spec. | ||
| /// </summary> | ||
| private UInt256 GetNextBaseFee(in BlockFeeHistorySearchInfo info) | ||
| { | ||
| BlockHeader? child = _blockTree.FindHeader(info.BlockNumber + 1, BlockTreeLookupOptions.RequireCanonical | BlockTreeLookupOptions.TotalDifficultyNotNeeded); | ||
| if (child is null || child.ParentHash != info.BlockHash) return info.BaseFeePerGasEst; | ||
|
|
||
| BlockHeader? header = _blockTree.FindHeader(info.BlockHash, BlockTreeLookupOptions.TotalDifficultyNotNeeded, info.BlockNumber); | ||
| return header is null ? info.BaseFeePerGasEst : BaseFeeCalculator.Calculate(header, _specProvider.GetSpec(child)); | ||
| } |
There was a problem hiding this comment.
Low (simplification): when the canonical child exists and child.ParentHash == info.BlockHash, the next base fee is already known: it is child.BaseFeePerGas. HeaderValidator requires that value to equal BaseFeeCalculator.Calculate(parent, GetSpec(child)). Before 1559, both are 0, because the calculator returns the parent's base fee.
Returning child.BaseFeePerGas would:
- remove the second
FindHeader(info.BlockHash, …)lookup and the recalculation - remove the
null-header fallback
You would still need BlockHash for the parent-hash check. On chains whose validator doesn't enforce the calculator (Taiko, for example), it would also report the actual next base fee instead of an estimate.
| /// <summary> | |
| /// The cached estimate picks the next block's spec from the block's own timestamp, so it is one block | |
| /// off at a timestamp fork. When the canonical child already exists, its timestamp picks the spec. | |
| /// </summary> | |
| private UInt256 GetNextBaseFee(in BlockFeeHistorySearchInfo info) | |
| { | |
| BlockHeader? child = _blockTree.FindHeader(info.BlockNumber + 1, BlockTreeLookupOptions.RequireCanonical | BlockTreeLookupOptions.TotalDifficultyNotNeeded); | |
| if (child is null || child.ParentHash != info.BlockHash) return info.BaseFeePerGasEst; | |
| BlockHeader? header = _blockTree.FindHeader(info.BlockHash, BlockTreeLookupOptions.TotalDifficultyNotNeeded, info.BlockNumber); | |
| return header is null ? info.BaseFeePerGasEst : BaseFeeCalculator.Calculate(header, _specProvider.GetSpec(child)); | |
| } | |
| /// <summary> | |
| /// Returns the base fee of the block after <paramref name="info"/>: the canonical child's actual value when | |
| /// the child exists, otherwise the estimate. | |
| /// </summary> | |
| /// <remarks> | |
| /// The estimate picks the next block's spec from the block's own timestamp, so at a timestamp fork it is one block off. | |
| /// </remarks> | |
| private UInt256 GetNextBaseFee(in BlockFeeHistorySearchInfo info) | |
| { | |
| BlockHeader? child = _blockTree.FindHeader(info.BlockNumber + 1, BlockTreeLookupOptions.RequireCanonical | BlockTreeLookupOptions.TotalDifficultyNotNeeded); | |
| return child is not null && child.ParentHash == info.BlockHash ? child.BaseFeePerGas : info.BaseFeePerGasEst; | |
| } |
The with_child_after_fork test would then need .WithBaseFeePerGas(1_104_166_666) on child, and it could drop the FindHeader(block.Hash!, …) stub.
Separately, the current <summary> explains the reason for the method rather than what it returns. Per AGENTS.md that explanation belongs in <remarks>; the suggestion above splits it that way.
There was a problem hiding this comment.
Applied in 58bff53: when the canonical child exists, GetNextBaseFee returns its BaseFeePerGas, the summary/remarks are split as suggested, and the test sets the child's base fee and drops the hash stub.
|
@claude review |
|
Claude finished @Marchhill's task in 37s —— View job Re-review of PR #14606 (after 58bff53)
Verdict: I found no Critical, High, Medium or Low issues, and all earlier findings are resolved. I did not build the code or run the tests here.
|
Validator passed StartingBlockTimestamp, in seconds, to SlotTime.GetSlot, which takes ms, so HistoricalRoots and HistoricalSummaries verification threw "before genesis".
SlotSchedule maps timestamps to slots and steps slots the way the CL does under EIP-8198 (consensus-specs#5710): the configured slot length from beacon genesis, then 10 s from the first boundary at or after the EIP-8198 activation. ISpecProvider.GetEip8198Timestamp finds that activation from the transition list; without it the schedule is the single configured length.
…hedule - eth_simulate EIP-7843 slot numbers come from the piecewise beacon grid. - eth_simulate and debug_traceCallMany default timestamps advance one scheduled slot per block, so 10 s from the EIP-8198 fork. - eth_baseFee and eth_fillTransaction take the next block's spec one slot after the head instead of at the head's timestamp. - The eth_estimateGas funded run predicts the next block one scheduled slot on.
Marchhill
changed the title
|
@claude review |
|
Claude finished @Marchhill's task in 1m 27s —— View job Re-review of PR #14606 (after bb41bb8)
Verdict: I found no Critical, High or Medium issues. There is one Low (performance) note. All earlier findings stay resolved. I did not build the code or run the tests here. What I checked
FindingLow (performance): the slot schedule is rebuilt on every request (inline),
|
|
|
||
| /// <summary>The spec of the block after <paramref name="head"/>, assumed to land in the next slot.</summary> | ||
| private IReleaseSpec GetNextBlockSpec(BlockHeader head) => | ||
| _specProvider.GetSpec(head.Number + 1, _specProvider.GetSlotSchedule(_secondsPerSlot).GetNextSlotTimestamp(head.Timestamp)); |
There was a problem hiding this comment.
Low (performance): GetSlotSchedule builds a new SlotSchedule on each call. Each build runs GetEip8198Timestamp, which loops over every TransitionActivations entry and calls GetSpec for each one. On mainnet that is about 20 spec lookups and one allocation, all to get a value fixed at startup.
Here it runs once per eth_baseFee and eth_fillTransaction call. It also runs per request in:
BlockchainBridge.CreateFundedRunContext, which is on theeth_estimateGaspathSimulateBridgeHelper.GetSlotNumber, once per simulated block after EIP-7843
That isn't a correctness problem, but it costs time on hot RPC paths for no benefit. Building the schedule once per instance, as SimulateTxExecutor, MempoolStatePrewarmer and EraE already do, removes the cost. For example, a private readonly SlotSchedule _slots = specProvider.GetSlotSchedule(secondsPerSlot ...) field here, and similar fields in BlockchainBridge and SimulateBridgeHelper.
The simulate helper may receive an overridden spec provider. If that is the case, keep it per call there, or cache it per provider instance.
There was a problem hiding this comment.
Done in 7bc644c. EthRpcModule and BlockchainBridge build the schedule once, lazily. SimulateBridgeHelper caches it per spec provider, so a request that brings an override provider rebuilds it once per request rather than once per block.
…d simulate provider
|
@claude review |
|
Claude finished @Marchhill's task in 45s —— View job Re-review of PR #14606 (after 7bc644c)
Verdict: I found no Critical, High or Medium issues, and one new Low (docs). The performance finding from the last review is resolved. I did not build the code or run the tests here. Slot-schedule caching: resolved
New findingLow (docs): the doc comment moved onto the field (inline),
Earlier findingsEverything from earlier reviews is still resolved:
|
| /// A failure without standard text names the gas the next block's rules fund, run with the blob fee cap | ||
| /// filled from the next block's blob base fee when the request left it out. | ||
| /// </summary> | ||
| private SlotSchedule? _slots; |
There was a problem hiding this comment.
Low (docs): this field sits between the existing <summary> (lines 310-313) and CreateFundedRunContext. As a result, the XML doc now attaches to _slots, and the method has no docs. IntelliSense will show the funded-run text on the field.
Fix: move private SlotSchedule? _slots; above the /// <summary> block, or next to the class's other fields.
There was a problem hiding this comment.
Moved the field above the doc block in 2cb2b3c.
|
@claude review |
1 similar comment
|
@claude review |
|
Claude finished @Marchhill's task in 16s —— View job Re-review of PR #14606 (after 2cb2b3c)
Verdict: I found no Critical, High, Medium or Low issues, and every earlier finding is resolved. I did not build the code or run the tests here. Doc placement: resolvedIn Earlier findingsAll are still resolved:
Spot-checkI re-read
|
…ionMs is set EIP-8198 networks such as Heze devnet v2 keep 12 s slots, so without an explicit eip8198SlotDurationMs the slot length after the fork stays Blocks.SecondsPerSlot. DesignSlotDurationMs remains only the base fee design constant: 5/48 applies whatever the slot length.
|
@claude review |
|
Claude finished @Marchhill's task in 42s —— View job Re-review of PR #14606 (after 0071ad6)
Verdict: I found no Critical, High, Medium or Low issues, plus one test-comment nit. Every earlier finding is still resolved. I did not build the code or run the tests here. Slot length falls back to
|
|
@claude review |
|
Claude finished @Marchhill's task in 19s —— View job Re-review of PR #14606 (after aecf2a0)
Verdict: I found no Critical, High, Medium or Low issues. The nit from the last review is resolved, and every earlier finding is still resolved. I did not build the code or run the tests here. Test comment wording (aecf2a0): resolved
Still open from earlier reviews (optional, not blocking)
Earlier findingsAll are still resolved:
|
|
Merged master and applied the reviewed lazy parent-scope and generated-BAL fixes. Specs 723 and focused consensus 87 tests pass; Heze v2 remains unfiltered. @claude review |
|
Claude finished @Marchhill's task in 1m 1s —— View job Re-review of PR #14606 (after 58d6a19)
Verdict: No Critical, High, Medium or Low issues. The optional lazy-parent-scope note from earlier reviews is resolved. I didn't build the code or run the tests here. Lazy parent scope (58d6a19): resolved
Generated BAL fallback (58d6a19)
Still open from earlier reviews (optional, not blocking)
Earlier findingsAll still resolved:
|
…d accept a slot duration
|
@claude review |
|
Claude finished @Marchhill's task in 32s —— View job Re-review of PR #14606 (after 9c52da8)
Verdict: No Critical, High, Medium or Low issues. There are two optional test-only nits. Every earlier finding is still resolved. I didn't build the code or run the tests here. Mainnet and Sepolia genesis tests (9c52da8)This commit only adds tests, in
Nits (test-only, optional)
Still open from earlier reviews (optional, not blocking)
Earlier findingsAll still resolved:
|
|
@claude review |
|
Claude finished @Marchhill's task in 14s —— View job Re-review of PR #14606 (after a63b992)
Verdict: I found no Critical, High, Medium or Low issues. The comment-wording nit from the last review is fixed, and every earlier finding is still resolved. I didn't build the code or run the tests here. Test comment wording (a63b992): resolved
Still open from earlier reviews (optional, not blocking)
Earlier findingsAll still resolved:
|
|
@claude review |
|
Claude finished @Marchhill's task in 39s —— View job Re-review of PR #14606 (after b9ca4ed)
Verdict: I found no Critical, High, Medium or Low issues, and every earlier finding is still resolved. I didn't build the code or run the tests here. This run isn't allowed to use Merge of
|
* feat(sync): count BAL catch-up blocks that execute because of the engine API or missing finality Blocks delivered through engine_newPayload, and catch-up blocks the consensus client has not reported finalized yet, now show up in nethermind_bal_catch_up_fallbacks under NewPayload and NotFinalized. Only blocks older than two epochs count, so following the head adds nothing. The decorator now delegates when it is nested under another instance, so a pipeline that loads the module twice no longer counts fallbacks twice. The config description documents when the catch-up applies. * feat(sync): log each block reconstructed from its access list at debug level * fix(test): build TestBlockchain's container once, and drop the nesting guard it needed TestBlockchain configured its container twice, so every module, decorator and IMainProcessingModule was registered twice and the BAL catch-up decorator was nested in the engine tests. Production registers it once, so the [ThreadStatic] guard against nesting goes. Rename the EngineApi reason, since forkchoiceUpdated reprocessing also uses the engine path, and shorten the config description. The engine test now checks the chain's own log, as the counters are shared with parallel tests. * docs(sync): shorten BAL catch-up guidance * fix(sync): follow slot schedule for BAL catch-up age --------- Co-authored-by: Marc Harvey-Hill <10379486+Marchhill@users.noreply.github.com>
|
@claude review |
|
Claude finished @Marchhill's task in 1m 10s —— View job Re-review of PR #14606 (after a3e4a73)
Verdict: I found no Critical, High or Medium issues. There is one Low, about the PR description. Every earlier finding is still resolved. I didn't build the code or run the tests here. BAL catch-up tracking (312fa8f)
FindingLow (PR description): the PR now also carries #14899, the BAL catch-up fallback metrics.
Merge of
|
Changes
Includes feat(sync): track executed BAL catch-up blocks #14899: BAL catch-up fallback metrics and the TestBlockchain registration fix.
Enable EIP-8198 Quick Slots in Bogota, including Geth
bogotaTimeand Parity fork labels.Change maximum base-fee movement from 1/8 to 5/48 at activation. The fraction is spec data (
IEip1559Spec.BaseFeeMaxChangeNumerator/Denominator), derived fromIsEip8198EnabledinDerivedSpecValuesasBaseFeeMaxChange.ScaledToSlot(10_000), which keeps the 1/8-per-12 s rate per second. While EIP-8198 is active it overrides a chainspeceip1559BaseFeeMaxChangeDenominator.Make slot scheduling fork-aware for RPC predictions, simulation, prewarming, payload pacing, and EraE. Fix EraE timestamp units.
GetSlotSchedulestarts a segment at each fork activation that changes the slot length.Add an optional
eip8198SlotDurationMs(chainspecparamsor Gethconfig, a whole number of seconds in ms): the slot length from EIP-8198, matching the CL'sSLOT_DURATION_MS_EIP8198. Unset keepsBlocks.SecondsPerSlot, so a genesis with onlybogotaTime(Heze devnet v2, 12 s) loads and needs nothing else. It only moves slot timing; the base fee fraction is 5/48 either way.Use the canonical child's base fee for historical
eth_feeHistoryresults.Run
tests-heze-devnet@v2.0.0in CI, including Bogota transitions, and load its blob schedule (target 12, maximum 18).Types of changes
Testing
Requires testing
If yes, did you write tests?
All 67,785 Heze v2 fixtures pass locally at
86e9b9386e, plus 135 FOCIL cases with parallel execution and batch reads. Specs: 625 passed. Regression coverage includesbogotaTime: 1792416379, slot scheduling, RPCs, EraE, and payload pacing.Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
Includes the FOCIL fix from #14916. Follows execution-specs#3703 and consensus-specs#5710. Mainnet's chainspec will set
eip8198SlotDurationMs: 10000when Hegota is scheduled.