feat(event): support rollback for block and transaction triggers#6833
feat(event): support rollback for block and transaction triggers#6833xxo1shine wants to merge 1 commit into
Conversation
… triggers On a chain reorg, BlockLogTrigger/TransactionLogTrigger subscribers previously never learned that an orphaned block's events were rolled back. Add the same "removed" semantics already used by ContractTrigger. - add a `removed` field to BlockLogTrigger/TransactionLogTrigger and the matching setRemoved() on their capsules - Manager (event version 0): refactor postBlockTrigger to emit a single real-time block (with removed), move the solidified-mode batch into postSolidityTrigger, add reOrgBlockTrigger to re-emit erased blocks with removed=true, rename reApplyLogsFilter to reApplyBlockEvents and re-emit the re-applied fork branch's block/tx triggers (forward), thread removed through processTransactionTrigger/postTransactionTrigger - RealtimeEventService (event version 1): post block/tx triggers synchronously to the plugin with the removed flag instead of the async triggerCapsuleQueue, avoiding overwrite of the shared cached capsule; drop the unused manager field - tests: capsule removed passthrough, version-0 emit (testReOrgBlockTriggerRemoved), rewritten RealtimeEventServiceTest, updated ManagerTest stub
xxo1shine
force-pushed
the
feat/event-block-tx-trigger-rollback
branch
from
b5787ea to
db801cc
Compare
| exception = e; | ||
| throw e; | ||
| } finally { | ||
| if (exception != null) { |
There was a problem hiding this comment.
[MUST] The failed switch-back path is still incomplete for block/transaction triggers. The erase loop above already emits removed=true for the old branch via reOrgBlockTrigger(), but if applying the new branch fails, this branch only reapplies the old blocks and never re-emits their block/tx triggers forward. Unlike ContractTrigger, block/tx triggers are not replayed during applyBlock(), and reApplyBlockEvents(first) only runs after the new branch is applied successfully. That leaves subscribers stuck in an incorrect "old branch was rolled back" state. Please replay block/tx triggers forward when restoring the old branch in the switch-back path (e.g. reApplyBlockEvents(second) after the re-apply loop) — this also closes the same pre-existing gap for JSON-RPC filters.
There was a problem hiding this comment.
@waynercheung, confirmed that the switch-back (failed-fork) path currently does not re-emit block/tx triggers. This is intentional — it keeps block/tx triggers consistent with the existing JSON-RPC filter rollback logic: reApplyLogsFilter (renamed to reApplyBlockEvents in this PR) has always been invoked only after the new branch is applied successfully, and the switch-back path never replays filters. So this is a pre-existing gap that equally affects JSON-RPC filters, not a regression introduced here.
The scope of this PR is to align block/tx trigger rollback with the existing mechanisms (ContractTrigger / JSON-RPC filter). To keep the change minimal and behavior consistent, this pre-existing switch-back scenario is intentionally left out of this PR; I'd suggest addressing it as a separate change that fixes block/tx triggers and JSON-RPC filters together, rather than landing a partial fix here.
| logger.warn("BlockLogTriggerCapsule is null. {}", blockEvent.getBlockId().getString()); | ||
| } else { | ||
| manager.getTriggerCapsuleQueue().offer(blockEvent.getBlockLogTriggerCapsule()); | ||
| blockEvent.getBlockLogTriggerCapsule().setRemoved(isRemove); |
There was a problem hiding this comment.
[SHOULD] This synchronous processTrigger() path removes the per-trigger exception isolation that block/tx triggers previously had in triggerCapsuleQueue (the queue consumer wraps each processTrigger() in catch(Throwable)). If any plugin listener throws from handleBlockEvent/handleTransactionTrigger, flush() aborts and the remaining tx/contract events for the same block are skipped — and the event was already polled off the queue, so they are lost, not retried. Please add a local try/catch around each trigger post here.
Note the contract-trigger posts below (lines 104-131) were already synchronous without per-trigger isolation, so to stay consistent please apply the same guard to all three (block/tx/contract) here rather than block/tx only.
There was a problem hiding this comment.
@waynercheung, I'd prefer to leave this without a per-trigger guard. The synchronous post path (postBlockTrigger/postTransactionTrigger) only does two things: serialize the trigger to JSON and hand it off — in native-queue mode it publishes to the message queue, in plugin mode the listener enqueues the event into the plugin's own internal queue (non-blocking, no real I/O; the code even notes "processTrigger() only enqueues events into the plugin's internal queue without blocking on actual I/O"). This step does not throw a business exception, so flush() won't abort mid-way on a single post and drop the remaining events of the same block.
Also, the contract-trigger posts below (lines 104-131) already have no per-trigger guard, so adding one only for block/tx would be inconsistent. So I'd leave it out of this PR; if a listener implementation that can throw at post time is introduced later, it would be better to add the guard to all three consistently then.
| } | ||
|
|
||
| @Test | ||
| public void testReOrgBlockTriggerRemoved() throws Exception { |
There was a problem hiding this comment.
[SHOULD] This test validates removed-flag passthrough, but it does not cover the real switchFork -> reOrgBlockTrigger -> apply failure -> switch-back flow. The fragile part here is the reorg sequencing and restore semantics, not the setter itself. Please add an integration-style test that exercises a failed fork switch and asserts the old branch event sequence end-to-end (switchBack() at line 1044 and switchForkShouldPostFullNodeFilterForNewBranch() at line 1693 already provide a real pushBlock harness to build on).
There was a problem hiding this comment.
@waynercheung, I'd prefer not to add a dedicated integration test in this PR. The reorg control flow is unchanged here — the switchFork erase/apply/switch-back sequencing stays as is, and the block/tx triggers only hook into two existing points: reOrgBlockTrigger mirrors the already-tested reOrgContractTrigger one-to-one, and the forward re-emit reuses the same reApplyBlockEvents method (which also handles the JSON-RPC filters, whose end-to-end behavior on this reorg path is already covered by switchForkShouldPostFullNodeFilterForNewBranch).
The genuinely new behavior is just the removed-flag passthrough and the emit direction on erase/re-apply, which the added unit tests already assert (reOrgBlockTrigger / reApplyBlockEvents / postBlockTrigger with removed=true/false). A full fork-switch integration test would mainly re-exercise control flow that is already covered. As for the failed switch-back path, as discussed it's intentionally out of scope for this PR (kept consistent with the JSON-RPC filters), so its end-to-end sequence isn't asserted here either. On balance I'll leave the integration test out of this PR.
2 participants
What does this PR do?
On a chain reorg, BlockLogTrigger/TransactionLogTrigger subscribers previously never learned that an orphaned block's events were rolled back. Add the same "removed" semantics already used by ContractTrigger.
removedfield to BlockLogTrigger/TransactionLogTrigger and the matching setRemoved() on their capsulesWhy are these changes required?
This PR has been tested by:
Follow up
Extra details