Merge master into eip8141-frame-txs-devnet7 - #12802
Merged
AnkushinDaniil merged 56 commits intoAug 13, 2026
Merged
Conversation
…12625) * diag(rpc-bench): run Nethermind with production-default runtime (no TC=0 pin) * reword comment for master * review: document expb divergence, add NODE_ENV_VARS escape hatch - README: the 'Alignment with expb' section no longer claims the removed env pins; documents the deliberate code-gen divergence and that JIT warm-up now lands inside the measured window; dotTrace reports are not comparable across this change - start-node.sh: reword comment (no warm-up phase exists yet), add NODE_ENV_VARS passthrough for deliberate one-off code-gen experiments * trim comments to one-liners; rationale stays in the PR * drop the Merge GC flags: inert here and misleading GCKeeper only runs on Engine API calls; this harness parks the node at a snapshot head and never sends newPayload, so the three flags changed nothing while implying the node ran a non-production GC configuration. * keep the image entrypoint for Nethermind The override skipped entrypoint.sh, which applies host tuning and enables a shipped PGO profile. Its comment claimed parity with expb, but expb only overrides the entrypoint for dotTrace, so normal expb runs (and production) do run entrypoint.sh - this harness was the outlier.
Co-authored-by: rubo <rubo@users.noreply.github.com>
* refactor(net): namespace snap messages by version
Prepare the snap subprotocol for a second version by moving the concrete
snap/1 messages, message codes and protocol handler into a versioned
namespace, so a snap/2 handler can be added alongside without touching
snap/1 code.
Snap/Messages/* -> Snap/V1/Messages/*
Snap/SnapMessageCode -> Snap/V1/Snap1MessageCode
Snap/SnapProtocolHandler -> Snap/V1/Snap1ProtocolHandler
P2P/P2PMessageKey.cs -> P2P/VersionedProtocol.cs (file renamed to
match the type it declares)
SnapMessageBase and SnapSerializerBase deliberately stay in
Snap.Messages: they are shared by all protocol versions, not specific to
snap/1.
Introduce SnapVersions constants and ISnapSyncPeer.SnapProtocolVersion so
version checks can replace the magic numbers currently spelled as 1.
PeerInfoExtensions.CanGetSnapData is renamed to CanGetTrieNodes to say
what it actually tests. The rename is nominal - the peer probe is
unchanged, so behaviour is identical.
No functional change.
* refactor(net): remove Snap2 version constant from SnapVersions
* address review comments
* rename
* Refactor SnapServer and SnapStateServer integration - Renamed SnapServer to SnapStateServer for clarity and consistency. - Updated WorldStateManager to use SnapStateServer instead of SnapServer. - Modified StateSyncFeedTestsBase to accommodate changes in SnapServer instantiation. - Adjusted SnapProviderTests to reflect the new SnapStateServer type. - Introduced SnapServerTests to validate SnapServer functionality. - Added SnapStateServerTests to ensure robust testing of state management. - Implemented new methods in SnapServer for handling bytecode and block access lists. - Enhanced test coverage for account range retrieval and storage management. * refactor: change SnapServer field type to interface ISnapServer * test: enhance SnapServerTests with additional block access list scenarios
Update Dockerfiles Co-authored-by: rubo <rubo@users.noreply.github.com>
PreWarmCaches_ReturnsAddressWarmEnvWhenScopeBuildThrows asserts Returned == Created, but with maxPoolSize 1 an env returned to the pool can be rented again without a Create, so a pool hit legitimately increments Returned twice against a single Create. Whether a hit occurs depends on worker interleaving: flaky on CI, fails deterministically on high-core machines (0/30 locally). ThrowingBuildPolicy.Return now refuses retention, so every rental is a fresh Create and the invariant holds under any interleaving (30/30 locally). Stack-trace instrumentation confirmed every rental is returned exactly once - the prewarmer itself is correct.
Co-authored-by: emlautarom1 <emlautarom1@users.noreply.github.com> Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
* fix(receipts): restore the post-merge flag before regeneration Stored headers do not carry IsPostMerge and regeneration bypasses the recovery step that restores it, so post-merge blocks re-executed with PREVRANDAO evaluating to the zeroed difficulty - any transaction reading it produced receipts failing the root check (~7% of post-merge blocks refused on mainnet archives deriving receipts from state). * test(receipts): dispose buffer, pin logged value * fix(receipts): classify post-merge via the switcher A difficulty heuristic misreads chains that repurpose the field - Taiko stores per-block ZK gas in Difficulty while AlwaysPoS - so ask IPoSSwitcher instead, mirroring MergeProcessingRecoveryStep. On mainnet the switcher's TD-null branch is the same difficulty check, so behavior there is unchanged. * test(receipts): pin the real switcher's TD-null derivation The production failure arrived as a mainnet-shaped header with TotalDifficulty unset; cover PoSSwitcher's TD-null branch end to end, not only the honour-the-switcher contract. * test(receipts): cover the switcher registration path A hand-injected switcher cannot catch a composition regression that leaves the container-resolved regenerator on the NoPoS default, so resolve it from a graph whose IPoSSwitcher registration is overridden the way a merge-enabled node overrides it. * fix(tests): mark RecoverReceiptsBlockchain.Create as hiding
feat(rpc): expose the node's ENR in admin_nodeInfo Nethermind is the only discv5-capable execution client whose admin_nodeInfo omits the node record. Tooling that bootstraps discv5-only networks reads the ENR from this endpoint and has to special-case Nethermind to the enode instead, which is useless once discv4 is disabled. NodeRecordProvider already maintains a signed, sequence-numbered self record, so surface it as an 'enr' field. Move INodeRecordProvider to Nethermind.Network so Nethermind.JsonRpc can reference it without a new dependency on Nethermind.Network.Discovery; the implementation stays in Discovery. The provider is only registered when discovery is enabled, so it is resolved optionally and the field is omitted otherwise.
* Test for EIP-7708 top frame log * [WIP] attach log to parent frame * Attach log to correct frame * Reuse common code in tests * More tests * Naming
…race (storage reads as 0x00) (#12429) * fix(flatdb): warm the trie from persistence only The trie warmer read the recyclable `_snapshots` and `_transientResource` while the warm job held only a `ReadOnlySnapshotBundle` lease, which does not cover them. A concurrent scope reset could recycle those under the running warmer, so a warm read could return a torn or foreign node. The warmer only needs to warm from persistence, so restrict its reads to the trie node cache and the `ReadOnlySnapshotBundle` - exactly what the lease covers. In-memory nodes are already hot and do not need warming. Claude-Session: https://claude.ai/code/session_01S3BG321zDG9BNjAgWhZhCX * fix(flat): warm the transient resource via a per-job lease The trie warmer now warms nodes into the per-job-pinned TransientResource (not just the trie node cache and ReadOnlySnapshotBundle), covered by a single transient lease held for the whole warm traversal. It still never reads the recyclable _snapshots. A warmer read on a torn-down bundle bails to a persistence-only read instead of spinning for a lease that will never land. Claude-Session: https://claude.ai/code/session_01S3BG321zDG9BNjAgWhZhCX * refactor(flat): drop the warmer transient ThreadStatic capture Measured TS-on vs always-per-node-lease on x10 (3 runs, heavy-block warmer load): newPayload 178 vs 178 ms, getProof p99 20.1 vs 20.2 ms, RSS identical. The ambient capture bought nothing, so keep only the per-node lease + ABA re-check (the actual recycle-race fix) and drop the ThreadStatic + the EnterWarmerTransientScope pins in the two warm-job callers. * fix(flat): register the transient return owner at pool checkout - ResourcePool.GetCachedResource now calls OnRented, so every checkout carries a registered return owner; a final ReleaseLease without one throws instead of silently dropping the resource (which leaked the BloomFilter allocation on the public AddSnapshot path) - re-check _isDisposed after the warmer's lease acquire: Dispose releases the owner lease but leaves _transientResource pointing at the recycled instance, so the identity re-check alone could latch a resource already re-rented by another bundle - document why ReleaseLease is distinct from Dispose: the pool contract reserves Dispose for destroying an over-capacity resource - run Nethermind.State.Flat.Test in the nethermind-tests.yml matrix; it was compiled but not run by any CI job - FlatDbManagerTests duplicate-snapshot test asserts the resource lands back in the checkout pool; new ResourcePoolTests cover the final-release return and the unregistered-release throw; refresh stale warmer test comments * fix(flat): pin the transient resource for prewarm dedupe reads ShouldQueuePrewarm read _transientResource without pinning it. The dedupe bloom lives on that recyclable resource and the call runs on prewarmer and BAL threads, so the owner could retire the resource mid-read: the pool Resets it and, on overflow, Disposes the BloomFilter, whose backing store is native memory. Route both overloads through the same lease + ABA re-check the warmer node reads already use, and decline the prewarm on a torn-down bundle. Rename the helper accordingly, since it is no longer warmer-only. The FlatWorldStateScopeProvider and FlatOverridableWorldScope test doubles returned the committed resource to the pool directly instead of releasing its lease, mirroring neither AddSnapshot implementation. That recycles the resource while a warmer lease is outstanding and returns it a second time when that lease is released; the scope provider double also returned the wrong instance and then returned it again on teardown. Both now release the lease, which is the single return-to-pool path. Test changes: - the persistence-only test now commits the written nodes into the bundle's recyclable _snapshots before reading, so the warmer's Unknown result is a genuine miss. Previously the node was still in the transient (SetStateNode writes both) and was itself Unknown, so the assertion held either way. - the churn test gives every epoch its own persisted node instance, so a read served from another epoch's recycled transient is caught by identity rather than by value, drives both recycle paths (CollectAndApplySnapshot swap and Dispose), exercises ShouldQueuePrewarm alongside the node reads, and joins the readers with a bounded wait instead of blocking forever. - new test: a warmer read and a prewarm check on a disposed bundle fall back to the leased persistence reader within a bounded wait, covering the Dispose bail-out deterministically.
… bag (#12672) * fix(jsonrpc): synchronise SubscriptionManager per-client bag The per-client subscription bag is a HashSet mutated and enumerated from multiple threads: concurrent subscribe requests (socket worker tasks), unsubscribe, and the Closed handler that fires on connection teardown. Concurrent HashSet access could corrupt it, dropping a subscription so its event handlers stayed attached and leaked. Lock on the bag for every add, remove, and snapshot-before-dispose. Fixes #12668 * refactor(jsonrpc): dispose client subscriptions under bag lock without snapshot Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(jsonrpc): race unsubscribe path too; drop bag field comment Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: lukasz.rozmej <lukasz.rozmej@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
) * test(rpc): eth_createAccessList affordability with omitted fee fields (execution-apis #854) execution-apis PR #854 clarifies that eth_createAccessList must not fail solely because an unfunded sender cannot afford client-selected default fees when all gas-fee fields are omitted. Nethermind already conforms (verified in hive rpc-compat); this adds a regression test mirroring the conformance fixture: unfunded sender, codeless recipient, zero value, no gas/fee fields -> {"accessList":[],"gasUsed":"0x5208"}. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(rpc): run affordability test on London chain, assert no in-body error, reuse helper Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: lukasz.rozmej <lukasz.rozmej@gmail.com>
…2635) * fix Signed-off-by: jsign <jsign.uy@gmail.com> * Tighten witness RLP JSON encoding --------- Signed-off-by: jsign <jsign.uy@gmail.com> Co-authored-by: jsign <jsign.uy@gmail.com>
…ut (#12681) * perf(state): skip trie warmup for read-only BAL accounts in flat layout With a suggested BAL the block's write set is known upfront, and trie nodes are only needed at commit for written accounts. Gate address trie-warm hints (HintBal, HintGet, HintWarmAccount) on the BAL write set so read-only accounts no longer trigger state-trie path walks. On BAL blocks dominated by cold account reads this removes up to ~8-10 wasted trie-node DB reads per unique read-only account from the measured processing window. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Apply suggestions from code review Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com> * refactor(state): extract QueueStateTrieWarmup and address review findings - Extract the NeedsStateTrieWarmup + PushAddressJob + increment pattern into QueueStateTrieWarmup, used by all three address warmup call sites - Hoist CancelHintBal above the empty-BAL early return so a stale write set never survives into the next block - Drop the inaccurate bloom false-positive-rate comment - Tests: parameterize warm-per-write-kind over balance/nonce/code/storage, add empty-BAL reset regression test, split the HintWarmAccount test, wrap scopes in using, use order-insensitive assertions Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Update src/Nethermind/Nethermind.State.Flat/ScopeProvider/FlatWorldStateScope.cs Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * refactor(test): reuse TestContext for recording-warmer scope construction Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(state): bind BAL warmup gate to HintBal lifecycle; address review findings - Keep _warmupWriteSet across StartWriteBatch: on the parallel BAL path the BAL apply commits mid-block, concurrently with tx workers, so clearing the gate in CancelHintBal collapsed it at a nondeterministic point. The gate is now replaced only by the next HintBal. - Drop the token from Task.Run in both HintBal implementations: a task cancelled before being dequeued never ran the finally that returns the pooled accountChanges array. The body already observes the token. - Align TrieStoreScopeProvider.HintBal with the flat scope: a new hint supersedes the previous one even when it carries no work itself. - Remove the stale prestate-load mutation mention from ReadOnlyBlockAccessList docs, state the immutability invariant, and seal the type. - Tests: pin the gate surviving StartWriteBatch and a second BAL replacing the previous write set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
* test(xdc): drop redundant trailing Assert.Pass in SpecialTransactionsTests * test(core): bound McsLock re-acquire test instead of passing unconditionally SingleThreadAcquireRelease asserted nothing and is subsumed by ReacquireAfterReleaseSucceeds, which now runs on a worker with a timeout so a broken release fails in seconds instead of hanging the test host. * test(flat): assert real postconditions instead of Assert.Pass Add_ConcurrentWithMightContain_ShouldWork now verifies no write is lost under concurrent readers (a bloom filter never false-negatives). DisposeAsync test renamed: FlatDbManager does not dispose the repository (the container owns it); it now asserts bounded drain completion and idempotent double-dispose. ConstructorAcceptsPersistedRepository removed - Not.Null on a fresh object, subsumed by every other test in the file. * test(merge): assert pending-validation cleanup instead of catch-only assertions The memory-leak test asserted only inside catch blocks and ended with Assert.Pass, so it passed whenever nothing threw; it now drives the handler directly and asserts the pending-validation count stays zero across repeated timed-out payloads. The TrySet double-completion test is removed: it was a weaker duplicate of the concurrent-calls test above it (same scenario, but swallowing OperationCanceledException and keying Assert.Fail on exception message text). * test(merge): await header-sync test helpers The helpers were async void and invoked without awaiting - including one un-awaited call inside the other helper - so their assertions raced the test body and failures could surface as host crashes or not at all. * test(network): restore DisconnectsAnalyzer assertions with deterministic flush capture All four tests had their assertions commented out as CI-flaky, leaving them assertion-free. The flakiness came from racing the 10ms flush timer: reports are now recorded while the default 10s interval is in effect and only then is the interval shortened, so a flush cannot fire mid-arrangement. Assertions scan captured flush reports, including that counters aggregate, reset after each flush, and cleared categories do not resurface (the analyzer double-buffers, so a lost clear shows up as a stale count in every other flush, not as a doubled count). * test: address review findings on strengthened tests Will_clear_after_report no longer issues a second report at all - a report can race the flush's enumerate-then-clear window (Timer.Stop does not drain a queued Elapsed callback, so no test-side quiesce is airtight). A lost clear is observable without it: the analyzer double-buffers, so a stale count resurfaces in later flushes, and the test asserts the category appears in exactly one flush across several more (mutation- verified). Bloom capacity raised to 100k - at 10k the saturated filter's ~34% false-positive rate masked a single lost write - and misses are collected into one assertion. The repeated-timeouts payload test is dropped rather than parameterized: the pending dictionary is keyed by block hash and the test resubmits one block, so the count can never exceed one and iterations add no coverage. The FlatDbManager dispose test states its intent with Assert.DoesNotThrowAsync and a corrected comment (WaitAsync bounds the wait, not the drain). Wait timeouts are named constants and polling uses Thread.Sleep(1). * test: simplify comments per ASD-STE100 and drop dead times parameter Comments now use short, active, single-topic sentences. The times parameter of ShouldEventuallyReport had no remaining non-default call site after the second-report removal. * test: use SpinWait.SpinUntil instead of a custom poll helper Keep one condition re-check after a timeout: a flush can land in SpinUntil's final sleep tick.
…lock (#12697) * fix(consensus): stop parallel tx execution once BAL validation rejects The parallel BAL block validator ran the incremental validator alongside the transaction workers, but a validator failure faulted neither the worker loop nor its cancellation token. `ParallelUnbalancedWork` only stops fetching new indices on caller cancellation or a transaction-worker fault, so a block rejected at an early transaction index still executed every remaining transaction before the foreground observed the failure at `GetResult()`. For a block whose invalidity is decided by a cheap prefix, that turns a sub-second rejection into slot-scale CPU work. `IncrementalValidationWorkItem` now owns a cancellation source that is signalled together with the stored exception, and the worker loop runs under that token, so workers stop pulling transactions as soon as validation becomes terminal. The resulting `OperationCanceledException` is translated back into the original validation failure, keeping the returned error identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(consensus): signal BAL validation failure with a flag, not cancellation Routing the stop signal through ParallelUnbalancedWork's cancellation token meant `For` ended by throwing an OperationCanceledException that existed only to be caught and translated back into the original validation failure. The token also needed a per-block CancellationTokenSource, whose recycling in Schedule was only safe because the previous block's validator had already been joined. Replace all of it with a volatile read of the exception the work item already stores: workers check `HasFailed` before doing any work, the loop drains its remaining indices without executing anything, and `GetResult()` reports the rejection on the normal return path. Same behaviour, no exception used as control flow, no extra state to keep in sync, and no ordering constraint between Schedule and the work that follows it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(test): trim comments and simplify the tail-cancellation test Comments cut back to the non-obvious reasoning only. The regression test now counts executions instead of collecting indices, so a single Is.InRange assertion covers both requirements — the decisive prefix ran, the tail did not — and the prefix array, the index bag and one assertion all go away. Transaction count is a plain constant rather than derived from the canonical lead, which the test never depended on: with uniform gas limits the tail sort is stable, so the schedule is natural order regardless. CreateParallelValidationTransactions takes an optional gas limit so the block no longer needs patching after construction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(consensus): address review — exempt iteration 0, loosen test bound Iteration 0 (WaitForBalWarmup + ApplyStateChanges) is now exempt from the HasFailed guard, so pre-execution keeps its previous semantics instead of being droppable when validation fails before any worker starts. Skipping it was traced as benign, but the fix only needs to stop transaction execution, so leaving the pre-execution step alone keeps the behavioural change narrower at no cost. The regression test's upper bound leaned on SpinWait outlasting exception unwinding, because the fake validator releases its gate before the work item stores the exception. Bound is now txCount / 8, which asserts the tail stopped without depending on cancellation-propagation timing — a revert still executes all 2048 and fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ns (#12696) * test(network): remove duplicate eth serializer tests ZeroNewBlockMessageSerializerTests.Roundtrip2 and V63 NodeDataMessageSerializerTests.Zero_roundtrip were verbatim copies of their Roundtrip siblings. Can_deserialize_own_eth_64 shared its body and one payload with Can_deserialize_eth_64; its unique payload moves there as another TestCase. * test(network): pin eth/62-66 wire encodings with hand-derived goldens Every serializer test file in Eth V62-V66 now asserts an exact wire encoding somewhere. The goldens are derived from the RLP rules and verified with an independent encoder (pyrlp + pycryptodome keccak); values shared across files live in EthSerializerGoldens. ToString smoke tests now assert the log line names the message type, and the 32-byte-selector test asserts the exact decoded hash. * test(network): address review feedback on serializer goldens - BlockBodies: pin the null-body framing (c1c0) and the empty-vs-absent withdrawals distinction (e9e8...c0c0) - Status To_string: assert against the independent Protocol.Eth constant - V63 NodeData: rename Roundtrip_with_nulls to Roundtrip_with_empty_entry (the data holds an empty array, not null)
…er) (#12628) * feat(tracing): add EIP-8037 stateGasTracer (execution-apis #852) Implements the `stateGasTracer` named tracer specified in execution-apis PR #852, returning the per-transaction two-dimensional gas summary `{gasUsed, regularGasUsed, stateGasUsed, gasRefund}` (EIP-8037/EIP-7778). The values are already computed for block-level gas accounting and carried on `GasConsumed`; the tracer only reads and formats them. Adds the missing applied EIP-3529 refund (capped) to `GasConsumed.GasRefund`, populated in the success and top-level-halt refund paths. The native tracer factory is threaded with `IReleaseSpec` so the fork is determined explicitly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(tracing): add EIP-8037 two-dimensional gas to callTracer (execution-apis #852) Adds `regularGasUsed`, `stateGasUsed` and `gasRefund` to the callTracer top-level frame for Amsterdam+ blocks, per execution-apis PR #852. The fields are gated on `IReleaseSpec.IsEip8037Enabled` (MUST NOT appear before the fork) and set only on the top frame (omitted on sub-frames). Values are read from the transaction's `GasConsumed` result, matching the stateGasTracer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(tracing): address #12628 review feedback - Restore native-tracer factory API back-compat: keep the public 4-arg GethLikeNativeTracerFactoryDelegate/RegisterTracer/CreateTracer so external plugin registrations stay source- and binary-compatible; built-ins receive the (nullable) IReleaseSpec via an internal spec-aware factory. - stateGasTracer: disable IsTracingOpLevelStorage/IsTracingStack so the terminal-only tracer stops invoking per-opcode storage/stack callbacks. - Consolidate the callTracer top-frame regularGasUsed/stateGasUsed/gasRefund into a single TwoDimensionalGas? value, removing the coupled nullables and the unsafe null-forgiving dereferences in the converter. - Document that regularGasUsed is floor-clamped (block-accounting value) so the two-dimensional invariant is exempted under the calldata floor. - Add an end-to-end stateGasTracer test executing a real Amsterdam tx through the TransactionProcessor (fresh SSTORE + in-tx reset) that exercises field selection and the GasConsumed.GasRefund plumbing; assert regularGasUsed + stateGasUsed == gasUsed + gasRefund. - Test cleanups: multiple-assert scope and a shared helper for the two callTracer Amsterdam cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(tracing): address #12628 re-review nits - Document the spec-less CreateTracer overload's pre-fork fallback and cover it with a factory test (back-compat public contract was untested). - stateGasTracer hex test now forces the ambient NumberConversion.Raw so it actually exercises StateGasTraceConverter's hex-quantity override. - E2E test: use a `using` tracer and correct the refund assertion message (the slot is reset to its original zero value within the tx, not pre-nonzero). - Convert the remaining `//` member comments to XML doc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: remove unused using in NativeStateGasTracerE2ETests Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(tracing): drop excess comments; non-nullable spec in native tracer factory Address flcl42 review on #12628: - Remove the newly added explanatory comments that restated the code (kept only a few essential EIP-referenced DTO docs and the hex-conversion note). - Make the release spec non-nullable through GethLikeNativeTracerFactory and the native tracers: drop the unused spec-less 4-arg CreateTracer overload (the only null source) — RegisterTracer, the actual plugin API, is unchanged. Also drops the brittle regularGasUsed occurrence-count assertion flcl42 flagged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: collapse double blank line before DeepNesting test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…V1 (#12691) * fix(simulate): skip EIP-3607 on the EIP-7928 BAL path in eth_simulateV1 eth_simulateV1 relaxes EIP-3607 so a state-overridden contract can be the tx `from`. That relaxation covered only the main execution (via SimulateTransactionProcessorAdapter). Under glamsterdam (EIP-7928), BlockProcessor runs transactions through the BlockAccessListManager's own tx processors, which bypass the adapter and re-enforce EIP-3607 — so a contract sender is rejected with `-38024 sender has deployed code` instead of reaching the normal balance/fee checks (hive rpc-compat divergence on glamsterdam-devnet-8: ethSimulate-simple-send-from-contract*, ethSimulate-override-address-twice). Relax EIP-3607 on the block execution context in SimulateBlockValidationTransactionsExecutor instead: ParallelBlockValidationTransactionsExecutor sets that context on both the main tx processor and the BAL manager, so both paths skip the check — while BlockProcessor still receives the unwrapped spec, preserving chain-specific release-spec interfaces (Taiko / XDC / Optimism). Verified end-to-end on a glamsterdam-at-genesis chain (the three hive fixtures flip -38024 -> -38014/-38012, matching besu/erigon/reth/geth) and by an integration test through the real EIP-7928 BAL path that fails -38024 without the fix and passes with it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(simulate): preserve PrevRandao in context rebuild; harden 3607 BAL test Address re-review on #12691: - SimulateBlockValidationTransactionsExecutor is now the single context-rebuild funnel for the simulate scope, so preserve the incoming PrevRandao (via BlockExecutionContext.WithPrevRandao*) instead of re-deriving the default — a BlockProcessor subclass (e.g. XdcBlockProcessor) may have supplied a non-default value. - Make eth_simulateV1_contract_sender_skips_eip3607_on_bal_path validation-enabled so the -38014 expectation is fork-independent and stable across the #12692 fix (with validation:false the -38014 relied on the BAL path ignoring NoValidation). Pin the EIP-7928 premise with an explicit BlockLevelAccessListsEnabled assert. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(simulate): forward BlobBaseFee too in the context rebuild Re-review follow-up on #12691: WithPrevRandao recomputes BlobBaseFee from the header, so the no-override branch was newly lossy for a processor that forced a non-derivable value (XdcBlockProcessor sets BlobBaseFee = 0 on a header cloned with ExcessBlobGas = 0; recomputation yields MinBlobGasPrice = 1). Collapse to a single WithPrevRandaoAndBlobBaseFee that forwards the incoming BlobBaseFee (or the block override when present), leaving Spec as the only field the rebuild changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(simulate): relax EIP-3607 via execution flag, not spec wrapping The eth_simulateV1 contract-sender fix relaxed EIP-3607 by wrapping the block execution context spec with WithoutEip3607(), which returns a NoEip3607Spec decorator implementing only IReleaseSpec. Because the tx processors read their spec from that context (TransactionProcessor.GetSpec => BlockExecutionContext.Spec), the decorator reached chain-specific processors that hard-cast it — e.g. TaikoTransactionProcessor.PayFees does (ITaikoReleaseSpec)spec and XdcTransactionProcessor casts on every tx — so eth_simulateV1 on Taiko/XDC threw InvalidCastException/InvalidOperationException instead of returning a result (Eip3607Transition defaults to 0, so the wrap is always active there). Replace the spec wrapping with a dedicated ExecutionOptions.SkipSenderCodeCheck policy flag gated in ValidateSender. The main simulate adapter ORs it into its Process call; the EIP-7928 BAL path receives it via a small BlockAccessListTxExecutionOptions injected into BlockAccessListManager and threaded to its ExecuteTransactionProcessorAdapter. The release spec now keeps its concrete runtime type on every path, so chain-specific interfaces survive. Regression test: Nethermind.Taiko.Test exercises a code-bearing sender through TaikoTransactionProcessor.PayFees — it passes with the flag and throws InvalidCastException under the old spec-wrapping. eth_simulateV1 BAL-path and full simulate suite remain green. Follow-up to #12691; addresses the residual type-erasure raised in its review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(simulate): trim explanatory comments to essentials Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(simulate): carry EIP-3607 relaxation on BlockExecutionContext Replace the ExecutionOptions.SkipSenderCodeCheck flag (and the machinery to thread it into the EIP-7928 BAL path) with a SkipSenderCodeCheck field on BlockExecutionContext. Both the main tx processor and the BAL manager's own processors already share the context via SetBlockExecutionContext, so the field reaches every path for free — no threading through BlockAccessListManager / TxProcessorPool / ExecuteTransactionProcessorAdapter, and no injected policy type. The context is the same shared channel #12691 originally relaxed on; this just uses a flag instead of a spec decorator, keeping the spec's concrete runtime type (so ITaikoReleaseSpec/IXdcReleaseSpec casts survive). Net simpler diff and consistent with the existing IsGenesis flag on the same struct. ValidateSender reads VirtualMachine.BlockExecutionContext.SkipSenderCodeCheck; the simulate executor sets it in its context rebuild. Taiko regression test updated to set the flag on the context (still throws InvalidCastException under spec-wrapping). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(simulate): carry EIP-3607 relaxation on TransactionProcessor Per review preference (@LukaszRozmej), move the SkipSenderCodeCheck flag from BlockExecutionContext onto TransactionProcessor. ValidateSender reads the processor's SkipSenderCodeCheck property. The simulate scope sets it type- preservingly on both creation paths: Intercept<ITransactionProcessor> for the main processor, and a factory decorator for the EIP-7928 BAL processors — so each chain keeps its concrete processor type (no spec wrapping, no processor-type swap). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: trim comments to the essential why Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(simulate): remove dead WithoutEip3607; address review polish Follow-up to the SkipSenderCodeCheck switch (review by @claude on #12691): - Remove the now-dead EIP-3607 spec wrapper — this PR dropped its last production caller. Deletes WithoutEip3607, NoEip3607Spec, GetNoEip3607Spec (std + zkevm) and the _noEip3607Specs cache; keeps WithoutEip158 (still live). Fixes two comments that cited the removed decorator. - Enforce the relaxation invariant: a shared Apply() throws if the resolved ITransactionProcessor isn't a TransactionProcessorBase, instead of silently no-op'ing back to -38024. Used by both the Intercept hook and the factory decorator. - XML-doc SkipSenderCodeCheck with its set-before-use / unsynchronised-read invariant. - Make SkipSenderCodeCheckTransactionProcessorFactory internal. - Revert SimulateBlockValidationTransactionsExecutor to master (the fix no longer touches it; its PrevRandao tweak was unrelated). - Assert fee payment in the Taiko regression test (matches its name). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(dns): verify EIP-1459 subtree hashes EIP-1459 serves every subtree entry from the subdomain base32(keccak256(entry)[..16]). The root signature covers only the enrtree-root entry, so this hash chain is the only binding between the signed root and the branch and ENR records a client consumes. The crawler never checked it, so any resolver or poisoned cache could substitute arbitrary node records and steer the crawl to attacker-chosen labels. Verification follows go-ethereum: unpadded standard base32, prefix comparison against keccak256 of the record, abbreviated labels accepted down to 12 decoded bytes (minHashLength). The tree root is exempt, as EIP-1459 serves it from the bare domain with no hash label. A mismatch logs at Warn and skips the record. No base32 decoder existed in src/Nethermind, so EnrTreeHash carries a small one, tested against labels produced by the reference implementation. Validated against live trees: all.mainnet.ethdisco.net crawls 3000 records with zero rejections, so the check does not misfire on anything the reference publisher emits. * refactor(dns): simplify and harden EnrTreeHash - Reuse Keccak.Size instead of a local MaxHashLength constant. - Decode base32 arithmetically instead of scanning the alphabet string. - Guard the decoded-length check against int overflow on hostile input. - Hash UTF-8 bytes from a stackalloc/pooled buffer instead of allocating. * refactor(dns): log hash-mismatch rejections at Debug, clarify base32 mapping --------- Co-authored-by: lukasz.rozmej <lukasz.rozmej@gmail.com>
…or benchmark workflows (#12708) * benchmarks: selectable dotTrace profiling mode (sampling | tracing | timeline) The dottrace input on run-rpc-benchmarks and run-expb-reproducible-benchmarks becomes a choice: false | sampling | tracing | timeline ('true' stays accepted as a legacy alias for sampling on API dispatches). The mode maps to the dotTrace CLI's --profiling-type: rpc-bench sets it on the wrapped entrypoint in start-node.sh; the EXPB workflow passes --dottrace-mode to expb (only when non-default, so pinned expb versions keep working for sampling runs; requires execution-payloads-benchmarks feature/dottrace-profiling-modes for the new modes). Timeline snapshots cannot be converted to XML by Reporter.exe, so the generate-dottrace-reports and summary jobs skip that mode - the raw .dtp artifact is still collected and uploaded for the dotTrace UI. Line-by-line is deliberately not offered: it needs PDBs the client docker images do not carry. EXPB additionally rejects trace_blocks with timeline, since per-block snapshots ride the MeasureProfiler API, which needs a performance session. * expb workflow: collect a dotnet-trace EventPipe sidecar with every dotTrace run Whenever dottrace is enabled the run also passes --dotnet-trace to expb: a host-side EventPipe session records gc/contention/threading/exception events (no CPU sampler - dotTrace owns the stacks) and the .nettrace ships in the same dottrace-* artifact. Requires expb feature/dottrace-profiling-modes. * rpcbench/expb: document the profiling modes and fix two review nits Docs still described the pre-mode behaviour: the rpc-bench README documented a dottrace command line with no --profiling-type and stated capture deliberately runs in default sampling mode, and its input table listed dottrace as a boolean. AGENTS.md's expb section had the same gap. Both now cover the four choices, when to reach for each, that timeline yields no XML, and the dotnet-trace sidecar - that section is what agents read to interpret run artifacts. Timeline snapshots now save as .dtt rather than .dtp. The report job is gated off for timeline either way, but the extension is what stops Reporter.exe's .dtp glob from picking up a snapshot it cannot convert if that gate is ever relaxed. The expb dottrace flag no longer special-cases sampling: the run already requires an expb that understands --dotnet-trace, so there is no older-expb compatibility left to preserve, and the flag string was spelled out twice in each of two copy-pasted job bodies. The trace_blocks default guard becomes an explicit if - as the last statement of its if body the AND-list left the block with status 1, surviving only through errexit's AND-OR exemption. * docs: scope the EventPipe sidecar to EXPB The rpc-bench README claimed every profiled run drops a .nettrace into the dottrace-rpcbench artifact, but --dotnet-trace was only wired into the EXPB workflow - nothing in run-rpc-benchmarks.yml or scripts/rpc-bench collects one, so the paragraph sent readers hunting for a file that is never produced, and promised it precisely for timeline runs, which have no XML either. Say what a timeline rpc-bench run actually yields: the .dtt snapshot alone.
* fix(jsonrpc): serialize receipt root as full-width DATA * test(jsonrpc): parameterize the receipt-root width cases * test(jsonrpc): pin the whole-byte leading-zero root case
…12699) * test(network): pin eth/71 and snap serializer wire encodings Every serializer test file in Eth V71 and Snap/V1 now asserts an exact wire encoding. The goldens are hand-derived from the RLP rules and verified with an independent encoder (pyrlp + pycryptodome keccak). Random request ids are pinned only in the goldened tests; ByteCodes gains a sibling roundtrip that keeps the constructor-assigned id, and GetAccountRange pins long.MaxValue to keep the eight-byte id path. * test(network): share repeated snap golden fragments The request-id, keccak-of-empty-string, and range-hash fragments moved into SnapSerializerGoldens. Each golden fragment and its test input come from one hex constant, so the expectation cannot drift from the input. * test(network): address review feedback on snap golden tests - ByteCodes: one parameterized Roundtrip; long.MaxValue pins the eight-byte request id deterministically instead of a random id - GetAccountRange: independent field asserts in EnterMultipleScope - SnapSerializerGoldens: expose RequestId1111 next to its RLP item; the remarks state which fragments share hex with inputs and that the keccak("") fragment is an independent literal on purpose
* Test for EIP-7708 top frame log * [WIP] attach log to parent frame * Attach log to correct frame * Reuse common code in tests * More tests * Phantom log test * Remove logs on a reverted frame * Naming * Fix leaking `ArrayPooList` * Code cleanup * Additional test * Simplify tests * Build fix
…12729) * fix(snap): reject storage range responses with unmatched slot lists A `StorageRanges` reply carries one slot list per requested account, but nothing verified that the responder returned no more lists than were asked for. `AddStorageRange` looped over the response count and indexed the request account list with it, so a longer-than-requested reply read past the end of the request. Reject such a response up front and retry the range, which also settles the active storage request accounting so that the range download phase can still reach completion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(snap): pin the slot list count boundary Cover equal and fewer slot lists than requested accounts so that widening the guard to `>=` cannot pass, which would reject every well-formed response. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
core-repository-dispatch-app
Bot
added
agentic 🤖
configuration
database
dependencies
Contributor
|
Claude finished @AnkushinDaniil's task in 9m 33s —— View job Review: merge of
|
| File | PR body says | Actually |
|---|---|---|
TxPool.cs |
took master | ✅ Correct. Master genuinely has no _transactionSnapshot (removed by #12685); all EIP-8141 logic — _expiringFrameTxCount, RemoveExpiredFrameTransactions, the frame filters, blob-pool Inserted/Removed wiring and its Dispose unsubscribe, ContainsTx dual-pool lookup — survives intact, and master's TryRecoverAddress reorg fix (#12757) is picked up. |
BlockProcessorTests.cs |
took master | ✅ Result is better than described — it kept both sides. Installs_predeploy_once_and_captures_it_in_bal (EIP-8141/8250) is still there, re-pointed at the new BalTxProcessorFactory primary ctor, alongside master's new parallel-validation test. |
GethLikeCallTracerTests.cs |
kept both | ✅ Correct, and the NativeCallTracer(tx, spec, options) migration is complete — the only production construction site (GethLikeNativeTracerFactory.cs:33) already passes releaseSpec. |
Also checked and clean:
TraceSimpleTransferActionStartordering inTransactionProcessor.cs— the devnet side carried the pre-Fix EIP-7708 tracing with logs #12577 placement (log before action-start, with a now-stale "keep tracer event order aligned" comment). The merge correctly took master's Fix EIP-7708 tracing with logs #12577 fix that hoists the action-start aboveReportLog. Dropping the devnet comment is right, not a loss.- EIP-8037 rename — all 5
CalculateBlockRegularGas→CalculateBlockExecutionGascall sites migrated, including the frame-tx one. No stale references toCumulativeRegularGasUsed,TryConsumeStateAndRegularGas, orRegularDimensionExceededremain. GasConsumedgained a 6thGasRefundmember as a defaulted positional parameter, so the devnet's existing 5-arg constructions still bind.- Infra (
Directory.Packages.props,.slnx,.github/,scripts/,nix/, Dockerfiles,AGENTS.md) is byte-identical to the master parent — no accidental devnet-side infra edits. The SSH.NET pin arrives via master's chore: update .NET packages #12791, so Green devnet7 CI: pin SSH.NET past the advisory, drop unused usings #12800 is indeed subsumed. - The one
packages.lock.jsondelta vs master (Nethermind.Evm.Precompilesunder Nethermind.TxPool) is the devnet's own dependency, correctly retained. - The three dropped usings are genuinely unused after the merge.
Findings
🔴 High — RlpReader.DecodeAddress() is no longer null-tolerant, and six frame-tx call sites still use it
Master's #12579 (bbe3b7e, Reject invalid fixed-size header RLP) changed the contract:
// before (devnet parent a9e547c)
public Address? DecodeAddress() // 0x80 → null
// after (merge tip)
public Address DecodeAddress() { ReadAddressPrefix(allowNull: false); ... } // 0x80 → throws RlpException
public Address? DecodeAddressOrNull() { ... } // 0x80 → nullReadAddressPrefix(allowNull: false) → TryReadFixedSizePrefix returns false for Rlp.EmptyByteArrayByte, and ThrowAddressDecodeException fires (RlpReader.cs:1037-1064).
Master migrated its own null-tolerant readers to DecodeAddressOrNull() — that is the bulk of the receipt-decoder churn in this PR. The six call sites the devnet branch added were not migrated, and there is no compile error, because Address? x = <Address> is a legal (merely redundant) assignment.
1a. TxFrameDecoder.cs:29 — a frame with an empty target can no longer be decoded
Address? target = decoderContext.DecodeAddress();The file's own XML doc states the contract this breaks:
"An empty target byte string decodes to null (resolves to the transaction sender)."
TxFrame.Target is Address? and the encoder writes writer.Encode(item.Target) unconditionally. So a frame tx containing a sender-targeted frame now round-trips as: encode → 0x80 → decode → RlpException. Any such transaction arriving over devp2p or inside a block body is rejected as malformed rather than executed. Failure scenario: peer sends a well-formed type-6 tx whose first frame has target = b''; TxFrameDecoder.DecodeInternal throws, the tx/block is dropped.
1b. TxFrameSignatureDecoder.cs:31 — a frame signature with an empty signer can no longer be decoded
Address? signer = decoderContext.DecodeAddress();TxFrameSignature.Signer is Address?, and FrameTxValidation.cs:389,392 explicitly branches on signature.Signer is null when counting intrinsic calldata tokens — so a null signer is a deliberately supported decoded state, not a defensive nullable. Same encode/decode asymmetry, same RlpException.
Both should be DecodeAddressOrNull().
Neither site is touched by this PR's diff, which is exactly why it slipped: the breaking change is on master's side of the merge, in a method the devnet files call. I found no test covering an empty target or empty signer, so a green CI run does not clear this.
🟠 Medium — nullable TxReceipt.Payer decoded with the strict reader (3 sites)
ReceiptStorageDecoder.cs:101, CompactReceiptStorageDecoder.cs:71, ReceiptMessageDecoder.cs:109 all do txReceipt.Payer = ctx.DecodeAddress();. Payer is Address?; the encoders write it unconditionally (ReceiptStorageDecoder.cs:190 / :303, Rlp.LengthOf((Address?)null) == 1). BlockReceiptsTracer.cs:168 assigns Payer from _frameTxPayer, which is null unless the processor went through ReportFrameTxReceipt — so a null payer on a TxType.FrameTx receipt is reachable, and it produces a self-inflicted RlpException when the receipt is read back from the DB. Inline comment posted on the ReceiptMessageDecoder hunk.
Separated from the High finding only because the existing test FrameTxReceiptDecoderTests always sets Payer = TestItem.AddressA, so today's happy paths do populate it; 1a/1b have no such backstop.
🟡 Low — dead null-check in FrameTxDecoder.cs:46
transaction.SenderAddress = decoderContext.DecodeAddress() ?? ThrowMissingSender();DecodeAddress() can no longer return null, so ThrowMissingSender() (line 178, "frame transaction sender must be a 20-byte address") is unreachable and a missing sender now surfaces as the generic "Unexpected RLP prefix…". Either switch to DecodeAddressOrNull() to keep the specific message, or delete ThrowMissingSender and rely on the strict reader deliberately.
🟡 Low — stale local name after the EIP-8037 rename
TransactionProcessorBase.FrameTx.cs:366: the comment above was updated to CalculateBlockExecutionGas, but the local is still ulong blockRegularGas. Cosmetic; worth one keystroke while the rename is fresh.
🟡 Low — PR description overstates two resolutions
BlockProcessorTests.cs is described as "took master" when it in fact kept both sides (good — the frame predeploy test survived). Worth correcting so a future reader auditing this merge doesn't go looking for a test that was never dropped.
Verdict
The merge mechanics are careful and the conflict resolutions are correct — including the two that reason about history (#12685's snapshot revert, #12577's tracer ordering) rather than resolving mechanically. The problem is the class of breakage a merge can't surface as a conflict: master tightened RlpReader.DecodeAddress()'s null contract and migrated its own callers, and the frame-tx callers that depend on the old null-tolerant behaviour were left behind with no compile error and no test coverage. Fixing the six sites is a small, mechanical change; the accompanying null-target / null-signer / null-payer round-trip tests are what stop it recurring on the next master merge.
2 findings need a fix before merge (1 High covering 2 call sites, 1 Medium covering 3); 3 Low are optional.
· branch daniil/merge-master-into-devnet7
AnkushinDaniil
marked this pull request as draft
August 13, 2026 13:57
Marchhill
approved these changes
Aug 13, 2026
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## eip8141-frame-txs-devnet7 #12802 +/- ##
============================================================
Coverage ? 0
============================================================
Files ? 0
Lines ? 0
Branches ? 0
============================================================
Hits ? 0
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…rNull Master #12579 split DecodeAddress into a non-null form that throws on 0x80 and a nullable DecodeAddressOrNull. The frame target and frame-signature signer are optional, so the merge left them on the throwing overload.
Marchhill
approved these changes
Aug 13, 2026
Contributor
EVM Opcode Benchmark DiffAggregated runs: base=3, pr=3 No significant regressions or improvements detected. |
AnkushinDaniil
marked this pull request as ready for review
August 13, 2026 15:57
AnkushinDaniil
merged commit Aug 13, 2026
560dae3
into
eip8141-frame-txs-devnet7
465 of 466 checks passed
Contributor
|
Claude finished @AnkushinDaniil's task in 3m 50s —— View job Review: merge of
|
| File | Line |
|---|---|
src/Nethermind/Nethermind.Serialization.Rlp/ReceiptStorageDecoder.cs |
101 |
src/Nethermind/Nethermind.Serialization.Rlp/CompactReceiptStorageDecoder.cs |
71 |
src/Nethermind/Nethermind.Serialization.Rlp/ReceiptMessageDecoder.cs |
109 |
All read txReceipt.Payer = ...DecodeAddress();. TxReceipt.Payer is Address? (TransactionReceipt.cs:94), and every encoder writes it unconditionally — writer.Encode(item.Payer) at ReceiptStorageDecoder.cs:190 and CompactReceiptStorageDecoder.cs:175, sized by Rlp.LengthOf(item.Payer) (which is 1, i.e. 0x80, for null). Post-merge DecodeAddress() is ReadAddressPrefix(allowNull: false) → RlpException on 0x80 (RlpReader.cs:360-364, 1056-1063). So the encode/decode pair is asymmetric: the writer accepts null, the reader rejects what it wrote.
Reachability — narrower than I described in the earlier pass, and worth stating precisely. TransactionProcessorBase.FrameTx.cs:428-446 is the only place a FrameTx receipt is marked, and it calls ReportFrameTxReceipt(payer, …) immediately before MarkAsSuccess/MarkAsFailed; payer there is a non-nullable Address. I also checked that BlockReceiptsTracer is handed to the processor unwrapped in block processing (no CancellationTxTracer wrapping in Nethermind.Consensus), so tracer is IFrameTxReceiptTracer holds and _frameTxPayer is populated on every in-block path. No currently-reachable production failure — I was overreaching in the earlier comment when I called it reachable.
What remains is a latent round-trip hazard, and it's the kind that surfaces later rather than now: any future path that builds a FrameTx receipt through a tracer that doesn't implement IFrameTxReceiptTracer (a composite, a plugin tracer, a test double) writes a null payer to the DB and then throws on read-back — a self-inflicted RlpException on a receipt this node itself wrote. The fix is one word per site, so the cost/benefit is lopsided:
txReceipt.Payer = decoderContext.DecodeAddressOrNull();Confirmed there is no test backstop: FrameTxReceiptDecoderTests.cs:204 always sets Payer = TestItem.AddressA, and :61 only asserts that regular receipts decode a null payer (they skip the frame block entirely). A null-payer FrameTx round-trip is untested in all three codecs.
🟡 Low — FrameTxDecoder.cs:46, dead null-check — still present:
transaction.SenderAddress = decoderContext.DecodeAddress() ?? ThrowMissingSender();DecodeAddress() no longer returns null, so ThrowMissingSender() (:178, "frame transaction sender must be a 20-byte address") is unreachable and a missing sender now surfaces as the generic "Unexpected RLP prefix of 128…". Either switch to DecodeAddressOrNull() to keep the specific message, or drop ThrowMissingSender and keep the strict reader deliberately.
🟡 Low — TransactionProcessorBase.FrameTx.cs:365, stale local name — still present: the comment above and the callee both say CalculateBlockExecutionGas, the local is still ulong blockRegularGas. Cosmetic.
Fresh sweep this pass
- Checked every devnet-only decoder for the other strict/null-tolerant pairs master introduced in Reject invalid fixed-size header RLP #12579 —
DecodeKeccak()vsDecodeKeccakOrNull(),DecodeBloom()vsDecodeBloomOrNull(). No mismatches inTxFrameDecoder,TxFrameSignatureDecoder,FrameTxDecoder, or the three receipt decoders.DecodeAddresswas the only tightened reader with stragglers. DecodeFrameReceipts/EncodeFrameReceipts(ReceiptStorageDecoder.cs:198-245) are symmetric — status, gas, logs, all non-nullable. No equivalent hazard.
Verdict
The merge itself holds up: the conflict resolutions reason about history rather than resolving mechanically, and the High finding was fixed before merge. Two things are worth a small follow-up PR — the three Payer sites (latent, not currently reachable, one-word fix each) and the now-dead ThrowMissingSender. Both deserve the round-trip tests that would have caught them: a null-Payer FrameTx receipt through storage/compact/message codecs, and — since the TxFrameDecoder/TxFrameSignatureDecoder fix landed without them — a null-Target and null-Signer frame round-trip, so the next master merge can't silently re-break the same contract.
Fix this →
· branch daniil/merge-master-into-devnet7
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
19 participants
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.
Forward-ports current
master(54 commits) into the frame-tx devnet base.This advances the shared devnet base so it stops trailing master and picks up the repo-wide fixes (SSH.NET advisory pin, EIP-8037 execution-gas rename, BAL primary-ctor refactor). It is a base bump, not a behaviour change to the frame-tx feature set.
Conflicts resolved by history, not mechanically:
TxPool.cs: took master. The_transactionSnapshotcache the base carried came from perf(engine): overlap the newPayload transactions-root computation with the serial prefix #12515 and master has since reverted it; keeping it would not compile and its concurrency rationale no longer applies to master's non-cachingGetPendingTransactions.BlockProcessorTests.cs: took master.BlockAccessListManageris now a primary constructor takingBalTxProcessorFactory; the old call is dead.GethLikeCallTracerTests.cs: kept both sides (the frame EIP-7708 empty-callstack test and master's Amsterdam two-dimensional gas tests) and moved the frame test onto the new three-argumentNativeCallTracer(tx, spec, options)ctor.Post-merge adaptation: frame-tx processor now calls
Eip8037BlockGasInclusionCheck.CalculateBlockExecutionGas(renamed fromCalculateBlockRegularGasin #12600, identical body). Three unused usings surfaced by IDE0005 dropped so lint stays green.Supersedes #12800 (that PR only pinned SSH.NET and dropped the same usings; both are included here).
Verified locally on the merge tip before pushing:
Nethermind.slnxbuild: 0 warnings, 0 errorsNethermind.TxPool.Test: 718 passed, 0 failedNethermind.Evm.Test(GethLikeCallTracer + FrameTx): 131 passed, 0 failedNethermind.Blockchain.Test(BlockProcessor + FrameTx): 67 passed, 0 failed