fix(giga): fail-fast broadcast_tx_commit and feed newBlockFilter from the notifier - #4012
fix(giga): fail-fast broadcast_tx_commit and feed newBlockFilter from the notifier#4012shemnon wants to merge 2 commits into
Conversation
… the notifier Autobahn has no EventBus wait for inclusion, so BroadcastTxCommit returns unsupported before InsertTx. eth_newBlockFilter reads committed Autobahn hashes from BlockHeaderNotifier instead of /events. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8b5dedf. Configure here.
| for _, fn := range listeners { | ||
| defer recoverAndLog() | ||
| fn(evt) | ||
| } |
There was a problem hiding this comment.
Panic recovery skips later listeners
Medium Severity
defer recoverAndLog() inside the listener loop does not isolate panics per callback. A panic in one listener aborts the loop; remaining listeners never run. TestBlockHeaderNotifier_SubscribePanicDoesNotSkipLaterListeners expects per-listener recovery, which needs an immediately invoked function around each call.
Reviewed by Cursor Bugbot for commit 8b5dedf. Configure here.
# Conflicts: # sei-tendermint/internal/rpc/core/mempool.go
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4012 +/- ##
==========================================
- Coverage 59.67% 58.61% -1.07%
==========================================
Files 2261 2162 -99
Lines 194324 182703 -11621
==========================================
- Hits 115967 107088 -8879
+ Misses 67631 65830 -1801
+ Partials 10726 9785 -941
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The broadcast_tx_commit fail-fast change is sound, but the notifier fan-out added in evmrpc/notifier.go is broken in two ways: publish returns before invoking listeners on the (normal) non-full-channel path, so eth_newBlockFilter receives nothing, and the panic guard uses a function-scoped defer inside the listener loop, so a panicking listener silently skips all later ones. Several of the PR's own new tests should be failing as a result.
Findings: 2 blocking | 1 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- None at the file/PR level.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| // publish pushes evt onto the newHeads channel with overwrite-on-full | ||
| // semantics, then invokes every subscribe listener. Used by both | ||
| // PublishStashed and OnBlockCommitted so the fan-out lives in one place. |
There was a problem hiding this comment.
[blocker] publish never reaches the new listener fan-out on its happy path. The first select still does case n.ch <- evt: return (line 141), so listeners are only invoked when the newHeads channel is full. In production headNotifier is shared between the WS server (whose SubscriptionAPI goroutine continuously drains recv()) and the HTTP FilterAPI, so the channel essentially never fills and appendBlockHash is never called — eth_newBlockFilter / eth_getFilterChanges return [] forever, which is the exact bug this PR set out to fix. On an HTTP-only node it "works" only after the buffer saturates.
The channel write needs to fall through to the fan-out rather than return, e.g. extract the overwrite-on-full send into a helper and call it before the listener loop:
func (n *BlockHeaderNotifier) publish(evt blockHeaderEvent) {
n.sendToHeadChan(evt)
n.notifyListeners(evt)
}This should be caught by the new tests — TestBlockHeaderNotifier_SubscribeGetsEveryEvent, TestBlockHeaderNotifier_SubscribeDoesNotStealFromRecv, all four tests in filter_block_notifier_test.go, and TestFilterBlockFilterAutobahn all publish into a non-full channel and assert the listener saw the event.
| copy(listeners, n.listeners) | ||
| n.mu.Unlock() | ||
| for _, fn := range listeners { | ||
| defer recoverAndLog() |
There was a problem hiding this comment.
[blocker] defer is function-scoped, not block-scoped, so defer recoverAndLog() inside the loop does not isolate each listener. If fn(evt) panics, the panic unwinds all of publish; the deferred recoverAndLog recovers at function exit, but the loop never resumes and every subsequent listener is skipped. It also stacks one defer per listener on the consensus-commit goroutine on every block.
TestBlockHeaderNotifier_SubscribePanicDoesNotSkipLaterListeners asserts precisely the opposite of what this code does and should be failing.
The package already has the right primitive — wrap each call so the recover is scoped to one listener:
for _, fn := range listeners {
runWithRecovery(func() { fn(evt) })
}| // More: https://docs.tendermint.com/master/rpc/#/Tx/broadcast_tx_commit | ||
| func (env *Environment) BroadcastTxCommit(ctx context.Context, req *coretypes.RequestBroadcastTx) (*coretypes.ResultBroadcastTxCommit, error) { | ||
| if env.gigaRouter().IsPresent() { | ||
| return nil, ErrBroadcastTxCommitUnsupported |
There was a problem hiding this comment.
[suggestion] This also changes eth_sendRawTransaction behaviour for operators running with evm.slow = true: evmrpc/send.go:136 routes through BroadcastTxCommit, so on Autobahn every raw EVM transaction now fails with broadcast_tx_commit is not supported on Autobahn and, unlike before, is never submitted (the old path called InsertTx first and only the wait timed out). Worth either routing the slow path to BroadcastTx when Autobahn is present, or calling the change out in release notes so evm.slow isn't left silently dropping transactions.
1 participant
Summary
BroadcastTxCommit(-b block) returnsErrBroadcastTxCommitUnsupportedbeforeInsertTxinstead of waiting on an empty EventBus/KV indexer (CON-352 hang). Comet is unchanged.eth_newBlockFilter/eth_getFilterChangestake Autobahn block hashes fromBlockHeaderNotifier(same FinalizeBlock hash asnewHeads/eth_getBlockBy*), not/events. HTTP FilterAPI now gets the notifier./tx,tx_search, TMsubscribe, and/eventsfail-fast is not in this PR (fork-gated).Linear: CON-409
Related: CON-352 (do not merge EventBus PR 3998 as the fix)
Test plan
go test ./sei-tendermint/internal/rpc/core/ -count=1 -run 'TestBroadcastTxCommit'— Autobahn fail-fast; Comet still hits mempool, not the sentinelgo test ./evmrpc/ -count=1 -run 'TestBlockHeaderNotifier_Subscribe|TestFilterAPI_NewBlockFilter|TestFilterBlockFilter'— notifier fan-out, Autobahn block filter hashes, Comet Events path unchangedseid tx … -b blockagainst an Autobahn node errors immediately;-b syncstill workseth_newBlockFilter+eth_getFilterChangeson Autobahn returns the overlay block hash, not0x000…0Made with Cursor