p2p: reconsider orphans when missing inputs are mined #35986

instagibbs commented at 11:06 AM on August 16, 2026: member

We reconsider for mempool entry of missing inputs, we should reconsider for mining of them too.

DrahtBot added the label P2P on Aug 16, 2026

DrahtBot commented at 11:07 AM on August 16, 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/35986.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	l0rinc, yuvicc, marcofleon

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

in src/node/txdownloadman_impl.cpp:104 in 33c6ffea58 outdated

  99 | @@ -100,6 +100,9 @@ void TxDownloadManagerImpl::BlockConnected(const std::shared_ptr<const CBlock>&
 100 |      m_orphanage->EraseForBlock(*pblock);
 101 |  
 102 |      for (const auto& ptx : pblock->vtx) {
 103 | +        // Reconsider potential child transactions.
 104 | +        m_orphanage->AddChildrenToWorkSet(*ptx, m_opts.m_rng);

l0rinc commented at 12:02 AM on August 17, 2026:

33c6ffe p2p: reconsider orphans when missing inputs are mined:

BlockConnected now calls AddChildrenToWorkSet for every tx in every newly connected block, so every output is checked even when the orphanage is empty or the block has no orphan children.

Now that these output checks run during block connection, could we add dedicated benchmarks for these three cases?

OrphanageAddChildrenEmptyOrphanage: measure the common empty-orphanage path and the work removed by the guard.
OrphanageAddChildrenForUnrelatedBlock: isolate the hashing and failed-lookup cost when no block output has an orphan child.
OrphanageAddChildrenForBlock: measure successful lookups and the work required to mark matching children for reconsideration.

For the empty case, could we mirror the guard in EraseForBlock here and return immediately when the orphanage is empty?

For the non-empty cases, like we discussed with @sipa in #35878, we could move SaltedOutpointHasher to normal-block SipHash-1-3 in a dedicated PR (not jumbo, since orphan prevouts can be peer-controlled). The reconsiderable wtxid set can probably use the faster jumbo path because those wtxids are computed locally. I'll push that PR soon, so we don't need to do any of that here.

instagibbs commented at 6:49 AM on August 17, 2026:

BlockConnected is a an async background job. I have done some light benchmarking to sanity check, but I don't think it's worth committing. Let me know if there is something concerning.

Added the guard checking for empty orphanage.

in test/functional/p2p_orphan_handling.py:408 in 33c6ffea58

 403 | +        self.generateblock(
 404 | +            node,
 405 | +            output=self.wallet.get_address(),
 406 | +            transactions=[parent["hex"]],
 407 | +        )
 408 | +        peer.sync_with_ping()

l0rinc commented at 12:03 AM on August 17, 2026:

33c6ffe p2p: reconsider orphans when missing inputs are mined:

nit: could we drop the redundant ping and keep the generateblock call compact like the other call sites in this test?

        self.generateblock(node, output=self.wallet.get_address(), transactions=[parent["hex"]])

instagibbs commented at 6:49 AM on August 17, 2026:

removed, it's not needed at all

in src/node/txdownloadman_impl.cpp:103 in 33c6ffea58 outdated

  99 | @@ -100,6 +100,9 @@ void TxDownloadManagerImpl::BlockConnected(const std::shared_ptr<const CBlock>&
 100 |      m_orphanage->EraseForBlock(*pblock);
 101 |  
 102 |      for (const auto& ptx : pblock->vtx) {
 103 | +        // Reconsider potential child transactions.

l0rinc commented at 12:04 AM on August 17, 2026:

33c6ffe p2p: reconsider orphans when missing inputs are mined:

EraseForBlock must run before AddChildrenToWorkSet so orphans confirmed or conflicted by the block are removed before any children are marked for reconsideration.

        // Reconsider potential child transactions after removing orphans confirmed or conflicted by the block.

instagibbs commented at 6:49 AM on August 17, 2026:

I'm not sure this comment helps, seems like not even a correctness issue in that if we got it the wrong way, we would mark something for reconsideration, then remove it?

l0rinc changes_requested

l0rinc commented at 12:08 AM on August 17, 2026: contributor

Concept ACK. I left a few comments to add benchmarks for the empty-orphanage, unrelated-block, and matching-child cases now that this is part of the critical path - and to mirror EraseForBlock’s empty-orphanage guard to act one one of those benchmarks.

yuvicc commented at 5:41 AM on August 17, 2026: contributor

Concept ACK

Makes sense to me, a parent confirmed without ever passing through our mempool leaves its children stranded in the orphanage, and MempoolAcceptedTx can't cover that case.

One thing which is already mentioned in the above comment to add a check for empty-orphanage similar to EraseForBlock function.

p2p: reconsider orphans when missing inputs are mined 9cc7dc50bd

instagibbs force-pushed on Aug 17, 2026

DrahtBot added the label CI failed on Aug 17, 2026

DrahtBot removed the label CI failed on Aug 17, 2026

l0rinc approved

l0rinc commented at 6:11 PM on August 17, 2026: contributor

Lightly tested code review ACK 9cc7dc50bdc9867d079ab7a111d39487a4566767

When orphan parents never entered the mempool and were first seen in mined blocks, their now-valid children were not reconsidered and remained in the orphanage. This was likely more visible during the recent wave of Slipstream COLDCARD rescues.

yuvicc commented at 4:51 AM on August 18, 2026: contributor

ACK 9cc7dc50bdc9867d079ab7a111d39487a4566767

marcofleon approved

marcofleon commented at 10:35 AM on August 18, 2026: contributor

ACK 9cc7dc50bdc9867d079ab7a111d39487a4566767

fanquake merged this on Aug 18, 2026

fanquake closed this on Aug 18, 2026