Skip to content

mempool: don't record a txid as seen until it has been fetched - #609

Draft
skazistp-cpu wants to merge 1 commit into
zcash:masterfrom
skazistp-cpu:fix/mempool-refresh-cancel-poisons-seen
Draft

skazistp-cpu wants to merge 1 commit into
zcash:masterfrom
skazistp-cpu:fix/mempool-refresh-cancel-poisons-seen

Conversation

@skazistp-cpu

@skazistp-cpu skazistp-cpu commented Sep 29, 2026 •

Copy link
Copy Markdown

Closes #615.

When one GetMempoolStream client disconnects during a mempool refresh, the transactions that refresh hadn't reached yet are never delivered to any client until the next block.

Problem

g_txidSeen and g_txList are shared by every mempool stream. refreshMempoolTxns runs on whichever client's goroutine gets there first, using that client's ctx. For each new txid, it:

  1. adds the txid to g_txidSeen, before fetching it
  2. calls getrawtransaction, and on any error does continue (the comment reads "mempool transactions can disappear")

Since #564 (98f38efb, "make RawRequest context-aware"), a cancelled ctx aborts the HTTP request immediately. If the refreshing client goes away partway through, every remaining getrawtransaction fails at once. Each of those txids has already been recorded as seen, so later refreshes, whichever client triggers them, skip them. The other connected wallets never get those pending transactions (incoming payments, for example) until the block that mines them resets the set.

Mobile wallets connect and drop mempool streams constantly. A refresh makes one sequential RPC per new txid while holding g_lock, so there's a real window for this.

Before #564, the same permanent skip already happened to any tx whose fetch hit a transient error, such as a timeout or a busy node.

Fix

A txid is recorded in g_txidSeen only after getrawtransaction succeeds. If the fetch fails because the refreshing client's ctx is done, the refresh stops and returns that error without recording anything. Any other failure leaves the txid unseen:

  • A transaction that really left the mempool won't be listed by the next getrawmempool, so nothing more happens.
  • One that is still listed gets retried on the next refresh, 2 seconds later.

A fetched transaction that fails to parse is still marked seen, as before, so a bad transaction isn't retried every 2 seconds. GetMempool's loop and tip handling are unchanged.

Tests

New file common/mempool_refresh_test.go:

test on master
the refreshing client cancels after the 1st of 3 txids, then another client refreshes returned <nil>, want context.Canceled and mempool list has 1 transactions, want 3
one getrawtransaction fails transiently, then the next refresh runs second refresh has 2 transactions, want 3
known txids are not fetched again passes (a guard for the change)

go test ./... and go test -race ./common/ pass, and gofmt is clean. go test -count=2 ./common/ fails on master as well as on this branch, because the existing tests share state across runs, so I ran every check with -count=1.

Prepared with AI assistance.

— Miroslav, Sebrona

@skazistp-cpu

Copy link
Copy Markdown
Author

For context: f4ef75b fixed the same problem for GetMempoolTx's cache. Its commit message describes the cancelled client's remaining fetches being "swallowed by the 'mempool transactions can disappear' path" and a degraded cache being published. GetMempoolStream's refreshMempoolTxns in common/mempool.go has the same shape and wasn't covered by that change. This PR applies the same idea there.

g_txidSeen and g_txList are shared by every GetMempoolStream client, but
refreshMempoolTxns runs with the context of whichever client's goroutine
happens to do the refresh. It marked each txid as seen before fetching
it, and treated any getrawtransaction error as "the transaction left the
mempool".

Since zcash#564 made RawRequest honor that context, a client that disconnects
during a refresh makes every remaining getrawtransaction fail at once.
Those txids are then recorded as seen and never fetched, so no other
client receives them until the next block. Before zcash#564 the same happened
to any transaction whose fetch hit a transient RPC error.

Mark a txid as seen only once it has been fetched. If the refreshing
client's context is done, stop and return its error. Any other fetch
error leaves the txid unseen, so it is retried if it is still listed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mempool: a client that disconnects mid-refresh hides pending transactions from every client until the next block

1 participant