txorphanage: add size accounting, use wtxids, support multiple announcers #28481

pull glozow wants to merge 9 commits into bitcoin:master from glozow:2023-08-orphanage-additions changing 5 files +658 −63
  1. glozow commented at 1:59 pm on September 14, 2023: member

    This PR contains changes to TxOrphanage needed for package relay stuff. See #27463 for project tracking and #28031 for how this code is used.

    Separating this PR was suggested in #28031 (review).

    Changes here:

    • Change params to wtxid instead of txid.
    • Track total size (CTransaction::GetTotalSize() of orphans stored, as well as the size per peer.
    • Support having multiple announcers for the same tx. This helps us retry resolving the same orphan with multiple peers if it fails the first time.
    • Store the deduplicated missing parent txids in OrphanTx so that we don’t need to calculate them again later on.
    • Return the erased wtxids from EraseForBlock which includes orphans that conflicted with the block. This helps us delete those orphan resolution attempts from our tracker.
    • Enable limiting the total size of orphans instead of just the total count. No new limits are introduced, the default is maximum count * maximum tx size.
    • Unit tests + fuzzer.
  2. DrahtBot commented at 1:59 pm on September 14, 2023: contributor

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

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28686 (refactor: Split per-peer parts of net module into new node/connection module by ajtowns)
    • #28658 (type-safe(r) GenTxid constructors by instagibbs)
    • #28107 (util: Type-safe transaction identifiers by dergoegge)

    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.

  3. glozow force-pushed on Sep 14, 2023
  4. DrahtBot added the label CI failed on Sep 14, 2023
  5. glozow force-pushed on Sep 14, 2023
  6. dergoegge commented at 3:29 pm on September 14, 2023: member 
    0$ echo "Af//////Oqf/AP8TAf8AEPr/AQAApZL6/wEA/1sB/wBbAAClkpJ4eP//S0tL/Q==" | base64 -d > orphanage_crash
1$ FUZZ=txorphan ./src/test/fuzz/fuzz orphanage_crash
2...
3test/fuzz/txorphan.cpp:98 operator(): Assertion `orphanage.BytesFromPeer(peer_id) >= ref->GetTotalSize()' failed.
4...
  7. maflcko commented at 6:58 am on September 15, 2023: member 
    028: error: argument name 'max_count' in comment does not match parameter name 'max_orphans' [bugprone-argument-comment,-warnings-as-errors]
1    orphanage.LimitOrphans(/*max_count=*/10, DEFAULT_MAX_ORPHAN_TOTAL_SIZE);
2                           ^
3./txorphanage.h:58:52: note: 'max_orphans' declared here
4    std::vector<uint256> LimitOrphans(unsigned int max_orphans, unsigned int max_total_size = DEFAULT_MAX_ORPHAN_TOTAL_SIZE)
5                                                   ^
  8. in src/net_processing.cpp:2986 in 6101c601f5 outdated 
    2981@@ -2982,7 +2982,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
2982                     m_recent_rejects.insert(porphanTx->GetHash());
2983                 }
2984             }
2985-            m_orphanage.EraseTx(orphanHash);
2986+            m_orphanage.EraseTx(orphan_wtxid);
    Yugthakkar04 commented at 11:29 am on September 15, 2023:
    use a r-lambda function here to ease things up.
    glozow commented at 4:35 pm on September 22, 2023:
    Uh, unsure what a r-lambda function is and pretty sure calling something else doesn’t make sense so I’m resolving this comment.
  9. in src/txorphanage.h:37 in 4d6b839616 outdated 
    35@@ -33,8 +36,8 @@ class TxOrphanage {
36      */
37     CTransactionRef GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
38 
39-    /** Erase an orphan by txid */
40-    int EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
    instagibbs commented at 4:50 pm on September 15, 2023:
    Need to also rename arg for EraseTxNoLock to wtxid
    instagibbs commented at 4:52 pm on September 15, 2023:

    4d6b839616c65b0ca5fefdb50ce0b8b1ffad8947

    txorphan.cpp fuzzer needs to change for all EraseTx() call sites because they’re still using txid

  10. in src/txorphanage.cpp:170 in ee2063496b outdated 
    138@@ -118,6 +139,8 @@ void TxOrphanage::EraseForPeer(NodeId peer)
139         }
140     }
141     if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer);
142+    if (!Assume(m_peer_bytes_used.count(peer) == 0)) m_peer_bytes_used.erase(peer);
    instagibbs commented at 5:08 pm on September 15, 2023:
    leave a comment that this is belt-and-suspenders only, the condition should fail since the item should have been already erased
  11. in src/txorphanage.h:30 in 05ea6fabe3 outdated 
    21@@ -22,7 +22,7 @@ class TxOrphanage {
22 public:
23     /** Add a new orphan transaction. If the tx already exists, add this peer to its list of announcers.
24       @returns true if the transaction was added as a new orphan. */
25-    bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
26+    bool AddTx(const CTransactionRef& tx, NodeId peer, const std::vector<uint256>& parent_txids) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
    instagibbs commented at 5:28 pm on September 15, 2023:
    Should describe the new argument as something like “all unique parent transaction txids”. I kind of wish this was generated inside AddTx but realize you’re avoiding double the work…
  12. in src/txorphanage.h:98 in 05ea6fabe3 outdated 
    91@@ -92,6 +92,9 @@ class TxOrphanage {
92      * remove this peer's entry from the map. */
93     void SubtractOrphanBytes(unsigned int size, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
94 
95+    /** Get an orphan's parent_txids, or std::nullopt if the orphan is not present. */
    instagibbs commented at 5:32 pm on September 15, 2023: 
    0    /** Get an orphan's unique parent_txids, or std::nullopt if the orphan is not present. */
  13. in src/txorphanage.h:61 in 05ea6fabe3 outdated 
    51     /** Erase all orphans included in or invalidated by a new block */
52     void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
53 
54-    /** Limit the orphanage to the given maximum */
55-    void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
56+    /** Limit the orphanage to the given maximum. Returns all expired entries. */
    instagibbs commented at 5:33 pm on September 15, 2023: 
    0    /** Limit the orphanage to the given maximum. Returns all expired entries by wtxid. */
  14. in src/txorphanage.cpp:178 in 05ea6fabe3 outdated 
    170@@ -169,10 +171,11 @@ void TxOrphanage::EraseForPeer(NodeId peer)
171     return wtxids;
172 }
173 
174-void TxOrphanage::LimitOrphans(unsigned int max_orphans)
175+std::vector<uint256> TxOrphanage::LimitOrphans(unsigned int max_orphans)
176 {
177     LOCK(m_mutex);
178 
179+    std::vector<uint256> expired;
    instagibbs commented at 5:33 pm on September 15, 2023:
    s/expired/expired_wtxids/
  15. in src/txorphanage.cpp:177 in 05ea6fabe3 outdated 
    175+std::vector<uint256> TxOrphanage::LimitOrphans(unsigned int max_orphans)
176 {
177     LOCK(m_mutex);
178 
179+    std::vector<uint256> expired;
180     unsigned int nEvicted = 0;
    instagibbs commented at 5:34 pm on September 15, 2023:
    can remove this and use size of expired(_wtxids) set isntead
  16. in src/txorphanage.h:52 in 6101c601f5 outdated 
    51 
52-    /** Erase all orphans announced by a peer (eg, after that peer disconnects) */
53-    void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
54+    /** Maybe erase all orphans announced by a peer (eg, after that peer disconnects). If an orphan
55+     * has been announced by another peer, don't erase, just remove this peer from the list of announcers. */
56+    std::vector<uint256> EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
    instagibbs commented at 5:39 pm on September 15, 2023:
    please remove this return #28031 (review)
  17. in src/txorphanage.cpp:286 in 1630da019b outdated 
    277@@ -278,7 +278,7 @@ bool TxOrphanage::HaveTxToReconsider(NodeId peer)
278     return false;
279 }
280 
281-void TxOrphanage::EraseForBlock(const CBlock& block)
282+std::vector<uint256> TxOrphanage::EraseForBlock(const CBlock& block)
    instagibbs commented at 5:45 pm on September 15, 2023:
    unused in this PR, but is exercised in tests so I’m happy for it to be used later :+1:
  18. in src/test/orphanage_tests.cpp:82 in 95fcb16ac4 outdated 
    79+    } else {
80+        for (const auto& outpoint : outpoints) {
81+            tx.vin.emplace_back(CTxIn(outpoint));
82+        }
83+    }
84+    // Ensure txid != wtxid
    instagibbs commented at 5:51 pm on September 15, 2023:
    could this get mechanically asserted as extra-belt-and-suspender
  19. in src/test/fuzz/txorphan.cpp:157 in 6101c601f5 outdated 
    152+                },
153+                [&] {
154+                    // check EraseForBlock
155+                    CBlock block;
156+                    block.vtx.emplace_back(tx);
157+                    orphanage.EraseForBlock(block);
    instagibbs commented at 5:52 pm on September 15, 2023:
    might as well capture the return and check it here too
  20. instagibbs commented at 5:55 pm on September 15, 2023: member
    initial comments, reviewed everything, also replicated the same fuzzer crash locally
  21. glozow commented at 3:54 pm on September 18, 2023: member
    Putting this in draft to focus review attention on the mempool stuff.
  22. glozow marked this as a draft on Sep 18, 2023
  23. Yugthakkar04 approved
  24. Yugthakkar04 commented at 5:55 pm on September 18, 2023: none
    add this to the first line of array or a bolleyan function and then to the first line of paragraph..
  25. Yugthakkar04 changes_requested
  26. Yugthakkar04 commented at 5:56 pm on September 18, 2023: none
    make this a transidental function and then work upon it and mechanically assert it up than.
  27. glozow commented at 4:35 pm on September 22, 2023: member
    Going to resolve comments + fuzz failure, and then add a getorphanagestats RPC before taking out of draft.
  28. glozow force-pushed on Sep 27, 2023
  29. glozow force-pushed on Sep 27, 2023
  30. glozow force-pushed on Sep 27, 2023
  31. [txorphange] erase and get orphans by wtxid instead of txid 
    Wtxids are better because they disambiguate between transactions with
the same txid but different witnesses. The package relay logic will work
with wtxids, e.g. upon receipt of an ancpkginfo containing wtxids of
transactions, it will look for those transactions in the orphanage.
    46739c4e6f
  32. [txorphanage] track size of stored orphans, total and by peer 
    No effect for now, just additional tracking. Enables:
- load balance orphan resolution, limit per-peer orphanage usage
- limit the total size of orphans instead of just the count
    976b2c9036
  33. [txorphanage] support multiple announcers 
    Add ability to add and track multiple announcers per orphan transaction,
erasing announcers but not the entire orphan.
    add6a6bda1
  34. [txorphanage] store parent txids in OrphanTx 4ad580ebdf
  35. [txorphanage] return erased wtxids from EraseForBlock 
    This information is already known; returning it is essentially free. A
later commit uses these wtxids to remove conflicted transactions from
the orphan resolution tracker.
    7c42498da9
  36. [txorphanage] include maximum total size in LimitOrphans 3278a5d19f
  37. [txorphanage] GetAllWtxids 59b87dce5e
  38. [unit test] orphanage: multiple announcers, eviction 5def0e9f99
  39. [fuzz] test new orphanage methods 56eef678a6
  40. glozow force-pushed on Sep 28, 2023
  41. DrahtBot removed the label CI failed on Sep 28, 2023
  42. DrahtBot added the label Needs rebase on Oct 26, 2023
  43. DrahtBot commented at 6:50 pm on October 26, 2023: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

  44. DrahtBot commented at 1:33 am on January 24, 2024: contributor

    ⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
  45. DrahtBot commented at 0:37 am on April 22, 2024: contributor

    ⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
  46. glozow commented at 3:01 pm on April 30, 2024: member
    Closing for now. The first commit is superseded by #30000, and the rest will come after TxDownloadManager refactoring
  47. glozow closed this on Apr 30, 2024


glozow DrahtBot dergoegge maflcko Yugthakkar04 instagibbs

Labels
Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-29 07:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me