Skip to content

fix: release InstantSend tracking after mempool eviction - #7839

Open
PastaPastaPasta wants to merge 1 commit into
dashpay:developfrom
PastaPastaPasta:codex/security-instantsend-eviction-cleanup
Open

PastaPastaPasta wants to merge 1 commit into
dashpay:developfrom
PastaPastaPasta:codex/security-instantsend-eviction-cleanup

Conversation

@PastaPastaPasta

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

InstantSend tracks every unlocked transaction that enters the mempool. When an ordinary unlocked transaction left the mempool through expiry or size trimming, TransactionIsRemoved returned early: only Platform transfers were released. The confirmation cleanup only handles mined entries, so the evicted transaction stayed tracked for good. Its full object, parent links, spent-outpoint index and timing record were all kept outside the mempool size limit. Sustained churn of transactions that are never locked or mined grows this memory without bound. Normal lock delivery limits the exposure in practice. The achievable growth rate has not been measured.

What was done?

  • TransactionIsRemoved now releases the tracking of any unlocked transaction through the existing RemoveNonLockedTx cleanup, which handles parent links, outpoint index and retry entries. It also drops the timing record.
  • TransactionIsRemoved is also called when a peer's transaction is rejected. On a node without txindex, a mined transaction that is not yet locked can be replayed and rejected after a reorg clears the recent-confirmed filter. That transaction's tracking must survive so that a later conflicting lock still finds its block. RemoveNonLockedTx therefore takes a keepMined flag, and the mined check and the erase happen together under cs_nonLocked.
  • Platform transfers keep their existing unconditional removal. Locked-transaction handling is unchanged. Block removals never emit this callback.

This is a Dash-specific fix. Bitcoin Core has nothing to backport.

How Has This Been Tested?

macOS arm64, existing depends build:

  • Full make -j1 passed.
  • ./src/test/test_dash --run_test=evo_islock_tests passed all 14 cases.
  • test/functional/p2p_instantsend.py and test/functional/feature_llmq_is_retroactive.py passed.
  • lint-includes.py, lint-include-guards.py, lint-circular-dependencies.py and git diff --check passed.

New test nonlocked_ordinary_tx_released_on_mempool_removal:

  • It registers the real NetInstantSend validation listener and adds standard valid transactions to the mempool. It then expires the mempool and, in a second pass, trims it. A weak pointer confirms that the last owner of each transaction is released.
  • With the production code unchanged, both release checks fail (exit 201).
  • A negative case tracks a transaction as mined and calls the public TransactionIsRemoved entry point. It checks that the transaction is still owned and that RetrieveISConflicts still reports it at that block. An earlier version of this fix removed mined entries unconditionally, and this case fails against it (exit 201).

An independent review approved the change. No sustained churn or memory-exhaustion run was done. The mined negative case uses the manager's public tracking calls, not a mined block or an end-to-end replay.

Breaking Changes

None. Tracking for mined transactions is kept, and locked-transaction and Platform-transfer handling are unchanged.

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

An ordinary transaction that left the mempool by expiry or size trimming
before it was locked or mined stayed in the InstantSend non-locked
tracker for good: TransactionIsRemoved only released Platform transfers,
and the confirmation cleanup skips unmined entries. Each evicted
transaction kept its full object, parent links, spent-outpoint index and
timing record, outside any mempool accounting.

Release the tracking for every unlocked transaction that is removed, and
drop its timing record. TransactionIsRemoved is also called when a peer's
transaction is rejected, which can happen for a transaction that is
already mined but not yet locked or fully confirmed. Its tracking must
survive so that a later conflicting lock still finds the block, so the
mined check and the erase happen together under cs_nonLocked. Platform
transfers keep their existing unconditional removal.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 7, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 10 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f5071c4d-3414-4951-83ef-1d8f5ab42060
📥 Commits

Reviewing files that changed from the base of the PR and between f35a24c and 312fb0d.

📒 Files selected for processing (3)
  • src/instantsend/instantsend.cpp
  • src/instantsend/instantsend.h
  • src/test/evo_islock_tests.cpp
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit 312fb0d) · triage: low

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Final validation — Phase 1 + Phase 2

Verified the exact-head diff and surrounding removal, peer-rejection, mined-conflict, and validation-notification paths; no actionable in-scope defects were found. The cleanup preserves ordinary mined tracking under the removal mutex, retains existing Platform-transfer behavior, and adds regression coverage for expiry, trimming, and mined-conflict retention. Verification was static: no builds or tests were run; the supplied CI snapshot shows a successful amd64 container job with arm64 and slim-container jobs still queued, while local test and mutation results remain author-reported evidence.

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: low by gpt-6.1-sol (effort low) — This is a small, contained tracking-cleanup fix with focused regression coverage and a mined-entry guard, rather than a large or intricate change to a critical surface.
  • Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer
  • Phase 1 model: glm-5.3-flash — zai quota: 5h 90% left, weekly 94% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

@thepastaclaw thepastaclaw added the pastaclaw:approved thepastaclaw's latest review approved this PR label Oct 7, 2026
@PastaPastaPasta PastaPastaPasta added this to the 24 milestone Oct 9, 2026
@github-actions

Copy link
Copy Markdown

Potential PR merge conflicts

This is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order.

If these PRs merge first

This PR will likely need a rebase:

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

Labels

backport-candidate-24.0.x pastaclaw:approved thepastaclaw's latest review approved this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants