mempool: remove all subsequent tx in pkg on failure #35017

instagibbs commented at 8:52 PM on April 6, 2026: member

This belt-and-suspenders check, if ever hit in production, could result in an inconsistent mempool if somehow the parent failed in the ConsensusScriptChecks but the child did not. Rather than allow the mempool to get in an inconsistent state, remove the following txs in the package.

DrahtBot added the label Mempool on Apr 6, 2026

DrahtBot commented at 8:52 PM on April 6, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35017.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ismaelsadeeq, marcofleon, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

in src/validation.cpp:1272 in 9a8cbc6356

1265 | @@ -1266,6 +1266,10 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
1266 |                                              ws.m_ptx->GetHash().ToString()));
1267 |              // Remove the transaction from the mempool.
1268 |              if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet();
1269 | +        }
1270 | +        // Remove first failing tx and all subsequent in package
1271 | +        if (!all_submitted) {
1272 | +            Assume(m_subpackage.m_changeset);

luke-jr commented at 6:16 PM on April 8, 2026:

Should be Assert, since the next line will nullptr deref if the check fails

instagibbs commented at 6:21 PM on April 8, 2026:

changed

instagibbs force-pushed on Apr 8, 2026

sedited requested review from ismaelsadeeq on Apr 20, 2026

in src/validation.cpp:1268 in 623796abdf

1265 | @@ -1266,6 +1266,10 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
1266 |                                              ws.m_ptx->GetHash().ToString()));
1267 |              // Remove the transaction from the mempool.
1268 |              if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet();

ismaelsadeeq commented at 9:32 AM on May 20, 2026:

This comment is now stale, and the changeset definition should move to the if branch?

diff --git a/src/validation.cpp b/src/validation.cpp
index dfe64d4981..5c2890b21d 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1264,11 +1264,10 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
             package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR,
                                   strprintf("BUG! PolicyScriptChecks succeeded but ConsensusScriptChecks failed: %s",
                                             ws.m_ptx->GetHash().ToString()));
-            // Remove the transaction from the mempool.
-            if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet();
         }
         // Remove first failing tx and all subsequent in package
         if (!all_submitted) {
+            if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet();
             Assert(m_subpackage.m_changeset);
             m_subpackage.m_changeset->StageRemoval(m_pool.GetIter(ws.m_ptx->GetHash()).value());
         }

instagibbs commented at 7:06 PM on May 20, 2026:

updated. and removed Assert as it's no longer needed

in src/validation.cpp:1259 in 623796abdf

ismaelsadeeq commented at 9:35 AM on May 20, 2026:

Since we are going to remove the child if one parent fails?

Should we prevent the ConsensusScriptChecks call by short-circuiting here?

        if (all_submitted && !ConsensusScriptChecks(args, ws)) {

instagibbs commented at 3:09 PM on May 20, 2026:

I think it's fine to check, and fail all of them? Do you see an additional footgun?

ismaelsadeeq commented at 3:25 PM on May 20, 2026:

Nope; I see no footgun. Just that we get the speed benefit of not jumping into that branch and doing the expensive check?

instagibbs commented at 5:09 PM on May 20, 2026:

In the real cases we're always eating the cost, I'd prefer clarity over speed on "never happen". I think it's clearer the way it is? Going to mark as resolved.

in src/validation.cpp:1271 in 623796abdf outdated

1265 | @@ -1266,6 +1266,10 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
1266 |                                              ws.m_ptx->GetHash().ToString()));
1267 |              // Remove the transaction from the mempool.
1268 |              if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet();
1269 | +        }
1270 | +        // Remove first failing tx and all subsequent in package
1271 | +        if (!all_submitted) {

ismaelsadeeq commented at 9:36 AM on May 20, 2026:

Is it possible to have a test for this edge-case?

instagibbs commented at 3:07 PM on May 20, 2026:

not worth it, should "never happen"?

ismaelsadeeq commented at 9:37 AM on May 20, 2026: member

Concept ACK

I left two suggestions and a question.

mempool: remove all subsequent tx in pkg on failure

This belt-and-suspenders check, if ever hit in production,
could result in an inconsistent mempool if somehow the
parent failed in the ConsensusScriptChecks but the child did
not. Rather than allow the mempool to get in an inconsistent
state, remove the following txs in the package.

ac9aa71b7f

instagibbs force-pushed on May 20, 2026

ismaelsadeeq commented at 7:45 PM on May 20, 2026: member

Code review ACK ac9aa71b7f9d6d0c694087dc7fa842c5cf573b1c

fanquake requested review from marcofleon on May 20, 2026

marcofleon commented at 3:29 PM on May 21, 2026: contributor

ACK ac9aa71b7f9d6d0c694087dc7fa842c5cf573b1c

Takes the removal of the current failing transaction out of the script check branch so that later iterations of the loop will remove all txs in the package, ensuring the dependent tx doesn't stay in the mempool. This would remove other valid parents in the package too, but those weren't in the mempool before anyway, so it's fine.

sedited approved

sedited commented at 9:22 PM on May 21, 2026: contributor

ACK ac9aa71b7f9d6d0c694087dc7fa842c5cf573b1c

sedited merged this on May 21, 2026

sedited closed this on May 21, 2026

rustaceanrob referenced this in commit 356fdfdd75 on May 22, 2026

stickies-v commented at 2:21 PM on May 22, 2026: contributor

Post-merge ACK ac9aa71b7f9d6d0c694087dc7fa842c5cf573b1c