p2p: opportunistically accept 1-parent-1-child packages #28970

pull glozow wants to merge 9 commits into bitcoin:master from glozow:2023-11-1p1c changing 11 files +1207 −15
  1. glozow commented at 4:25 pm on November 29, 2023: member

    This enables 1p1c packages to propagate in the “happy case” (i.e. not reliable if there are adversaries) and contains a lot of package relay-related code. See #27463 for overall package relay tracking.

    Rationale: This is “non-robust 1-parent-1-child package relay” which is immediately useful.

    • Relaying 1-parent-1-child CPFP when mempool min feerate is high would be a subset of all package relay use cases, but a pretty significant improvement over what we have today, where such transactions don’t propagate at all. [1]
    • Today, a miner can run this with a normal/small maxmempool to get revenue from 1p1c CPFP’d transactions without losing out on the ones with parents below mempool minimum feerate.
    • The majority of this code is useful for building more featureful/robust package relay e.g. see the code in #27742.

    The first 2 commits are followups from #29619:

    Q: What makes this short of a more full package relay feature?

    (1) it only supports packages in which 1 of the parents needs to be CPFP’d by the child. That includes 1-parent-1-child packages and situations in which the other parents already pay for themselves (and are thus in mempool already when the package is submitted). More general package relay is a future improvement that requires more engineering in mempool and validation - see #27463.

    (2) We rely on having kept the child in orphanage, and don’t make any attempt to protect it while we wait to receive the parent. If we are experiencing a lot of orphanage churn (e.g. an adversary is purposefully sending us a lot of transactions with missing inputs), we will fail to submit packages. This limitation has been around for 12+ years, see #27742 which adds a token bucket scheme for protecting package-related orphans at a limited rate per peer.

    (3) Our orphan-handling logic is somewhat opportunistic; we don’t make much effort to resolve an orphan beyond asking the child’s sender for the parents. This means we may miss packages if the first sender fails to give us the parent (intentionally or unintentionally). To make this more robust, we need receiver-side logic to retry orphan resolution with multiple peers. This is also an existing problem which has a proposed solution in #28031.

    [1]: see this writeup and its links https://github.com/bitcoin/bips/blob/02ec218c7857ef60914e9a3d383b68caf987f70b/bip-0331.mediawiki#propagate-high-feerate-transactions

  2. glozow added the label P2P on Nov 29, 2023
  3. DrahtBot commented at 4:25 pm on November 29, 2023: 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, sr-gi, theStack, dergoegge, achow101
    Concept ACK murchandamus, sdaftuar

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. glozow renamed this:
    p2p: opportunistically accept 1-parent-1-child packages
    [WIP] p2p: opportunistically accept 1-parent-1-child packages
    on Nov 29, 2023
  5. DrahtBot added the label CI failed on Nov 29, 2023
  6. DrahtBot added the label Needs rebase on Dec 11, 2023
  7. glozow force-pushed on Dec 11, 2023
  8. glozow force-pushed on Dec 11, 2023
  9. DrahtBot removed the label CI failed on Dec 11, 2023
  10. DrahtBot removed the label Needs rebase on Dec 11, 2023
  11. glozow commented at 7:31 pm on December 11, 2023: member
    Rebased and fixed CI. This is in draft because I’m focusing on v3 stuff, can be reviewed for its approach.
  12. glozow force-pushed on Dec 18, 2023
  13. DrahtBot added the label CI failed on Jan 16, 2024
  14. instagibbs commented at 2:43 pm on February 22, 2024: member
    ready for un-draft?
  15. glozow force-pushed on Feb 22, 2024
  16. DrahtBot removed the label CI failed on Feb 22, 2024
  17. glozow renamed this:
    [WIP] p2p: opportunistically accept 1-parent-1-child packages
    p2p: opportunistically accept 1-parent-1-child packages
    on Feb 23, 2024
  18. glozow marked this as ready for review on Feb 27, 2024
  19. in src/net_processing.cpp:3079 in bbf1b836fa outdated
    3085-            m_orphanage.AddChildrenToWorkSet(*porphanTx);
    3086-            m_orphanage.EraseTx(orphanHash);
    3087-            for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) {
    3088-                AddToCompactExtraTransactions(removedTx);
    3089-            }
    3090+            ProcessValidTx(porphanTx, peer.m_id, result.m_replaced_transactions.value());
    


    instagibbs commented at 4:26 pm on February 28, 2024:
    I know it’s populated due to this being accepted, but I think Assume(result.m_replaced_transactions.has_value()) would be a good belt and suspenders to avoid UB in case of regression e.g., we redefined std::nulltopt mean no replacements
  20. in src/net_processing.cpp:590 in 3eddfb2147 outdated
    581@@ -582,6 +582,15 @@ class PeerManagerImpl final : public PeerManager
    582      */
    583     bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer);
    584 
    585+    enum class InvalidTxTask : uint8_t {
    586+        NONE,
    587+        ADD_ORPHAN,
    588+    };
    589+    /** Handle a transaction whose result was MempoolAcceptResult::ResultType::INVALID.
    590+     * @returns true if this transaction is an orphan we should try to resolve. */
    


    instagibbs commented at 4:34 pm on February 28, 2024:
    this doesn’t return a boolean
  21. in src/net_processing.cpp:4377 in 3eddfb2147 outdated
    4384-            unique_parents.reserve(tx.vin.size());
    4385-            for (const CTxIn& txin : tx.vin) {
    4386-                // We start with all parents, and then remove duplicates below.
    4387-                unique_parents.push_back(txin.prevout.hash);
    4388+            for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) {
    4389+                AddToCompactExtraTransactions(removedTx);
    


    instagibbs commented at 4:38 pm on February 28, 2024:
    copy/paste error? isn’t this already done in ProcessValidTx just above?

    glozow commented at 10:19 am on March 11, 2024:
    fixed
  22. in src/net_processing.cpp:3109 in 3eddfb2147 outdated
    3238-                    // We only add the txid if it differs from the wtxid, to
    3239-                    // avoid wasting entries in the rolling bloom filter.
    3240-                    m_recent_rejects.insert(porphanTx->GetHash().ToUint256());
    3241-                }
    3242-            }
    3243-            m_orphanage.EraseTx(orphanHash);
    


    instagibbs commented at 2:43 pm on March 4, 2024:
    previously MISSING_INPUTS would have resulted in a continuation of the loop. A side-effect of this change is that we will detect if the orphan was rejected due to m_recent_rejects entries of its parents, which seems like a strict improvement?

    glozow commented at 2:30 pm on March 8, 2024:
    Rewrote to be more strict refactor to give reviewers less to think about. I do think it makes sense that if a parent has been rejected since the last time we looked at this orphan, we get rid of it, but I suppose we can think about that later.
  23. in src/net_processing.cpp:3104 in 3eddfb2147 outdated
    3099+            // parents so avoid re-requesting it from other peers.
    3100+            // Here we add both the txid and the wtxid, as we know that
    3101+            // regardless of what witness is provided, we will not accept
    3102+            // this, so we don't need to allow for redownload of this txid
    3103+            // from any of our non-wtxidrelay peers.
    3104+            m_recent_rejects.insert(tx->GetHash().ToUint256());
    


    instagibbs commented at 2:45 pm on March 4, 2024:
    future work: might make sense to delete this transaction from the orphanage as well in this case if it exists so we don’t try it with other peer?
  24. in src/net_processing.cpp:3060 in 3eddfb2147 outdated
    3055+             nodeid,
    3056+             state.ToString());
    3057+    // Maybe punish peer that gave us an tx
    3058+    MaybePunishNodeForTx(nodeid, state);
    3059+
    3060+    switch (state.GetResult()) {
    


    instagibbs commented at 3:02 pm on March 4, 2024:
    we used to call m_orphanage.EraseTx(orphanHash); for each non-TX_MISSING_INPUTS possibility, but due to early return we don’t. Can we sync this back up or justify each case this has changed for?

    glozow commented at 2:30 pm on March 8, 2024:
    Old behavior should be preserved now
  25. in src/net_processing.cpp:4415 in 3eddfb2147 outdated
    4424-                // from any of our non-wtxidrelay peers.
    4425-                m_recent_rejects.insert(tx.GetHash().ToUint256());
    4426-                m_recent_rejects.insert(tx.GetWitnessHash().ToUint256());
    4427-                m_txrequest.ForgetTxHash(tx.GetHash());
    4428-                m_txrequest.ForgetTxHash(tx.GetWitnessHash());
    4429+            } else if (RecursiveDynamicUsage(*ptx) < 100000) {
    


    instagibbs commented at 3:13 pm on March 4, 2024:
    with changes, we now add to extra txns even if witness stripped; seems wrong

    glozow commented at 2:31 pm on March 8, 2024:
    fixed
  26. in test/functional/p2p_1p1c_package_relay.py:185 in 19015e3c9a outdated
    176+        self.test_individual_logic()
    177+
    178+        self.log.info("Check end-to-end package relay across multiple nodes")
    179+        self.packages_to_submit = []
    180+        self.transactions_to_presend = [[]] * self.num_nodes
    181+        self.replacement_packages = []
    


    instagibbs commented at 3:24 pm on March 4, 2024:
    this is never set

    glozow commented at 12:01 pm on March 13, 2024:
    Oops, forgot to delete from a package RBF test I built on top. Fixed
  27. instagibbs commented at 3:25 pm on March 4, 2024: member
    I focused on the first two refactoring commits. Might be helpful zeroing in on these since we probably need to justify each small behavior change or make sure we’re aligning with old behavior.
  28. in test/functional/p2p_1p1c_package_relay.py:40 in 19015e3c9a outdated
    34+FEERATE_1SAT_VB = Decimal("0.00001000")
    35+
    36+class PackageRelayTest(BitcoinTestFramework):
    37+    def set_test_params(self):
    38+        self.setup_clean_chain = True
    39+        self.num_nodes = 4
    


    brunoerg commented at 6:36 pm on March 4, 2024:

    In 19015e3c9aed5709e776ef15bf9e73f126c7ea29: It seems to be using 3 nodes instead of 4. Or am I missing something?

    E.g.:

     0diff --git a/test/functional/p2p_1p1c_package_relay.py b/test/functional/p2p_1p1c_package_relay.py
     1index 6a3ba7c3df..dda671dede 100755
     2--- a/test/functional/p2p_1p1c_package_relay.py
     3+++ b/test/functional/p2p_1p1c_package_relay.py
     4@@ -36,7 +36,7 @@ FEERATE_1SAT_VB = Decimal("0.00001000")
     5 class PackageRelayTest(BitcoinTestFramework):
     6     def set_test_params(self):
     7         self.setup_clean_chain = True
     8-        self.num_nodes = 4
     9+        self.num_nodes = 3
    10         self.extra_args = [[
    11             "-datacarriersize=100000",
    12             "-maxmempool=5",
    13@@ -85,7 +85,7 @@ class PackageRelayTest(BitcoinTestFramework):
    14         # Child should already be in orphanage
    15         self.transactions_to_presend[1] = [high_fee_child["tx"]]
    16         # Parent would have been previously rejected
    17-        self.transactions_to_presend[3] = [low_fee_parent["tx"]]
    18+        self.transactions_to_presend[2] = [low_fee_parent["tx"]]
    19 
    20     def test_individual_logic(self):
    21         node = self.nodes[0]
    22@@ -213,7 +213,7 @@ class PackageRelayTest(BitcoinTestFramework):
    23 
    24         self.log.info("Submit replacement package to node3")
    25         for package_hex in self.replacement_packages:
    26-            self.nodes[3].submitpackage(package_hex)
    27+            self.nodes[2].submitpackage(package_hex)
    28 
    29         self.log.info("Wait for mempools to sync")
    30         self.sync_mempools(timeout=20)
    

    glozow commented at 11:23 am on March 5, 2024:
    The idea is this is a “network” test to see that the package propagates across multiple hops. 1 node is the sender, 1 node pre-receives the parent, 1 node pre-receives the child, and 1 node pre-receives nothing.

    brunoerg commented at 11:48 am on March 5, 2024:
    Got it, thank you.

    glozow commented at 2:45 pm on March 20, 2024:
    added a comment on top of where the package is created
  29. glozow commented at 11:24 am on March 5, 2024: member
    Working on pulling the first 2 commits out into a separate PR.
  30. in src/net_processing.cpp:3168 in 3eddfb2147 outdated
    3076+        // Do not add txids of witness transactions or witness-stripped
    3077+        // transactions to the filter, as they can have been malleated;
    3078+        // adding such txids to the reject filter would potentially
    3079+        // interfere with relay of valid transactions from peers that
    3080+        // do not support wtxid-based relay. See
    3081+        // https://github.com/bitcoin/bitcoin/issues/8279 for details.
    


    instagibbs commented at 3:11 pm on March 6, 2024:
    @sdaftuar honestly this comment/link doesn’t make sense to me. If it’s witness-stripped, we can’t add it to m_recent_rejects since that would blind us to any real version of the tx?

    sdaftuar commented at 3:34 pm on March 6, 2024:

    I had to remind myself of how this works, but the concern contemplated in that comment is that if the only copy of a non-witness-stripped version of a transaction is from a node that has not yet upgraded to wtxid-based-relay, then an adversary could blind us to that transaction by relaying a witness-stripped version of it (and then, since txid==wtxid for witness-stripped transactions, the hash that we’d use to fetch the transaction from the non-wtxid-relay peer who has the correct version would be in our m_recent_rejects, and we wouldn’t ever request it).

    At the time that I wrote that comment, I think I also said that once wtxid-based-relay is sufficiently deployed, that we could stop worrying about this… I don’t recall when wtxid-relay was deployed but maybe we’re at that point already?

    There’s another link to a comment thread in the PR that implemented wtxid-relay which discusses the potential impact on downstream projects, so maybe if we were to make a behavior change we should communicate that in advance so that no one is surprised.


    glozow commented at 4:00 pm on March 6, 2024:

    It seems like the comment thread is concerned with what happens if: (1) the sender is using non-wtxidrelay (and thus only announces transactions by txid), (2) all of its peers are caching witness-stripped rejections by wtxid (i.e. txid), (3) there is an attacker trying to censor the sender’s tx by sending witness-stripped versions of the tx ahead of the sender.

    This isn’t a problem if (1) isn’t true, i.e. the sender is using wtxidrelay, e.g. any non-eol version of Bitcoin Core. If we see that the vast majority of nodes (that send transactions) are sending a wtxidrelay message, perhaps it would be safe to remove the special casing (with a message to downstream projects)?

    Edit: perhaps not, if btcd is an example of (1)? https://github.com/btcsuite/btcd/blob/a4f447006e7538f9e3d5ae90f54700c97fea9c3d/peer/peer.go#L2214


    glozow commented at 10:42 am on March 21, 2024:
    Resolving as out of scope for this PR. Also fwiw, btcd doing announcements by txid seems to be a reason to keep it for now…
  31. glozow force-pushed on Mar 8, 2024
  32. glozow force-pushed on Mar 8, 2024
  33. DrahtBot added the label CI failed on Mar 8, 2024
  34. DrahtBot commented at 2:23 pm on March 8, 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/22437958726

  35. DrahtBot removed the label CI failed on Mar 8, 2024
  36. glozow commented at 10:54 am on March 11, 2024: member
    Opened #29619 for the first 2 commits, moving this to draft.
  37. glozow marked this as a draft on Mar 11, 2024
  38. achow101 referenced this in commit 264ca9db24 on Mar 13, 2024
  39. glozow force-pushed on Mar 13, 2024
  40. glozow force-pushed on Mar 13, 2024
  41. glozow force-pushed on Mar 13, 2024
  42. glozow commented at 12:08 pm on March 13, 2024: member
    rebased and added followups from #29619
  43. glozow marked this as ready for review on Mar 13, 2024
  44. in src/net_processing.cpp:861 in 1435bfac51 outdated
    856+     * Upon receiving an announcement for a transaction, if it exists in this filter, do not
    857+     * download the txdata.
    858+     *
    859+     * Reset this filter when the chain tip changes.
    860+     *
    861+     * Parameters are picked to be the same false positive rate but half the capacity as
    


    instagibbs commented at 7:20 pm on March 13, 2024:
    there any science to these choices?

    glozow commented at 9:26 am on March 19, 2024:
    Not much science, no. It makes sense to me to use the same false positive rate. As for size, I’ll run my node for a bit longer and maybe use the ratio of m_recent_rejects / m_recent_rejects_reconsiderable usage to give a more scientific number.

    glozow commented at 4:47 pm on March 20, 2024:
    Actually, rethinking this, if the capacity is informed by maximum churn, it should just be the same as m_recent_rejects.
  45. in src/net_processing.cpp:857 in 1435bfac51 outdated
    805@@ -806,7 +806,7 @@ class PeerManagerImpl final : public PeerManager
    806     /** Stalling timeout for blocks in IBD */
    807     std::atomic<std::chrono::seconds> m_block_stalling_timeout{BLOCK_STALLING_TIMEOUT_DEFAULT};
    808 
    809-    bool AlreadyHaveTx(const GenTxid& gtxid)
    810+    bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable)
    


    instagibbs commented at 7:36 pm on March 13, 2024:
    include_reconsiderable seems to be true on every call-site. Perhaps this is vestigial from “real” package relay PRs of times past? Might be worth noting/making it default true, or dropping altogether.

    glozow commented at 9:26 am on March 19, 2024:
    Vestigial from a previous iteration of this PR and future changes. Will drop EDIT: I forgot to flip some

    glozow commented at 5:32 pm on March 20, 2024:
    Turns out I actually just forgot to switch from true to false in a few places, and hadn’t included a test for which these were important. I’ve added them back now and wrote a test (see “package basic nonsegwit”). Basically, we need to make sure we don’t include reconsiderable when we’re filtering for rejected parents on a TX_MISSING_INPUTS and when we’re sending the GETDATAs, in case our parent was a low-feerate nonsegwit tx that we saw previously.

    instagibbs commented at 1:11 pm on March 27, 2024:
    can confirm, changes seem logical, and there’s test coverage for each AlreadyHaveTx instance

    instagibbs commented at 10:12 am on April 3, 2024:
    Good time to add documentation to this function

    glozow commented at 3:43 pm on April 5, 2024:
    Added. Also WOW, I realized we somehow lost the line where m_recent_rejects_reconsiderable is reset. Fixed that.

    instagibbs commented at 4:37 pm on April 5, 2024:

    we should have tests(in master?) for this I hope.

    something below minfee is rejected, block comes in, node should respond to an INV for the same thing again


    glozow commented at 4:53 pm on April 5, 2024:

    we should have tests(in master?) for this I hope.

    apparently not?


    glozow commented at 11:06 am on April 16, 2024:
    marking as resolved since #29827 does this
  46. in src/net_processing.cpp:2229 in 1435bfac51 outdated
    2225@@ -2209,6 +2226,8 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid)
    2226 
    2227     if (m_orphanage.HaveTx(gtxid)) return true;
    2228 
    2229+    if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(gtxid.GetHash())) return true;
    


    instagibbs commented at 7:37 pm on March 13, 2024:
    0    if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true;
    

    glozow commented at 5:42 pm on March 20, 2024:
    done
  47. in src/net_processing.cpp:611 in 47efa940c4 outdated
    595@@ -596,6 +596,12 @@ class PeerManagerImpl final : public PeerManager
    596     void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
    597         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
    598 
    599+    void ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, NodeId nodeid)
    600+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
    601+
    602+    void MaybeProcess1P1CPackage(const CTransactionRef& ptx, NodeId nodeid)
    


    instagibbs commented at 7:39 pm on March 13, 2024:
    please give some documentation

    glozow commented at 4:01 pm on March 20, 2024:
    added
  48. in src/net_processing.cpp:602 in 47efa940c4 outdated
    595@@ -596,6 +596,12 @@ class PeerManagerImpl final : public PeerManager
    596     void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
    597         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
    598 
    599+    void ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, NodeId nodeid)
    


    instagibbs commented at 7:40 pm on March 13, 2024:
    please give some documentation

    glozow commented at 4:01 pm on March 20, 2024:
    added
  49. in src/net_processing.cpp:4651 in 47efa940c4 outdated
    4551@@ -4450,6 +4552,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    4552         if (state.IsInvalid()) {
    4553             ProcessInvalidTx(pfrom.GetId(), ptx, state, /*maybe_add_extra_compact_tx=*/true);
    4554         }
    4555+        if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) {
    


    instagibbs commented at 6:38 pm on March 14, 2024:
    0        if (m_recent_rejects_reconsiderable.contains(wtxid)) {
    

    think this is the same check and would match other call of the function


    glozow commented at 3:00 pm on March 20, 2024:

    I prefer how it is right now as

    • it’s clear that it’s based on the result we just got from mempool validation
    • it doesn’t require peerman having direct access to m_recent_rejects_reconsiderable (which I would want to move into txdownloadman module)

    instagibbs commented at 7:44 pm on March 26, 2024:
    with the added Assume() in MaybeProcess1P1CPackage I think it’s ok now from this reader’s standpoint, thanks
  50. in src/net_processing.cpp:4429 in 47efa940c4 outdated
    4425@@ -4334,6 +4426,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    4426         m_txrequest.ReceivedResponse(pfrom.GetId(), txid);
    4427         if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid);
    4428 
    4429+        // If we find this transaction in m_recent_rejects_reconsiderable, we shouldn't try it by
    


    instagibbs commented at 7:13 pm on March 14, 2024:
    Mention that this is “merely” an optimization(and also reduces the churn in the bloom filter as well)?

    glozow commented at 3:20 pm on March 20, 2024:
    What do you mean by optimization? It’s the same idea as m_recent_rejects, we don’t want to waste bandwidth even though we are willing to retry low feerate things
  51. in src/net_processing.cpp:4432 in 47efa940c4 outdated
    4425@@ -4334,6 +4426,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    4426         m_txrequest.ReceivedResponse(pfrom.GetId(), txid);
    4427         if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid);
    4428 
    4429+        // If we find this transaction in m_recent_rejects_reconsiderable, we shouldn't try it by
    4430+        // itself again. However, look for a matching child in the orphanage and maybe submit it
    4431+        // again as a package.
    4432+        if (m_recent_rejects_reconsiderable.contains(wtxid)) {
    


    instagibbs commented at 7:31 pm on March 14, 2024:
    if MaybeProcess1P1CPackage handles the (non)existence in the m_recent_rejects_reconsiderable filter, you could stick this inside the AlreadyHaveTx block, which would allow existing logging to continue happening, and seems pretty natural?

    glozow commented at 3:21 pm on March 20, 2024:
    Ah good point as I just realize the previous thing was breaking the forcerelay stuff for low feerate txns. Moved inside that block.
  52. in src/net_processing.cpp:3271 in 47efa940c4 outdated
    3229+            }
    3230+        }
    3231+    }
    3232+}
    3233+
    3234+void PeerManagerImpl::MaybeProcess1P1CPackage(const CTransactionRef& ptx, NodeId nodeid)
    


    instagibbs commented at 12:44 pm on March 15, 2024:
    0void PeerManagerImpl::MaybeProcess1P1CPackage(const CTransactionRef& parent_ptx, NodeId nodeid)
    

    glozow commented at 5:13 pm on March 20, 2024:
    Well, it might not be a parent :shrug: most likely it’s just a low feerate transaction actually. I didn’t change but don’t feel strongly
  53. in src/net_processing.cpp:3243 in 47efa940c4 outdated
    3238+    AssertLockHeld(cs_main);
    3239+
    3240+    const auto cpfp_candidates{m_orphanage.GetChildren(ptx, nodeid)};
    3241+    bool tried_package_cpfp{false};
    3242+
    3243+    const auto& txid{ptx->GetHash()};
    


    instagibbs commented at 12:46 pm on March 15, 2024:
    0    const auto& parent_txid{parent_ptx->GetHash()};
    

    as well as wtxid just below


    glozow commented at 5:43 pm on March 20, 2024:
    done
  54. in src/net_processing.cpp:3306 in 47efa940c4 outdated
    3240+    const auto cpfp_candidates{m_orphanage.GetChildren(ptx, nodeid)};
    3241+    bool tried_package_cpfp{false};
    3242+
    3243+    const auto& txid{ptx->GetHash()};
    3244+    const auto& wtxid{ptx->GetWitnessHash()};
    3245+
    


    instagibbs commented at 12:50 pm on March 15, 2024:
    0
    1    Assume(m_recent_rejects_reconsiderable.contains(parent_wtxid.ToUint256()));
    

    glozow commented at 5:29 pm on March 20, 2024:
    added
  55. in src/net_processing.cpp:3205 in 47efa940c4 outdated
    3200+    // Iterate backwards to erase in-package descendants from the orphanage before they become
    3201+    // relevant in AddChildrenToWorkSet.
    3202+    for (auto package_it = package.rbegin(); package_it != package.rend(); ++package_it) {
    3203+        const auto& tx = *package_it;
    3204+        const auto it_result{package_result.m_tx_results.find(tx->GetWitnessHash())};
    3205+        if (it_result != package_result.m_tx_results.end()) {
    


    instagibbs commented at 5:02 pm on March 15, 2024:
    0        if (Assume(it_result != package_result.m_tx_results.end())) {
    
  56. in src/net_processing.cpp:3284 in 47efa940c4 outdated
    3218+                    // Don't add to vExtraTxnForCompact, as these transactions would already be
    3219+                    // there as an orphan or too low feerate tx.
    3220+                    ProcessInvalidTx(nodeid, tx, tx_result.m_state, /*maybe_add_extra_compact_tx=*/false);
    3221+                    break;
    3222+                }
    3223+                case MempoolAcceptResult::ResultType::MEMPOOL_ENTRY:
    


    instagibbs commented at 6:08 pm on March 15, 2024:
    have you considered moving this above INVALID and letting it fall through to call ProcessInvalidTx as well?

    glozow commented at 5:13 pm on March 20, 2024:
    Why? There wouldn’t be anything interesting in state

    instagibbs commented at 7:44 pm on March 26, 2024:
    slight readability preference, feel free to ignore
  57. in src/net_processing.cpp:3218 in 47efa940c4 outdated
    3213+                    break;
    3214+                }
    3215+                case MempoolAcceptResult::ResultType::INVALID:
    3216+                case MempoolAcceptResult::ResultType::DIFFERENT_WITNESS:
    3217+                {
    3218+                    // Don't add to vExtraTxnForCompact, as these transactions would already be
    


    instagibbs commented at 6:25 pm on March 15, 2024:

    We don’t add orphans to vExtraTxnForCompact though, just too low aka reconsiderable?

    maybe something like:

    // All packages currently considered are 1p1c, which means // any entrants to be added in vExtraTxnForCompact have already been added


    glozow commented at 5:11 pm on March 20, 2024:
    we do add orphans to vExtraTxnForCompact though?
  58. in test/functional/p2p_1p1c_package_relay.py:172 in 2f25d2e084 outdated
    167+        peer1.peer_disconnect()
    168+        peer2.peer_disconnect()
    169+        self.sync_all()
    170+
    171+    def run_test(self):
    172+        self.ctr = 0
    


    instagibbs commented at 7:45 pm on March 15, 2024:
    unused
  59. in test/functional/p2p_1p1c_package_relay.py:204 in 2f25d2e084 outdated
    201+                peer.send_and_ping(msg_tx(tx))
    202+            # This disconnect removes any sent orphans from the orphanage (EraseForPeer) and times
    203+            # out the in-flight requests.  It is currently required for the test to pass right now,
    204+            # because the node will not (re)try requesting orphan parents from multiple peers if the
    205+            # first one fails.
    206+            # TODO: remove this and test that the node retries orphan resolution with other peers
    


    instagibbs commented at 7:57 pm on March 15, 2024:
    unsure what the test is covering if it’s being deleted on disconnect?

    glozow commented at 5:44 pm on March 20, 2024:
    I guess not a lot. But it is worth checking that it having seen (and rejected) the child before shouldn’t impact its acceptance of the package later?
  60. in test/functional/p2p_1p1c_package_relay.py:217 in 2f25d2e084 outdated
    212+            self.nodes[0].submitpackage(package_hex)
    213+
    214+        self.log.info("Wait for mempools to sync")
    215+        self.sync_mempools(timeout=20)
    216+
    217+        self.log.info("Wait for mempools to sync")
    


    instagibbs commented at 7:58 pm on March 15, 2024:
    double-sync for what?

    glozow commented at 2:57 pm on March 20, 2024:
    (originally there was an original broadcast, then a package RBF broadcast) no reason, deleted
  61. in test/functional/p2p_1p1c_package_relay.py:115 in 2f25d2e084 outdated
    101+        peer2 = node.add_p2p_connection(P2PInterface())
    102+
    103+        self.log.info("Check that tx caches low feerate rejections")
    104+        parent_wtxid_int = int(low_fee_parent["tx"].getwtxid(), 16)
    105+        peer1.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=parent_wtxid_int)]))
    106+        peer1.wait_for_getdata([parent_wtxid_int])
    


    instagibbs commented at 8:16 pm on March 15, 2024:
    had this timeout locally once

    instagibbs commented at 5:44 pm on March 26, 2024:
    can’t seem to recreate anymore :shrug:

    theStack commented at 0:55 am on March 27, 2024:

    Seems I had beginner’s luck, got the timeout on this line after a few runs on my machine. Here is the compressed dir of the failed test run: https://github.com/theStack/bitcoin/raw/pr28970_failed_testrun/pr28970_waitforgetdata_timeout.tar.gz

    Apparently the node has seen the wtx in the inv already before (see node0/regtest/debug.log:11237) :eyes: . I guess this is could be caused by a different MiniWallet instance spending the same UTXO in an earlier sub-test, resulting in the same transaction?


    glozow commented at 10:47 am on March 27, 2024:

    Nice @theStack :D I was having trouble recreating it, duplicate tx sounds most plausible to me.

    I split the 4 node test and 1 node test into separate files (grabbed commit from #29735 to reuse setup) which hopefully gets rid of this.

  62. in test/functional/p2p_1p1c_package_relay.py:182 in 2f25d2e084 outdated
    177+        self.raise_network_minfee()
    178+
    179+        self.log.info("Check 1p1c validation logic on a single node")
    180+        self.test_individual_logic()
    181+
    182+        self.log.info("Check end-to-end package relay across multiple nodes")
    


    instagibbs commented at 8:18 pm on March 15, 2024:
    could this subtest be in its own subroutine

    glozow commented at 10:38 am on March 21, 2024:
    done
  63. instagibbs commented at 1:58 pm on March 18, 2024: member
    some comments, running patch on a listening node longer-term to test behavior
  64. glozow force-pushed on Mar 20, 2024
  65. glozow commented at 5:55 pm on March 20, 2024: member
    Rebased for silent conflict with #28950
  66. glozow force-pushed on Mar 20, 2024
  67. DrahtBot added the label CI failed on Mar 20, 2024
  68. DrahtBot commented at 5:55 pm on March 20, 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/22896614286

  69. DrahtBot removed the label CI failed on Mar 20, 2024
  70. dergoegge commented at 12:21 pm on March 25, 2024: member

    Enable 1-parent-1-child package relay 🚀 (just using the existing protocol messages).

    Imo, this isn’t really what this PR does because “package relay” (at least to me) is something that is robust and users can rely on. As you note yourself, there is more work required to make this robust.

    To make this more robust, we need receiver-side logic to retry orphan resolution with multiple peers. To make this robust, we need to make efforts to protect some amount of orphans from eviction.

    Why are we not making things more robust in or before this PR?

  71. instagibbs commented at 12:28 pm on March 25, 2024: member

    Imo, this isn’t really what this PR does because “package relay” (at least to me) is something that is robust and users can rely on. As you note yourself, there is more work required to make this robust.

    that sounds like confusing semantic games to me? We can add “non-robust” to the title to be clearer if we think it can get merged…

    Why are we not making things more robust in or before this PR?

    Why would we tie up an improvement on relay on other improvements to relay? This feature is useful by itself in non-adversarial network conditions, which ends up being deliverable security and a bit of increased miner fee revenue.

    Of course, if we have a orphanage hardening PR ready for merge, we should merge that.

  72. glozow commented at 12:33 pm on March 25, 2024: member

    Why are we not making things more robust in or before this PR?

    Why have we been okay with handling any transactions with missing parents despite all of these orphanage problems, for more than a decade? Clearly because it’s useful even if not 100% reliable. There are various improvements we can make to relay. I have already implemented them, and decided I would prefer to propose the more impactful improvements before the less impactful ones.

  73. dergoegge commented at 1:41 pm on March 25, 2024: member

    that sounds like confusing semantic games to me? We can add “non-robust” to the title to be clearer if we think it can get merged…

    I think my issue is with the PR description and how this will be marketed, not the title. The title describes it quite well: we will sometimes accept 1p1c packages (although it could also mention that these packages are constructed from the orphanage). That is different from package relay (again, to me “package relay” implies a level of reliability).

    This feature is useful by itself in non-adversarial network conditions, which ends up being deliverable security

    “deliverable security”: Is this referring to the increased cost of having to mess with relay to prevent pkgs from relaying? as opposed to the current state where these pkgs just don’t relay at all?

    Why have we been okay with handling any transactions with missing parents despite all of these orphanage problems, for more than a decade?

    Because it saves bandwidth in the honest case. Afaict, relay would be reliable without the orphanage, although much more bandwidth intensive.


    I guess my wording was confusing? I have no problem with the approach in this PR nor was I suggesting to block this on other relay improvements. I was just commenting on the PR description and asking a question.

    There clearly are good answers to my question but I don’t appreciate the condescending tone after being asked to review this PR.

  74. instagibbs commented at 1:47 pm on March 25, 2024: member

    “deliverable security”: Is this referring to the increased cost of having to mess with relay to prevent pkgs from relaying? as opposed to the current state where these pkgs just don’t relay at all?

    Assuming there’s a path of nodes to a miner in which they are not having their orphanages aggressively churned by an adversary, these size 2 packages should be able to propagate. So if say a LN node goes offline or refuses to sign an updated commitment transaction that raises the transaction fee, minfee rises, the counterparty can still go to chain. It raises the bar a bit for an adversary, and the more benign cases can be resolved in the average case, letting people deploy liquidity elsewhere.

    FWIW I ran this branch over the last week, and when my minfee wasn’t 1 I was getting about one extra package relayed every half hour.

  75. glozow commented at 2:28 pm on March 25, 2024: member

    I’ve updated the PR description to be more descriptive about what this PR does and doesn’t do. The 1p1c “package relay” thing wasn’t meant as false advertising; I added it to the PR description 1 month after opening because it seemed like people were underestimating how useful this is (someone was surprised when I said this PR propagates 1p1c packages).

    I apologize for the tone - the comment read to me like a blocking criticism since it wasn’t accompanied by any other review comments. Hopefully the suggestion has been adequately addressed with the description update now.

  76. Roasbeef commented at 0:19 am on March 26, 2024: none

    Popping in here to mention that even w/ the limitations described above, this would be super useful for LN as is.

    Today we’re able to use the anchor outputs on commitment transactions to CPFP a force close (* caveats re pinning, RBF, etc, etc). This works OK during calm mempool conditions, but if a rapid spike occurs that causes the bottom of the mempool to fall out, then CPFP no longer works as the parent has been evicted. During one of the recent more persistent fee spikes, we saw this happening rather frequently, which then led to cascading force closes (outgoing HTLC can’t timeout due to non confirmation, incoming times out, repeat) and a lots of pain and user confusion (can only wait or use a transaction “accelerator”).

    IIUC, with this PR, upgraded peers will optimistically attempt to fetch the parent (the commitment txn) when they see a transaction that appears to be fee bumping it (anchor output spend). With this behavior, on a best effort basis, if we had another massive spike, then commitment transactions would be able to confirm as upgraded peers would fetch the parent to consider/propagate. I think this is very attractive as it doesn’t necessarily require a new p2p upgrade to gain the benefits, instead as nodes start to progressively update, then more of these otherwise unrecognized packages will start to propagate.

    If major LN node operators are made aware of this patch, and start to run it with the node backing their LN nodes, then assuming a relay path to miners (who seem to be willing to mine/relay just about anything these days), this would be a rather nice stop gap while the greater v3/cluster work proceeds in the background.

  77. in test/functional/p2p_1p1c_package_relay.py:80 in 5405171740 outdated
    75+        self.log.debug("Check that all nodes' mempool minimum feerate is above min relay feerate")
    76+        for node in self.nodes:
    77+            assert_equal(node.getmempoolinfo()['minrelaytxfee'], FEERATE_1SAT_VB)
    78+            assert_greater_than(node.getmempoolinfo()['mempoolminfee'], FEERATE_1SAT_VB)
    79+
    80+    def create_packages(self):
    


    instagibbs commented at 4:20 pm on March 26, 2024:
    nit: I’d rather this return the packages then spookily populate them

    glozow commented at 5:23 pm on March 28, 2024:
    not sure if I achieved spooky, but I’m having it return the lists now :ghost:
  78. in src/txorphanage.cpp:257 in f4d8fe713a outdated
    248+    std::vector<CTransactionRef> children_found;
    249+
    250+    for (unsigned int i = 0; i < parent->vout.size(); i++) {
    251+        const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(parent->GetHash(), i));
    252+        if (it_by_prev != m_outpoint_to_orphan_it.end()) {
    253+            for (const auto& elem : it_by_prev->second) {
    


    instagibbs commented at 7:39 pm on March 26, 2024:
    to be clear we may have multiple orphans per outpoint lookup, yes?

    glozow commented at 10:42 am on March 27, 2024:
    Yep, we can have conflicting orphans. Should not break here if we find one.
  79. in src/txorphanage.h:55 in f4d8fe713a outdated
    50@@ -51,6 +51,10 @@ class TxOrphanage {
    51     /** Does this peer have any work to do? */
    52     bool HaveTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);;
    53 
    54+    /** Get all children of this parent. */
    55+    std::vector<CTransactionRef> GetChildren(const CTransactionRef& parent, NodeId peer) const
    


    instagibbs commented at 7:39 pm on March 26, 2024:
    could stand a unit test and some basic fuzz coverage

    glozow commented at 5:38 pm on March 28, 2024:
    added unit tests and fuzz coverage
  80. in src/net_processing.cpp:4539 in 774f6d0ff5 outdated
    4531@@ -4420,7 +4532,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    4532                     // protocol for getting all unconfirmed parents.
    4533                     const auto gtxid{GenTxid::Txid(parent_txid)};
    4534                     AddKnownTx(*peer, parent_txid);
    4535-                    if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) AddTxAnnouncement(pfrom, gtxid, current_time);
    4536+                    // Skip m_recent_rejects_reconsiderable because the missing parent may have been
    4537+                    // previously rejected for being too low feerate, and this orphan may be able to
    4538+                    // CPFP it if we consider them as a package.
    


    instagibbs commented at 8:05 pm on March 26, 2024:
    this change also only matters for the non-segwit case since we’re checking via txid, correct?

    glozow commented at 10:32 am on March 27, 2024:
    Correct. If it’s a witness tx, we would have cached the error by wtxid but the request is by txid, so this would miss it.
  81. in src/net_processing.cpp:4485 in 774f6d0ff5 outdated
    4481@@ -4378,6 +4482,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    4482             // peer simply for relaying a tx that our m_recent_rejects has caught,
    4483             // regardless of false positives.
    4484             return;
    4485+        } else if (m_recent_rejects_reconsiderable.contains(wtxid)) {
    


    instagibbs commented at 8:09 pm on March 26, 2024:
    tiny preference for putting this case first

    glozow commented at 4:10 pm on March 28, 2024:

    Ok. I’ve actually ended up nesting it inside the AlreadyHave to preserve the ForceRelay behavior… wanted to avoid a behavior change in the case

    1. tx is low feerate (goes into m_recent_rejects_reconsiderable)
    2. tx is later accepted (e.g. due to 1p1c)
    3. forcrelay peer sends it to us again -> we should RelayTransaction
  82. instagibbs commented at 8:12 pm on March 26, 2024: member
    no issues running the patch so far, some more comments
  83. theStack referenced this in commit a5212a2d0b on Mar 27, 2024
  84. in src/txorphanage.cpp:245 in f4d8fe713a outdated
    240@@ -241,3 +241,19 @@ void TxOrphanage::EraseForBlock(const CBlock& block)
    241         LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx included or conflicted by block\n", nErased);
    242     }
    243 }
    244+
    245+std::vector<CTransactionRef> TxOrphanage::GetChildren(const CTransactionRef& parent, NodeId peer) const
    


    brunoerg commented at 1:19 pm on March 28, 2024:
    In f4d8fe713a036c4d2c1e7c2329077c34f75c8608: It seems NodeId peer is not used in GetChildren.

    glozow commented at 4:33 pm on March 28, 2024:
    removed
  85. in test/functional/p2p_1p1c_package_relay.py:44 in 5405171740 outdated
    39+        self.setup_clean_chain = True
    40+        self.num_nodes = 4
    41+        self.extra_args = [[
    42+            "-datacarriersize=100000",
    43+            "-maxmempool=5",
    44+            "-whitelist=noban@127.0.0.1",  # immediate tx relay
    


    brunoerg commented at 1:23 pm on March 28, 2024:
    In 5405171740aa77e1eb1110fa8be97318edba380a: nit: could set noban_tx_relay

    glozow commented at 5:18 pm on March 28, 2024:
    done
  86. glozow force-pushed on Mar 28, 2024
  87. glozow commented at 6:03 pm on March 28, 2024: member

    Thanks for the review, addressed comments and

    • fixed a bug in GetChildren
    • expanded + added more docs to the individual logic tests
    • got rid of the assert_debug_log lines in the functional tests. The only “coverage” that’s lost is us skipping children with which we’ve already tried+failed the package submission. I have a branch with them if you want to use it to test the code, but log asserts didn’t feel appropriate to have in the functional test.
    • added more logging, realized it was annoyingly hard to look up success/fail package evals while searching logs on my node

    fwiw running this on a -maxmempool=150 node, for the past few days, on average:

    • 222 attempted package validations per day
    • 85 packages (so 170 txns) accepted per day
    • 57% of the package transactions accepted ended up in a block. These are all transactions that we would have otherwise rejected, so this logic is definitely useful at helping us maximize the fees in our limited mempool space.
  88. DrahtBot added the label CI failed on Mar 28, 2024
  89. DrahtBot commented at 8:11 pm on March 28, 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/23211258926

  90. in test/functional/p2p_opportunistic_1p1c.py:44 in 5d7fa1f684 outdated
    39+            "-datacarriersize=100000",
    40+            "-maxmempool=5",
    41+        ]]
    42+        self.supports_cli = False
    43+
    44+    def raise_network_minfee(self):
    


    theStack commented at 6:37 pm on March 30, 2024:
    nit: raise_network_minfee is unused in this file, can be removed

    glozow commented at 12:53 pm on April 2, 2024:
    removed, thanks
  91. in test/functional/p2p_opportunistic_1p1c.py:61 in 5d7fa1f684 outdated
    56+        self.log.info("Check that opportunistic 1p1c logic works when child is received before parent")
    57+
    58+        low_fee_parent = self.wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
    59+        high_fee_child = self.wallet.create_self_transfer(utxo_to_spend=low_fee_parent["new_utxo"], fee_rate=20*FEERATE_1SAT_VB)
    60+
    61+        peer_sender = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, connection_type="outbound-full-relay")
    


    theStack commented at 6:42 pm on March 30, 2024:

    nit: could create the peers as inbound connections instead, here and in other instances:

    0        peer_sender = node.add_p2p_connection(P2PInterface())
    

    (it doesn’t really matter though, the code would just be a bit shorter)


    glozow commented at 12:57 pm on April 2, 2024:
    Changed thanks. Before I added noban, I was using it to make tests run faster.
  92. in src/test/orphanage_tests.cpp:174 in 3842f9a884 outdated
    169+    const NodeId node{0};
    170+    std::vector<COutPoint> empty_outpoints;
    171+
    172+    auto parent1 = MakeTransactionSpending(empty_outpoints, det_rand);
    173+    auto parent2 = MakeTransactionSpending(empty_outpoints, det_rand);
    174+    auto parent3 = MakeTransactionSpending(empty_outpoints, det_rand);
    


    theStack commented at 6:45 pm on March 30, 2024:
    nit: parent3 is unused, can remove this line and the two comparisons below

    glozow commented at 12:52 pm on April 2, 2024:
    fixed
  93. in src/net_processing.cpp:2285 in 97e16c20ca outdated
    2240@@ -2228,7 +2241,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside
    2241 
    2242     if (m_orphanage.HaveTx(gtxid)) return true;
    2243 
    2244-    if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(gtxid.GetHash())) return true;
    2245+    if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true;
    


    theStack commented at 6:48 pm on March 30, 2024:
    nit: could use hash directly in the previous commit where this line is introduced (edea2269416ad2bcd017e1d6ee9ae30bd225c1e9), for smaller diff

    glozow commented at 8:01 am on April 3, 2024:
    done
  94. theStack commented at 6:52 pm on March 30, 2024: contributor

    Concept ACK

    Found some nitty stuff mostly in tests so far, planning to do another review round next week.

  95. glozow force-pushed on Apr 2, 2024
  96. glozow force-pushed on Apr 2, 2024
  97. in src/txorphanage.cpp:252 in d3d7dcf004 outdated
    247+    LOCK(m_mutex);
    248+
    249+    // First construct a set of iterators to ensure we do not return duplicates of the same tx.
    250+    std::set<OrphanMap::iterator, IteratorComparator> set_child_iterators;
    251+
    252+    // For each input, get all entries spending this prevout.
    


    instagibbs commented at 9:47 am on April 3, 2024:
    0    // For each output, get all entries spending this prevout.
    

    glozow commented at 3:47 pm on April 5, 2024:
    done
  98. in src/test/orphanage_tests.cpp:176 in b49b634556 outdated
    171+
    172+    auto parent1 = MakeTransactionSpending(empty_outpoints, det_rand);
    173+    auto parent2 = MakeTransactionSpending(empty_outpoints, det_rand);
    174+
    175+    // Make sure these parents have different txids otherwise this test won't make sense.
    176+    BOOST_CHECK(parent1->GetHash() != parent2->GetHash());
    


    instagibbs commented at 9:59 am on April 3, 2024:
    probably better to have it while loop making tx until this is true, rather than fail randomly

    glozow commented at 3:46 pm on April 5, 2024:
    done
  99. in src/test/orphanage_tests.cpp:233 in b49b634556 outdated
    199+    }
    200+    auto parent2_children{orphanage.GetChildren(parent2)};
    201+    BOOST_CHECK_EQUAL(parent2_children.size(), expected_parent2_children.size());
    202+    for (const auto& child : parent2_children) {
    203+        BOOST_CHECK(expected_parent2_children.count(child->GetWitnessHash()) > 0);
    204+    }
    


    instagibbs commented at 10:04 am on April 3, 2024:
    quick test case that returns empty would be good too

    glozow commented at 3:46 pm on April 5, 2024:
    added
  100. in src/net_processing.cpp:4568 in 53f1e65f30 outdated
    4353@@ -4354,7 +4354,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    4354         const TxValidationState& state = result.m_state;
    4355 
    4356         if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
    4357-            ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value());
    4358+            Assume(result.m_replaced_transactions.has_value());
    4359+            std::list<CTransactionRef> empty_replacement_list;
    4360+            ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value_or(empty_replacement_list));
    


    murchandamus commented at 6:31 pm on April 3, 2024:
    In guard against MempoolAcceptResult::m_replaced_transactions (53f1e65f30a0a6b931e97743113e0227748680df): I am not well-acquainted with net_processing.cpp, but I figured I could still mention that it is unclear to me from the commit message and the code change how this change fits in the context. Were we previously assuming that we would always have a non-empty list for m_replaced_transactions in the context of this call?

    theStack commented at 8:16 pm on April 3, 2024:

    @murchandamus: I was asking myself the same a few days ago and started with some review notes for each commit. The one for 53f1e65f30a0a6b931e97743113e0227748680df might fit to your question (note that it’s not about empty vs. non-empty, but more about set-to-nothing vs. set-to-something, since it’s an std::optional):

    0The only way to create an ATMP result of type `MempoolAcceptResult::ResultType::VALID` is using the
    1static method `MempoolAccepptResult::Success`, which in turn calls the private successful case constructor of
    2`MempoolAcceptResult`. This one always sets `m_replaced_transactions`, therefore
    3`result.m_replaced_transactions.has_value()` in the modified code path should always be true.
    

    (maybe it makes sense to include it in the commit message, if that is correct) The same three lines of code are introduced in PeerManagerImpl::ProcessPackageResult (commit d6df19000118768678c7afd4b9330b4604bb37ce), I assume that this is the reason to also do it on the other place in the code for consistency.


    glozow commented at 8:42 pm on April 3, 2024:

    it is unclear to me from the commit message and the code change how this change fits in the context.

    (Note that this commit is a followup from #29619, I’ve now linked to the comments in the PR description)

    Yes, it should always have a value when the result is VALID. This is just adding a belt-and-suspenders juuust in case.


    murchandamus commented at 9:01 pm on April 3, 2024:
    Thanks! Great, I’ll attempt another more thorough review at a later time.

    mzumsande commented at 3:04 pm on April 15, 2024:
    I asked myself the same question, found this discussion too late. I also think it would be good to mention this in the commit message (so it becomes clear that it has nothing to do with whether m_replaced_transactions has entries or is empty).

    glozow commented at 11:05 am on April 16, 2024:
    Elaborated in commit message
  101. in src/test/orphanage_tests.cpp:198 in b49b634556 outdated
    193+    std::set<Wtxid> expected_parent2_children{child_p2n1->GetWitnessHash(), child_p1n0_p2n0->GetWitnessHash()};
    194+
    195+    auto parent1_children{orphanage.GetChildren(parent1)};
    196+    BOOST_CHECK_EQUAL(parent1_children.size(), expected_parent1_children.size());
    197+    for (const auto& child : parent1_children) {
    198+        BOOST_CHECK(expected_parent1_children.count(child->GetWitnessHash()) > 0);
    


    murchandamus commented at 7:28 pm on April 3, 2024:

    Nit: Could use std::set::contains here and below

    0        BOOST_CHECK(expected_parent1_children.contains(child->GetWitnessHash()));
    

    glozow commented at 3:46 pm on April 5, 2024:
    done, yay for 20
  102. murchandamus commented at 7:51 pm on April 3, 2024: contributor
    Did a quick pass. I am surprised we are able to get this big of an improvement without p2p changes, seems like a big win. Big Concept ACK, but I must admit this part of the codebase is a bit out of my wheelhouse.
  103. in src/net_processing.cpp:6189 in d6df190001 outdated
    6185@@ -6061,7 +6186,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
    6186                 entry.second.GetHash().ToString(), entry.first);
    6187         }
    6188         for (const GenTxid& gtxid : requestable) {
    6189-            if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) {
    6190+            // Skip m_recent_rejects_reconsiderable because we may be requesting a missing parent
    


    instagibbs commented at 9:07 am on April 4, 2024:
    I have to admit I’m struggling on these “skip” comments and cases. Every time I read this I have trouble re-deriving the logic.

    glozow commented at 3:46 pm on April 5, 2024:
    Ok hm. I’ve simplified the wording a bit. Maybe that helps?

    instagibbs commented at 4:37 pm on April 5, 2024:
    think you forgot to push?

    glozow commented at 4:49 pm on April 5, 2024:

    think you forgot to push?

    Github published everything instead of adding my comment to the group -_- yes, pushing in a second.

  104. instagibbs commented at 9:55 am on April 4, 2024: member

    reviewed through 81912ae6649fac5a0d671d2cabe344f787193997

    LGTM so far

    will review tests

  105. in test/functional/p2p_opportunistic_1p1c.py:377 in c5e196b8e7 outdated
    172+        assert low_fee_parent["txid"] in node_mempool
    173+        assert high_fee_child["txid"] in node_mempool
    174+
    175+        node.disconnect_p2ps()
    176+
    177+    def run_test(self):
    


    instagibbs commented at 10:25 am on April 4, 2024:

    add a case where:

    1. CONSENSUS-invalid child is propagated to peer
    2. low_fee parent is given
    3. package evaluation is attempted
    4. peer is disconnected

    to ensure we’re handling invalid tx properly


    glozow commented at 4:21 pm on April 5, 2024:
    Wait oof, we don’t require the orphanage child to be provided by the same peer who sent the low-feerate tx. I think this would mean you can get other people disconnected by sending a bogus child of the package they’re sending.

    instagibbs commented at 4:45 pm on April 5, 2024:
    looks like peer id needs to be used again: #28970 (review)

    glozow commented at 4:57 pm on April 5, 2024:
    Hm no, I think we should return pairs of tx and the fromPeer, and just attribute the error to the right peer. If we restrict it to same sender, then we can also block packages by sending the children fast and refusing to send the parent.

    glozow commented at 5:17 pm on April 5, 2024:
    Last push should fix this with the above approach. Still need to write a test case for the peers being different.
  106. instagibbs commented at 10:27 am on April 4, 2024: member
    reviewed through c5e196b8e7b610cc9e6321b76fee4f0c45c1448e
  107. DrahtBot removed the label CI failed on Apr 4, 2024
  108. glozow force-pushed on Apr 5, 2024
  109. glozow force-pushed on Apr 5, 2024
  110. DrahtBot added the label CI failed on Apr 6, 2024
  111. glozow force-pushed on Apr 9, 2024
  112. in src/net_processing.cpp:603 in e313001b1b outdated
    595@@ -596,6 +596,19 @@ class PeerManagerImpl final : public PeerManager
    596     void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
    597         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
    598 
    599+    /** Handle the results of package validation: calls ProcessValidTx and ProcessInvalidTx for
    600+     * individual transactions, and caches rejection for the package as a group.
    601+     *   */
    


    instagibbs commented at 9:28 am on April 9, 2024:
    0     * The senders arg should be populated in same order as individual transactions
    1     * in the package_result argument.
    2     */
    

    glozow commented at 12:57 pm on April 11, 2024:
    added something similar
  113. in src/net_processing.cpp:3224 in e313001b1b outdated
    3219+    }
    3220+    if (!Assume(!package.empty())) return;
    3221+
    3222+    // Iterate backwards to erase in-package descendants from the orphanage before they become
    3223+    // relevant in AddChildrenToWorkSet.
    3224+    for (auto i{package.size() - 1}; i >= 0; --i) {
    


    instagibbs commented at 7:20 am on April 10, 2024:
    0net_processing.cpp:3224:40: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
    1 3224 |     for (auto i{package.size() - 1}; i >= 0; --i) {
    2      |            
    

    instagibbs commented at 7:27 am on April 10, 2024:
    this is also causing the sanitizer run failure

    glozow commented at 12:34 pm on April 10, 2024:
    Woops yes will fix
  114. in src/policy/packages.cpp:156 in c502734fa5 outdated
    151+uint256 GetCombinedHash(const std::vector<Wtxid>& wtxids)
    152+{
    153+    std::vector<Wtxid> wtxids_copy(wtxids.cbegin(), wtxids.cend());
    154+    std::sort(wtxids_copy.begin(), wtxids_copy.end());
    155+    return (HashWriter() << wtxids_copy).GetHash();
    156+}
    


    instagibbs commented at 10:10 am on April 10, 2024:
    newline?

    glozow commented at 3:55 pm on April 18, 2024:
    (deleted GetCombinedHash)
  115. in src/policy/packages.h:94 in c502734fa5 outdated
    87@@ -88,4 +88,10 @@ bool IsChildWithParents(const Package& package);
    88  * other (the package is a "tree").
    89  */
    90 bool IsChildWithParentsTree(const Package& package);
    91+
    92+/** Get the hash of these wtxids, concatenated in lexicographical order. */
    93+uint256 GetCombinedHash(const std::vector<Wtxid>& wtxids);
    94+/** Get the hash of these transactions' wtxids, concatenated in lexicographical order. */
    


    instagibbs commented at 10:11 am on April 10, 2024:
    newline?

    glozow commented at 3:55 pm on April 18, 2024:
    (deleted GetCombinedHash)
  116. glozow force-pushed on Apr 11, 2024
  117. glozow commented at 12:58 pm on April 11, 2024: member
    Rebased for #29735 and fixed failure.
  118. DrahtBot removed the label CI failed on Apr 11, 2024
  119. glozow commented at 9:55 am on April 15, 2024: member

    Update since my last comment #28970 (comment) Running for 20 days:

    • My node made 5218 package evaluation attempts (x2 = 10,436 transactions). That’s ~260 attempts per day.
    • Of those attempts, 2260 were successful (x2 = 4520 transactions). That’s ~43% acceptance rate.
    • Some were repeats. I have 4202 unique txids accepted through package evaluation. I don’t know if they were repeated packages or not.
    • Of the accepted transactions, 3232 (77%) were later mined in a block.
  120. in src/policy/packages.h:92 in 75855a4d33 outdated
    87@@ -88,4 +88,10 @@ bool IsChildWithParents(const Package& package);
    88  * other (the package is a "tree").
    89  */
    90 bool IsChildWithParentsTree(const Package& package);
    91+
    92+/** Get the hash of these wtxids, concatenated in lexicographical order. */
    


    mzumsande commented at 3:59 pm on April 15, 2024:

    I think that they are sorted by internal order, not reversed-byte order (because they are sorted by Wtxid / uint256, not by uint256.GetHex()). Is that on purpose? In any case, maybe it would be useful to specify in the doc which order is used to avoid possbile confusion.

    Also, the tests added in this commit only test for relative order between multiple transactions but not if they are actually sorted in lexicographical order, so that could also be done.


    glozow commented at 10:49 am on April 16, 2024:
    I’ve added more explicit tests and changed this to be ordered based on uint256.GetHex() instead. I don’t know enough to say which sorting is better here, but this seems like the natural ordering when I’m reading the hex strings as a human.
  121. in src/net_processing.cpp:3179 in 3535115973 outdated
    3129@@ -3097,7 +3130,14 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx
    3130         // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
    3131         // for concerns around weakening security of unupgraded nodes
    3132         // if we start doing this too early.
    3133-        m_recent_rejects.insert(ptx->GetWitnessHash().ToUint256());
    3134+        if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) {
    3135+            // If the result is TX_RECONSIDERABLE, add it to m_recent_rejects_reconsiderable
    3136+            // because we should not download or submit this transaction by itself again, but may
    3137+            // submit it as part of a package later.
    3138+            m_recent_rejects_reconsiderable.insert(ptx->GetWitnessHash().ToUint256());
    


    mzumsande commented at 5:56 pm on April 15, 2024:
    Should update ProcessInvalidTx doc above too (it lists all member variables that the function updates).

    glozow commented at 11:06 am on April 16, 2024:
    updated
  122. in src/net_processing.cpp:3272 in f5989537e7 outdated
    3267+{
    3268+    AssertLockNotHeld(m_peer_mutex);
    3269+    AssertLockHeld(g_msgproc_mutex);
    3270+    AssertLockHeld(cs_main);
    3271+
    3272+    const auto cpfp_candidates{m_orphanage.GetChildren(ptx)};
    


    mzumsande commented at 6:59 pm on April 15, 2024:
    have you considered returning early here if cpfp_candidates is empty? It should work regardless, but seems conceptually simpler than checking all the steps below can handle that case (and may be slightly faster too).

    glozow commented at 11:06 am on April 16, 2024:
    Done, and edited the logging a bit
  123. in src/net_processing.cpp:3222 in f5989537e7 outdated
    3217+    AssertLockHeld(cs_main);
    3218+
    3219+    if (package_result.m_state.IsInvalid()) {
    3220+        m_recent_rejects_reconsiderable.insert(GetPackageHash(package));
    3221+    }
    3222+    if (!Assume(!package.empty())) return;
    


    mzumsande commented at 7:27 pm on April 15, 2024:
    Could we assume more strictly here that the package size is 2 (for now)?

    glozow commented at 11:06 am on April 16, 2024:
    added
  124. in src/net_processing.cpp:3317 in f5989537e7 outdated
    3287+
    3288+    for (const auto index : tx_indices) {
    3289+        // If we already tried a package and failed for any reason, the combined hash was
    3290+        // cached in m_recent_rejects_reconsiderable.
    3291+        Package maybe_cpfp_package{ptx, cpfp_candidates.at(index).first};
    3292+        if (!m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {
    


    mzumsande commented at 8:15 pm on April 15, 2024:
    Why put the package hash into m_recent_rejects_reconsiderable instead of m_recent_rejects? We never reconsider a failed package after all as far as I understand it.

    glozow commented at 8:03 am on April 16, 2024:

    Yes I think we could use either m_recent_rejects_reconsiderable and m_recent_rejects right now to get the same behavior.

    I suppose one mild benefit of using m_recent_rejects_reconsiderable is that our m_recent_rejects bloom filter churns less frequently.

    The other benefit is extensibility in the future. In more general ancestor package relay, we could reject a parent+child for being too low feerate, but later accept it as parent+child+grandchild (where the grandchild is very high feerate).


    instagibbs commented at 11:36 am on April 16, 2024:

    The other benefit is extensibility in the future. In more general ancestor package relay, we could reject a parent+child for being too low feerate, but later accept it as parent+child+grandchild (where the grandchild is very high feerate).

    Perhaps this doesn’t matter but I’m not sure I understand the distinction here. We need the combined hash committed somewhere in a bloom filter to not fetch the same ancestor package again. If it’s different at all, we’ll fetch it regardless of which filter we add it to?


    glozow commented at 12:25 pm on April 16, 2024:
    Hm I think you’re right, it wouldn’t make a difference with downloads. Crossing that part out. Were we thinking of this within validation maybe? Linearize + chunk the package, see that a chunk has already been rejected as too low feerate, drop it?
  125. in src/net_processing.cpp:3288 in f5989537e7 outdated
    3276+
    3277+    CTransactionRef tx_orphan{nullptr};
    3278+    NodeId orphan_sender{-1};
    3279+    Assume(m_recent_rejects_reconsiderable.contains(parent_wtxid.ToUint256()));
    3280+
    3281+    // Create a 1p1c package using the first child that hasn't already been rejected. Sort
    


    mzumsande commented at 8:37 pm on April 15, 2024:
    Why do we only try once in the case there are multiple children in the orphanage, instead of trying multiple times until one package succeeds? To avoid some kind of spamming attacks that could exhaust our computing power?

    glozow commented at 7:51 am on April 16, 2024:

    Yes exactly, this is to bound computation (imagine if somebody sent us 100 fake orphans descended from 1 transaction and we processed them all here). My idea was to do something similar to regular orphan processing, where we have a work queue and limit to 1 item per ProcessMessages.

    There is no work queue here, though, and we drop the parent as soon as we try 1 (pass or fail). At coredev, we discussed adding a work queue for 1p1c as well. However, since it involves finding a way to store the low feerate parent, and we have plenty of low hanging fruit for improving orphan handling, we’ll save that for a later PR.

  126. mzumsande commented at 9:02 pm on April 15, 2024: contributor
    Started reviewing - haven’t looked at the tests yet.
  127. guard against MempoolAcceptResult::m_replaced_transactions
    It should never be a nullopt when the transaction result is valid -
    Assume() this is the case. However, as a belt-and-suspenders just in
    case it is nullopt, use an empty list.
    6f4da19cc3
  128. [doc] restore comment about why we check if ptx HasWitness before caching rejected txid c3c1e15831
  129. glozow force-pushed on Apr 16, 2024
  130. glozow commented at 11:09 am on April 16, 2024: member
    thanks for the review @mzumsande!
  131. in test/functional/p2p_opportunistic_1p1c.py:201 in 142d584dab outdated
    196+
    197+        # 2. Node requests the missing parent by txid.
    198+        parent_txid_int = int(low_fee_parent["txid"], 16)
    199+        peer_sender.wait_for_getdata([parent_txid_int])
    200+
    201+        # 3. Sender relays the parent. Parent+Child are evaluated as a package and accepted.
    


    instagibbs commented at 5:25 pm on April 16, 2024:
    0        # 3. Sender relays the parent. Parent+Child are evaluated as a package and rejected.
    
  132. in test/functional/p2p_opportunistic_1p1c.py:189 in 142d584dab outdated
    184+        hex_orphan_no_sig = node.createrawtransaction([{"txid": coin["txid"], "vout": coin["vout"]}], {address : coin["value"] - Decimal("0.0001")})
    185+        tx_orphan_bad_wit = tx_from_hex(hex_orphan_no_sig)
    186+        tx_orphan_bad_wit.wit.vtxinwit.append(CTxInWitness())
    187+        tx_orphan_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
    188+
    189+        peer_sender = node.add_p2p_connection(P2PInterface())
    


    instagibbs commented at 5:31 pm on April 16, 2024:

    would be good to explicitly test that the parent-giver isn’t punished in this scenario, and test if parent is consensus-bad it results in something expected.

      0diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py
      1index 603cbf08a9..1c887289dc 100755
      2--- a/test/functional/p2p_opportunistic_1p1c.py
      3+++ b/test/functional/p2p_opportunistic_1p1c.py
      4@@ -164,78 +164,129 @@ class PackageRelayTest(BitcoinTestFramework):
      5         # itself and with a child. This is necessary, otherwise high_fee_child can be censored.
      6         parent_txid_int = int(low_fee_parent["txid"], 16)
      7         peer_sender.wait_for_getdata([parent_txid_int])
      8 
      9         # 7. The low feerate parent + high feerate child are submitted as a package.
     10         peer_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))
     11 
     12         # 8. Both transactions should now be in mempool
     13         node_mempool = node.getrawmempool()
     14         assert low_fee_parent["txid"] in node_mempool
     15         assert high_fee_child["txid"] in node_mempool
     16 
     17         node.disconnect_p2ps()
     18 
     19     def test_orphan_consensus_failure(self):
     20         node = self.nodes[0]
     21         low_fee_parent = self.wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
     22         coin = low_fee_parent["new_utxo"]
     23         address = node.get_deterministic_priv_key().address
     24         # Create raw transaction spending the parent, but with no signature (a consensus error).
     25         hex_orphan_no_sig = node.createrawtransaction([{"txid": coin["txid"], "vout": coin["vout"]}], {address : coin["value"] - Decimal("0.0001")})
     26         tx_orphan_bad_wit = tx_from_hex(hex_orphan_no_sig)
     27         tx_orphan_bad_wit.wit.vtxinwit.append(CTxInWitness())
     28         tx_orphan_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
     29 
     30-        peer_sender = node.add_p2p_connection(P2PInterface())
     31+        child_peer_sender = node.add_p2p_connection(P2PInterface())
     32+        parent_peer_sender = node.add_p2p_connection(P2PInterface())
     33 
     34         # 1. Child is received first. It is missing an input.
     35         child_wtxid_int = int(tx_orphan_bad_wit.getwtxid(), 16)
     36-        peer_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=child_wtxid_int)]))
     37-        peer_sender.wait_for_getdata([child_wtxid_int])
     38-        peer_sender.send_and_ping(msg_tx(tx_orphan_bad_wit))
     39+        child_peer_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=child_wtxid_int)]))
     40+        child_peer_sender.wait_for_getdata([child_wtxid_int])
     41+        child_peer_sender.send_and_ping(msg_tx(tx_orphan_bad_wit))
     42 
     43         # 2. Node requests the missing parent by txid.
     44         parent_txid_int = int(low_fee_parent["txid"], 16)
     45-        peer_sender.wait_for_getdata([parent_txid_int])
     46+        child_peer_sender.wait_for_getdata([parent_txid_int])
     47 
     48-        # 3. Sender relays the parent. Parent+Child are evaluated as a package and accepted.
     49-        peer_sender.send_message(msg_tx(low_fee_parent["tx"]))
     50+        # 3. "Honest" sender relays the parent. Parent+Child are evaluated as a package and accepted.
     51+        parent_peer_sender.send_message(msg_tx(low_fee_parent["tx"]))
     52 
     53         # 4. Transactions should not be in mempool.
     54         node_mempool = node.getrawmempool()
     55         assert low_fee_parent["txid"] not in node_mempool
     56         assert tx_orphan_bad_wit.rehash() not in node_mempool
     57 
     58         # 5. Peer sent a consensus-invalid transaction.
     59-        peer_sender.wait_for_disconnect()
     60+        child_peer_sender.wait_for_disconnect()
     61+
     62+        # 6. "Honest" peer unpunished
     63+        parent_peer_sender.sync_with_ping()
     64+
     65+    def test_parent_consensus_failure(self):
     66+        node = self.nodes[0]
     67+        low_fee_parent = self.wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
     68+
     69+        # Add bad sigature to parent
     70+        tx_parent_bad_wit = tx_from_hex(low_fee_parent["hex"])
     71+        tx_parent_bad_wit.wit.vtxinwit.append(CTxInWitness())
     72+        tx_parent_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
     73+
     74+        coin = low_fee_parent["new_utxo"]
     75+        address = node.get_deterministic_priv_key().address
     76+        # Create raw transaction spending the parent, but with no signature (a consensus error).
     77+        hex_orphan_no_sig = node.createrawtransaction([{"txid": coin["txid"], "vout": coin["vout"]}], {address : coin["value"] - Decimal("0.0001")})
     78+        tx_orphan_bad_wit = tx_from_hex(hex_orphan_no_sig)
     79+        tx_orphan_bad_wit.wit.vtxinwit.append(CTxInWitness())
     80+        tx_orphan_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
     81+
     82+        child_peer_sender = node.add_p2p_connection(P2PInterface())
     83+        parent_peer_sender = node.add_p2p_connection(P2PInterface())
     84+
     85+        # 1. Child is received first. It is missing an input.
     86+        child_wtxid_int = int(tx_orphan_bad_wit.getwtxid(), 16)
     87+        child_peer_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=child_wtxid_int)]))
     88+        child_peer_sender.wait_for_getdata([child_wtxid_int])
     89+        child_peer_sender.send_and_ping(msg_tx(tx_orphan_bad_wit))
     90+
     91+        # 2. Node requests the missing parent by txid.
     92+        parent_txid_int = int(tx_parent_bad_wit.rehash(), 16)
     93+        child_peer_sender.wait_for_getdata([parent_txid_int])
     94+
     95+        # 3. Parent sender relays the parent. Only parent should be evaluated.
     96+        parent_peer_sender.send_message(msg_tx(tx_parent_bad_wit))
     97+
     98+        # 4. Transactions should not be in mempool.
     99+        node_mempool = node.getrawmempool()
    100+        assert tx_parent_bad_wit.rehash() not in node_mempool
    101+        assert tx_orphan_bad_wit.rehash() not in node_mempool
    102+
    103+        # 5. Peer sent a consensus-invalid transaction.
    104+        parent_peer_sender.wait_for_disconnect()
    105+
    106+        # 6. Child-sending peer unpunished for now!
    107+        child_peer_sender.sync_with_ping()
    108 
    109     def run_test(self):
    110         node = self.nodes[0]
    111         self.wallet = MiniWallet(node)
    112         self.wallet_nonsegwit = MiniWallet(node, mode=MiniWalletMode.RAW_P2PK)
    113         self.generate(self.wallet_nonsegwit, 10)
    114         self.generate(self.wallet, 20)
    115 
    116         fill_mempool(self, node, self.wallet)
    117 
    118         self.log.info("Check opportunistic 1p1c logic when parent (txid != wtxid) is received before child")
    119         self.test_basic_parent_then_child(self.wallet)
    120 
    121         self.log.info("Check opportunistic 1p1c logic when parent (txid == wtxid) is received before child")
    122         self.test_basic_parent_then_child(self.wallet_nonsegwit)
    123 
    124         self.log.info("Check opportunistic 1p1c logic when child is received before parent")
    125         self.test_basic_child_then_parent()
    126 
    127         self.log.info("Check opportunistic 1p1c logic when 2 candidate children exist (parent txid != wtxid)")
    128         self.test_low_and_high_child(self.wallet)
    129 
    130         self.log.info("Check opportunistic 1p1c logic when 2 candidate children exist (parent txid == wtxid)")
    131         self.test_low_and_high_child(self.wallet_nonsegwit)
    132 
    133         self.log.info("Check opportunistic 1p1c logic with consensus-invalid orphan causes disconnect")
    134         self.test_orphan_consensus_failure()
    135 
    136+        self.log.info("Check opportunistic 1p1c logic doesn't evaluate package with consensus-invalid parent")
    137+        self.test_parent_consensus_failure()
    138+
    139 
    140 if __name__ == '__main__':
    141     PackageRelayTest().main()
    

    glozow commented at 12:24 pm on April 19, 2024:
    Added this test and tweaked a bit to check that orphan stays + can still be resolved.
  133. in test/functional/p2p_opportunistic_1p1c.py:115 in 142d584dab outdated
    110+        node.disconnect_p2ps()
    111+
    112+    def test_low_and_high_child(self, wallet):
    113+        node = self.nodes[0]
    114+        low_fee_parent = wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
    115+        low_fee_child = wallet.create_self_transfer(utxo_to_spend=low_fee_parent["new_utxo"], fee_rate=2*FEERATE_1SAT_VB)
    


    instagibbs commented at 6:06 pm on April 16, 2024:
    suggestion: call this med_fee_child and use the mempoolminfee directly since that’s a more meaningful value than 2*FEERATE_1SAT_VB which is below the floating minfee

    glozow commented at 12:24 pm on April 19, 2024:
    done
  134. in src/test/txpackage_tests.cpp:66 in 54d0c78d10 outdated
    61+    Wtxid wtxid_6{WtxidFromString("0xe065bac15f62bb4e761d761db928ddee65a47296b2b776785abb912cdec474e3")};
    62+
    63+    BOOST_CHECK(wtxid_0.GetHex() < wtxid_1.GetHex());
    64+    BOOST_CHECK(wtxid_1.GetHex() < wtxid_2.GetHex());
    65+    BOOST_CHECK(wtxid_2.GetHex() < wtxid_3.GetHex());
    66+    BOOST_CHECK(wtxid_1.GetHex() < wtxid_3.GetHex());
    


    sr-gi commented at 7:51 pm on April 16, 2024:

    In: 54d0c78d104fb5412a194816590a06cad8cadf80

    This seems redundant


    glozow commented at 3:54 pm on April 18, 2024:
    deleted
  135. in src/net_processing.cpp:4357 in 6f4da19cc3 outdated
    4353@@ -4354,7 +4354,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    4354         const TxValidationState& state = result.m_state;
    4355 
    4356         if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
    4357-            ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value());
    4358+            Assume(result.m_replaced_transactions.has_value());
    


    sdaftuar commented at 12:42 pm on April 17, 2024:

    This is a code style nit, but I think I agree with https://github.com/bitcoin/bitcoin/pull/21062/files#r571975710 that there’s not much benefit from using a std::optional on m_replaced_transactions? It just seems to lead to extra code around understanding when the field is set or not, when I think it would be simpler to say that it’s just a list which is either empty or non-empty based on whether a replacement took place.

    Anyway I have no objection to the changes in this commit; just wanted to mention it so that if others agree, then perhaps this is a change we could make in the future.


    glozow commented at 10:04 am on April 23, 2024:
    I definitely agree it’s awkward and doesn’t have much benefit. I’ll leave the assume/handling here, and maybe in a followup we can change it to non-optional.

    glozow commented at 12:43 pm on May 1, 2024:
    Done in #30012
  136. in src/net_processing.cpp:3337 in 25c712711d outdated
    3288+    // Create a 1p1c package using the first child that hasn't already been rejected. Sort
    3289+    // the children in random order to not create a bias that attackers can use to delay
    3290+    // package acceptance. Create a random permutation of the indices.
    3291+    std::vector<size_t> tx_indices(cpfp_candidates.size());
    3292+    std::iota(tx_indices.begin(), tx_indices.end(), 0);
    3293+    Shuffle(tx_indices.begin(), tx_indices.end(), m_rng);
    


    mzumsande commented at 1:20 pm on April 17, 2024:
    would it make sense to match peers and (if possible) always pick a child from the orphanage that was sent to us by the same peer that sent us the parent (instead of a random one)? That way, it wouldn’t be possible that a third peer could send us multiple low-fee children that we’d store in the orphanage, in the hope that we pick one of those and reject the package.

    instagibbs commented at 1:37 pm on April 17, 2024:

    I think I made this point in person from the philosophical standpoint that orphanage-churning aside, we probably shouldn’t allow peers to “cross-talk” when it comes to package evaluation and possible punishment.

    In practice I think an adversary can just churn the orphanage until Future Work happens, but still might be a good conceptual framework to adhere to?


    glozow commented at 1:57 pm on April 17, 2024:
    great idea, will change 👍

    glozow commented at 3:56 pm on April 18, 2024:
    Did this now. We first gather all children by the same peer and try by recency order. If we cannot find one that matches, we look for children not by this peer, and try in random order.

    instagibbs commented at 4:32 pm on April 18, 2024:
    For some code deduplication you could just have GetChildrenFromPeer which takes std::optional<NodeId>, returning full std::vector<std::pair<CTransactionRef, NodeId>> list if std::nullopt or filtered to the specific nodeid otherwise. You can call it filtered once, then if a suitable orphan isn’t found, call it again unfiltered.

    glozow commented at 1:53 pm on April 24, 2024:
    I don’t think that is much simpler. Also I think this is easier to delete if/when we don’t try orphans from different peers in the future.
  137. in src/test/txpackage_tests.cpp:77 in 142d584dab outdated
    72+    DataStream stream4{
    73+        ParseHex("02000000000101964b8aa63509579ca6086e6012eeaa4c2f4dd1e283da29b67c8eea38b3c6fd220000000000fdffffff0294c618000000000017a9145afbbb42f4e83312666d0697f9e66259912ecde38768fa2c0000000000160014897388a0889390fd0e153a22bb2cf9d8f019faf50247304402200547406380719f84d68cf4e96cc3e4a1688309ef475b150be2b471c70ea562aa02206d255f5acc40fd95981874d77201d2eb07883657ce1c796513f32b6079545cdf0121023ae77335cefcb5ab4c1dc1fb0d2acfece184e593727d7d5906c78e564c7c11d125cf0c00"),
    74+    };
    75+    CTransaction tx4(deserialize, TX_WITH_WITNESS, stream4);
    76+    CTransactionRef ptx4{MakeTransactionRef(tx4)};
    77+    Txid txid_4{TxidFromString("0xbd0f71c1d5e50589063e134fad22053cdae5ab2320db5bf5e540198b0b5a4e69")};
    


    sr-gi commented at 2:55 pm on April 17, 2024:

    In: 54d0c78d104fb5412a194816590a06cad8cadf80

    I don’t see what the point of building the txid from a literal in this way and comparing it to the one obtained via GetHash() is. I’m guessing you’re trying to make the point that the txids are actually what you are claiming them to be (as opposed to just writing them in a comment), so the reader can manually check the difference between internal and human-readable lexicographic ordering. Is that really necessary?


    glozow commented at 3:54 pm on April 18, 2024:
    I figure it’s easier to read the test this way; you can easily tell that the lexicographical ordering is what I claim it to be
  138. in src/test/txpackage_tests.cpp:123 in 54d0c78d10 outdated
    118+    std::vector<CTransactionRef> package_564{ptx5, ptx6, ptx4};
    119+    std::vector<CTransactionRef> package_546{ptx5, ptx4, ptx6};
    120+    std::vector<CTransactionRef> package_645{ptx6, ptx4, ptx5};
    121+    std::vector<CTransactionRef> package_654{ptx6, ptx5, ptx4};
    122+    // All of them must have the same package hash 2dc93431aa6eb415a4e0995f77c78098d7ed97ca6f123a0d77a7747339525acd
    123+    std::vector<uint256> expected_order_txns{wtxid_4.ToUint256(), wtxid_5.ToUint256(), wtxid_6.ToUint256()};
    


    sr-gi commented at 3:55 pm on April 17, 2024:

    In: 54d0c78d104fb5412a194816590a06cad8cadf80

    Calling ToUint256() shouldn’t be needed


    glozow commented at 3:48 pm on April 18, 2024:
    done, thanks
  139. in src/test/txpackage_tests.cpp:137 in 54d0c78d10 outdated
    132+    std::vector<Wtxid> wtxids_546{wtxid_5, wtxid_4, wtxid_6};
    133+    BOOST_CHECK_EQUAL(calculated_hash_456, GetCombinedHash(wtxids_546));
    134+    std::vector<Wtxid> wtxids_654{wtxid_6, wtxid_5, wtxid_4};
    135+    BOOST_CHECK_EQUAL(calculated_hash_456, GetCombinedHash(wtxids_654));
    136+
    137+    std::vector<uint256> expected_order_wtxids{wtxid_1.ToUint256(), wtxid_2.ToUint256(), wtxid_3.ToUint256(),
    


    sr-gi commented at 3:56 pm on April 17, 2024:

    In: 54d0c78d104fb5412a194816590a06cad8cadf80

    Same as before, no need to cast to ToUint256


    glozow commented at 3:48 pm on April 18, 2024:
    done, thanks
  140. in src/test/txpackage_tests.cpp:113 in 54d0c78d10 outdated
    108+    BOOST_CHECK(txid_4.ToUint256() != wtxid_4.ToUint256());
    109+    BOOST_CHECK(txid_6.ToUint256() != wtxid_6.ToUint256());
    110+
    111+    // Testing that package hash is sorting by wtxids, not txids.
    112+    // For tx4 and tx5, txid order != wtxid order
    113+    BOOST_CHECK(txid_5.GetHex() < txid_4.GetHex());
    


    sr-gi commented at 4:06 pm on April 17, 2024:

    In: 54d0c78d104fb5412a194816590a06cad8cadf80

    I’m struggling to see the usefulness of this. You are showing that, for the provided transactions, the ordering may be different based on the representation used (wtxid/txid/ToUint256/GetHex), but I don’t think this clearly shows that the package hash is using one or the other.

    You are already proving that the order is the one you are expecting by manually computing calculated_hash_456. You could also create different orderings based on ToUint256 (instead of GetHex) and txid (instead of wtxid) and check how those are not equal to calculated_hash_456 (after having checked that all of the permutations of GetPackageHash are equal to calculated_hash_456)


    glozow commented at 3:53 pm on April 18, 2024:
    Ok added tests calculating what the hashes would be if we used another order + checking they’re not the same
  141. in src/test/txpackage_tests.cpp:299 in 54d0c78d10 outdated
    295@@ -190,17 +296,28 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup)
    296         BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted");
    297         BOOST_CHECK(IsChildWithParents({tx_parent, tx_child}));
    298         BOOST_CHECK(IsChildWithParentsTree({tx_parent, tx_child}));
    299+        BOOST_CHECK_EQUAL(GetPackageHash({tx_child}), GetCombinedHash({tx_child->GetWitnessHash()}));
    


    sr-gi commented at 5:23 pm on April 17, 2024:

    In: 54d0c78d104fb5412a194816590a06cad8cadf80

    nit: I think it wouldn’t hurt to have a comment here along the lines of:

    0/// Check that `GetPackageHash`/ `GetCombinedHash` are consistent with each other, and that the input order does not affect the resulting hash 
    

    glozow commented at 3:48 pm on April 18, 2024:
    (deleted GetCombinedHash)
  142. in src/test/orphanage_tests.cpp:195 in 91f4efa420 outdated
    190+    BOOST_CHECK(orphanage.AddTx(child_p1n0_p1n1, node));
    191+    BOOST_CHECK(orphanage.AddTx(child_p1n0_p2n0, node));
    192+
    193+    // Check that GetChildren returns what is expected.
    194+    std::set<Wtxid> expected_parent1_children{child_p1n0->GetWitnessHash(), child_p1n0_p2n0->GetWitnessHash(), child_p1n0_p1n1->GetWitnessHash()};
    195+    std::set<Wtxid> expected_parent2_children{child_p2n1->GetWitnessHash(), child_p1n0_p2n0->GetWitnessHash()};
    


    sr-gi commented at 8:08 pm on April 17, 2024:

    in 91f4efa420958a93f4620379f8830231f276b23b

    This should also be comparable by CTransactionRef, shouldn’t it? So GetWitnessHash doesn’t need to be called here and in following loops


    glozow commented at 3:54 pm on April 18, 2024:
    done
  143. in src/test/orphanage_tests.cpp:205 in 91f4efa420 outdated
    200+        BOOST_CHECK(expected_parent1_children.contains(child->GetWitnessHash()));
    201+    }
    202+    auto parent2_children{orphanage.GetChildren(parent2)};
    203+    BOOST_CHECK_EQUAL(parent2_children.size(), expected_parent2_children.size());
    204+    for (const auto& [child, peer] : parent2_children) {
    205+        BOOST_CHECK(expected_parent2_children.count(child->GetWitnessHash()) > 0);
    


    sr-gi commented at 8:14 pm on April 17, 2024:

    in: 91f4efa420958a93f4620379f8830231f276b23b

    nit: Use contains instead of count(...) > 0 for consistency with the previous check (previous loop)


    glozow commented at 3:54 pm on April 18, 2024:
    done
  144. sr-gi commented at 8:17 pm on April 17, 2024: member
    First pass. Reviewed up to 91f4efa420958a93f4620379f8830231f276b23b
  145. in test/functional/p2p_1p1c_network.py:111 in 142d584dab outdated
    106+        for i in range(self.num_nodes):
    107+            peer = self.peers[i]
    108+            for tx in transactions_to_presend[i]:
    109+                inv = CInv(t=MSG_WTX, h=int(tx.getwtxid(), 16))
    110+                peer.send_and_ping(msg_inv([inv]))
    111+                peer.wait_for_getdata([int(tx.getwtxid(), 16)])
    


    theStack commented at 7:25 am on April 18, 2024:
    nit, feel free to ignore: strictly speaking those 3 lines are not needed, as sending in the tx unsolicitedly (without prior inv/getdata) works as well, though not adhering to the typical protocol flow.

    glozow commented at 3:48 pm on April 18, 2024:
    removed
  146. glozow force-pushed on Apr 18, 2024
  147. glozow commented at 4:02 pm on April 18, 2024: member

    Main changes:

    • Try same-peer orphanage children (most recent first) before different-peer ones (randomized). Replaced GetChildren with GetChildrenFromSamePeer and GetChildrenFromDifferentPeer.
    • Deleted GetCombinedHash because it’s unused. Also deleted the tests for it
    • Changed GetPackageHash a bit. Using single-sha256 instead of double-sha256 and using just wtxids instead of serialized vector
  148. glozow force-pushed on Apr 18, 2024
  149. DrahtBot commented at 4:29 pm on April 18, 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/23986234746

  150. DrahtBot added the label CI failed on Apr 18, 2024
  151. glozow commented at 9:40 am on April 19, 2024: member
    debugging the p2p_opportunistic_1p1c.py failure. I think the wallet one is unrelated.
  152. glozow force-pushed on Apr 19, 2024
  153. DrahtBot removed the label CI failed on Apr 19, 2024
  154. glozow commented at 12:32 pm on April 19, 2024: member
    Ok ready for review again.
  155. in test/functional/p2p_opportunistic_1p1c.py:56 in a053911cb9 outdated
    51+            "-datacarriersize=100000",
    52+            "-maxmempool=5",
    53+        ]]
    54+        self.supports_cli = False
    55+
    56+    def subest_check(self):
    


    instagibbs commented at 2:08 pm on April 19, 2024:
    duplicated cleanup, or something?

    glozow commented at 2:18 pm on April 19, 2024:
    Ah forgot to delete, yes

    glozow commented at 11:57 am on April 22, 2024:
    deleted
  156. in test/functional/p2p_opportunistic_1p1c.py:278 in a053911cb9 outdated
    273+        assert high_fee_child["txid"] not in node_mempool
    274+
    275+        # 5. Peer sent a consensus-invalid transaction.
    276+        fake_parent_sender.wait_for_disconnect()
    277+
    278+        self.log.info("Check opportunistic 1p1c logic with consensus-invalid parent causes disconnect of the correct peer")
    


    instagibbs commented at 2:56 pm on April 19, 2024:
    duplicate log?

    glozow commented at 11:58 am on April 22, 2024:
    Ah that was supposed to be another log, should be fixed now
  157. Fabcien referenced this in commit 67c0d93982 on Apr 19, 2024
  158. in src/test/txpackage_tests.cpp:85 in 5c8aa65764 outdated
    80+    BOOST_CHECK_EQUAL(tx_2.GetWitnessHash(), wtxid_2);
    81+    BOOST_CHECK_EQUAL(tx_3.GetWitnessHash(), wtxid_3);
    82+
    83+    BOOST_CHECK(wtxid_1.GetHex() < wtxid_2.GetHex());
    84+    BOOST_CHECK(wtxid_2.GetHex() < wtxid_3.GetHex());
    85+    BOOST_CHECK(wtxid_1.GetHex() < wtxid_3.GetHex());
    


    sr-gi commented at 4:18 pm on April 20, 2024:

    In 5c8aa657642ef24e711a73c28278644f14117d73

    This last check is still redundant


    glozow commented at 11:44 am on April 22, 2024:
    deleted
  159. in src/txorphanage.cpp:286 in ca9f03f220 outdated
    281+std::vector<std::pair<CTransactionRef, NodeId>> TxOrphanage::GetChildrenFromDifferentPeer(const CTransactionRef& parent, NodeId nodeid) const
    282+{
    283+    LOCK(m_mutex);
    284+
    285+    // First construct vector of iterators to ensure we do not return duplicates of the same tx.
    286+    std::vector<OrphanMap::iterator> unique_iters;
    


    sr-gi commented at 4:56 pm on April 20, 2024:

    In ca9f03f2207854c849a14d7af2fcd91a5f675e14

    Wouldn’t it be easier (and simpler) to use a set here instead of a vector (as used to be the case in 9dc967195c4965973be0174ae6041be70a886c7a). It makes sense to use a vec in GetChildrenFromSamePeer given you are sorting based on a custom order, but here a set should be equivalent and involve less boilerplate (?)


    glozow commented at 11:39 am on April 22, 2024:
    I copied this from the way we do unique_parents in orphan parent requests (see the discussion on #19596 about the dynamic memory usage and speed). However I don’t mind either way and agree a std::set would be simpler, so happy to change if people prefer using a set.
  160. in src/txorphanage.cpp:251 in ca9f03f220 outdated
    246+{
    247+    LOCK(m_mutex);
    248+
    249+    // First construct a vector of iterators to ensure we do not return duplicates of the same tx
    250+    // and so we can sort by nTimeExpire.
    251+    std::vector<OrphanMap::iterator> unique_iters;
    


    sr-gi commented at 5:10 pm on April 20, 2024:

    In ca9f03f2207854c849a14d7af2fcd91a5f675e14

    I think this is misleading (both the variable name and the comment).

    The reason a vector is chosen is so you can sort based on nTimeExpire. Removing the duplicates comes later (for most of the function, the iters may not be unique).

    For the sake of future readers it may be worth changing it


    glozow commented at 11:56 am on April 22, 2024:
    I think it’s pretty common to name a temporary data structure based on its intended result. But ok, I’ve changed it to iters now.
  161. in src/test/orphanage_tests.cpp:200 in 42859548ab outdated
    195+        BOOST_CHECK(orphanage.AddTx(child_p1n0_p2n0, node1));
    196+
    197+        std::set<CTransactionRef> expected_parent1_children{child_p1n0, child_p1n0_p2n0, child_p1n0_p1n1};
    198+        std::set<CTransactionRef> expected_parent2_children{child_p2n1, child_p1n0_p2n0};
    199+
    200+        // Check contents and ensure transactions are returned in order of recency.
    


    sr-gi commented at 5:43 pm on April 20, 2024:

    In 42859548ab5aebf40da6089b85065f7c204b992a

    I don’t think the order of recency is being tested


    glozow commented at 11:40 am on April 22, 2024:
    Ah, forgot to remove the comment. We need to make time parameterizable across TxOrphanage members to test order, so not testing / saving for followup.

    glozow commented at 11:44 am on April 22, 2024:
    deleted
  162. in src/test/orphanage_tests.cpp:203 in 42859548ab outdated
    198+        std::set<CTransactionRef> expected_parent2_children{child_p2n1, child_p1n0_p2n0};
    199+
    200+        // Check contents and ensure transactions are returned in order of recency.
    201+        const auto parent1_children_from_sender{orphanage.GetChildrenFromSamePeer(parent1, node1)};
    202+        for (const auto& tx : parent1_children_from_sender) {
    203+            BOOST_CHECK(expected_parent1_children.contains(tx));
    


    sr-gi commented at 5:50 pm on April 20, 2024:

    In 42859548ab5aebf40da6089b85065f7c204b992a

    To make sure the two collections are equal you also need to check the their sizes match. This applies to the four cases.


    glozow commented at 11:44 am on April 22, 2024:
    done
  163. sr-gi commented at 6:01 pm on April 20, 2024: member

    Second pass (up to the same height: 42859548ab5aebf40da6089b85065f7c204b992a)

    Left some additional comments on the updated approach.

  164. glozow force-pushed on Apr 22, 2024
  165. in src/test/orphanage_tests.cpp:225 in 507f5ea12a outdated
    206+        BOOST_CHECK_EQUAL(parent2_children_from_sender.size(), expected_parent2_children.size());
    207+        for (const auto& tx : parent2_children_from_sender) {
    208+            BOOST_CHECK(expected_parent2_children.contains(tx));
    209+        }
    210+        // The peer must match
    211+        BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent1, node2).empty());
    


    instagibbs commented at 4:39 pm on April 22, 2024:

    nit

    0        BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent1, node2).empty());
    1        BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent2, node2).empty());
    

    glozow commented at 9:57 am on April 23, 2024:
    Done. Also refactored orphanage_tests to be more readable.
  166. in src/test/orphanage_tests.cpp:226 in 507f5ea12a outdated
    221+        for (const auto& [tx, peer] : parent2_children_not_other) {
    222+            BOOST_CHECK(expected_parent2_children.contains(tx));
    223+            BOOST_CHECK_EQUAL(peer, node1);
    224+        }
    225+
    226+        // There shouldn't be any children of this tx
    


    instagibbs commented at 4:41 pm on April 22, 2024:

    nit

    0        // There shouldn't be any children of this tx in orphanage
    
  167. instagibbs commented at 5:01 pm on April 22, 2024: member

    reviewed through 7d220c6a5c0e0c5e8cfe79ebd2eae6e845d1d983

    tested and confirmed fuzz coverage is hitting meaningful GetChildrenFrom* results

    continuing longer range testing

  168. in src/net_processing.cpp:4617 in 668313e00d outdated
    4585@@ -4432,7 +4586,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    4586                     // protocol for getting all unconfirmed parents.
    4587                     const auto gtxid{GenTxid::Txid(parent_txid)};
    4588                     AddKnownTx(*peer, parent_txid);
    4589-                    if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) AddTxAnnouncement(pfrom, gtxid, current_time);
    4590+                    // Exclude m_recent_rejects_reconsiderable: the missing parent may have been
    4591+                    // previously rejected for being too low feerate. This orphan might CPFP it.
    4592+                    if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) AddTxAnnouncement(pfrom, gtxid, current_time);
    


    sdaftuar commented at 5:23 pm on April 22, 2024:

    Let’s say we’ve got a transaction T that is missing inputs. We loop through the missing parents and see that none are in m_recent_rejects, so we drop into this block of code.

    If there’s more than 1 missing parent in m_recent_rejects_reconsiderable, is there any benefit to fetching them? It seems like we could tell in advance that validation would fail in that circumstance, because we only try 1P1C packages.

    I don’t know if it’s worth additional complexity to deal with this case, but just wanted to flag the potential bandwidth waste if this were a common pattern.


    glozow commented at 12:11 pm on April 23, 2024:

    Good point, I don’t think this is too complex so I’ve added it.

    While working on this, I remembered/realized that if there are any other unconfirmed parents at all, we will reject it because the package must be child-with-unconfirmed-parents, enforced here: https://github.com/bitcoin/bitcoin/blob/256e1703197fdddd78bc6d659431cd0fc3b63cde/src/validation.cpp#L1558-L1564

    IIRC this was the way to check that a package was “2 generations only”. But I don’t know how useful this is, and it’s quite annoying here, so perhaps we should consider getting rid of this restriction in a followup… Then we can accept packages where the child has parents already in mempool.

  169. in src/test/fuzz/txorphan.cpp:164 in b9caa4cfcb outdated
    155@@ -136,6 +156,12 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
    156                     orphanage.LimitOrphans(limit, limit_orphans_rng);
    157                     Assert(orphanage.Size() <= limit);
    158                 });
    159+
    160+        // Set tx as potential parent to be used for future GetChildren() calls.
    161+        if (!ptx_potential_parent || fuzzed_data_provider.ConsumeBool()) {
    162+            ptx_potential_parent = tx;
    163+        }
    


    sr-gi commented at 7:23 pm on April 22, 2024:

    In b9caa4cfcbd1e176ab3ecd61973ab6721570aecb

    I’m not too familiar with fuzzing, but wouldn’t it be better if this be set unconditionally? This will trigger on the first iteration of the loop, and then based on a coin flip AFAICT, which means that if the variable is not overwritten, we will call orphanage.AddChildrenToWorkSet(*ptx_potential_parent); with the same transaction multiple times. This is harmless, given the internals just add data to a set, so multiple calls won’t change it, but it is redundant (and so are the fors that will trigger latter).


    sr-gi commented at 3:24 pm on April 23, 2024:
    At the very least, if this is kept as is, the assignment should be moved to the outer context. Right now it is part of LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10 * DEFAULT_MAX_ORPHAN_TRANSACTIONS), which means that it would potentially be set to the same value over and over again

    glozow commented at 9:23 am on April 26, 2024:
    Thanks, I’ve moved it to the outer loop. Kept bool, seems harmless and I imagine we can find some more interesting code paths by sometimes not setting it immediately.
  170. in src/net_processing.cpp:3315 in 668313e00d outdated
    3283+    const auto cpfp_candidates_same_peer{m_orphanage.GetChildrenFromSamePeer(ptx, nodeid)};
    3284+
    3285+    // These children should be sorted from most newest to oldest. In the case of children that
    3286+    // replace each other, this helps us accept the highest feerate (most recent) one most
    3287+    // efficiently.
    3288+    for (const auto& child : cpfp_candidates_same_peer) {
    


    sdaftuar commented at 10:27 pm on April 22, 2024:
    Just an observation: I guess the more common case will likely be multiple children of the same parent transaction (rather than conflicting children). Still, we have no idea which child will have the best chance of successfully bumping it, so this ordering seems as good as any.

    glozow commented at 9:59 am on April 23, 2024:
    Edited the comment to say this is a probably uncommon case
  171. sdaftuar approved
  172. sdaftuar commented at 10:38 pm on April 22, 2024: member
    Code review ACK (apart from the tests, which I only skimmed). Will test…
  173. glozow force-pushed on Apr 23, 2024
  174. glozow force-pushed on Apr 23, 2024
  175. glozow force-pushed on Apr 23, 2024
  176. in src/net_processing.cpp:3335 in bb3bf0a39c outdated
    3330+
    3331+    if (tx_orphan) {
    3332+        const Package package_1p1c{ptx, tx_orphan};
    3333+        const std::vector<NodeId> senders{nodeid, orphan_sender};
    3334+        const auto package_result{ProcessNewPackage(m_chainman.ActiveChainstate(), m_mempool, package_1p1c, /*test_accept=*/false, /*client_maxfeerate=*/std::nullopt)};
    3335+        LogDebug(BCLog::TXPACKAGES, "package evaluation for parent %s (wtxid=%s) + child %s (wtxid=%s) in orphanage: %s\n",
    


    instagibbs commented at 12:26 pm on April 23, 2024:

    FWIW: I added logging here to see what number of parents and given by different peers than the orphan. After a day of running it’s 0 out of 162.

    Is there a reason to think this should be a common pattern?


    sdaftuar commented at 12:31 pm on April 23, 2024:
    If a parent+child are relayed roughly simultaneously across the network, then I would expect there to be times when we download the transactions out of order (eg because we send a request for the parent to a different peer than the request for the child).

    instagibbs commented at 12:40 pm on April 23, 2024:

    I guess it’ll likely be something like:

    1. peer A sends child, child is put in orphanage and parent tx request queued(but not yet sent)
    2. peer B has slightly out of date feefilter for your node, sends INV for parent

    though I’m not convinced this really will make a difference


    glozow commented at 12:55 pm on April 23, 2024:
    fwiw it’s definitely not common, but I do see them occasionally and have a handful of acceptances of packages from 2 different peers

    glozow commented at 1:00 pm on April 23, 2024:

    though I’m not convinced this really will make a difference

    Is the suggestion to drop this and only try same-peer packages?


    sdaftuar commented at 1:04 pm on April 23, 2024:

    I guess it’ll likely be something like:

    1. peer A sends child, child is put in orphanage and parent tx request queued(but not yet sent)
    2. peer B has slightly out of date feefilter for your node, sends INV for parent

    What I’d expect to happen is that we get inv’s for parent+child from both Peer A and Peer B, and we happen to request the parent from A and the child from B, but the child arrives first – it’s put in the orphanage, and since a request for the parent is already in flight to A, we don’t send another request for the parent to B.

    (In this scenario I guess I’m assuming no feefilter, or that the feefilter value is slightly stale.)


    instagibbs commented at 1:25 pm on April 23, 2024:

    Is the suggestion to drop this and only try same-peer packages?

    If this is hitting actual usage, no, because it’s coded and I actually reviewed it, and tests seem to cover it. I’m unsure if in BIP331-like world it would be used since it would be receiver-driven, but that might be thinking too far ahead.

    (In this scenario I guess I’m assuming no feefilter, or that the feefilter value is slightly stale.)

    Latter scenario seems likely in practice as I see things right on the bubble all the time.


    glozow commented at 2:00 pm on April 24, 2024:
    Ok, marking this as resolved
  177. in test/functional/p2p_opportunistic_1p1c.py:312 in 55b1280c52 outdated
    307+        # 2. Send child.
    308+        peer_sender.send_and_ping(msg_tx(child_bumping["tx"]))
    309+
    310+        # 3. Node should not request any parents, as it should recognize that it will not accept
    311+        # multi-parent-1-child packages.
    312+        node.bumpmocktime(60)
    


    instagibbs commented at 2:29 pm on April 23, 2024:
    is there a constant we can put here?

    glozow commented at 2:02 pm on April 24, 2024:
    Added
  178. in src/test/fuzz/txorphan.cpp:1 in 55b1280c52


    sr-gi commented at 2:50 pm on April 23, 2024:

    Reviewing the changes in this file I realized something seems to be odd with the way the test was initially designed:

    duplicate_input is defined as a flag that, if set, will allow duplicate inputs to be used when building the transaction. However, the way this is approached is weird: if duplicate_input is not set, upon picking prevout as input, we will kick it off outpoints to make sure we don’t pick it up again as another input. Later on, all selected inputs are added back to outpoints, so they can be used again in the next iteration. However, this is done unconditionally, therefore, if duplicate_input is set, we will end up adding the same element to the collection twice (given we would not have kicked it off).

    Hence, the more an input is picked, the more copies of that element will end up added to the collection, and the more likely that input will be picked. PickValue is already picking things randomly (AFAICT), so there is no need to have duplicates in the collection.


    sr-gi commented at 2:52 pm on April 23, 2024:
    I also found the way num_out is picked to be a bit strange. num_in is picked based on the outpoints size, given that is our input candidate set. However, num_out is picked in the same way, and it doesn’t really need to be bound by that, just by making sure the transaction stays within its size limits

    sr-gi commented at 2:53 pm on April 23, 2024:
    I don’t think this needs to be fixed in this PR btw, a followup would do

    glozow commented at 9:20 am on April 26, 2024:
    As discussed offline, I agree the fuzz test has a few quirks that could be fixed up, but I think they are out of scope for this PR so I’m going to mark this resolved.
  179. in test/functional/p2p_opportunistic_1p1c.py:377 in 55b1280c52 outdated
    312+        node.bumpmocktime(60)
    313+        peer_sender.sync_with_ping()
    314+        assert "getdata" not in peer_sender.last_message
    315+
    316+
    317+    def run_test(self):
    


    instagibbs commented at 3:38 pm on April 23, 2024:

    regression test for the Assume() that was hit? And unsetting the mocktime to not have interference in subtests…

      0diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py
      1index 2eaa2a0a79..f3a741498b 100755
      2--- a/test/functional/p2p_opportunistic_1p1c.py
      3+++ b/test/functional/p2p_opportunistic_1p1c.py
      4@@ -291,58 +291,97 @@ class PackageRelayTest(BitcoinTestFramework):
      5         child_bumping = self.wallet_nonsegwit.create_self_transfer_multi(
      6             utxos_to_spend=[parent_low_1["new_utxo"], parent_low_2["new_utxo"]],
      7             fee_per_output=999*parent_low_1["tx"].get_vsize(),
      8         )
      9 
     10         peer_sender = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=4, connection_type="outbound-full-relay")
     11 
     12         # 1. Send both parents. Each should be rejected for being too low feerate.
     13         # Send unsolicited so that we can later check that no "getdata" was ever received.
     14         peer_sender.send_and_ping(msg_tx(parent_low_1["tx"]))
     15         peer_sender.send_and_ping(msg_tx(parent_low_2["tx"]))
     16 
     17         # parent_low_1 and parent_low_2 are rejected for being low feerate.
     18         assert parent_low_1["txid"] not in node.getrawmempool()
     19         assert parent_low_2["txid"] not in node.getrawmempool()
     20 
     21         # 2. Send child.
     22         peer_sender.send_and_ping(msg_tx(child_bumping["tx"]))
     23 
     24         # 3. Node should not request any parents, as it should recognize that it will not accept
     25         # multi-parent-1-child packages.
     26         node.bumpmocktime(60)
     27         peer_sender.sync_with_ping()
     28         assert "getdata" not in peer_sender.last_message
     29 
     30+        node.setmocktime(0)
     31+
     32+
     33+ [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
     34+    def test_in_mempool_parent(self):
     35+        self.log.info("Check that node rejects a 1p1c package if another parent is already in mempool")
     36+
     37+        node = self.nodes[0]
     38+        #node.setmocktime(int(time.time()))
     39+
     40+        # 2-parent-1-child package where first parent is above mempool minfeerate but second parents is below
     41+        parent_high = self.wallet_nonsegwit.create_self_transfer(fee_rate=999*FEERATE_1SAT_VB, confirmed_only=True)
     42+        parent_low = self.wallet_nonsegwit.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
     43+        child_bumping = self.wallet_nonsegwit.create_self_transfer_multi(
     44+            utxos_to_spend=[parent_high["new_utxo"], parent_low["new_utxo"]],
     45+            fee_per_output=999*parent_low["tx"].get_vsize(),
     46+        )
     47+
     48+        peer_sender = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=4, connection_type="outbound-full-relay")
     49+
     50+        # 1. Send first parent which will be accepted.
     51+        peer_sender.send_and_ping(msg_tx(parent_high["tx"]))
     52+        assert parent_high["txid"] in node.getrawmempool()
     53+
     54+        # 2. Send child.
     55+        peer_sender.send_and_ping(msg_tx(child_bumping["tx"]))
     56+
     57+        # 3. Node should request parent_low via txid, but node will reject child_bumping + parent_low
     58+        #    due to `package-not-child-with-unconfirmed-parents`
     59+        #node.bumpmocktime(60)
     60+        parent_low_txid_int = int(parent_low["txid"], 16)
     61+        peer_sender.wait_for_getdata([parent_low_txid_int])
     62+        peer_sender.send_and_ping(msg_tx(parent_low["tx"]))
     63+
     64+        node_mempool = node.getrawmempool()
     65+        assert parent_low["txid"] not in node_mempool
     66+        assert child_bumping["txid"] not in node_mempool
     67+        assert parent_high["txid"] in node_mempool
     68 
     69     def run_test(self):
     70         node = self.nodes[0]
     71         self.wallet = MiniWallet(node)
     72         self.wallet_nonsegwit = MiniWallet(node, mode=MiniWalletMode.RAW_P2PK)
     73         self.generate(self.wallet_nonsegwit, 10)
     74         self.generate(self.wallet, 20)
     75 
     76         filler_wallet = MiniWallet(node)
     77         fill_mempool(self, node, filler_wallet)
     78 
     79         self.log.info("Check opportunistic 1p1c logic when parent (txid != wtxid) is received before child")
     80         self.test_basic_parent_then_child(self.wallet)
     81 
     82         self.log.info("Check opportunistic 1p1c logic when parent (txid == wtxid) is received before child")
     83         self.test_basic_parent_then_child(self.wallet_nonsegwit)
     84 
     85         self.log.info("Check opportunistic 1p1c logic when child is received before parent")
     86         self.test_basic_child_then_parent()
     87 
     88         self.log.info("Check opportunistic 1p1c logic when 2 candidate children exist (parent txid != wtxid)")
     89         self.test_low_and_high_child(self.wallet)
     90 
     91         self.log.info("Check opportunistic 1p1c logic when 2 candidate children exist (parent txid == wtxid)")
     92         self.test_low_and_high_child(self.wallet_nonsegwit)
     93 
     94         self.test_orphan_consensus_failure()
     95         self.test_parent_consensus_failure()
     96         self.test_multiple_parents()
     97+        self.test_in_mempool_parent()
     98 
     99 
    100 if __name__ == '__main__':
    101     PackageRelayTest().main()
    

    glozow commented at 2:01 pm on April 24, 2024:
    Added a similar test
  180. instagibbs commented at 4:33 pm on April 23, 2024: member

    reviewed through 55b1280c52af81aa6ea0860799fa16da49f51447

    Looks good, have a suggestion for a test catching the Assume() issue(which does actually get hit in mainnet pretty fast with debug on)

  181. in src/net_processing.cpp:608 in bb3bf0a39c outdated
    603+     *   */
    604+    void ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, const std::vector<NodeId>& senders)
    605+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
    606+
    607+    /** Look for a child of this transaction in the orphanage to form a 1-parent-1-child package,
    608+     * skipping any combinations have already been tried, and submit them together to the mempool.
    


    sr-gi commented at 5:18 pm on April 23, 2024:

    In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

    nit: skipping any combinations that have already been tried (?)


    glozow commented at 9:24 am on April 26, 2024:
    added
  182. in src/net_processing.cpp:3289 in bb3bf0a39c outdated
    3284+    // Prefer children from this peer. This helps prevent censorship attempts in which an attacker
    3285+    // sends lots of fake children for the parent, and we (unluckily) keep selecting the fake
    3286+    // children instead of the real one provided by the honest peer.
    3287+    const auto cpfp_candidates_same_peer{m_orphanage.GetChildrenFromSamePeer(ptx, nodeid)};
    3288+
    3289+    // These children should be sorted from most newest to oldest. In the (probably uncommon) case
    


    sr-gi commented at 6:00 pm on April 23, 2024:

    In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

    nit: most newest


    glozow commented at 9:35 am on April 26, 2024:
    haha oops, deleted
  183. in src/net_processing.cpp:3280 in bb3bf0a39c outdated
    3275+    AssertLockHeld(cs_main);
    3276+
    3277+    const auto& parent_txid{ptx->GetHash()};
    3278+    const auto& parent_wtxid{ptx->GetWitnessHash()};
    3279+
    3280+    CTransactionRef tx_orphan{nullptr};
    


    sr-gi commented at 6:47 pm on April 23, 2024:

    In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

    I think you can potentially get rid of this variable by having a reference to a Package instead, which is set to the maybe_cpfp_package when the package cannot be found in m_recent_rejects_reconsiderable.

    That way you won’t need to construct the package again in the current if (tx_orphan) scope.


    glozow commented at 9:34 am on April 26, 2024:
    Ok I’ve done a bit of refactoring to split this into 2 functions for finding the package and validating/processing the package, and made a PackageToValidate struct that’s difficult to misuse. Got rid of these “set me if you find something” variables.

    sr-gi commented at 2:50 pm on April 26, 2024:
    Much cleaner now
  184. in src/net_processing.cpp:3248 in bb3bf0a39c outdated
    3216+    AssertLockHeld(g_msgproc_mutex);
    3217+    AssertLockHeld(cs_main);
    3218+
    3219+    if (package_result.m_state.IsInvalid()) {
    3220+        m_recent_rejects_reconsiderable.insert(GetPackageHash(package));
    3221+    }
    


    sr-gi commented at 6:56 pm on April 23, 2024:

    In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

    Shouldn’t this be moved after the two next checks, to avoid bloating the rolling bloom filter with data that is unconditionally invalid? The only way for a package of this type to be actually resonsiderable is if it fails due to a transaction being rejected, but not if PCKG_POLICY or PCKG_MEMPOOL_ERROR


    glozow commented at 9:30 am on April 26, 2024:
    Packages aren’t ever reconsiderable since we never submit anything beyond 1p1c. I’ve clarified this in the docs now. I definitely think we should cache a rejection when the error is PCKG_POLICY or PCKG_MEMPOOL_ERROR.

    sr-gi commented at 2:58 pm on April 26, 2024:
    Why caching this in m_recent_rejects_reconsiderable instead of m_recent_rejects if we are not going to reconsider them?

    glozow commented at 8:25 am on April 27, 2024:

    Same question as #28970 (review)

    I like that this doesn’t add extra burden to m_recent_rejects (which is probably the busier of the 2 filters even though they have the same size). It makes more sense if you think of each tx within the package as reconsiderable, even though the package isn’t?


    sr-gi commented at 1:46 pm on April 29, 2024:
    I think, design-wise, it’s a bit counter-intuitive, but I do agree that not overloading m_recent_rejects is potentially better
  185. in src/test/txpackage_tests.cpp:121 in 6edf8ec0c5 outdated
    116+    BOOST_CHECK(hash_if_by_txid != calculated_hash_123);
    117+
    118+    uint256 hash_if_use_txid = (HashWriter() << txid_2 << txid_1 << txid_3).GetSHA256();
    119+    BOOST_CHECK(hash_if_use_txid != calculated_hash_123);
    120+
    121+    uint256 hash_if_use_int_order = (HashWriter() << wtxid_2 << wtxid_1 << txid_3).GetSHA256();
    


    theStack commented at 1:33 pm on April 24, 2024:

    Was it intended to mix both wtxids and txids for hashing in this test-case? (IIUC the meaning of “int order” here, this would be wtxid2, wtxid1, wtxid3, i.e. identical with hash_if_by_txid, so I guess one of the two test-cases can be removed).

    0    uint256 hash_if_use_int_order = (HashWriter() << wtxid_2 << wtxid_1 << wtxid_3).GetSHA256();
    

    glozow commented at 4:35 pm on April 24, 2024:
    Yeah that’s a typo :+1:

    glozow commented at 9:18 am on April 26, 2024:
    fixed
  186. in src/test/txpackage_tests.cpp:98 in 6edf8ec0c5 outdated
    92+    BOOST_CHECK_EQUAL(tx_3.GetHash(), txid_3);
    93+
    94+    BOOST_CHECK(txid_2.GetHex() < txid_1.GetHex());
    95+
    96+    BOOST_CHECK(txid_1.ToUint256() != wtxid_1.ToUint256());
    97+    BOOST_CHECK(txid_3.ToUint256() != wtxid_3.ToUint256());
    


    theStack commented at 1:34 pm on April 24, 2024:

    nit: for the sake of completeness, could also check that txid and wtxid are equal for non-segwit tx 2:

    0    BOOST_CHECK(txid_1.ToUint256() != wtxid_1.ToUint256());
    1    BOOST_CHECK_EQUAL(txid_2.ToUint256(), wtxid_2.ToUint256());
    2    BOOST_CHECK(txid_3.ToUint256() != wtxid_3.ToUint256());
    

    glozow commented at 9:18 am on April 26, 2024:
    added
  187. glozow force-pushed on Apr 24, 2024
  188. instagibbs approved
  189. instagibbs commented at 2:13 pm on April 24, 2024: member

    ACK https://github.com/bitcoin/bitcoin/pull/28970/commits/30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

    Changes are test only, and new subtest causes a debug-build crash when the PCKG_POLICY change is reverted.

  190. DrahtBot requested review from theStack on Apr 24, 2024
  191. DrahtBot requested review from murchandamus on Apr 24, 2024
  192. DrahtBot requested review from sdaftuar on Apr 24, 2024
  193. in src/net_processing.cpp:3267 in bb3bf0a39c outdated
    3262+                }
    3263+            }
    3264+        }
    3265+        package_iter++;
    3266+        // There should be a sender for each tx, but fall back to the last one otherwise.
    3267+        if (Assume(senders_iter != senders.rend())) senders_iter++;
    


    theStack commented at 3:42 pm on April 24, 2024:
    nit: as alternative, could check at the beginning of the function if the sizes of package and senders match and return early if they don’t (not sure how this function is used in the future if the 1p1c limit is lifted, but i guess a size mismatch should never happen, unless there is a bug at the call-site?). that would be slightly simpler to reason imho.

    glozow commented at 4:35 pm on April 24, 2024:

    I think we’d want to make sure that even if we had a bad senders vector we would still call ProcessValidTx / ProcessInvalidTx (seems kind of bad not to). Maybe a better approach is to use -1 when we don’t know what it is - I do realize we’re not handling the empty vector case.

    (Yeah maybe this is moot as the 1 caller should always make senders properly…)


    glozow commented at 9:31 am on April 26, 2024:
    Ok I have deleted this now and created a PackageToValidate struct where the only constructor available guarantees there is a sender for each tx. So I’ve removed this Assume stuff as it’s probably overly paranoid.
  194. in src/net_processing.cpp:3327 in bb3bf0a39c outdated
    3322+            Package maybe_cpfp_package{ptx, cpfp_candidates_different_peer.at(index).first};
    3323+            if (!m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {
    3324+                tx_orphan = cpfp_candidates_different_peer.at(index).first;
    3325+                orphan_sender = cpfp_candidates_different_peer.at(index).second;
    3326+                break;
    3327+            }
    


    theStack commented at 3:45 pm on April 24, 2024:

    refactoring nit to avoid multiple accesses in cpfp_candidates_different_peer (feel free to ignore):

    0            const auto [candidate_child, candidate_peer] = cpfp_candidates_different_peer.at(index);
    1            const Package maybe_cpfp_package{ptx, candidate_child};
    2             if (!m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {
    3                tx_orphan = candidate_child;
    4                orphan_sender = candidate_peer;
    5                break;
    6            }
    

    glozow commented at 10:09 am on April 26, 2024:
    done
  195. in test/functional/p2p_1p1c_network.py:129 in 30c9e6bc4e outdated
    124+        packages_to_submit.append(package_hex_4)
    125+
    126+        # node0: sender
    127+        # node1: pre-received the children (orphan)
    128+        # node3: pre-received the parents (too low fee)
    129+        # All nodes receive high_parent_51 ahead of time.
    


    theStack commented at 4:17 pm on April 24, 2024:
    0        # All nodes receive parent_31 ahead of time.
    

    glozow commented at 8:48 am on April 26, 2024:
    done
  196. theStack commented at 4:19 pm on April 24, 2024: contributor
    Another round through (modulo the fuzz commit), the code looks correct to me, just left a few nits below. Planning to do another review tomorrow with focus on the main commit bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6, especially going through the AlreadyHaveTx call-sites and their include_reconsiderable values to check once more.
  197. DrahtBot requested review from theStack on Apr 24, 2024
  198. in test/functional/p2p_1p1c_network.py:67 in 30c9e6bc4e outdated
    62+        package_hex_basic = [low_fee_parent["hex"], high_fee_child["hex"]]
    63+        return package_hex_basic, low_fee_parent["tx"], high_fee_child["tx"]
    64+
    65+    def create_package_2outs(self, wallet):
    66+        # First create a tester tx to see the vsize, and then adjust the fees
    67+        placeholder_fee = 100000
    


    sr-gi commented at 7:25 pm on April 24, 2024:

    In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

    Is the placeholder fee needed? create_self_transfer_multi already has a default fee, and the value shouldn’t matter, should it?


    glozow commented at 8:45 am on April 26, 2024:
    removed
  199. in test/functional/p2p_1p1c_network.py:74 in 30c9e6bc4e outdated
    69+
    70+        low_fee_parent_2outs_tester = wallet.create_self_transfer_multi(
    71+            utxos_to_spend=[utxo_for_2outs],
    72+            num_outputs=2,
    73+            fee_per_output=placeholder_fee,
    74+            confirmed_only=True
    


    sr-gi commented at 7:26 pm on April 24, 2024:

    In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

    confirmed_only is only used if no utxos are provided, but this is not the case, so it shouldn’t be needed.


    glozow commented at 8:45 am on April 26, 2024:
    removed
  200. in test/functional/p2p_1p1c_network.py:119 in 30c9e6bc4e outdated
    114+        # 4: parent + child package where the child spends 2 different outputs from the parent.
    115+        package_hex_4, parent_4, child_4 = self.create_package_2outs(self.wallet)
    116+
    117+        # Assemble return results
    118+        packages_to_submit = []
    119+        txns_to_send = [[]] * self.num_nodes
    


    sr-gi commented at 7:40 pm on April 24, 2024:

    In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

    nit: There’s no need to reserve here, this could be defined as txns_to_send = [[]] and subsequent calls could call append instead


    glozow commented at 8:48 am on April 26, 2024:
    I’ve changed them to just construct everything in 1 line, should be clearer