fix(jsonrpc): post log/block filters for reorg-applied blocks#6819
fix(jsonrpc): post log/block filters for reorg-applied blocks#68190xbigapple wants to merge 1 commit into
Conversation
|
|
||
| // Post the FULL-stream block and logs filters for each block of the new canonical branch | ||
| // (oldest-first), the same way blockTrigger does on the normal path. | ||
| private void reApplyLogsFilter(List<KhaosBlock> newBranch) { |
There was a problem hiding this comment.
[NIT] Document all known gaps between reApplyLogsFilter and blockTrigger
reApplyLogsFilter mirrors only the FULL JSON-RPC filter portion of blockTrigger. Three additional responsibilities that blockTrigger carries are absent on the reorg path, and none of them are recorded in the code:
-
Switch-back (failed-rollback) path is uncovered.
reApplyLogsFilter(first)sits after the for-loop inswitchFork, so an exception thrown byapplyBlockpropagates before this line is reached — intentionally leaving the failed-rollback path without filter notifications. This is correct behaviour, but without a comment a future maintainer changing the exception handling (e.g. adding retry logic) may inadvertently introduce duplicate or missing filter events. -
Event-subscribe triggers are not fired. On the normal extend path
blockTriggeralso callspostBlockTrigger(block)andpostSolidityTrigger(newSolid)(event-subscribe v0), and updateslastUsedSolidityNum(event-plugin v1). None of these happen for blocks applied throughswitchFork. dApps relying on WebSocket event subscriptions will silently miss block and transaction events for every reorg'd block. This is documented in the PR description as a known limitation, but there is no in-code record. -
postSolidityFilteris not called here.blockTriggeralso callspostSolidityFilter(oldSolid, newSolid)for solidity-JSON-RPC-enabled nodes. Solidification events for reorg'd blocks do eventually arrive when the solidification height advances and a subsequentblockTriggercall invokespostSolidityFilteragainst the (now-updated) canonical chain — but this deferred-coverage contract is implicit and unrecorded. -
Future divergence risk. Because
reApplyLogsFilteris an explicit partial mirror ofblockTrigger, any future addition toblockTriggerwill silently be absent from the reorg path. Without a comment linking the two methods, this is easy to miss.
Suggestion: Add a Javadoc or block comment above reApplyLogsFilter listing the out-of-scope items (event subscribe, postSolidityFilter deferred path, switch-back) and noting that this method must be kept in sync with the FULL-filter section of blockTrigger. Add a short inline comment at the reApplyLogsFilter(first) call site noting that it only runs on the successful reorg path.
| } | ||
|
|
||
| private boolean hasFullLogsFilter(BlockingQueue<FilterTriggerCapsule> queue, BlockCapsule b, | ||
| boolean removed) { |
There was a problem hiding this comment.
[NIT] Documentation & test backlog (2 items rolled up)
Grouped as one comment since both are doc-or-test asks with no production-code impact.
-
BlockFilterCapsulenot asserted for the reorg path.reApplyLogsFiltercalls bothpostBlockFilter(false)andpostLogsFilter(false, false), buthasFullLogsFilteronly inspectsLogsFilterCapsuleinstances. TheBlockFilterCapsuleitems produced foreth_newBlockFiltersubscribers are unverified: ifpostBlockFilterwere removed or broken, the test would still pass. Consider adding a helper analogous tohasFullLogsFilterthat checks for aBlockFilterCapsulewith the expected block hash, and asserting it for b1 and b2 after the reorg. -
hasFullLogsFiltername is broader than its implementation. The method name implies it validates the full FULL-node filter pipeline, but the body filters exclusively forLogsFilterCapsule. This can mislead a future reader who might expect it covers block filters too. Consider renaming tohasLogsFilterCapsule, or adding a brief Javadoc clarifying the scope.
Suggestion: (1) Add a hasBlockFilterCapsule(queue, b) helper and assert it for b1/b2 in the reorg test; (2) rename or annotate hasFullLogsFilter to reflect its actual scope.
What does this PR do?
Adds
reApplyLogsFilter(List<KhaosBlock>), invoked afterswitchForkhas fully applied the new branch (oldest-first), mirroringblockTrigger's jsonrpc FULL handling so reorg'd blocks reach the FULL stream like the normal extend path.Why are these changes required?
On a reorg,
pushBlockcallsswitchFork()and returns early, never reachingblockTrigger()— the only place that posts the "added" FULL filters (postBlockFilter/postLogsFilter(block, false, false)). The rollback side withdraws the orphaned branch's logs (reOrgLogsFilter,removed=true), but nothing re-posts the new branch's logs: withdraw old, never add new, violating reorg semantics.So dApps using
eth_newFilter+eth_getFilterChangesmiss every log in blocks applied viaswitchFork.This PR has been tested by:
Follow up
Extra details