Skip to content

fix(giga): fail-fast broadcast_tx_commit and feed newBlockFilter from the notifier - #4012

Open
shemnon wants to merge 3 commits into
mainfrom
shemnon/remove-dash-b-block-support
Open

fix(giga): fail-fast broadcast_tx_commit and feed newBlockFilter from the notifier#4012
shemnon wants to merge 3 commits into
mainfrom
shemnon/remove-dash-b-block-support

Conversation

@shemnon

@shemnon shemnon commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Autobahn BroadcastTxCommit (-b block) returns ErrBroadcastTxCommitUnsupported before InsertTx instead of waiting on an empty EventBus/KV indexer (CON-352 hang). Comet is unchanged.
  • eth_newBlockFilter / eth_getFilterChanges take Autobahn block hashes from BlockHeaderNotifier (same FinalizeBlock hash as newHeads / eth_getBlockBy*), not /events. HTTP FilterAPI now gets the notifier.
  • CosmJS-style cosmos /tx, tx_search, TM subscribe, and /events fail-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 sentinel
  • go test ./evmrpc/ -count=1 -run 'TestBlockHeaderNotifier_Subscribe|TestFilterAPI_NewBlockFilter|TestFilterBlockFilter' — notifier fan-out, Autobahn block filter hashes, Comet Events path unchanged
  • Confirm seid tx … -b block against an Autobahn node errors immediately; -b sync still works
  • Confirm eth_newBlockFilter + eth_getFilterChanges on Autobahn returns the overlay block hash, not 0x000…0

Made with Cursor

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ 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.

Comment thread evmrpc/notifier.go
Comment thread evmrpc/notifier.go
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedAug 29, 2026, 4:38 PM

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.23077% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.21%. Comparing base (2a542a2) to head (fe09dc3).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
evmrpc/filter.go 89.18% 2 Missing and 2 partials ⚠️
evmrpc/notifier.go 88.88% 2 Missing ⚠️
app/app.go 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4012      +/-   ##
==========================================
- Coverage   61.26%   60.21%   -1.05%     
==========================================
  Files        2153     2051     -102     
  Lines      188241   176176   -12065     
==========================================
- Hits       115322   106088    -9234     
+ Misses      62147    60303    -1844     
+ Partials    10772     9785     -987     
Flag Coverage Δ
sei-chain-pr 62.10% <89.23%> (?)
sei-db 69.80% <ø> (-0.22%) ⬇️
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
app/abci.go 59.21% <ø> (ø)
evmrpc/send.go 53.19% <100.00%> (+1.41%) ⬆️
evmrpc/server.go 85.16% <100.00%> (+0.07%) ⬆️
evmrpc/tests/utils.go 71.64% <ø> (ø)
sei-tendermint/internal/rpc/core/mempool.go 45.50% <100.00%> (+4.50%) ⬆️
app/app.go 71.29% <50.00%> (+0.06%) ⬆️
evmrpc/notifier.go 96.22% <88.88%> (+0.98%) ⬆️
evmrpc/filter.go 76.17% <89.18%> (+2.26%) ⬆️

... and 104 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

seidroid[bot]
seidroid Bot previously requested changes Aug 25, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread evmrpc/notifier.go
Comment thread evmrpc/notifier.go Outdated
Comment thread sei-tendermint/internal/rpc/core/mempool.go
Autobahn has no EventBus wait for inclusion, so BroadcastTxCommit
returns unsupported before InsertTx. eth_newBlockFilter reads committed
Autobahn hashes from BlockHeaderNotifier instead of /events.
@shemnon
shemnon force-pushed the shemnon/remove-dash-b-block-support branch from 4651d51 to acefef1 Compare August 28, 2026 22:44
shemnon and others added 2 commits August 28, 2026 20:26
BroadcastTxCommit now fail-fasts before InsertTx, which would drop
eth_sendRawTransaction when evm.slow=true. Route that path to
BroadcastTx when Autobahn is present so only -b block breaks.

Co-authored-by: Cursor <cursoragent@cursor.com>
@shemnon

shemnon commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

@seidroid review

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both previously-flagged blockers are genuinely fixed — publish now falls through to the listener fan-out and each listener is individually wrapped in runWithRecovery — and the evm.slow + Autobahn interaction raised last round is handled by the new SendConfig.autobahn flag with tests for both branches. Remaining findings are non-blocking: unbounded per-filter hash accumulation done on the consensus commit goroutine, an Autobahn signal inferred from notifier presence that an in-tree TODO plans to invalidate, and a test that hangs rather than fails on regression.

Findings: 0 blocking | 3 non-blocking | 3 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • None at the file/PR level.
  • 3 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread evmrpc/filter.go
if f.typ != BlocksSubscription {
continue
}
f.blockHashes = append(f.blockHashes, hash)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] blockHashes grows without bound and the growth happens on the consensus commit goroutine.

appendBlockHash is reached from App.CommitPublishStashedpublish, so every committed block does O(#filters) work while holding filtersMu (also taken by cleanupExpiredFilters, NewBlockFilter, and the log branch of GetFilterChanges). Before this PR a BlocksSubscription held only a cursor string and all the work was on the RPC goroutine.

A block filter that is created and never polled accumulates one 32-byte hash per block until the 120s FilterTimeout (evmrpc/config/config.go:331) expires it, and NewBlockFilter is unauthenticated with no cap on filter count and no globalRPSLimiter check. At Autobahn block rates that is a few tens of KB per idle filter, so it is amplification rather than an outright DoS — but it is new memory proportional to (block rate × filters) that nothing bounds.

Capping the slice and dropping the oldest entries on overflow would match the overwrite-on-full philosophy the notifier already documents for recv(), e.g. bound it by a.filterConfig.maxBlock.

Comment thread evmrpc/server.go
globalBlockCache := NewBlockCache(3000)
cacheCreationMutex := &sync.Mutex{}
sendAPI := NewSendAPI(tmClient, txConfigProvider, NewSendConfig(config.Slow, config.EnableSimulation), k, beginBlockKeepers, ctxProvider, homeDir, simulateConfig, app, antehandler, ConnectionTypeHTTP, methodTimeout, globalBlockCache, cacheCreationMutex, watermarks)
sendAPI := NewSendAPI(tmClient, txConfigProvider, NewSendConfig(config.Slow, config.EnableSimulation, blockHeaderNotifier != nil), k, beginBlockKeepers, ctxProvider, homeDir, simulateConfig, app, antehandler, ConnectionTypeHTTP, methodTimeout, globalBlockCache, cacheCreationMutex, watermarks)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] blockHeaderNotifier != nil is used as the "is Autobahn" signal, but the notifier's own construction site says that equivalence is temporary.

app/app.go:694 creates the notifier only when tmConfig.AutobahnConfigFile != "", so this is accurate today. But the comment immediately above it (app/app.go:685-693) describes the plan to feed non-Autobahn newHeads from the notifier too, gated only on encoder-parity work: "Until that's verified, keep this gate so non-Autobahn newHeads semantics are unchanged by this PR."

When that gate is removed, SendConfig.autobahn silently becomes true on Comet nodes and evm.slow stops waiting for inclusion — a behaviour change with no compile error and no failing test to catch it, since TestSendRawTransactionSlowOnCometUsesBroadcastTxCommit constructs SendConfig directly rather than going through NewEVMHTTPServer. (FilterAPI switching to the notifier path on Comet at that point is probably fine or even desirable; the send path is the one that regresses.)

Threading the actual Autobahn flag through, or at minimum a comment here and at app/app.go:694 recording that the two are coupled, would keep the two changes from drifting apart.

)
require.NoError(t, err)

// Setup: empty KV indexer; TimeoutBroadcastTxCommit stays 0 so a wait regression hangs.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Leaving TimeoutBroadcastTxCommit at 0 means a regression hangs the whole package instead of failing this test.

The intent is clear and the comment is explicit about it, but with no timeout and no deadline on t.Context(), a reintroduced InsertTx + event-log wait blocks until the Go test binary's 10-minute panic, which takes down every other test in internal/rpc/core with a stack dump rather than producing a targeted failure.

Setting a short Config.RPC.TimeoutBroadcastTxCommit (or a context.WithTimeout) gives the same detection — a regression returns some other error and require.ErrorIs(err, ErrBroadcastTxCommitUnsupported) fails — while keeping the failure local and fast.

@seidroid
seidroid Bot dismissed their stale review August 29, 2026 17:24

Superseded: latest AI review found no blocking issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant