p2p: index TxOrphanage by wtxid, allow entries with same txid

glozow commented at 8:59 pm on April 29, 2024: member

Part of #27463 in the “make orphan handling more robust” section.

Currently the main map in TxOrphanage is indexed by txid; we do not allow 2 transactions with the same txid into TxOrphanage. This means that if we receive a transaction and want to store it in orphanage, we’ll fail to do so if a same-txid-different-witness version of the tx already exists in the orphanage. The existing orphanage entry can stay until it expires 20 minutes later, or until we find that it is invalid.

This means an attacker can try to block/delay us accepting an orphan transaction by sending a mutated version of the child ahead of time. See included test.

Prior to #28970, we don’t rely on the orphanage for anything and it would be relatively difficult to guess what transaction will go to a node’s orphanage. After the parent(s) are accepted, if anybody sends us the correct transaction, we’ll end up accepting it. However, this is a bit more painful for 1p1c: it’s easier for an attacker to tell when a tx is going to hit a node’s orphanage, and we need to store the correct orphan + receive the parent before we’ll consider the package. If we start out with a bad orphan, we can’t evict it until we receive the parent + try the 1p1c, and then we’ll need to download the real child, put it in orphanage, download the parent again, and then retry 1p1c.

DrahtBot commented at 8:59 pm on April 29, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, AngusP, theStack, sr-gi, stickies-v, itornaza
Stale ACK	mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label P2P on Apr 29, 2024

osarukun approved

DrahtBot added the label CI failed on Apr 29, 2024

DrahtBot commented at 11:14 pm on April 29, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24396449433

glozow commented at 3:26 pm on April 30, 2024: member

Will fix the linter when I rebase

glozow force-pushed on May 1, 2024

DrahtBot removed the label CI failed on May 1, 2024

glozow marked this as ready for review on May 1, 2024

in src/txorphanage.h:26 in 4b4dfaa8f3 outdated

23@@ -24,7 +24,7 @@ class TxOrphanage {
24     bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
25 
26     /** Check if we already have an orphan transaction (by txid or wtxid) */

stickies-v commented at 2:43 pm on May 1, 2024:

0    /** Check if we already have an orphan transaction */

glozow commented at 11:16 am on May 10, 2024:

fixed thanks

in src/txorphanage.cpp:67 in 4b4dfaa8f3 outdated

66     AssertLockHeld(m_mutex);
67-    std::map<Txid, OrphanTx>::iterator it = m_orphans.find(txid);
68+    std::map<Wtxid, OrphanTx>::iterator it = m_orphans.find(wtxid);
69     if (it == m_orphans.end())
70         return 0;
71+    const auto& txid = it->second.tx->GetHash();

stickies-v commented at 3:14 pm on May 1, 2024:

nit: looks like this is only used for logging (and thus can be declared right before the logging) - do we still want/need to log the txid? I’d think wtxid suffices? But nvm if that’s contentious.

glozow commented at 10:53 am on May 10, 2024:

Same as above, imo definitely want to log txid if we have it.

in src/txorphanage.cpp:107 in 4b4dfaa8f3 outdated

105-        std::map<Txid, OrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid
106+        std::map<Wtxid, OrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid
107         if (maybeErase->second.fromPeer == peer)
108         {
109-            nErased += EraseTxNoLock(maybeErase->second.tx->GetHash());
110+            nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());

stickies-v commented at 3:21 pm on May 1, 2024:

nit: can’t we just use first?

0            nErased += EraseTxNoLock(maybeErase->first);

stickies-v commented at 6:45 pm on May 1, 2024:

Or alternatively:

 0diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp
 1index 504a1f7ec3..bed92e6b32 100644
 2--- a/src/txorphanage.cpp
 3+++ b/src/txorphanage.cpp
 4@@ -98,13 +98,11 @@ void TxOrphanage::EraseForPeer(NodeId peer)
 5     m_peer_work_set.erase(peer);
 6 
 7     int nErased = 0;
 8-    std::map<Wtxid, OrphanTx>::iterator iter = m_orphans.begin();
 9-    while (iter != m_orphans.end())
10-    {
11-        std::map<Wtxid, OrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid
12-        if (maybeErase->second.fromPeer == peer)
13+    for (auto iter = m_orphans.begin(); iter != m_orphans.end(); ) {
14+        const auto& [wtxid, tx] = *iter++; // increment to avoid iterator becoming invalid
15+        if (tx.fromPeer == peer)
16         {
17-            nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());
18+            nErased += EraseTxNoLock(wtxid);
19         }
20     }
21     if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer);

instagibbs commented at 2:47 pm on May 10, 2024:

this could still happen

glozow commented at 2:02 pm on May 13, 2024:

this was (mostly) taken

in src/net_processing.cpp:2300 in 646aa4da91 outdated

2294@@ -2295,7 +2295,9 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside
2295 
2296     const uint256& hash = gtxid.GetHash();
2297 
2298-    if (m_orphanage.HaveTx(gtxid)) return true;
2299+    // Orphanage is checked by wtxid. However, even if this is a txid, look up the same hash in
2300+    // case this is a non-segwit transaction in the orphanage.
2301+    if (m_orphanage.HaveTx(Wtxid::FromUint256(gtxid.GetHash()))) return true;

instagibbs commented at 4:19 pm on May 1, 2024:

0    if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true;

stickies-v commented at 7:24 pm on May 1, 2024:

It’s not immediately obvious to me why it is safe to just convert any gtxid into a Wtxid here, especially since there are callsites of AlreadyHaveTx where a GenTxId::Txid is passed, such as e.g. here. If it is indeed safe, I think it could be a better approach to update AlreadyHaveTx to take a Wtxid and push the conversion further to the edge, so it can be reviewed on a case-by-case basis?

AngusP commented at 10:22 pm on May 1, 2024:

(Ignore me if I’m understanding) – coinbase transactions are required to have all 0 Wtxids (0x0000....0000) which means for an INV message for a witness coinbase you have to use the non-witness txid to reference the transaction? - so AlreadyHaveTx in that case is explicitly not using a Wtxid? (Or are coinbases never INV’d?)

Also perhaps not great/confusing to treat Wtxid and Txids as always interchangeable (though the types allow it) because of the coinbase Wtxid caveat?

glozow commented at 11:12 am on May 2, 2024:

It’s not immediately obvious to me why it is safe to just convert any gtxid into a Wtxid here

The argument I’m making in this PR is that in all the places we are checking by txid, we shouldn’t be doing that, because we’re missing same-txid-different-witness cases. But yeah totally fair to say it’s difficult to review it together, so I’ll split the first commit to make it more explicit where the change is happening and why that’s ok.

I think it could be a better approach to update AlreadyHaveTx to take a Wtxid

fwiw I disagree, as we do want to check txids in the other data structures.

coinbase transactions are required to have all 0 Wtxids (0x0000….0000) which means for an INV message for a witness coinbase you have to use the non-witness txid to reference the transaction? - so AlreadyHaveTx in that case is explicitly not using a Wtxid? (Or are coinbases never INV’d?)

That link is referring to the wtxid of the coinbase transaction when calculating the witness commitment; that’s not the case in any other situation. Also, yes, if we get a coinbase transaction in tx relay we’d drop that pretty much immediately and wouldn’t be putting it in orphanage: https://github.com/bitcoin/bitcoin/blob/9d1a286f20b8a602ffe72928bcd79be09fdbf9d0/src/validation.cpp#L735-L742

stickies-v commented at 2:03 pm on May 2, 2024:

The argument I’m making in this PR is that in all the places we are checking by txid, we shouldn’t be doing that, because we’re missing same-txid-different-witness cases.

I agree, the rationale makes sense. My concern is about segwit txs (txid != wtxid) where we check by txid (for whatever reason: intentional because we don’t have witness data, legacy and we never updated it, buggy, …): just creating a wtxid from the txid hash as is done here seems like it has potential to start missing things we previously wouldn’t - since TxOrphanage now only indexes by wtxid?

I’ve made a branch where I’ve created 2 overloads of AlreadyHaveTx: one that takes Wtxid and one that takes Txid, to force all the conversions to the edges. I implemented the Txid to perform less checks, based on the documentation (didn’t verify the code) that m_recent_rejects_reconsiderable and m_recent_rejects uses wtxid, whereas m_recent_confirmed_transactions uses both txid and wtxid.

There are 4 callsites of AlreadyHaveTx. For 2 of them, 1) here and 2) here, the conversion seems trivial/sensible.

A third is more confusing to me. 3) Here in ProcessMessage, I initially used the Txid overload, but that fails the p2p_orphan_handling.py::test_orphan_of_orphan test. Switching to Wtxid makes it pass, but I don’t understand why. Since with this pull, TxOrphanage indexes on Wtxid, calling HaveTx on a blind conversion of parent_txid into a Wtxid seems like it shouldn’t find anything in the orphanage, unless we’re dealing with non-segwit transaction?

Finally, for the 4) fourth here, I think my refactoring is a no-op, we just blindly call the different function signatures based on GenTxid::IsWtxid(), no real difference with the current implementation except perhaps for making things more explicit.

stickies-v commented at 2:08 pm on May 2, 2024:

To be clear: I’m not saying this PR needs to overhaul AlreadyHaveTx like I did, and I think having TxOrphanage index on Wtxid instead of Txid is better. I just think that if we’re going to be calling TxOrphanage methods with (what are in fact) txids, we should probably have TxOrphanage also keep a Txid index and not do blind conversion such as is happening here? I think that would address my concerns and make things easier to review?

instagibbs commented at 2:09 pm on May 2, 2024:

fwiw, I think the in-orphanage txid==wtixd case seems pretty rare all things considered. I had 4 over a 24 hour period, vs about 400 times where we fetched a segwit parent via txid though that particular txid was in the orphanage already.

iow I’m not sure extra complexity is worth it, vs just dropping it altogether(and eating a tiny bit more of bandwidth?)

stickies-v commented at 2:23 pm on May 2, 2024:

iow I’m not sure extra complexity is worth it,

Oh, fair enough. I don’t have a good enough view of the attack vectors here. In my review, I was operating on the assumption that we wouldn’t want to miss anything we’re currently doing. If it’s okay to relax that, that could make sense - but it should probably be made explicit then?

instagibbs commented at 2:31 pm on May 2, 2024:

IIUC If the check is removed, you’ll waste bandwidth when you get orphan chains that are non-segwit, but you won’t keep or relay them, so it shouldn’t churn the orphanage.

Would it help if the comment was touched up to be explicit about what it’s saving us from doing?

itornaza commented at 6:21 pm on May 3, 2024:

imo wtxid and txid being double SHA256 outputs and wtxid being essentially a superset of txid, makes this blind conversion legitimate in this case.

glozow commented at 11:15 am on May 10, 2024:

done

glozow commented at 11:41 am on May 10, 2024:

Split up the commit, hope it helps (https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2049866191)

instagibbs commented at 4:33 pm on May 1, 2024: member

code changes are quite mechanical, thankfully

concept ACK

in src/test/fuzz/txorphan.cpp:116 in 4b4dfaa8f3 outdated

111@@ -112,21 +112,21 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
112                     {
113                         CTransactionRef ref = orphanage.GetTxToReconsider(peer_id);
114                         if (ref) {
115-                            bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetWitnessHash()));
116+                            bool have_tx = orphanage.HaveTx(ref->GetWitnessHash());
117                             Assert(have_tx);

stickies-v commented at 5:20 pm on May 1, 2024:

nit: can just one-line

0                            Assert(orphanage.HaveTx(ref->GetWitnessHash()));

glozow commented at 11:15 am on May 10, 2024:

done

instagibbs commented at 6:20 pm on May 1, 2024: member

should be able to add fuzz coverage easily for differing wtxids but same txids in src/test/fuzz/txorphan.cpp

stickies-v commented at 7:25 pm on May 1, 2024: contributor

Concept ACK, but I’m concerned about the behaviour change in AlreadyHaveTx, which seems quite non-trivial to review so I’ll need to look into further.

in src/txorphanage.cpp:24 in 646aa4da91 outdated

22@@ -23,7 +23,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
23 
24     const Txid& hash = tx->GetHash();

sr-gi commented at 5:40 pm on May 3, 2024:

In 646aa4da91c03a0e72d086cae281aa0688f2f41d

This is only used for logging now. Do we care about logging the txid now that the map is keyed by wtxid?

glozow commented at 10:53 am on May 10, 2024:

Yes, I would definitely say to log both txid and wtxid when we have it. A lot of things use txid only, e.g. if you want to trace a tx through logs or look it up in mempool. Would be a pain to not have txid imo.

in src/txorphanage.cpp:110 in 646aa4da91 outdated

108         {
109-            nErased += EraseTxNoLock(maybeErase->second.tx->GetHash());
110+            nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());
111         }
112     }
113     if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer);

sr-gi commented at 5:54 pm on May 3, 2024:

In 646aa4da91c03a0e72d086cae281aa0688f2f41d

nit: nErased is a counter right? Shouldn’t this read txs instead?

glozow commented at 1:58 pm on May 13, 2024:

Ok I’ve added a commit to fix this and another log to say “transaction(s)”. I took the opportunity to add a few nice things to the logs as well.

in src/txorphanage.cpp:165 in 646aa4da91 outdated

155@@ -159,7 +156,7 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx)
156             for (const auto& elem : it_by_prev->second) {
157                 // Get this source peer's work set, emplacing an empty set if it didn't exist
158                 // (note: if this peer wasn't still connected, we would have removed the orphan tx already)
159-                std::set<Txid>& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second;
160+                std::set<Wtxid>& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second;
161                 // Add this tx to the work set
162                 orphan_work_set.insert(elem->first);
163                 LogPrint(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n",

sr-gi commented at 5:56 pm on May 3, 2024:

In 646aa4da91c03a0e72d086cae281aa0688f2f41d

Same here, do we still care about logging txids? I guess this applies to all the file, so not pointing it out again

glozow commented at 10:56 am on May 10, 2024:

Yes

in src/txorphanage.cpp:223 in 646aa4da91 outdated

218@@ -226,7 +219,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block)
219             if (itByPrev == m_outpoint_to_orphan_it.end()) continue;
220             for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) {
221                 const CTransaction& orphanTx = *(*mi)->second.tx;
222-                const auto& orphanHash = orphanTx.GetHash();
223+                const auto& orphanHash = orphanTx.GetWitnessHash();
224                 vOrphanErase.push_back(orphanHash);

sr-gi commented at 6:10 pm on May 3, 2024:

In 646aa4da91c03a0e72d086cae281aa0688f2f41d

This should not be called orphanHash anymore, but also, this could be inlined

sr-gi commented at 6:12 pm on May 3, 2024:

Also, in L231:

for (const auto& orphanHash : vOrphanErase) -> for (const auto& wtxid : vOrphanErase)

glozow commented at 11:16 am on May 10, 2024:

inlined. didn’t do the rename as I don’t think it’s incorrect and want to minimize diff

itornaza commented at 6:14 pm on May 3, 2024: none

concept ACK

After being caught out of guard due to the short notice on this PR at the Bitcoin Review Club, I took time to review the code changes and read the comments above.

To my understanding, we need to check the wtxid for screening transactions for inclusion in the orphanage in order to catch witness malleations as well. This was not previously possible because we where only checking against txid that does not include the [witness] section of a transaction as defined in BIP141.

This change needs to be implemented only for the orphanage and not the rest of the structures that AlreadyHaveTx() checks against, since txid might be relevant there. The new behavior is implicitly documented in the bool TxOrphanage::HaveTx(const Wtxid& wtxid) const function definition which now takes wtxid as its single argument.

sr-gi commented at 7:51 pm on May 3, 2024: member

Concept ACK

glozow force-pushed on May 10, 2024

glozow commented at 11:40 am on May 10, 2024: member

Ok I have split the first commit into:

net processing change to stop querying by txid
2 refactors changing the TxOrphanage methods to be by Wtxid only
TxOrphanage change to index by wtxid and thus allow entries with the same txid

Hopefully in review, we can convince ourselves that the first commit is the right thing to do, separately from the other changes. The tests may help with that - also added one for invs that helps illustrate what the cases are, and added unit tests.

I thought a bit about whether we should check for same-txid-different-witness but from the same peer and replace that in orphanage. Given that a Bitcoin Core node doesn’t ever do witness replacement, this seems like unnecessary complexity that won’t ever get used between normally-operating nodes. So I did not add this.

in test/functional/p2p_orphan_handling.py:71 in 65f69bf533 outdated

67         self._tx_received = []
68         self._getdata_received = []
69+        self._wtxidrelay = wtxidrelay
70+
71+    def on_version(self, message):
72+        # Avoid sending verack in response to version.

instagibbs commented at 3:55 pm on May 10, 2024:

but you are sending verack just below?

glozow commented at 11:42 am on May 13, 2024:

Oops, this is a bad copy-paste. I forgot to delete this comment I copied over.

in test/functional/p2p_orphan_handling.py:72 in 65f69bf533 outdated

68         self._getdata_received = []
69+        self._wtxidrelay = wtxidrelay
70+
71+    def on_version(self, message):
72+        # Avoid sending verack in response to version.
73+        # When calling add_p2p_connection, wait_for_verack=False must be set (see

instagibbs commented at 3:56 pm on May 10, 2024:

but wait_for_verack=False isn’t being set?

glozow commented at 11:43 am on May 13, 2024:

Same - forgot to delete, sorry

glozow commented at 1:16 pm on May 13, 2024:

fixed

in test/functional/p2p_orphan_handling.py:467 in 65f69bf533 outdated

462+        node.bumpmocktime(GETDATA_TX_INTERVAL)
463+
464+        # 4. The parent is requested. Honest peer sends it.
465+        honest_peer.wait_for_getdata([parent_txid_int])
466+        honest_peer.send_message(msg_tx(tx_parent["tx"]))
467+        honest_peer.sync_with_ping()

instagibbs commented at 4:13 pm on May 10, 2024:

might want to note this line is necessary for the reconsideration step to always happen

glozow commented at 1:17 pm on May 13, 2024:

added comment

in test/functional/p2p_orphan_handling.py:499 in 65f69bf533 outdated

494+        honest_peer = node.add_p2p_connection(P2PInterface())
495+
496+        # 1. Fake orphan is received first. It is missing an input.
497+        bad_peer.send_and_ping(msg_tx(tx_orphan_bad_wit))
498+
499+        # 2. Node requests the missing parent by txid.

instagibbs commented at 4:31 pm on May 10, 2024:

0        # 2. Node requests tx_grandparent by txid.

glozow commented at 1:17 pm on May 13, 2024:

done

in test/functional/p2p_orphan_handling.py:505 in 65f69bf533 outdated

500+        grandparent_txid_int = int(tx_grandparent["txid"], 16)
501+        node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY)
502+        bad_peer.wait_for_getdata([grandparent_txid_int])
503+
504+        # 3. Honest peer relays the granchild, which is missing a parent. The parent by txid already
505+        # exists in orphanage, but should be re-requested.

instagibbs commented at 4:33 pm on May 10, 2024:

0        # exists in orphanage, but should be re-requested due to having witness data.

glozow commented at 1:19 pm on May 13, 2024:

elaborated

in test/functional/p2p_orphan_handling.py:517 in 65f69bf533 outdated

512+        # 4. Honest peer relays the real child, which is also missing parents and should be placed
513+        # in the orphanage.
514+        with node.assert_debug_log(["stored orphan tx"]):
515+            honest_peer.send_and_ping(msg_tx(tx_middle["tx"]))
516+
517+        # 5. The parent is requested. Honest peer sends it.

instagibbs commented at 4:40 pm on May 10, 2024:

0        assert_equal(node.getrawmempool(), [])
1
2        # 5. The parent is requested. Honest peer sends it.

glozow commented at 1:19 pm on May 13, 2024:

added

in test/functional/p2p_orphan_handling.py:519 in 65f69bf533 outdated

514+        with node.assert_debug_log(["stored orphan tx"]):
515+            honest_peer.send_and_ping(msg_tx(tx_middle["tx"]))
516+
517+        # 5. The parent is requested. Honest peer sends it.
518+        honest_peer.send_message(msg_tx(tx_grandparent["tx"]))
519+        honest_peer.sync_with_ping()

instagibbs commented at 4:42 pm on May 10, 2024:

is send_message + sync_with_ping always enough to resolve two generations of orphans to ensure we hit the mempool?

glozow commented at 1:39 pm on May 13, 2024:

I would say so. We do all ProcessOrphanTx before processing the ping; the child will be added to workset when the parent is accepted through ProcessOrphanTx

in test/functional/p2p_orphan_handling.py:552 in 65f69bf533 outdated

547+        # 2. Node requests the missing parent by txid.
548+        parent_txid_int = int(tx_parent["txid"], 16)
549+        node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY)
550+        bad_peer.wait_for_getdata([parent_txid_int])
551+
552+        # 3. Honest peer announces the real parent, by txid (this isn't common but the node should

instagibbs commented at 4:44 pm on May 10, 2024:

0        # 3. Honest peer announces the real child by txid (this isn't common but the node should

glozow commented at 1:40 pm on May 13, 2024:

gah thanks

in test/functional/p2p_orphan_handling.py:531 in 65f69bf533 outdated

526+        assert tx_grandchild["txid"] in node_mempool
527+        assert_equal(node.getmempoolentry(tx_middle["txid"])["wtxid"], tx_middle["wtxid"])
528+
529+    @cleanup
530+    def test_orphan_txid_inv(self):
531+        self.log.info("Check what happens when node receives announcement with same txid as tx in orphanage")

instagibbs commented at 4:48 pm on May 10, 2024:

micronit to sound more prescriptive

0        self.log.info("Check node handles receiving announcement with same txid as tx in orphanage")

instagibbs commented at 4:50 pm on May 10, 2024:

note for readers: If the child tx ended up being the same wtxid, and in that case the node would simply not add to the orphanage or request anything further.

glozow commented at 1:39 pm on May 13, 2024:

changed to be more perscriptive

instagibbs commented at 4:53 pm on May 10, 2024: member

reviewed through 65f69bf533f66ecfb2695b03204bd437f1fe692d

Given that a Bitcoin Core node doesn’t ever do witness replacement, this seems like unnecessary complexity that won’t ever get used between normally-operating nodes. So I did not add this.

Makes sense, seems unlikely to ever be used in a 20 minute window lifetime of an orphan given that we don’t do these kinds of replacements.

also opened #30082 seeing we’re touching EraseForPeer et al a bit in not quite “trivial” ways

in test/functional/p2p_orphan_handling.py:574 in 65f69bf533 outdated

569+        # 6. After parent is accepted, orphans should be reconsidered.
570+        # The real child should be accepted and the fake one rejected.
571+        node_mempool = node.getrawmempool()
572+        assert tx_parent["txid"] in node_mempool
573+        assert tx_child["txid"] in node_mempool
574+        assert node.getmempoolentry(tx_child["txid"])["wtxid"] == tx_child["wtxid"]

theStack commented at 10:07 pm on May 12, 2024:

nit:

0        assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"])

glozow commented at 1:41 pm on May 13, 2024:

done

in test/functional/p2p_orphan_handling.py:474 in 65f69bf533 outdated

469+        # 5. After parent is accepted, orphans should be reconsidered.
470+        # The real child should be accepted and the fake one rejected.
471+        node_mempool = node.getrawmempool()
472+        assert tx_parent["txid"] in node_mempool
473+        assert tx_child["txid"] in node_mempool
474+        assert node.getmempoolentry(tx_child["txid"])["wtxid"] == tx_child["wtxid"]

theStack commented at 10:23 pm on May 12, 2024:

nit:

0        assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"])

glozow commented at 1:17 pm on May 13, 2024:

done

theStack commented at 10:28 pm on May 12, 2024: contributor

Concept ACK

Code changes look good at first glance, still thinking about the implications of 9d2e9054134cedbf688368445eb6d2de14e2a8bd (w.r.t. discussion in #30000 (review)). Verified that both the unit tests and each of the newly introduced functional sub-tests fail without the main commit 4206ba3a36b329d1cc41df93cf59b685912d6d94, as expected.

glozow force-pushed on May 13, 2024

glozow commented at 2:00 pm on May 13, 2024: member

Thanks @instagibbs @theStack @sr-gi! Addressed the comments and added a few minor log improvements.

in src/txorphanage.cpp:66 in 838f63e2f4 outdated

70+    auto it_by_wtxid = m_wtxid_to_orphan_it.find(wtxid);
71+    if (it_by_wtxid == m_wtxid_to_orphan_it.end()) return 0;
72+
73+    std::map<Txid, OrphanTx>::iterator it = it_by_wtxid->second;
74     if (it == m_orphans.end())
75         return 0;

stickies-v commented at 3:41 pm on May 13, 2024:

In c31f148166f01a9167d82501a77823785d28a841:

If this is indeed a refactor commit, I think this line should never be hit. I ran the unit and functional tests with the below diff and didn’t get any errors locally. Still, perhaps worth updating the commit like this to make that explicit, and then remove the assertion again in the next commit?

 0diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp
 1index ca8a0e3a92..7132660695 100644
 2--- a/src/txorphanage.cpp
 3+++ b/src/txorphanage.cpp
 4@@ -68,7 +68,7 @@ int TxOrphanage::EraseTxNoLock(const Wtxid& wtxid)
 5 
 6     std::map<Txid, OrphanTx>::iterator it = it_by_wtxid->second;
 7     if (it == m_orphans.end())
 8-        return 0;
 9+        Assert(false);  // we should have already returned early based on it_by_wtxid.
10     for (const CTxIn& txin : it->second.tx->vin)
11     {
12         auto itPrev = m_outpoint_to_orphan_it.find(txin.prevout);

glozow commented at 8:10 am on May 16, 2024:

Good idea. Though it seems that this doesn’t apply anymore since the commit is already merged, so resolving.

in src/test/orphanage_tests.cpp:195 in 822b2f1477 outdated

189@@ -180,6 +190,49 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
190     BOOST_CHECK(orphanage.CountOrphans() == 0);
191 }
192 
193+BOOST_AUTO_TEST_CASE(same_txid_diff_witness)
194+{
195+    FastRandomContext det_rand{true};

stickies-v commented at 4:43 pm on May 13, 2024:

tidy nit:

0    FastRandomContext det_rand{/*fDeterministic=*/true};

DrahtBot added the label CI failed on May 13, 2024

in test/functional/p2p_orphan_handling.py:519 in 1aea9f2157 outdated

514+        # in the orphanage.
515+        with node.assert_debug_log(["stored orphan tx"]):
516+            honest_peer.send_and_ping(msg_tx(tx_middle["tx"]))
517+        assert_equal(len(node.getrawmempool()), 0)
518+
519+        # 5. Hondest peer sends tx_grandparent

instagibbs commented at 4:58 pm on May 13, 2024:

Hondest

glozow commented at 9:04 am on May 14, 2024:

haha fixed

glozow force-pushed on May 13, 2024

glozow commented at 5:03 pm on May 13, 2024: member

Ah, p2p_invalid_tx.py failed because assert_debug_log didn’t match anymore (those should be unit tests!)

in test/functional/p2p_orphan_handling.py:543 in 1aea9f2157 outdated

538+        tx_child = self.wallet.create_self_transfer(utxo_to_spend=tx_parent["new_utxo"])
539+        tx_orphan_bad_wit = self.create_malleated_version(tx_child)
540+
541+        bad_peer = node.add_p2p_connection(PeerTxRelayer())
542+        # Must not send wtxidrelay because otherwise the inv(TX) will be ignored later
543+        honest_peer = node.add_p2p_connection(PeerTxRelayer(wtxidrelay=False))

instagibbs commented at 5:06 pm on May 13, 2024:

I don’t think either of these need to be PeerTxRelayer, just use P2PInterface and pass in wtxidrelay to remove the custom on_version handshake?

glozow commented at 9:05 am on May 14, 2024:

Ah I missed that P2PInterface already had that! Thanks, deleted the unnecessary stuff.

in src/txorphanage.cpp:50 in 16483fee7c outdated

46@@ -47,7 +47,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
47         m_outpoint_to_orphan_it[txin.prevout].insert(ret.first);
48     }
49 
50-    LogPrint(BCLog::TXPACKAGES, "stored orphan tx %s (wtxid=%s) (mapsz %u outsz %u)\n", hash.ToString(), wtxid.ToString(),
51+    LogPrint(BCLog::TXPACKAGES, "stored orphan tx %s (wtxid=%s), size: %u (mapsz %u outsz %u)\n", hash.ToString(), wtxid.ToString(), sz,

instagibbs commented at 5:08 pm on May 13, 2024:

weight no “size”? :pleading_face:

glozow commented at 9:10 am on May 14, 2024:

changed to “weight” !

instagibbs commented at 5:19 pm on May 13, 2024: member

reviewed through 16483fee7c6d93722bfb79fce9efbe841ec13d6a

git range-diff master 65f69bf 16483fee7c6d93722bfb79fce9efbe841ec13d6a

in test/functional/p2p_orphan_handling.py:571 in 1aea9f2157 outdated

566+        node.bumpmocktime(GETDATA_TX_INTERVAL)
567+        honest_peer.wait_for_getdata([parent_txid_int])
568+        honest_peer.send_and_ping(msg_tx(tx_parent["tx"]))
569+
570+        # 6. After parent is accepted, orphans should be reconsidered.
571+        # The real child should be accepted and the fake one rejected.

mzumsande commented at 8:48 pm on May 13, 2024:

i don’t think that’s a problem, just mentioning that whether we disconnect the bad peer for misbehavior depends on the order in which the ProcessOrphanTx is called, so it could be random: If the valid pair of txns is processed first, the invalid tx is removed with “txn-same-nonwitness-data-in-mempool” and no punishment. If the invalid pair is processed first, the bad peer is disconnected.

glozow commented at 9:09 am on May 14, 2024:

Good point, I’ve added a comment to help explain why we aren’t asserting the exact error/disconnection.

in src/net_processing.cpp:2300 in 16483fee7c outdated

2294@@ -2295,7 +2295,12 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside
2295 
2296     const uint256& hash = gtxid.GetHash();
2297 
2298-    if (m_orphanage.HaveTx(gtxid)) return true;
2299+    // Always query by wtxid, even if gtxid has a Txid type.
2300+    // It is possible that the transaction in the orphanage has the same txid but a different
2301+    // witness (e.g. malleated by an attacker) and we don't want to return false positives.

AngusP commented at 9:40 pm on May 13, 2024:

Nit: given the discussion in the PR, you could extend this with a note on why we might still want to keep multiple same-txid-diff-witness transactions in the orphanage

0    // It is possible that the transaction in the orphanage has the same txid but a different
1    // witness (e.g. malleated by an attacker) and we don't want to return false positives,
2    // nor can we tell which of the same-txid-different-witness transactions in the orphanage
3    // could be evicted without seeing their parent transactions.

glozow commented at 9:33 am on May 14, 2024:

Added a sentence to the comment, though slightly different wording. I’ve also expanded it in general.

in test/functional/p2p_orphan_handling.py:504 in 16483fee7c outdated

499+        # 2. Node requests missing tx_grandparent by txid.
500+        grandparent_txid_int = int(tx_grandparent["txid"], 16)
501+        node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY)
502+        bad_peer.wait_for_getdata([grandparent_txid_int])
503+
504+        # 3. Honest peer relays the granchild, which is missing a parent. The parent by txid already

AngusP commented at 10:02 pm on May 13, 2024:

0        # 3. Honest peer relays the grandchild, which is missing a parent. The parent by txid already

glozow commented at 9:04 am on May 14, 2024:

fixed

AngusP approved

AngusP commented at 10:04 pm on May 13, 2024: contributor

tACK 16483fee7c6d93722bfb79fce9efbe841ec13d6a with some nits

DrahtBot requested review from theStack on May 13, 2024

DrahtBot requested review from stickies-v on May 13, 2024

DrahtBot requested review from instagibbs on May 13, 2024

DrahtBot requested review from itornaza on May 13, 2024

DrahtBot requested review from sr-gi on May 13, 2024

in test/functional/p2p_orphan_handling.py:465 in 1aea9f2157 outdated

460+        # from multiple nodes at the same time)
461+        node.bumpmocktime(GETDATA_TX_INTERVAL)
462+
463+        # 4. The parent is requested. Honest peer sends it.
464+        honest_peer.wait_for_getdata([parent_txid_int])
465+        honest_peer.send_message(msg_tx(tx_parent["tx"]))

mzumsande commented at 11:07 pm on May 13, 2024:

nit: could use send_and_ping here too (as in the other subtests).

glozow commented at 9:04 am on May 14, 2024:

done

mzumsande commented at 11:17 pm on May 13, 2024: contributor

Tested ACK 16483fee7c6d93722bfb79fce9efbe841ec13d6a

I was at first skeptical about the first commit for similar reasons as stickies-v above, but after thinking about it more and reading the discussion it makes sense to me to cast a Txid to a Wtxid here. I also played with the functional tests a bit (e.g. giving the parent in test_orphan_txid_inv a low fee and testing that everything still works if 1p1c package validation is used).

DrahtBot removed the label CI failed on May 14, 2024

[p2p] don't query orphanage by txid 7e475b9648

[refactor] TxOrphanage::HaveTx only by wtxid efcc593017

[refactor] TxOrphanage::EraseTx by wtxid

No behavior change right now, as transactions in the orphanage are
unique by txid. This makes the next commit easier to review.

c31f148166

[p2p] allow entries with the same txid in TxOrphanage

Index by wtxid instead of txid to allow entries with the same txid but
different witnesses in orphanage. This prevents an attacker from
blocking a transaction from entering the orphanage by sending a mutated
version of it.

8923edfc1f

[unit test] TxOrphanage handling of same-txid-different-witness txns 6675f6428d

glozow force-pushed on May 14, 2024

glozow commented at 9:36 am on May 14, 2024: member

Thanks @mzumsande @AngusP @instagibbs! Addressed comments. I’ve also expanded the “cast from txid to wtxid” part to be slightly more verbose but hopefully easier to understand.

[functional test] attackers sending mutated orphans b16da7eda7

[log] updates in TxOrphanage

- Add elapsed time in "remove orphan" log
- Add size in "stored orphan" log
- grammar edit

0fb17bf61a

glozow force-pushed on May 14, 2024

DrahtBot added the label CI failed on May 14, 2024

DrahtBot commented at 9:39 am on May 14, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24941257406

DrahtBot removed the label CI failed on May 14, 2024

instagibbs approved

instagibbs commented at 11:57 am on May 14, 2024: member

ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22

reviewed via git range-diff master 16483fee7c6d93722bfb79fce9efbe841ec13d6a 0fb17bf61a40b73a2b81a18e70b3de180c917f22

DrahtBot requested review from AngusP on May 14, 2024

DrahtBot requested review from mzumsande on May 14, 2024

in src/net_processing.cpp:2311 in 7e475b9648 outdated

2307+        //
2308+        // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
2309+        // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
2310+        // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
2311+        if (m_orphanage.HaveTx(GenTxid::Wtxid(hash))) return true;
2312+    }

stickies-v commented at 2:36 pm on May 14, 2024:

In 7e475b9648bbee04f5825b922ba0399373eaa5a9:

I think having the if/else path in this commit is helpful, since it distinguishes between when we cast the txid and when we don’t.

nit: I think this already works pretty well, but some bits (e.g. “guess what the wtxid is”) are not super clear imo. I’ve summarized my understanding into the below, feel free to use what you like.

 0        // Never query by txid: it is possible that an existing transaction in the orphanage has the
 1        // same txid but a different witness, which would give us a false positive result. If we
 2        // don't request the transaction based on this result, an attacker could prevent us from
 3        // downloading a transaction by intentionally creating a malleated version of it.
 4        //
 5        // False positive are unsafe and must be avoided.False negatives are acceptable and just
 6        // lead to increased bandwidth usage.
 7        //
 8        // When gtxid is a txid it means when we don't have enough information to query by wtxid,
 9        // e.g. because the transaction was announced by a non-wtxid-relay peer, or because we're
10        // checking the prevouts of a transaction. It doesn't mean that the transaction has no
11        // witness data, so we need to distinguish between those cases.
12        //
13        // If the transaction:
14        // 1) does not have witness data, we can safely query the orphanage by casting a txid to a
15        //    wtxid because they are equivalent. False positives and false negatives are impossible.
16        // 2) has witness data, the txid and wtxid are different by definition, making this query a
17        //    no-op. False positives are not possible, but we may have false negatives.
18        //
19        // So, by querying for a txid cast to a wtxid, we avoid false positives entirely, but we
20        // save bandwidth for the case where txid == wtxid.

side-note rant: I think the fact that we need such a verbose comment is a pretty strong sign AlreadyHaveTx should be refactored, e.g. using {Wt,T}xid types as I suggested here, but I appreciate that it’s mostly orthogonal to this PR. Renaming AlreadyHaveTx to e.g. ShouldRequestTx or something would probably already clarify quite a bit, too (although also not blocking for this PR either).

glozow commented at 8:28 am on May 16, 2024:

I’ve summarized my understanding into the below

Looks correct to me

I think the fact that we need such a verbose comment is a pretty strong sign AlreadyHaveTx should be refactored, e.g. using {Wt,T}xid types as I suggested #30000 (review), but I appreciate that it’s mostly orthogonal to this PR.

I think it’s really a sign to ban txid-based transaction relay. Just kidding. I agree it’s hard to reason about logic dealing with a hash that can be txid, wtxid, or bot. I think it’s just one of the common things that trips people in tx relay, just like everybody’s mempool fuzzer gets snagged on CPFP carve out, and that for loop in init.cpp that claims reviewer victims every time it’s touched.

Renaming AlreadyHaveTx to e.g. ShouldRequestTx or something would probably already clarify quite a bit, too (although also not blocking for this PR either).

Yeah, AlreadyHaveTx should probably have a name that reflects its purpose of “should I bother {requesting, downloading, validating} this tx”.

Do note that #30110 makes AlreadyHaveTx a function internal to the TxDownloadImpl, i.e. no longer something that peerman knows about. Funnily, TxDownloadImpl::AlreadyHaveTx might be a fine name when it’s in that context. I get that it’s confusing, but I’d ask that we defer renaming until after that PR since it would conflict + maybe be less problematic afterwards.

in src/net_processing.cpp:2314 in 0fb17bf61a

2310+        //
2311+        // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
2312+        // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
2313+        // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
2314+        if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true;
2315+    }

glozow commented at 3:20 pm on May 14, 2024:

(since @sr-gi mentioned this offline) Yes, the last push turned this 1 line into 2 identical lines. The purpose is to illustrate to current reviewers and future readers more clearly… if wtxid, we do this. if txid, we also do this, but notice that we are actually doing a “cast” and that’s ok because we are guessing what the wtxid is.

Also, perhaps these will diverge again in the future if GenTxid becomes a std::variant<Txid, Wtxid>.

in src/txorphanage.cpp:26 in 0fb17bf61a

22@@ -23,7 +23,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
23 
24     const Txid& hash = tx->GetHash();
25     const Wtxid& wtxid = tx->GetWitnessHash();
26-    if (m_orphans.count(hash))
27+    if (m_orphans.count(wtxid))

stickies-v commented at 3:21 pm on May 14, 2024:

c++-20 nit:

0    if (m_orphans.contains(wtxid))

in src/txorphanage.cpp:175 in 0fb17bf61a

176-    if (gtxid.IsWtxid()) {
177-        return m_wtxid_to_orphan_it.count(Wtxid::FromUint256(gtxid.GetHash()));
178-    } else {
179-        return m_orphans.count(Txid::FromUint256(gtxid.GetHash()));
180-    }
181+    return m_orphans.count(wtxid);

stickies-v commented at 3:22 pm on May 14, 2024:

c++20-nit:

0    return m_orphans.contains(wtxid);

in src/net_processing.cpp:2308 in 8923edfc1f outdated

2301@@ -2302,7 +2302,10 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside
2302         // Never query by txid: it is possible that the transaction in the orphanage has the same
2303         // txid but a different witness, which would give us a false positive result. If we decided
2304         // not to request the transaction based on this result, an attacker could prevent us from
2305-        // downloading a transaction by intentionally creating a malleated version of it.
2306+        // downloading a transaction by intentionally creating a malleated version of it.  While
2307+        // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
2308+        // discerning which one that is, so the orphanage can store multiple transactions with the
2309+        // same txid.

stickies-v commented at 3:32 pm on May 14, 2024:

In 8923edfc1f12ebc6a074651c084ba7d249074799:

nit: I feel like documentation on why TxOrphanage indexes by wtxid/allows multiple versions of the same transaction should be documented in TxOrphanage instead of here.

AngusP commented at 4:20 pm on May 14, 2024: contributor

ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22

in test/functional/p2p_orphan_handling.py:436 in 0fb17bf61a

431+        tx_orphan_bad_wit = self.create_malleated_version(tx_child)
432+
433+        bad_peer = node.add_p2p_connection(P2PInterface())
434+        honest_peer = node.add_p2p_connection(P2PInterface())
435+
436+        # 1. Fake orphan is received first. It is missing an input.

stickies-v commented at 4:31 pm on May 14, 2024:

nit: I first interpreted that as that an input had been removed from the transaction, perhaps better to use “parent” as to stay consistent with the language in the next steps?

0        # 1. Fake orphan is received first, the parent is not yet broadcast.

in test/functional/p2p_orphan_handling.py:431 in b16da7eda7 outdated

426+
427+        # Create the real child
428+        tx_child = self.wallet.create_self_transfer(utxo_to_spend=tx_parent["new_utxo"])
429+
430+        # Create a fake version of the child
431+        tx_orphan_bad_wit = self.create_malleated_version(tx_child)

theStack commented at 4:42 pm on May 14, 2024:

nit, potential follow-up material: the test is imho a bit easier to follow if the naming refers to the original (non-malleated) counter-part:

0        tx_child_bad_wit = self.create_malleated_version(tx_child)

(same as for the other two sub-tests below)

in test/functional/p2p_orphan_handling.py:476 in 0fb17bf61a

471+
472+        # Create middle tx (both parent and child) which will be in orphanage.
473+        tx_middle = self.wallet.create_self_transfer(utxo_to_spend=tx_grandparent["new_utxo"])
474+
475+        # Create a fake version of the middle tx
476+        tx_orphan_bad_wit = self.create_malleated_version(tx_middle)

stickies-v commented at 4:45 pm on May 14, 2024:

nit: would tx_middle_bad_wit be more consistent naming here?

theStack approved

theStack commented at 4:55 pm on May 14, 2024: contributor

ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22

I like the latest, more verbose version of the first commit (7e475b9648bbee04f5825b922ba0399373eaa5a9), as it was significantly easier to grasp what’s going on and convince myself that this is the behaviour we want. The only small drawback I see that it looks odd if both the if- and else-branch do exactly the same thing, and it could invite people to open deduplication refactor PRs (or leave them wondering if it’s a bug). Maybe adding some “this is intended” comment would make sense for a follow-up.

in test/functional/p2p_orphan_handling.py:459 in 0fb17bf61a

454+        honest_peer.wait_for_getdata([parent_txid_int])
455+        # Sync with ping to ensure orphans are reconsidered
456+        honest_peer.send_and_ping(msg_tx(tx_parent["tx"]))
457+
458+        # 5. After parent is accepted, orphans should be reconsidered.
459+        # The real child should be accepted and the fake one rejected.

sr-gi commented at 5:36 pm on May 14, 2024:

In b16da7eda76944719713be68b61f03d4acdd3e16

The fake one is not really rejected, is it? It’ll stay in the orphanage until a new block is mined (or until it times out)

glozow commented at 10:10 am on May 15, 2024:

It is rejected. The acceptance of the parent triggers the node to reconsider all children in orphanage (grep AddChildrenToWorkset. If the failure isn’t TX_MISSING_INPUTS, it is removed from orphanage. https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net_processing.cpp#L3402-L3414

sr-gi commented at 1:09 pm on May 15, 2024:

Oh my bad, I was thinking about children that may have multiple missing parents, not only the conflicting one.

in test/functional/p2p_orphan_handling.py:563 in 0fb17bf61a

558+
559+        # 6. After parent is accepted, orphans should be reconsidered.
560+        # The real child should be accepted and the fake one rejected. This may happen in either
561+        # order since the message-processing is randomized. If tx_orphan_bad_wit is validated first,
562+        # its consensus error leads to disconnection of bad_peer. If tx_child is validated first,
563+        # tx_orphan_bad_wit is rejected for txn-same-nonwitness-data-in-mempool (no punishment).

sr-gi commented at 5:47 pm on May 14, 2024:

In b16da7eda76944719713be68b61f03d4acdd3e16

Is this the case? Didn’t we prioritize children from the same peer, so given the parent was given by the honest, and he also has presented a child, we will pick that one?

https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net_processing.cpp#L3341-L3344

https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net_processing.cpp#L3361-L3363

glozow commented at 10:08 am on May 15, 2024:

You’re referring to the 1p1c logic, which isn’t executed here (notice that all of the feerates are “normal” feerates). Here, the orphans are reconsidered with ProcessOrphanTx, which is called at the top of ProcessMessages. https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net_processing.cpp#L5355-L5369

ProcessMessages is called for each peer in a random order https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net.cpp#L2945-L2955

sr-gi commented at 1:09 pm on May 15, 2024:

Yep, my bad, nvm

AngusP commented at 5:48 pm on May 14, 2024: contributor

Maybe adding some “this is intended” comment would make sense for a follow-up.

Could make the ‘cast’ explicit in the second branch, though there’s already the comment making it pretty clear this is intentional so may be unnecessary

0const auto guessed_wtxid = Wtxid::FromUint256(hash);
1if (m_orphanage.HaveTx(guessed_wtxid)) return true;

sr-gi commented at 5:48 pm on May 14, 2024: member

crACK 0fb17bf

Non-blocking comments

stickies-v approved

stickies-v commented at 6:34 pm on May 14, 2024: contributor

ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22

It took me a while to grok the implications of this PR, but the latest version of this PR makes it much more reviewable and I’m now comfortable that the changes are safe and desired. Also, nice work on the tests!

What I missed/misunderstood in my earlier review is that the reduced orphanage hit rate is a goal, and not a bug/something to minimize. Related nit: perhaps the commit message of 7e475b9648bbee04f5825b922ba0399373eaa5a9 could be explicit about this reduction and why we want it?

I remain uneasy about AlreadyHaveTx() not really doing what the name indicates (because of the GenTxid ambiguity), but is not a huge concern and I think should not block this PR / can be addressed separately if it is desired.

itornaza commented at 6:34 pm on May 14, 2024: none

trACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22

ryanofsky assigned ryanofsky on May 15, 2024

ryanofsky merged this on May 15, 2024

ryanofsky closed this on May 15, 2024

ryanofsky commented at 1:58 pm on May 15, 2024: contributor

I merged this as-is since it has so many acks, but it probably makes sense to follow up on the last few comments and suggestions.

glozow deleted the branch on May 15, 2024

p2p: index TxOrphanage by wtxid, allow entries with same txid #30000

Code Coverage

Reviews

Conflicts