Repository navigation
fix(event): keep event notifications consistent with committed blocks - #25
0xbigapple wants to merge 1 commit into
Conversation
Contract triggers were posted from processTransaction before a block's outcome was known, so a block that failed after executing transactions, or a candidate branch that failed to switch, leaked forward events with no matching removal. Post them only after the block is committed: from blockTrigger on the regular path, and after the whole branch is applied on a fork switch. After a failed switch, re-emit the restored branch's forward notifications (contract triggers, logs filter, block/transaction triggers); skip the block filter, whose original notification was never removed. If the restore itself fails partway, re-emit the already recommitted prefix before propagating the failure so it is not left withdrawn. A notification failure after commit requests a node exit, as blockTrigger already does. Share one per-block helper between the forward and reorg-removal paths, which also guards the removal path against a transaction loaded without a trace after a restart.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
What does this PR do?
blockTriggeron the regular path, and after the whole candidate branch has been applied on a fork switch. They stay ahead of the block trigger, so the order seen by the event plugin is unchanged.blockTriggeralready does: the ledger is committed and the notification cannot be repaired, so the node requests exit. On the restore path the original switch exception is attached as suppressed.Why are these changes required?
Contract triggers were posted at the end of
processTransaction, as each transaction executed. Anything that throws later insideapplyBlocklands in thecatch (Throwable)ofpushBlock, which rolls back the session, drops the block fromkhaosDband rethrows. Nothing there withdraws the triggers already posted for the block's earlier transactions. A fork switch has the same gap: when a later block of the candidate branch fails,switchForkerases the applied blocks but their triggers stay published. Subscribers were left holding forward events (removed=false) for blocks that never became canonical, with no removal to follow.The restore path had the mirror problem, missing events instead of extra ones. Erasing the previous branch withdrew its logs filter events and block/transaction triggers (
removed=true). Re-applying it went throughapplyBlock, which re-posted contract triggers fromprocessTransaction, butreApplyBlockEventsruns only when the candidate branch succeeds, so the restored branch's logs filter events and block/transaction triggers were never published again.This PR has been tested by:
ManagerTestpushBlockShouldPostContractTriggersOnlyAfterCommit: through the real VM, a block that fails after executing a contract deployment posts nothing; the committed block posts one forward trigger. Fails on the previous code.switchForkShouldReemitOldBranchEventsAfterFailedCandidateSwitch: the old head's logs filter and contract trigger go forward, removed, restored; its block filter is not republished; the failed candidate blocks emit nothing;eth_getFilterChangesreturns the removal, then the restoration.switchForkShouldReemitRestoredPrefixEventsWhenRestoreFails: through the real VM, a two-block old branch whose restore fails on the second block; the first block's contract and logs filter events are withdrawn then republished, its block filter is not, and the failed blocks emit nothing. Fails on the previous code.testPostContractTriggersUsesAppliedBlockTraceAndCapturedSolidifiedHeight,testPostForkBranchEventsRequestsExitOnReplayFailure,postContractTriggersSkipsTransactionsWithoutTrace: the deferred-trigger helper on its own (block hash and solidified height, exit on replay failure, skipping a transaction without a trace).Follow up
None.
Extra details
Deferred triggers are rebuilt from the transaction traces kept on the applied block, so nothing is copied while the outcome is unknown. The solidified block number attached to them is captured before the block is applied, which is the value the immediate path reported too:
processTransactionruns before the consensus step that advances it. The forward and reorg-removal paths share one per-block helper, so the removal path now also skips a transaction loaded without a trace (a block read from the database after a restart) instead of dereferencing null.processTransactionno longer reads the block's merkle root, soManagerMockTestmoves its timing stub to another call to keep covering the slow-transaction log branch.Summary by cubic
Fixes event notifications so subscribers only see events for blocks that actually become canonical: contract triggers are deferred until commit, and a failed fork switch now republishes the restored branch's events.
blockTriggeron the regular path and after the whole branch is applied on a fork switch.blockTriggeralready handles it.Written for commit c660c2d. Summary will update on new commits.