fix(mempool): tolerate missing/foreign edges on eviction#234
Merged
Conversation
Conflicting (RBF) spends can transiently coexist in the local mempool view across snapshot rounds; add() then overwrites the earlier tx's entry in `edges`, so evicting the earlier tx finds its outpoint entry missing or owned by the conflicting tx. This tripped the eviction assert roughly once an hour on mainnet (electrum.blockstream.info canary, 2026-07-15), taking the whole server down for a recoverable bookkeeping inconsistency. Only remove an edge if the evicted tx still owns it; warn otherwise.
test_rest_address asserted the prefix search returns exactly one address, but other randomly-generated wallet addresses (change outputs) can share the 8-char prefix (~1/1024 per address), failing the run by lottery. Assert addr1 is among the matches instead.
There was a problem hiding this comment.
Pull request overview
This PR prevents Mempool::remove() from panicking when an evicted transaction’s input outpoint is missing from (or no longer owned in) edges, which can legitimately occur when conflicting (RBF) spends transiently overlap across mempool snapshot rounds.
Changes:
- Replaces a strict
assert!on edge removal with conditional removal only when the evicted tx still owns the edge. - Adds a warning log when an edge is missing or owned by a different transaction instead of crashing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses review: avoid the get+remove double hash lookup and split the warning into explicit missing vs conflicting-owner cases.
The conflicting-spend coexistence isn't produced by update() itself (evictions precede additions against one consistent snapshot); the injector is broadcast_raw()/submit_package() adding transactions via add_by_txid(s) outside the sync loop. Point the comment at the real mechanism, drop the dated incident detail (lives in the fix's commit message), and remove a test assert made redundant by the any() below it.
Reproduces the two-step failure exactly as seen in production: 1. tx A is indexed; its replacement B (same outpoint) is injected via the broadcast endpoint while A is still indexed, clobbering A's edges entry. 2. The sync evicting A passed the old assert by STEALING B's edge (any Some() satisfied it), silently corrupting spend lookups for B. 3. When B itself leaves the mempool (confirmed/replaced/expired), its edge is already gone and the old assert panicked with 'missing mempool edge for outpoint', killing the sync loop. Fails on the pre-fix code with that exact panic; passes with the ownership-checked eviction.
Collaborator
|
Seems related to this: I'll open a formal issue for it Edit: Issue is now being tracked here |
|
I just tested this branch and I was able to RBF a TRUC transaction so it fixes the issue 👍 |
This was referenced Jul 16, 2026
Collaborator
replied with more info here: #236 (comment) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The strict
assert!inMempool::remove()(from the eviction-accounting work in 477c1a3) panics when an evicted tx's input outpoint is missing fromedges, which legitimately happens when conflicting (RBF) spends transiently coexist across mempool snapshot rounds: the lateradd()overwrites the earlier tx's entry, so the earlier tx's eviction finds the entry gone or owned by its replacement.This crashed the server about once per hour:
Fix: only remove the edge if the evicted tx still owns it, warn otherwise. The worst case of a clobbered entry (replacement tx's edge stays live) is identical to today's behavior between snapshot rounds, and a warning beats a full restart + mempool re-sync.