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

pull glozow wants to merge 7 commits into bitcoin:master from glozow:2024-04-orphan-use-wtxid changing 7 files +296 −65
  1. 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.

  2. 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.

  3. DrahtBot added the label P2P on Apr 29, 2024
  4. osarukun approved
  5. DrahtBot added the label CI failed on Apr 29, 2024
  6. 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

  7. glozow commented at 3:26 pm on April 30, 2024: member
    Will fix the linter when I rebase
  8. glozow force-pushed on May 1, 2024
  9. DrahtBot removed the label CI failed on May 1, 2024
  10. glozow marked this as ready for review on May 1, 2024
  11. 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
  12. 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.
  13. 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
  14. 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:
  15. instagibbs commented at 4:33 pm on May 1, 2024: member

    code changes are quite mechanical, thankfully

    concept ACK

  16. 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
  17. 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
  18. 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.
  19. 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.
  20. 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.
  21. 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
  22. 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
  23. 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.

  24. sr-gi commented at 7:51 pm on May 3, 2024: member
    Concept ACK
  25. glozow force-pushed on May 10, 2024
  26. 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.

  27. 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.
  28. 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
  29. 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
  30. 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
  31. 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
  32. 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
  33. 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
  34. 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
  35. 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
  36. 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

  37. 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
  38. 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
  39. 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.

  40. glozow force-pushed on May 13, 2024
  41. 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.
  42. 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.
  43. 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};
    
  44. DrahtBot added the label CI failed on May 13, 2024
  45. 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
  46. glozow force-pushed on May 13, 2024
  47. 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!)
  48. 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.
  49. 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” !
  50. instagibbs commented at 5:19 pm on May 13, 2024: member

    reviewed through 16483fee7c6d93722bfb79fce9efbe841ec13d6a

    git range-diff master 65f69bf 16483fee7c6d93722bfb79fce9efbe841ec13d6a

  51. 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.
  52. 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.
  53. 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
  54. AngusP approved
  55. AngusP commented at 10:04 pm on May 13, 2024: contributor
    tACK 16483fee7c6d93722bfb79fce9efbe841ec13d6a with some nits
  56. DrahtBot requested review from theStack on May 13, 2024
  57. DrahtBot requested review from stickies-v on May 13, 2024
  58. DrahtBot requested review from instagibbs on May 13, 2024
  59. DrahtBot requested review from itornaza on May 13, 2024
  60. DrahtBot requested review from sr-gi on May 13, 2024
  61. 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
  62. 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).

  63. DrahtBot removed the label CI failed on May 14, 2024
  64. [p2p] don't query orphanage by txid 7e475b9648
  65. [refactor] TxOrphanage::HaveTx only by wtxid efcc593017
  66. [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
  67. [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
  68. [unit test] TxOrphanage handling of same-txid-different-witness txns 6675f6428d
  69. glozow force-pushed on May 14, 2024
  70. 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.
  71. [functional test] attackers sending mutated orphans b16da7eda7
  72. [log] updates in TxOrphanage
    - Add elapsed time in "remove orphan" log
    - Add size in "stored orphan" log
    - grammar edit
    0fb17bf61a
  73. glozow force-pushed on May 14, 2024
  74. DrahtBot added the label CI failed on May 14, 2024
  75. 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

  76. DrahtBot removed the label CI failed on May 14, 2024
  77. instagibbs approved
  78. instagibbs commented at 11:57 am on May 14, 2024: member

    ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22

    reviewed via git range-diff master 16483fee7c6d93722bfb79fce9efbe841ec13d6a 0fb17bf61a40b73a2b81a18e70b3de180c917f22

  79. DrahtBot requested review from AngusP on May 14, 2024
  80. DrahtBot requested review from mzumsande on May 14, 2024
  81. 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.

  82. 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>.

  83. 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))
    
  84. 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);
    
  85. 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.

  86. AngusP commented at 4:20 pm on May 14, 2024: contributor
    ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22
  87. 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.
    
  88. 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)

  89. 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?
  90. theStack approved
  91. 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.

  92. 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.
  93. 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
  94. 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;
    
  95. sr-gi commented at 5:48 pm on May 14, 2024: member

    crACK 0fb17bf

    Non-blocking comments

  96. stickies-v approved
  97. 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.

  98. itornaza commented at 6:34 pm on May 14, 2024: none
    trACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22
  99. ryanofsky assigned ryanofsky on May 15, 2024
  100. ryanofsky merged this on May 15, 2024
  101. ryanofsky closed this on May 15, 2024

  102. 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.
  103. glozow deleted the branch on May 15, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me