fix(eth_createAccessList): preserve reverted-frame accesses and exclude precompiles#11431
Conversation
…mpile addresses without storage keys
…ompile addresses with empty storage keys
manusw7
requested review from
LukaszRozmej,
benaadams and
svlachakis
as code owners
|
Claude finished @manusw7's task in 9m 39s —— View job PR Review
SummaryThe PR correctly fixes three distinct
The design is sound. Findings below: Findings
CorrectnessThe The EIP-7702 delegation address capture in Security / PerformanceNo security concerns. This is RPC-layer simulation code only — no hot-path execution path is affected. The |
manusw7
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Collect active precompile addresses so that they can | ||
| // be excluded from the access list if they carry no storage keys | ||
| IReleaseSpec releaseSpec = specProvider.GetSpec(header); | ||
| Address[] precompileAddresses = [.. releaseSpec.Precompiles.Select(static p => (Address)p)]; |
There was a problem hiding this comment.
Medium — LINQ violation: The repo rules in .agents/rules/coding-style.md explicitly prohibit LINQ when a simple for/foreach works. Precompiles is a FrozenSet<AddressAsKey> with a known Count, so this should be rewritten as a loop:
Address[] precompileAddresses = new Address[releaseSpec.Precompiles.Count];
int idx = 0;
foreach (AddressAsKey p in releaseSpec.Precompiles)
precompileAddresses[idx++] = (Address)p;
| Address contractAddress = typeof(TOpCreate) == typeof(OpCreate) | ||
| ? ContractAddress.From(env.ExecutingAccount, state.GetNonce(env.ExecutingAccount)) | ||
| : ContractAddress.From(env.ExecutingAccount, salt, initCode.Span); | ||
|
|
There was a problem hiding this comment.
Low — Misleading comment: The comment says "captures the new contract pre-Berlin" but this guard fires for any fork when IsTracingAccess=true, including Berlin+. Change to something like:
// Also runs under access-list tracing so eth_createAccessList captures the new contract address.
| public readonly HashSet<Address> AllAccessedAddresses => _trackingState.AllAccessedAddresses; | ||
| public readonly HashSet<StorageCell> AllAccessedStorageCells => _trackingState.AllAccessedStorageCells; |
There was a problem hiding this comment.
Low — Mutable public sets: AllAccessedAddresses and AllAccessedStorageCells are exposed as mutable HashSet<T>. Any caller can .Add() / .Remove() directly, bypassing the _isTracingAccess guard and journal invariants. Since TransactionProcessor only needs to iterate them for ReportAccess, consider exposing them as IReadOnlyCollection<T> (or even IEnumerable<T>) to communicate the intent and prevent accidental modification:
public readonly IReadOnlyCollection<Address> AllAccessedAddresses => _trackingState.AllAccessedAddresses;
public readonly IReadOnlyCollection<StorageCell> AllAccessedStorageCells => _trackingState.AllAccessedStorageCells;HashSet<T> implements IReadOnlyCollection<T>, so this requires no cast at the call site.
|
|
||
| callOutput.AccessList.Should().NotBeNull(); | ||
| callOutput.AccessList!.Any(e => e.Address == PrecompiledAddresses.ECRecover).Should().BeFalse(); | ||
| callOutput.AccessList.Any(e => e.Address == TestItem.AddressC).Should().BeTrue(); |
There was a problem hiding this comment.
Medium — Missing coverage for optimize=false: The new precompile-exclusion logic applies to both paths (optimize=true and optimize=false), but only the optimize=true case is tested here. Please add a parallel test with optimize=false to confirm precompiles with no storage keys are still excluded from the returned access list via the updated new([header.GasBeneficiary, .. precompileAddresses]) initializer.
Also worth adding: a case where the precompile does have a storage key (unusual but theoretically possible) to document that it is deliberately included in that situation.
| if (isTracingAccess) | ||
| { | ||
| if (isTracingAccess) | ||
| { | ||
| accessTracker.WarmUp(address); | ||
| } | ||
|
|
||
| if (!spec.IsPrecompile(address) && accessTracker.WarmUp(address)) | ||
| { | ||
| result = UpdateGas(ref gas, GasCostOf.ColdAccountAccess); | ||
| } | ||
| else if (chargeForWarm) | ||
| { | ||
| result = UpdateGas(ref gas, GasCostOf.WarmStateRead); | ||
| } | ||
| accessTracker.WarmUp(address); | ||
| } | ||
|
|
||
| return result; | ||
| if (!spec.UseHotAndColdStorage) | ||
| return true; | ||
|
|
||
| if (!spec.IsPrecompile(address) && accessTracker.WarmUp(address)) | ||
| { | ||
| return UpdateGas(ref gas, GasCostOf.ColdAccountAccess); | ||
| } | ||
| if (chargeForWarm) | ||
| { | ||
| return UpdateGas(ref gas, GasCostOf.WarmStateRead); | ||
| } |
There was a problem hiding this comment.
Note — Pre-existing double WarmUp (no action needed, but awareness important)
When isTracingAccess=true AND spec.UseHotAndColdStorage=true (Berlin+), WarmUp(address) is now called twice: once at line 141 (tracing block, adds to AccessedAddresses returning true), and once at line 147 (gas block, returns false because the address is already warm). Cold-access gas (GasCostOf.ColdAccountAccess) is therefore never charged in eth_createAccessList simulations on Berlin+.
This was already the case in the pre-PR code (same double WarmUp existed), and is arguably correct semantics: the generated access list will pre-warm every address in the actual transaction, so warm-access gas is the right estimate. However it is worth a brief comment to prevent future readers from being confused by the apparent miss:
// Note: isTracingAccess pre-warms the address above, so this second WarmUp returns false
// (already warm). Cold gas is intentionally not charged here — the caller will include
// these addresses in an EIP-2930 access list, making them warm on actual execution.
if (!spec.IsPrecompile(address) && accessTracker.WarmUp(address))…address handling and update comments
… precompile addresses based on storage keys
…structor for clarity
|
@claude review again |
|
Claude finished @manusw7's task in 3m 21s —— View job PR Review
SummaryThis re-review confirms all five findings from the previous review have been addressed.
Fresh pass — no new findingsThe implementation is correct and complete:
TestsThree
Three Mergeable — no blocking findings. |
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
IMO this feels off:
- eth_createAccessList for pre-berlin blocks doesn't have much sense. If there are tests for it we should consider convincing EF to just drop them.
- Having separate modes just for that sounds overkill
- This PR probably has negative performance effects
IMO not worth it at all
manusw7
changed the title
|
Claude finished @manusw7's task in 6m 59s —— View job PR Review
Summary of changes vs. prior reviewsThis is the third review pass. The PR has been substantially simplified since the 2nd review: the previous approach of two extra Reviewer requests status:
Findings
CorrectnessThe revised approach is correct for
PerformanceThe SecurityNo security concerns. This is RPC simulation code; the hot-path EVM execution is unaffected when Mergeable: No — @LukaszRozmej's |
…ving LINQ and using pre-allocated arrays
|
@claude review again. Also note that change request was about PreBerlin tests (which were dropped), rn |
|
Claude finished @manusw7's task in 3m 25s —— View job PR Review (4th pass)
Status of 3rd-review findings
On finding #3: @LukaszRozmej's inline comment was attached to the test file at the time it contained both pre-Berlin tests and reverted-frame tests. The pre-Berlin tests were the primary driver of the CHANGES_REQUESTED review (concerns 1–3 in their top-level comment). The 3 remaining new tests cover reverted sub-frame behaviour — they're topically consistent with the existing Fresh pass — no new findings
Journal growth is bounded: Pool reuse is safe:
Gas-estimate semantics — With Tests — The 3 RecommendationMergeable. All medium findings are resolved, no new findings. The implementation is correct, minimal, and has no hot-path impact. The only process note: @LukaszRozmej has an open CHANGES_REQUESTED review. Given that all three of their primary concerns have been addressed (pre-Berlin scope dropped, extra-HashSet allocation replaced by a |
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
Check if tests can be deduplicated more
svlachakis
left a comment
svlachakis
left a comment
There was a problem hiding this comment.
LGTM, please check @LukaszRozmej's comments
…<Address> in ConvergeAccessList
Collapse the two precompile-filter tests differing only by `optimize` into a single parameterized test, and extract the shared mock setup into a helper used by both tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 participants
Fixes #11209
Changes
StackAccessTracker: addedisTracingAccessflag; when set,Restore()skips rolling backAccessedAddresses/AccessedStorageCellson sub-frame revert — so all addresses and storage keys touched inside a reverted frame survive to the final access list report.BlockchainBridge.BuildAddressesToOptimize: includes all active precompile addresses for the given spec, so precompiles with no storage keys are excluded from the returned access list.BlockchainBridge.CreateAccessList(with and without storage keys).Types of changes
Testing
Requires testing
If yes, did you write tests?
Notes on testing
src/Nethermind/Nethermind.Evm.Test/Tracing/AccessTxTracerTests.cssrc/Nethermind/Nethermind.Facade.Test/BlockchainBridgeTests.csDocumentation
Requires documentation update
Requires explanation in Release Notes
Fixed
eth_createAccessListto return complete access lists: addresses and storage cells first touched inside reverted sub-frames are now preserved, and active precompile addresses with no storage keys are excluded from the result.