fix(mempool): reconcile conflicts before insertion

[EddieHouston]

Summary

Prevent submitted replacement transactions from temporarily coexisting with the transactions they replace in electrs' local mempool.

• detect locally indexed transactions that spend the same prevouts as an incoming transaction or package
• follow mempool spend edges to collect and remove all descendants of those conflicts
• reconcile conflicts before mutating any indexes, while keeping the ownership-aware edge removal from fix(mempool): tolerate missing/foreign edges on eviction #234 as defense in depth
• reject an internally conflicting incoming batch before insertion
• cover both broadcast_raw and submit_package immediate-insertion paths through the shared Mempool::add() implementation

This prevents the inconsistent state tracked in #236, where txstore and history could contain two conflicting spenders while edges could represent only one.

Fixes #236

Tests

• cargo test --test rest test_rest_mempool_rbf_reconciled -- --nocapture
• cargo test --test rest -- --nocapture (26 passed)
• cargo check --tests
• cargo check --tests --features liquid

The REST regressions verify immediately after both raw and package broadcast—before the next periodic mempool sync—that the replaced transaction is absent and the replacement is queryable.

[shesek]

This can fail when inserting submitted txs, if they descent from ancestor transactions that bitcoind has in its mempool view but we don't yet.

We could proactively fetch missing ancestors for submitted txs, but I would opt to keep this simpler and just avoid adding it to the local mempool view until the next periodic sync (the current behavior).

There's one thing we could improve though: broadcast_raw()/submit_package() could explicitly identify this failure case and return a 202 Accepted with a message saying the tx was submitted to the network but still not available in the local view. Currently this will fail with a 400, despite the network submission being successful.

[EddieHouston]

Agreed that we should not proactively fetch missing unconfirmed ancestors here. If a submitted transaction depends on an ancestor present in bitcoind but not yet in electrs’s local view, immediate insertion fails and the next periodic snapshot adds the complete dependency set.

I traced the response path again: both broadcast_raw() and submit_package() currently discard the local add_by_txid(s) result after successful daemon submission (let _ = ...). Therefore this case returns the normal successful daemon response (currently 200, not 400) while local visibility is deferred. I left that behavior unchanged. We could separately expose this outcome as 202 Accepted, but that would require changing the query/REST response contract. Maybe better in a new PR?

[shesek]

Ah right, sorry, I misremembered add_by_txid(s) errors as being propagated.

I agree that the current behavior, returning 200 in both cases, is already acceptable.

But I still think it would be nicer to use 202 to indicate deferred processing to clients, e.g. so that Esplora can display an appropriate message instead of immediately redirecting to the tx page and showing a "tx not found" error.

It's not high priority and could be done in a separate PR, but it's also a small change that could make sense to bundle with this PR if we expand its scope to more generally improve handling of submitted txs. It does have some potential to break clients that explicitly check for 200, but I think it should be quite common to treat any 2xx as successful.

Otoh, the fact that we don't have official point releases, a changelog for breaking changes or versioned API endpoints does make me more hesitant to change the REST contract. Perhaps a good time to prioritize addressing that?

[shesek]

It's worth documenting the conflict-free assumption we have here for add().

[shesek]

Ah right, sorry, I misremembered add_by_txid(s) errors as being propagated.

I agree that the current behavior, returning 200 in both cases, is already acceptable.

But I still think it would be nicer to use 202 to indicate deferred processing to clients, e.g. so that Esplora can display an appropriate message instead of immediately redirecting to the tx page and showing a "tx not found" error.

It's not high priority and could be done in a separate PR, but it's also a small change that could make sense to bundle with this PR if we expand its scope to more generally improve handling of submitted txs. It does have some potential to break clients that explicitly check for 200, but I think it should be quite common to treat any 2xx as successful.

Otoh, the fact that we don't have official point releases, a changelog for breaking changes or versioned API endpoints does make me more hesitant to change the REST contract. Perhaps a good time to prioritize addressing that?

[shesek]

Can this failure case actually happen if the tx package was considered valid and accepted by bitcoind?

[shesek]

This PR makes add_submitted() itself safe and fixes the state inconsistency created directly by add_by_txid(s), but the fact that add_submitted() writes can be interleaved within the periodic update() sync sequence can make update() unsafe.

This is the current update() sequence of operations:

1. Fetch bitcoind's mempool txids
2. [Under a read lock, list the local txids], diff with bitcoind's txids
3. [Under a write lock, remove() local txs not appearing in bitcoind's txids]
4. Fetch bitcoind's mempool txs we don't yet have (+ process to handle missed replacements and their descendants)
5. [Under a write lock, add() newly fetched txs]

It can fail in three ways:

1. The indexer runs steps (1) and (2) and sees that tx A was evicted from bitcoind. Then, tx B which conflicts with tx A is added via add_submitted(), causing the local eviction of A. Then the indexer continues to step (3) and tries to remove the already-removed A, resulting in a panic.
2. The indexer runs steps (1) to (4), sees that bitcoind has tx A which is missing locally, and fetches it. Then, add_submitted() is also called by a user for the same tx A (which will be accepted since bitcoind returns a successful reply for re-submission of known mempool txs). Then the indexer continues to step (5) and re-indexes the already-indexed tx A -- which doesn't panic, resulting in corrupted mempool state (duplicated history entries etc).
3. The indexer runs steps (1) to (4), sees that bitcoind has tx A which is missing locally, and fetches it. Then, add_submitted() is called with tx B which conflicts with tx A. Then the indexer continues to step (5) and indexes the conflicting tx A alongside tx B -- which also doesn't panic, resulting in corrupted mempool state.

Failure 1 is easy to fix: combine steps (2) and (3) to list the local txids, diff them and run local eviction all under the same write lock.

Failure 2 is also quite easy: detect already-indexed txids, either dropping them before calling add() or making add() itself ignore them. I lean towards the first, keeping add() a raw lower-level operation with no unnecessary checks that expects callers to pre-validate and provide consistent txs.

Failure 3 is more tricky. It requires conflict detection similar to what this PR implements, checking the txs fetched from bitcoind against the local view for conflicts, between steps (4) and (5). However unlike the add_submitted() conflict resolution which removes local conflicts in favor of the submitted txs, here we should favor the local txs (which must've been accepted by bitcoind and written through add_submitted() after we took the step (1) snapshot) and drop the conflicting txs fetched from bitcoind (and their descendants too, similarly to how replaced txs are handled).

There's also a much simpler alternative we can consider that solves all 3 failures and also avoids other similar failures more holistically, at the cost of degraded user experience: prevent interleaved add_submitted() writes entirely while update() is running. We could either reject local writes (deferring to the periodic sync like we do when there are unknown ancestors, which should eventually return a 202), or we could use a mutex to hold up add_submitted() writes until update() completes.