stagedsync: mid-block resume follow-ups — prior-receipt reconstruction, notifications, blob gas - #1
Closed
yperbasis wants to merge 1 commit into
Conversation
Follow-ups to the resume-boundary receipt fix: - parallel: re-derive prefix receipts for resumed (partial) blocks so block-end Finalize sees the full set (post-Prague EIP-6110 deposit requests are extracted from receipt logs and checked against header.RequestsHash) and notification dispatch is no longer skipped; blockResult.receiptsComplete gates RecentReceipts.Add - serial: publish the already-reconstructed finalizeReceipts to RecentReceipts for resumed blocks, and seed se.blobGasUsed from ReceiptAsOf so persisted cumulative blob gas does not restart at the batch boundary - parallel: error on a missing in-memory prev receipt instead of silently writing zero offsets; drop the always-true prev-TxIndex clause - drop the now write-only txResult.blobGasUsed field - tests: pin prefix reconstruction and the missing-receipt error; rename the resume test to match what it pins and trim dead setup
Author
|
erigontech#22110 has been merged into erigontech:main, so this follow-up moves to a PR against erigontech/erigon main (same branch, rebased). Link follows below. |
Author
|
Moved to erigontech#22235. |
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.
Follow-up to erigontech#22110 — targets its branch (
fix/parallel-log-index-inconsistency), so merging this lands the changes in that PR rather thanmain. It addresses the remaining findings from the request-changes review and the unresolved Copilot threads, plus two adjacent defects found while re-reviewing.Changes
1. Reconstruct prefix receipts for resumed blocks in the parallel executor
blockExecutor.nextResultpassed only the resumed batch's receipts toengine.Finalize. Post-Prague,Merge.Finalizederives EIP-6110 deposit requests from the receipts' logs and validatesheader.RequestsHashinternally (the!isPartialskip ofblockValidatordoes not cover it), so a mid-block resume of a block whose already-executed prefix emitted deposit logs would reject a valid block asErrInvalidBlock. The serial executor already reconstructs prior receipts for exactly this case viareceipts.DerivePriorReceipts(see erigontech#20452); the parallel block-end path now does the same.DerivePriorReceiptstries RCacheV2 first, so with the receipt cache enabled this is a cache read, not a replay.2. Notifications for resumed blocks (both executors)
With the prefix reconstructed the full receipt set is available, so
RecentReceipts.Addno longer needs to skip resumed blocks — this is the "reconstruct the prefix" option from the parent PR's review point 1, and it 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.blockResultnow carriesreceiptsComplete(true for full blocks and for successfully reconstructed partial ones) and the apply loop gatesAddon it instead of!isPartial. The serial path gets the same completion using its already-reconstructedfinalizeReceipts. If reconstruction fails, behavior degrades to exactly the previous one (warn + skip the notification). This also covers the resume-at-block-end shape from the parent review's point 2, whereReceiptswas empty and the completed block emitted newHeads but no log notifications.3. Fail loudly on a missing in-memory prev receipt
The nil guard added in the parent PR's last commit silently left
cumulativeGasUsed/firstLogIndexat zero — the same corruption class the PR eliminates — wheneverfinalizedResults[tx-1]misses. In-order finalization makes that unreachable today, so it is now an error instead of silent bad data. Falling through toReceiptAsOfwould not be safe there: the DB cannot see this batch's receipts, which are still unflushed inSharedDomains.mem. Also drops thebe.tasks[tx-1].Task.Version().TxIndex >= 0clause — under the outertxVersion.TxIndex > 0gate a block-begin predecessor is impossible, so the clause was always true.4. Serial executor: seed cumulative blob gas on resume
Serial's resume branch discarded
ReceiptAsOf'scumBlobGasUsedwhilese.blobGasUsedrestarts at 0, soApplyTxIndexespersisted under-counted cumulative blob gas for the whole resumed tail — the serial sibling of the write-side fix the parent PR made for parallel, and the source of the values the parallel fallback trusts on a later resume. The pre-resume cumulative is now folded in.5. Cleanup
txResult.blobGasUsedbecame write-only onceApplyTxIndexesswitched tocumulativeBlobGasUsed; the field and its per-published-tx store are removed.TestParallelResumeBoundaryAndNotificationsis renamed toTestParallelResumeBoundaryOffsets(it pins the offsets, not notification dispatch), dead setup is removed (workerCountonly feedspe.run, which the test never calls; thevalidateTasks.inProgress/publishTasks.pendingpresets are re-established bynextResultitself), anddatadir.New(t.TempDir())replaces the hand-rolled temp dir.Tests
TestParallelResumeReconstructsPriorReceipts— new; red on the parent branch (blockResultcarried only the tail receipt), green here: the prefix tx is re-derived, its offsets line up with the DB-fallback values (21000 / 31000), and the block is markedreceiptsComplete.TestParallelFinalizeMissingPrevReceiptErrors— new; red on the parent branch (silent zero offsets, no error), green here.TestParallelResumeBoundaryOffsets— the parent PR's test, still green, plus assertsreceiptsComplete == falsewhen reconstruction is skipped.serialExecutordirectly; they mirror 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.blockGasUsedandApplyCountare still tail-scoped, so enabling it needs counter reconstruction, not just receipts.ReceiptStoresFirstLogIdxpredicate threshold (v1.1 vs v2.0, dating to adjust receipt data files version erigontech/erigon#16677) — separate pre-existing issue.