stagedsync: Fix logIndex reset and missing websocket notifications in parallel execution - #22110
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes receipt log-index continuity and websocket log notification dispatch in the staged sync parallel execution path, addressing incorrect logIndex values observed by eth_getLogs when execution resumes mid-block across batches/restarts.
Changes:
- Fix receipt finalization when a batch starts mid-block by loading prior receipt state from
ReceiptDomainviarawtemporaldb.ReceiptAsOf. - Adjust receipt notification enqueue logic to (intended) allow websocket log subscribers to receive notifications when a block completes even if execution started in a partial batch.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e1866c1 to
54213ad
Compare
97116e5 to
62e28e2
Compare
we don't "execute blocks incompletely". State snapshots can end at midblock however and so execution needs to resume from middle of the block. |
|
one weird thing:
but it works out in end logs is empty, so currentReceipt.FirstLogIndexWithinBlock gets set with the right value.. I'd suggest adding a comment about this
|
|
You are totally right that I’ve added a clear code comment explaining this mathematical trick. Thanks for catching this! |
62e28e2 to
51a09e2
Compare
yperbasis
left a comment
There was a problem hiding this comment.
The receipt-index reconstruction itself checks out, but a few things need fixing before merge:
-
Partial blocks now notify with tail-only receipts (
exec3_parallel.go:579). For anisPartialblock,blockResult.Receiptscontains only the resumed tail (built frombe.results; the task producer skips the prefix), whileTxs/Headerare the full block's.RecentReceipts.Addstores this as the block's complete set, soeth_subscribelog/receipt clients get the tail presented as the whole block — diverging frometh_getLogs, replayingRemovedon unwind for the tail only, and bypassing the!isPartial-gated receipts-root/bloom validation. Please either reconstruct the prefix (receipts.DerivePriorReceipts, as the serial path does forFinalize) or keep skipping partial blocks like serial does (startTxIndex == 0, exec3_serial.go:421); publishing an incomplete set is worse than either. -
The resume-at-block-end offset still gets zero notifications (
exec3_parallel.go:2505). If the boundary falls between the last user tx and the block-end txNum, the resumed executor's only task is block-end:Receiptsis empty,Addearly-returns, and the completed block emits newHeads but no logs/receipts — the #22106 symptom remains. The fallback also runs for block-end tasks even thoughfinalizeSystemTxnever consumesprevReceipt; please excludeIsBlockEndtasks. -
The legacy-schema check can never fire (
exec3_parallel.go:2506).ReceiptStoresFirstLogIdxtestsCurrentDomainVersion < v1.1, but the exec stage only ever sees{3,0}(schema default) or{1,1}(AdjustReceiptCurrentVersionIfNeededpins legacy datadirs to exactly v1.1), so the guard is dead code — while 3.0-written data tails still pass through and get exactly the shifted indexes the error message claims to prevent. The underlying problem is the predicate threshold (arguablyLess(V2_0); dates to #16677) — better to fix that than ship a check that suggests protection it doesn't provide. -
be.blobGasUsed = cumBlobGasUsedseeds from a slot the parallel executor writes per-tx, not cumulative (exec3_parallel.go:2525): the publish loop passestx.GetBlobGas()asApplyTxIndexes'cummulativeBlobGasarg (exec3_parallel.go:2740), unlike serial which passes the running cumulative (exec3_serial.go:556). On parallel-synced datadirs the seed is the previous tx's own blob gas (0 for non-blob txs). Latent today because the validator is skipped for partial blocks, but wrong — fix the write side to store the running cumulative, or drop the seed. -
Shape: branch explicitly on
tx == 0instead ofprevReceipt == nil(the nil case is otherwise impossible; if that invariant ever breaks, this silently reconstructs from pre-batch DB state instead of failing loudly), and consider passing the two scalars throughfinalizeinto the existingCreateReceiptlike serial does (exec3_serial.go:452-461) —finalizehas a single call site — rather than a synthetic receipt whoseFirstLogIndexWithinBlockholds the next tx's index. That also removes the need for the 7-line comment (which restatesCreateNextReceipt's math and already drifts from it); the// Initialize cumulative blob gas...comment just restates the assignment and can go. -
Per repo guidelines (CLAUDE.md, TDD for bug fixes), please add a test pinning the resume boundary (first task with
TxIndex > 0at slice index 0 → reconstructed receipt values) and the notification behavior — nothing currently prevents a regression of the #22106 corruption.
548f3be to
190a5aa
Compare
|
Hi @yperbasis, Thanks for the detailed feedback. I have gone through all your points and done the needful changes. Please find the updates below:
Please review and let me know if it looks good now. |
Head branch was pushed to by a user without write access
2e23ca9 to
484611a
Compare
… parallel execution (#22110) ### Issue When Erigon is doing parallel block execution (`EXEC3_PARALLEL=true`), the state snapshots can end at step boundaries which fall mid-block. So when we restart the node or resume sync from these snapshots, the execution stage needs to start/resume execution from the middle of the block (running only the remaining transactions of that block). While finalizing the transaction receipts for this partial block execution, the code in `exec3_parallel.go` was checking `if txVersion.TxIndex > 0 && tx > 0` to fetch the previous transaction's receipt from the memory results map. But for the first transaction task of this new batch (where the local slice task index `tx` is `0`, but the actual block-level index `txVersion.TxIndex` is `> 0`), this check was failing and `prevReceipt` remained `nil`. Because of this, it called `CreateNextReceipt(nil)` and the starting log index and cumulative gas for that mid-block transaction got reset to `0`. This wrong log index got written directly to the database `ReceiptDomain` and caused inconsistent `logIndex` values when clients queried `eth_getLogs`. Additionally, for partial blocks the websocket notification path was publishing tail-only receipts (only the resumed portion) via `RecentReceipts.Add`, which `eth_subscribe` log/receipt clients would receive as the complete block — diverging from `eth_getLogs` results. ### Fix The main fix is in the parallel executor's receipt finalization loop in `exec3_parallel.go`. We changed the `finalize` function signature to accept `cumulativeGasUsed` and `firstLogIndex` directly instead of a `prevReceipt` pointer, and it now calls `CreateReceipt` instead of `CreateNextReceipt`. This matches how the serial executor handles receipt creation. We also branch explicitly on `tx == 0` (batch boundary) instead of checking `prevReceipt == nil`. When the first task in a batch has `TxIndex > 0` (meaning we are resuming mid-block), we query the temporal database using`rawtemporaldb.ReceiptAsOf` to fetch the previous transaction's cumulative gas used, cumulative blob gas used, and log index offset. We excluded `IsBlockEnd` tasks from this fallback since `finalizeSystemTx` never consumes `prevReceipt` anyway. Also added a two-step nil check on `be.finalizedResults[tx-1]` before accessing `.Receipt` since `finalizedResults` is a map and a missing key would return nil and cause a panic. For the notification issue, we gated `RecentReceipts.Add` behind `!applyResult.isPartial` and moved it outside the block validation scope so that incomplete tail-only receipts are not published for partial blocks. We now use `applyResult.Txs` and `applyResult.Header` directly instead of fetching from the database. This is consistent with how the serial path gates behind `startTxIndex == 0` in `exec3_serial.go:421`. For the blob gas issue, we added a `cumulativeBlobGasUsed` field to `execResult` and snapshot `be.blobGasUsed` into it at finalization time. The publish loop now uses this per-result snapshot instead of reading `be.blobGasUsed` directly, which could have been overwritten by a later transaction. We also initialize `be.blobGasUsed` from the database `cumBlobGasUsed` value at the resume boundary. Added `TestParallelResumeBoundaryAndNotifications` unit test to pin theresume boundary receipt reconstruction (first task with `TxIndex > 0` at slice index 0 → correct offsets from DB) and the `isPartial` flag that controls notification skipping. Closes: #22106 (cherry picked from commit a01ede3)
…s, blooms, blob gas (erigontech#22235) Follow-up to erigontech#22110. Completes mid-block resume handling in both executors and extends the erigontech#21332 receipt-bloom reuse to the remaining call sites. ## Changes ### 1. Prefix receipts for resumed blocks (parallel executor) `blockExecutor.nextResult` passed only the resumed batch's receipts to `engine.Finalize`. Post-Prague, `Merge.Finalize` derives EIP-6110 deposit requests from the receipts' logs and validates `header.RequestsHash` internally (the `!isPartial` skip of `blockValidator` does not cover it), and AuRa's `Finalize` reads receipt logs unconditionally (epoch-end signalling) — so a mid-block resume of a block whose already-executed prefix emitted relevant logs would reject a valid block as `ErrInvalidBlock`. Both executors now re-derive the prefix receipts through a shared `txExecutor.reconstructPriorReceipts` helper wrapping `receipts.DerivePriorReceipts` (RCacheV2 read first, replay fallback), so `Finalize` sees the full set. A reconstruction failure fails the batch with a retryable error rather than proceeding into a receipts-dependent `Finalize` that would misclassify the valid block as invalid (parallel: a false INVALID verdict to the CL plus bad-header-LRU poisoning; serial: a `BadBlock` unwind). Serial's plain-error path unwinds with `ExecUnwind` and self-heals by re-executing the block from its start. ### 2. Notifications for resumed blocks (both executors) With the prefix reconstructed the full receipt set is available, so resumed blocks now reach `RecentReceipts.Add`: `blockResult` carries `receiptsComplete` (true for full blocks, successfully reconstructed partial ones, and resumes at tx index 0) and the apply loop gates `Add` on it instead of `!isPartial`; the serial path mirrors this with `priorComplete`/`finalizeReceipts`. This delivers the remaining half of erigontech#22106 — the notification dispatcher has no DB fallback, so a block absent from the cache never reaches websocket log/receipt subscribers — including the resume-at-block-end shape, where the completed block emitted newHeads but no log notifications. Published partial-block sets are completed via `receipts.DeriveFields`, which fills `Bloom` when missing: the post-exec block validator — otherwise the only bloom filler on this path — is skipped for partial blocks, so resumed-block receipts would reach `eth_subscribe("transactionReceipts")` / `eth_sendRawTransactionSync` with `logsBloom = 0x0`. ### 3. Fail loudly on a missing in-memory prev receipt A missing `finalizedResults[tx-1]` receipt in the parallel finalize path is an error instead of silently persisting zero `cumulativeGasUsed`/`firstLogIndex` — the corruption class this PR eliminates. In-order finalization makes it unreachable today. Falling through to `ReceiptAsOf` would not be safe there: the DB cannot see this batch's receipts, which are still unflushed in `SharedDomains.mem`. Non-chain task types (test/benchmark tasks) are excluded from the offsets lookup — `finalize()` legitimately creates no receipts for them. ### 4. Serial executor: seed cumulative blob gas on resume Serial's resume branch folds `ReceiptAsOf`'s pre-resume `cumBlobGasUsed` into `se.blobGasUsed` (which restarts at 0), so `ApplyTxIndexes` persists correct cumulative blob gas for the whole resumed tail — matching the parallel executor's boundary seeding and producing the values a later resume's fallback reads. ### 5. Receipt-bloom reuse (extends erigontech#21332) Per-receipt blooms are computed once and reused instead of re-hashing the same logs: - `types.Receipts.MergedBloom()` ORs per-receipt blooms. - `ExecuteBlockEphemerally` derives the block bloom via `MergedBloom` — `ApplyTransaction` populated every receipt's bloom, so `CreateBloom(receipts)` hashed all logs a second time. Mainly benefits the verify snapshot/history re-execution tools and t8n. - `ethutils.MarshalReceipt` reuses `receipt.Bloom` when set — previously every `eth_getTransactionReceipt` / `eth_getBlockReceipts` response re-hashed each receipt's logs — and hashes logs only for sources that leave it unset. - `receipts.DeriveFields` fills `Bloom` only when missing, so replay-produced receipts are not re-hashed. `NewBlock`'s `CreateBloom(receipts)` stays as-is: its callers do not guarantee per-receipt blooms. `DefaultBlockPostValidation` already carries erigontech#21332. ### 6. Cleanup - `txResult.blobGasUsed` was write-only; removed. - The resume tests share `newResumeTestDB` / `seedResumeTestDB` / `newResumeTestExec` helpers instead of ~45 duplicated scaffold lines each. ## Tests - `TestParallelResumeReconstructsPriorReceipts` — prefix receipts re-derived with correct offsets (21000 / 31000); block marked `receiptsComplete`. - `TestParallelResumeReconstructionFailureErrors` — reconstruction failure fails the batch instead of degrading. - `TestParallelFinalizeMissingPrevReceiptErrors` — missing prev receipt errors instead of zero offsets. - `TestParallelResumeBoundaryOffsets` — resume-boundary `ReceiptAsOf` fallback offsets; `receiptsComplete == false` when reconstruction is skipped. - `TestDeriveFieldsSetsBloom`, `TestDeriveFieldsPreservesExistingBloom`, `TestMarshalReceiptReusesReceiptBloom`, `TestReceiptsMergedBloom` — bloom fill and reuse. - The serial half of items 1, 2 and 4 has no unit harness that drives `serialExecutor` directly; it mirrors the tested parallel logic. Flagging that explicitly rather than pretending coverage — suggestions welcome. ## Deliberately not addressed - `blockValidator` (receipts-root/bloom) stays skipped for partial blocks: `be.blockGasUsed` and `ApplyCount` are still tail-scoped, so enabling it needs counter reconstruction, not just receipts. Tracked in erigontech#22237. - The notification dispatcher still has no fallback for cache misses it can hit for other reasons (eviction, fork-validator `Clear`); a dispatcher-side regeneration would be the deeper fix for those classes. Tracked in erigontech#22240. - The `ReceiptStoresFirstLogIdx` predicate threshold (v1.1 vs v2.0, dating to erigontech#16677) — separate pre-existing issue. - Datadirs written by the pre-erigontech#22110 parallel executor hold per-tx values in the cumulative blob-gas column; nothing migrates them.
Issue
When Erigon is doing parallel block execution (
EXEC3_PARALLEL=true), the state snapshots can end at step boundaries which fall mid-block. So when we restart the node or resume sync from these snapshots, the execution stage needs to start/resume execution from the middle of the block (running only the remaining transactions of that block).While finalizing the transaction receipts for this partial block execution, the code in
exec3_parallel.gowas checkingif txVersion.TxIndex > 0 && tx > 0to fetch the previous transaction's receipt from the memory results map. But for the first transaction task of this new batch (where the local slice task indextxis0, but the actual block-level indextxVersion.TxIndexis> 0), this check was failing andprevReceiptremainednil. Because of this, it calledCreateNextReceipt(nil)and the starting log index and cumulative gas for that mid-block transaction got reset to0. This wrong log index got written directly to the databaseReceiptDomainand caused inconsistentlogIndexvalues when clients queriedeth_getLogs.Additionally, for partial blocks the websocket notification path was publishing tail-only receipts (only the resumed portion) via
RecentReceipts.Add, whicheth_subscribelog/receipt clients would receive as the complete block — diverging frometh_getLogsresults.Fix
The main fix is in the parallel executor's receipt finalization loop in
exec3_parallel.go. We changed thefinalizefunction signature to acceptcumulativeGasUsedandfirstLogIndexdirectly instead of aprevReceiptpointer, and it now callsCreateReceiptinstead ofCreateNextReceipt. This matches how the serial executor handles receipt creation. We also branch explicitly ontx == 0(batch boundary) instead of checkingprevReceipt == nil. When the first task in a batch hasTxIndex > 0(meaning we are resuming mid-block), we query the temporal database usingrawtemporaldb.ReceiptAsOfto fetch the previous transaction's cumulative gas used, cumulative blob gas used, and log index offset. We excludedIsBlockEndtasks from this fallback sincefinalizeSystemTxnever consumesprevReceiptanyway. Also added a two-step nil check onbe.finalizedResults[tx-1]before accessing.ReceiptsincefinalizedResultsis a map and a missing key would return nil and cause a panic.For the notification issue, we gated
RecentReceipts.Addbehind!applyResult.isPartialand moved it outside the block validation scope so that incomplete tail-only receipts are not published for partial blocks. We now useapplyResult.TxsandapplyResult.Headerdirectly instead of fetching from the database. This is consistent with how the serial path gates behindstartTxIndex == 0inexec3_serial.go:421.For the blob gas issue, we added a
cumulativeBlobGasUsedfield toexecResultand snapshotbe.blobGasUsedinto it at finalization time. The publish loop now uses this per-result snapshot instead of readingbe.blobGasUseddirectly, which could have been overwritten by a later transaction. We also initializebe.blobGasUsedfrom the databasecumBlobGasUsedvalue at the resume boundary.Added
TestParallelResumeBoundaryAndNotificationsunit test to pin theresume boundary receipt reconstruction (first task withTxIndex > 0at slice index 0 → correct offsets from DB) and theisPartialflag that controls notification skipping.Closes: #22106