Repository navigation
Conversation
Implements ethereum/EIPs#12552: the maximum change is seconds_elapsed / (8 * 12 * slots_elapsed) from the header timestamps and EIP-7843 slot numbers, capped at the whole base fee as in execution-specs#3873. Under EIP-8198 the slot number must increase. Predictions place the next block in the slot after the parent.
ethereum/EIPs#12552 drops the provisional blob values; a slot duration change brings its own EIP-7892 blobSchedule entry (execution-specs#3871). Bogota now inherits Amsterdam's target 14, maximum 21 and update fraction 11684671.
Contributor
Author
|
@claude review |
Contributor
|
Claude finished @Marchhill's task in 2m 24s —— View job Review of PR #14984: EIP-8198 base fee derived from block spacing
VerdictI found no correctness problems. The one blocker is the one you already listed in the description: the Heze/Bogota fixtures need to be refilled from execution-specs#3873 before this can merge. The core rule (
|
| Severity | Finding |
|---|---|
| High (acknowledged) | Fixtures: tests-heze-devnet@v2.0.0 puts every block at slot 0 and uses 5/48. With this PR, about 22k Bogota block tests and about 25k engine tests fail with SlotNumberNotAfterParent or a base-fee mismatch. The description already says to merge only after Heze fixtures are refilled from execution-specs#3873, so this is gating, not a code defect. |
| Low (acknowledged) | Tx filters: BaseFeeTxFilter and MinGasPriceTxFilter now price a 12 s slot. At shorter slots they overstate a rising base fee, so the producer skips some transactions the block could include. Before this change the result here was exact. This doesn't affect consensus. Details are in the inline comment. |
| Low | Comment: ChainHeadInfoProvider.cs:132, // Under EIP-8198 the next block is assumed to land in the next slot., repeats CalculateForNextSlot's name and doc (inline). |
Minor notes (no action needed)
- The predictions in
FeeHistoryOracleandInclusionListBuilderstill pick the spec from the parent's timestamp. So the first block after Bogota activates is estimated with the 1/8 rule. The remarks already document this. T8nValidatornow readsslotNumberandparentSlotNumberfor the base fee. As far as I can see, the env'sslotNumberisn't applied to the header the tool executes. That was already the case for EIP-7843 and is outside this PR.- On performance, validation adds one
UInt256.Modper header when EIP-8198 is active, which is negligible. TheSlotScheduleinstances are built lazily and cached once.
Not done
- I didn't build the code or run the tests. The fetch and deepen needed for a proper merge-base diff required approval, so I reviewed by static reading only.
🤖 AI agent (Claude Code / Opus 5.5) on behalf of @Marchhill
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| CurrentBaseFee = header.BaseFeePerGas; | ||
| IReleaseSpec childSpec = SpecProvider.GetSpec(header.Number + 1, header.Timestamp); | ||
| NextBaseFee = childSpec.IsEip1559Enabled ? BaseFeeCalculator.Calculate(header, childSpec) : UInt256.Zero; | ||
| // Under EIP-8198 the next block is assumed to land in the next slot. |
Contributor
There was a problem hiding this comment.
Low (comment quality): this repeats what CalculateForNextSlot already says in its name and XML doc. Consider dropping it.
🤖 AI agent (Claude Code / Opus 5.5) on behalf of @Marchhill
| } | ||
|
|
||
| UInt256 baseFee = BaseFeeCalculator.Calculate(parentHeader, spec); | ||
| UInt256 baseFee = BaseFeeCalculator.CalculateForReferenceSlot(parentHeader, spec); |
Contributor
There was a problem hiding this comment.
Low (acknowledged in the PR description): before this change, the parent-only base fee here was exact. Under EIP-8198 on a chain with slots other than 12 s, the 12 s reference overstates a rising base fee. At 10 s slots that is 1/8 instead of 5/48, so the producer's TxFilterPipeline skips transactions whose MaxFeePerGas falls between the two, even though the block could include them. This doesn't affect consensus, because the AuRa processing filter doesn't use it and is off post-merge. It does cost some inclusion when fees are rising.
If you want to fix it later, TxPoolTxSource already has targetBlock and the exact baseFee. Passing the predicted base fee through, or pricing for the next slot as the other prediction paths do, would make it exact again. It's fine to leave it as a follow-up.
🤖 AI agent (Claude Code / Opus 5.5) on behalf of @Marchhill
Contributor
Author
There was a problem hiding this comment.
Fixed in baf4ed0: the producer pipelines pass the slot schedule, so both filters price the slot after the parent. Without a schedule they keep the 12 s reference.
The base fee and min gas price filters now take the slot schedule from the producer's tx source factories, so under EIP-8198 they price the slot after the parent rather than a 12 s reference slot. Drop a comment that repeated CalculateForNextSlot.
Contributor
Author
|
@claude review |
Contributor
|
Claude finished @Marchhill's task in 46s —— View job Re-review of PR #14984 (after
|
| Severity | Finding |
|---|---|
| High (acknowledged) | Fixtures: tests-heze-devnet@v2.0.0 puts every block at slot 0 and uses 5/48, so about 22k Bogota block tests and about 25k engine tests fail. The description already makes the refilled fixtures from execution-specs#3873 a merge gate. |
| Low | AuRa tx filters still use the 12 s reference. AuRaTxPoolTxSourceFactory.cs:75 and :95 still call the parameterless .WithBaseFeeFilter(), and the AuRa producer's min-gas-price filter is built by TxFilterBuilders.CreateStandardMinGasPriceTxFilter without a schedule. If a Gnosis-style chain (5 s slots) ever enables EIP-8198, its producer will overstate a rising base fee as before. Nothing schedules that today, so it's fine to leave as a follow-up. |
| Low | Test assertions: Eip8198TxFilterTests uses Assert.That(x.Equals(y), Is.True), so a failure doesn't show the actual result. Is.EqualTo would (inline). |
Earlier findings
Low: the tx filters overstate the base fee at non-12 s slotsis fixed bybaf4ed0.Low: the redundantis removed.ChainHeadInfoProvidercomment
Not done
- I didn't build the code or run the tests; this re-review is static reading only.
🤖 AI agent (Claude Code / Opus 5.5) on behalf of @Marchhill
| Transaction tx = Tx(FeeBetweenTheRules); | ||
| using (Assert.EnterMultipleScope()) | ||
| { | ||
| Assert.That(new BaseFeeTxFilter(new SlotSchedule(10)).IsAllowed(tx, Parent(), Spec).Equals(AcceptTxResult.Accepted), Is.True, "10 s slots"); |
Contributor
There was a problem hiding this comment.
Low (test style): Assert.That(x.Equals(y), Is.True, ...) only reports Expected: True But was: False when it fails. Is.EqualTo shows the actual AcceptTxResult, which includes the rejection reason. The same applies to lines 46–48 and 60–61.
Suggested change
| Assert.That(new BaseFeeTxFilter(new SlotSchedule(10)).IsAllowed(tx, Parent(), Spec).Equals(AcceptTxResult.Accepted), Is.True, "10 s slots"); | |
| Assert.That(new BaseFeeTxFilter(new SlotSchedule(10)).IsAllowed(tx, Parent(), Spec), Is.EqualTo(AcceptTxResult.Accepted), "10 s slots"); |
🤖 AI agent (Claude Code / Opus 5.5) on behalf of @Marchhill
Contributor
Author
There was a problem hiding this comment.
Fixed in 6aa8081; also wired the AuRa producer filters to the slot schedule.
The AuRa producer's base fee and min gas price filters take the slot schedule as the standard producer's do. The filter tests assert the result with Is.EqualTo.
Contributor
Author
|
@claude review |
Contributor
|
Claude finished @Marchhill's task in 28s —— View job Re-review of PR #14984 (after
|
| Severity | Finding |
|---|---|
| High (acknowledged) | Fixtures: tests-heze-devnet@v2.0.0 puts every block at slot 0 and uses 5/48. About 22k Bogota block tests and about 25k engine tests fail with SlotNumberNotAfterParent or a base-fee mismatch. The description already says to merge only after the fixtures are refilled from execution-specs#3873. This is a merge gate, not a code defect. |
Earlier findings
Low: the AuRa tx filters still use the 12 s reference: fixed in6aa8081.Low:: fixed inAssert.That(x.Equals(y), Is.True)inEip8198TxFilterTests6aa8081.Low: the tx filters overstate the base fee at slot lengths other than 12 s: fixed inbaf4ed0.Low: the redundant: removed inChainHeadInfoProvidercommentbaf4ed0.
Not done
- I didn't build the code or run the tests. This re-review is static reading only.
🤖 AI agent (Claude Code / Opus 5.5) on behalf of @Marchhill
# Conflicts: # src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.cs
Contributor
Stateless guest costRestored from Only absolute measurements are shown.
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Implements ethereum/EIPs#12552 at
57884154, stacked on #14606. The max base-fee change is now derived from the spacing of a block and its parent instead of being a fixed 5/48.delta = target_fee_gas_delta * s // (D * 12 * k), wheres = header.timestamp - parent.timestamp,k = header.slot_number - parent.slot_numberandD = BaseFeeMaxChangeDenominator(8). It multiplies before dividing.sis capped atD * 12 * k, so the change is never more than the whole base fee. The cap comes from execution-specs#3873; the EIP text doesn't have it.IBaseFeeCalculator.Calculatenow takes the child's timestamp and slot number. Validation, block production, the tx source, simulate, trace, Clique, Xdc, Optimism and t8n pass the real header.eth_baseFee,eth_fillTransaction, theeth_feeHistoryhead estimate, the txpool'sNextBaseFee, the inclusion-list builder and the prewarmer assume the next block lands one slot after the parent, using the slot schedule (eip8198SlotDurationMs/Blocks.SecondsPerSlot).BaseFeeMaxChangeNumerator,BaseFeeMaxChange.ScaledToSlotandEip8198Constants.DesignSlotDurationMs. EIP-8198 no longer overrides a configured denominator; it scales it.blobSchedule, and the fixtures throughconfig.blobSchedule.No behaviour changes before EIP-8198 activates.
Fixture status: merge after new fixtures
tests-heze-devnet@v2.0.0was filled with 5/48 and puts every block at slot 0, so most Bogota block tests fail here withSlotNumberNotAfterParent. The remaining failures are base-fee mismatches.Fixtures filled from execution-specs#3873 (
c768e6c5) foreip8198_quick_slotsandeip7843_slotnumpass:The engine variants from that branch use
engine_newPayloadV5for Bogota, but Bogota here is V6 because it includes FOCIL, so they can't be run as is.Heze v2, Bogota and its transitions:
base_fee_wrong_rule_across_fork[fork_block_old_rule](now valid at 12 s)fork_block_old_ruleThe PR has to wait for Heze fixtures filled with execution-specs#3873, which uses increasing slot numbers and the derived rule.
Types of changes
Testing
Requires testing
If yes, did you write tests?
Unit tests cover 12/10/8/6 s slots, missed slots, slot not increasing, a parent without a slot number, spacing above 96 s, overflow of 256 and 64 bits, pre-fork forks, activation, and the RPC prediction paths.