Skip to content

fix(zebrad): release the per-peer mempool slot on verification timeout - #11229

Open
natalieesk wants to merge 1 commit into
mainfrom
mempool_timeout_peer_slot_10684
Open

fix(zebrad): release the per-peer mempool slot on verification timeout#11229
natalieesk wants to merge 1 commit into
mainfrom
mempool_timeout_peer_slot_10684

Conversation

@natalieesk

@natalieesk natalieesk commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Motivation

Closes #10684.

The mempool downloader's verification-timeout arm (Downloads::poll_next, zebrad/src/components/mempool/downloads.rs) removed the cancel_handles entry with a bare remove (the GHSA-65jj-fmw8-468q memory fix) but discarded the stored source and never released the peer's pending_per_peer slot. After MAX_INBOUND_CONCURRENCY_PER_PEER (5) timed-out transactions from one source, that source's count stayed pinned at the cap with no tasks left, so further queueing from it returned FullQueue.

Solution

Route the timeout arm through the shared terminal-cleanup path (completed_txid = Some(txid)) instead of a bare cancel_handles.remove. That shared block already removes the handle (dropping the resident Gossip, preserving the GHSA-65jj fix, for all sources) and calls release_peer_slot, so success, verifier-error, and timeout now share one uniform path. completed_txid only drives cleanup; the returned Err((txid, elapsed)) is unchanged.

Low risk, confined to mempool inbound-download accounting: no consensus, state-format, RPC, or config change; no DB format bump.

Tests

verification_timeout_releases_peer_slot (mempool/tests/vector.rs) times out MAX_INBOUND_CONCURRENCY_PER_PEER peer-sourced transactions under start_paused, drains them, then queues another from the same source and asserts it succeeds. Fails before the fix (FullQueue), passes after; the GHSA-65jj test still passes. cargo fmt/clippy clean.

Specifications & References

Preserves the GHSA-65jj-fmw8-468q memory fix.

Follow-up Work

None.

AI Disclosure

  • No AI tools were used in this PR
  • AI tools were used: Claude (Claude Code) — wrote the fix and the regression test.

PR Checklist

  • The PR title follows conventional commits format: type(scope): description
  • The PR follows the contribution guidelines.
  • This change was discussed in an issue or with the team beforehand.
  • The solution is tested.
  • The documentation and changelogs are up to date.

The mempool downloader's verification-timeout arm removed the `cancel_handles`
entry (the GHSA-65jj memory fix) with a bare `remove`, discarding the stored
`source` and skipping the shared per-peer slot release. After
`MAX_INBOUND_CONCURRENCY_PER_PEER` timed-out transactions from one source, that
source's `pending_per_peer` count stayed pinned at the cap with no tasks left,
so further queueing from it returned `FullQueue`.

Destructure the removed handle and call `release_peer_slot`, matching the
success and verifier-error arms while preserving the GHSA-65jj handle removal.
Adds a regression test that times out `MAX_INBOUND_CONCURRENCY_PER_PEER`
peer-sourced transactions and asserts a further one from the same source queues.
@v12-auditor

v12-auditor Bot commented Aug 10, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. V12 found three issues worth reviewing.

Open the full results here.

FindingSeverityDetails
F-205832 🔵 Low
Cancelled downloads free peer slot while task still resident

Downloads::cancel(mined_ids) removes the cancel_handles entry and immediately calls release_peer_slot, decrementing pending_per_peer[source], but it never removes the corresponding JoinHandle from self.pending. The spawned task stays resident in the FuturesUnordered until it observes the cancel oneshot through the biased tokio::select! arm and a subsequent poll_next reaps it. Every other release site couples the slot release to the FuturesUnordered removal: the shared terminal cleanup runs inside poll_next in the same invocation where the completed future is removed, and cancel_all takes and drops pending wholesale while clearing pending_per_peer. Because Mempool::poll_ready runs its drain loop before calling tx_downloads.cancel(&mined_ids) in the TipAction::Grow branch, the cancelled tasks are guaranteed still resident in pending when poll_ready returns, and the next buffered call servicing Request::QueueFromPeer reads the already-decremented counter. This is pre-existing and untouched by the reviewed diff, whose timeout-arm release is correctly synchronized inside poll_next.

F-205833 🟠 High
Crawler rate-limit constant shadows configured verification deadline

download_if_needed_and_verify wraps the entire download-and-verify future in tokio::time::timeout(RATE_LIMIT_DELAY, fut) inside the spawned task. RATE_LIMIT_DELAY is the mempool crawler's scheduling interval of 73 seconds, not a transaction-processing deadline. The same future is separately protected by a tower::timeout::Timeout layer carrying TRANSACTION_VERIFY_TIMEOUT, which resolves to BLOCK_VERIFY_TIMEOUT = 480 seconds and is genuinely installed at Timeout::new(self.tx_verifier.clone(), TRANSACTION_VERIFY_TIMEOUT). Because 73s is strictly shorter and both clocks start in the same spawned task, the 480s layer can never fire first and is unreachable dead configuration, as is the 360s UTXO_LOOKUP_TIMEOUT nested inside the verifier. Only the 20s TRANSACTION_DOWNLOAD_TIMEOUT remains reachable. This is pre-existing; the reviewed diff changes only the completed_txid value in the timeout arm.

F-205834 🟡 Medium
Verification timeouts record no rejection or backoff

The three terminal arms in Mempool::poll_ready are handled asymmetrically. The verifier-error arm logs, bumps a metric, inserts into invalidated_ids, and crucially calls storage.reject_if_needed(tx_id, error). The timeout arm logs, inserts into invalidated_ids, and bumps a metric, but performs no rejection-list insert, no backoff, and no misbehaviour scoring. invalidated_ids has no suppression effect: its only terminal consumer is self.transaction_sender.send(MempoolChange::invalidated(invalidated_ids)), a broadcast notification to RPC/indexer subscribers. Admission suppression is driven solely by should_download_or_verify, which consults mempool membership and rejection_error; a timed-out txid appears in neither. After the reviewed diff correctly releases the per-peer slot on timeout, the AlreadyQueued guard and the per-peer cap both read state the cleanup just cleared, so the same peer can immediately re-advertise the same txid and re-occupy a slot indefinitely with no recorded backoff.

Analyzed one file, diff 05d129b...31e555e.

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

Labels

A-mempool Area: Memory pool transactions A-network Area: Network protocol updates or fixes C-audit Category: Issues arising from audit findings C-bug Category: This is a bug C-security Category: Security issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mempool verification timeout does not release the per-peer queue slot

1 participant