Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling

glozow commented at 3:30 pm on July 5, 2023: member

See #27463 for full project tracking. Please see #27742 for how this PR fits into the big picture. This PR is based on a more recent commit than that one.

This branch includes: (1) (now merged, see #30110) Introduces TxDownloadManager, which handles all transaction downloading. Adds tests for TxDownloadManager. (2) Adds an “orphan resolution module”. It adds all announcers of an orphan as potential resolution candidates, in a tracker implemented as a TxRequestTracker. In this PR, “orphan resolution” means requesting missing parents by getdata(MSG_TX | MSG_WITNESS_FLAG, missing_txid). In a future PR, we’ll add another resolution method, requesting ancestor wtxids using getdata(MSG_ANCPKGINFO, orphan_wtxid). (3) Makes TxDownloadManager internally thread-safe

glozow added the label P2P on Jul 5, 2023

DrahtBot commented at 3:30 pm on July 5, 2023: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28031.

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:

#31397 (p2p: track and use all potential peers for orphan resolution by glozow)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)

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.

glozow force-pushed on Jul 5, 2023

DrahtBot added the label CI failed on Jul 5, 2023

in src/net_processing.cpp:2966 in 543273d96e outdated

2959@@ -2960,9 +2960,10 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
2960         const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx);
2961         const TxValidationState& state = result.m_state;
2962         const uint256& orphanHash = porphanTx->GetHash();
2963+        const uint256 orphan_wtxid = porphanTx->GetWitnessHash();
2964 
2965         if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
2966-            LogPrint(BCLog::MEMPOOL, "   accepted orphan tx %s\n", orphanHash.ToString());
2967+            LogPrint(BCLog::MEMPOOL, "   accepted orphan tx %s\n", orphan_wtxid.ToString());

instagibbs commented at 5:50 pm on July 5, 2023:

make these be TXPACKAGES? then you get the entire “story” with a single log type (which helped me diagnose the Timeout issue)

glozow commented at 3:04 pm on July 24, 2023:

Done :+1: This now has 2 logs:

TXPACKAGES “accepted orphan tx (wtxid)”
MEMPOOL “AcceptToMemoryPool … (txid)” which matches the one in ProcessMessage for a tx message

achow101 commented at 6:22 pm on July 31, 2023:

In 543273d96e896adf5531ed961856aa0eb70cbe57 “[log] log ProcessOrphanTx() events by wtxid”

Perhaps log both txid and wtxid?

glozow commented at 9:13 pm on August 14, 2023:

Logging both :+1:

in src/node/txpackagetracker.cpp:30 in 116378efc1 outdated

25+
26+    /** Tracks orphans for which we need to request ancestor information. All hashes stored are
27+     * wtxids, i.e., the wtxid of the orphan. However, the Announcement::m_is_wtxid field is used to
28+     * indicate whether we would request the ancestor information by wtxid (via package relay) or by
29+     * txid (via prevouts of the missing inputs). */
30+    TxRequestTracker orphan_request_tracker GUARDED_BY(m_mutex);

instagibbs commented at 6:02 pm on July 5, 2023:

m_orphan_request_tracker :pray:

glozow commented at 3:37 pm on July 24, 2023:

Done

in src/node/txpackagetracker.cpp:75 in 116378efc1 outdated

70+    size_t OrphanageSize() { return m_orphanage.Size(); }
71+    void MempoolAcceptedTx(const CTransactionRef& ptx) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
72+    {
73+        LOCK(m_mutex);
74+        m_orphanage.AddChildrenToWorkSet(*ptx);
75+        m_orphanage.EraseTx(ptx->GetWitnessHash());

instagibbs commented at 6:05 pm on July 5, 2023:

seems wrong/very noisy without this? e.g., this line https://github.com/bitcoin/bitcoin/pull/28031/files#diff-ece439372a3e31da3141ed8fda99b37381e32cdab17ca26fffd5dfd916c300c8R124 will fire constantly

0        m_orphanage.EraseTx(ptx->GetWitnessHash());
1        orphan_request_tracker.ForgetTxHash(ptx->GetWitnessHash());

glozow commented at 9:09 pm on August 14, 2023:

Thanks, fixed

in src/node/txpackagetracker.cpp:124 in 116378efc1 outdated

119+        AssertLockNotHeld(m_mutex);
120+        LOCK(m_mutex);
121+        std::vector<std::pair<NodeId, GenTxid>> expired;
122+        auto tracker_requestable = orphan_request_tracker.GetRequestable(nodeid, current_time, &expired);
123+        for (const auto& entry : expired) {
124+            LogPrint(BCLog::TXPACKAGES, "\nTimeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "ancpkginfo" : "orphan parent",

instagibbs commented at 6:08 pm on July 5, 2023:

any principle on prefixing and postfixing \n to everything in these logs?

mzumsande commented at 10:20 pm on July 12, 2023:

I don’t think there should ever be a \n prefix, why would we want to separate the meta information (timestamp, threadname etc.) from the actual log entry?

glozow commented at 3:31 pm on July 24, 2023:

Removed the prefix

in src/node/txpackagetracker.cpp:136 in 116378efc1 outdated

131+            if (!ptx) {
132+                // We can't request ancpkginfo and we have no way of knowing what the missing
133+                // parents are (it could also be that the orphan has already been resolved).
134+                // Give up.
135+                orphan_request_tracker.ForgetTxHash(gtxid.GetHash());
136+                LogPrint(BCLog::TXPACKAGES, "\nForgetting orphan %s from peer=%d\n", gtxid.GetHash().ToString(), nodeid);

instagibbs commented at 6:10 pm on July 5, 2023:

0                LogPrint(BCLog::TXPACKAGES, "\nForgetting orphan request %s from peer=%d\n", gtxid.GetHash().ToString(), nodeid);

in src/node/txpackagetracker.cpp:80 in 116378efc1 outdated

75+        m_orphanage.EraseTx(ptx->GetWitnessHash());
76+    }
77+    void MempoolRejectedTx(const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
78+    {
79+        LOCK(m_mutex);
80+        m_orphanage.EraseTx(wtxid);

instagibbs commented at 6:26 pm on July 5, 2023:

trying to fetch after failure seems wrong

0        m_orphanage.EraseTx(wtxid);
1        orphan_request_tracker.ForgetTxHash(wtxid);

glozow commented at 9:09 pm on August 14, 2023:

Fixed

in src/txorphanage.cpp:21 in 29d9d326d5 outdated

13@@ -15,6 +14,16 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60;
14 /** Minimum time between orphan transactions expire time checks in seconds */
15 static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60;
16 
17+void TxOrphanage::SubtractOrphanBytes(unsigned int size, NodeId peer)
18+{
19+    AssertLockHeld(m_mutex);
20+    Assume(m_peer_bytes_used.count(peer) > 0);
21+    Assume(m_peer_bytes_used.at(peer) >= size);

instagibbs commented at 8:37 pm on July 5, 2023:

since we’re using size_t for these fields, do we want to continue on with UB for release builds or do an assert?

glozow commented at 9:10 pm on August 14, 2023:

Changed these to if(!Assume(...)) return;

in src/txorphanage.cpp:31 in 6cd8be8a4c outdated

27@@ -28,6 +28,10 @@ void TxOrphanage::SubtractOrphanBytes(unsigned int size, NodeId peer)
28 bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
29 {
30     LOCK(m_mutex);
31+    if (tx == nullptr) {

instagibbs commented at 8:41 pm on July 5, 2023:

[txorphanage] handle AddTx(nullptr)

Could we motivate this change in the commit message?

glozow commented at 4:26 pm on July 24, 2023:

Dropped the commit instead

in src/net_processing.cpp:5926 in b5ab45e595 outdated

5922@@ -5926,6 +5923,22 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
5923             }
5924         }
5925 
5926+        auto requestable_orphans = m_txpackagetracker->GetOrphanRequests(pto->GetId(), current_time);

instagibbs commented at 8:51 pm on July 5, 2023:

requestable_orphans is more like requestable_parents?

glozow commented at 3:21 pm on July 24, 2023:

(this is deleted now)

in src/net_processing.cpp:5928 in b5ab45e595 outdated

5922@@ -5926,6 +5923,22 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
5923             }
5924         }
5925 
5926+        auto requestable_orphans = m_txpackagetracker->GetOrphanRequests(pto->GetId(), current_time);
5927+        for (const auto& gtxid : requestable_orphans) {
5928+            if (AlreadyHaveTx(gtxid, /*include_orphanage=*/false)) {

instagibbs commented at 9:00 pm on July 5, 2023:

Suggested comment, something like:

“We do not presume the parent will still be in the orphanage by the time a response is received, so we exclude the orphanage from the check when deciding what to request.”

If that’s wrong, then it needs better explanation than what exists in the commit message :)

glozow commented at 3:29 pm on July 24, 2023:

Good point. The reason for excluding orphanage is actually not applicable yet so I have dropped it for now.

This is really only applicable when we are requesting the ancpkginfo for a tx. We want to exclude orphanage because otherwise, AlreadyHaveTx will return true and we will never request ancpkginfos.

in src/node/txpackagetracker.cpp:54 in b5ab45e595 outdated

52+        for (const CTransactionRef& ptx : block.vtx) {
53+            block_wtxids.insert(ptx->GetWitnessHash());
54+        }
55+        for (const auto& wtxid : wtxids_erased) {
56+            if (block_wtxids.count(wtxid) == 0) {
57+                conflicted_wtxids.insert(wtxid);

instagibbs commented at 1:48 pm on July 6, 2023:

why not just ForgetTxHash here? Why gate it on not being in the block?

I’m presuming following commits will reveal the answer…

glozow commented at 9:21 pm on August 14, 2023:

Removed the special casing. I can’t remember why it comes into play later, but if I do, I’ll add it when it’s needed.

in src/node/txpackagetracker.h:74 in b5ab45e595 outdated

69+     */
70+    void AddOrphanTx(NodeId nodeid, const uint256& wtxid, const CTransactionRef& tx, bool is_preferred, std::chrono::microseconds reqtime);
71+
72+    /** Number of packages we are working on with this peer. Includes any entries in the orphan
73+     * tracker and in-flight requests. */
74+    size_t Count(NodeId nodeid) const;

instagibbs commented at 2:03 pm on July 6, 2023:

in other words, it includes anything they’ve told us about, even if we haven’t acted on it in any way?

glozow commented at 9:12 pm on August 14, 2023:

Replaced the exposure of Count to CheckIsEmpty() functions

in src/txorphanage.h:153 in 116378efc1 outdated

123@@ -98,6 +124,9 @@ class TxOrphanage {
124      *  transactions using their witness ids. */
125     std::map<uint256, OrphanMap::iterator> m_wtxid_to_orphan_it GUARDED_BY(m_mutex);
126 
127+    /** Map from nodeid to the amount of orphans provided by this peer, in bytes. */

instagibbs commented at 3:15 pm on July 6, 2023:

Might be good to note that this will “multi-count” a single known orphan, counting the bytes for each node. Was wondering early in the PR why the update for this field and m_total_orphan_bytes wasn’t atomic

glozow commented at 4:17 pm on July 24, 2023:

Added in comment

DrahtBot commented at 3:25 pm on July 6, 2023: contributor

Looks like the CI fails:

0�[0m�[0;31mp2p_orphan_handling.py                                 | ✖ Failed  | 2407 s

glozow commented at 3:32 pm on July 6, 2023: member

Investigating, thanks @DrahtBot

in src/txorphanage.cpp:198 in d92b017f68 outdated

194@@ -181,9 +195,15 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx)
195         const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(tx.GetHash(), i));
196         if (it_by_prev != m_outpoint_to_orphan_it.end()) {
197             for (const auto& elem : it_by_prev->second) {
198+                Assume(elem->second.announcers.size() >= 1);

instagibbs commented at 3:37 pm on July 6, 2023:

you allow this to be empty one line below with a break; just belt and suspenders?

glozow commented at 4:17 pm on July 24, 2023:

Yes, belt and suspenders - commented

in src/txorphanage.cpp:201 in d92b017f68 outdated

194@@ -181,9 +195,15 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx)
195         const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(tx.GetHash(), i));
196         if (it_by_prev != m_outpoint_to_orphan_it.end()) {
197             for (const auto& elem : it_by_prev->second) {
198+                Assume(elem->second.announcers.size() >= 1);
199+                if (elem->second.announcers.empty()) break;
200+                // Pick a random peer from announcers set.
201+                FastRandomContext rng;

instagibbs commented at 3:38 pm on July 6, 2023:

initialize outside of loop?

glozow commented at 4:17 pm on July 24, 2023:

Done thanks

in src/test/orphanage_tests.cpp:366 in b43033cfb8 outdated

212+        BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count);
213+        BOOST_CHECK_EQUAL(orphanage.TotalOrphanBytes(), expected_total_size);
214+        BOOST_CHECK_EQUAL(orphanage.BytesFromPeer(node0), expected_node0_size);
215+        BOOST_CHECK_EQUAL(orphanage.BytesFromPeer(node1), expected_node1_size);
216+        // if EraseForPeer is called for an orphan with multiple announcers, the orphanage should only
217+        // decrement the number of bytes for that peer.

instagibbs commented at 3:50 pm on July 6, 2023:

also check that EraseTxNoLock wasn’t called, i.e. m_orphans still has the orphan tx

glozow commented at 4:21 pm on July 24, 2023:

Added

in src/test/orphanage_tests.cpp:383 in b43033cfb8 outdated

227+        expected_total_size -= tx_size;
228+        expected_node1_size -= tx_size;
229+        BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count);
230+        BOOST_CHECK_EQUAL(orphanage.TotalOrphanBytes(), expected_total_size);
231+        BOOST_CHECK_EQUAL(orphanage.BytesFromPeer(node0), expected_node0_size);
232+        BOOST_CHECK_EQUAL(orphanage.BytesFromPeer(node1), expected_node1_size);

instagibbs commented at 3:50 pm on July 6, 2023:

check that the orphan tx is now missing from m_orphans?

glozow commented at 4:21 pm on July 24, 2023:

Added

in test/functional/p2p_segwit.py:2057 in 0d945feaf6 outdated

2057@@ -2058,7 +2058,7 @@ def received_wtxidrelay():
2058         test_transaction_acceptance(self.nodes[0], self.wtx_node, tx2, with_witness=True, accepted=False)
2059 
2060         # Expect a request for parent (tx) by txid despite use of WTX peer
2061-        self.wtx_node.wait_for_getdata([tx.sha256], 60)

instagibbs commented at 4:36 pm on July 6, 2023:

can we programatically justify this magic :)

instagibbs commented at 4:39 pm on July 6, 2023: member

some initial comments through https://github.com/bitcoin/bitcoin/pull/28031/commits/116378efc1c9c1fe0d26cb42e2bdbb5770815c35

Log changes suggested are helpful for tracing what’s happening in the orphanage on my node I’m testing.

in src/txorphanage.cpp:34 in 116378efc1 outdated

29 {
30     LOCK(m_mutex);
31+    if (tx == nullptr) {
32+        Assume(false);
33+        return false;
34+    }

dergoegge commented at 4:04 pm on July 12, 2023:

Would be cleaner to write:

0    if (!Assume(tx)) {
1        return false;
2    }

glozow commented at 9:12 pm on August 14, 2023:

Done

in src/net_processing.cpp:1448 in 116378efc1 outdated

1440@@ -1434,6 +1441,49 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
1441     }
1442 }
1443 
1444+void PeerManagerImpl::AddOrphanAnnouncer(NodeId nodeid, const uint256& orphan_wtxid, const CTransactionRef& tx, std::chrono::microseconds current_time)
1445+{
1446+    AssertLockHeld(::cs_main); // For m_txrequest
1447+    const bool connected = m_connman.ForNode(nodeid, [](CNode* node) { return node->fSuccessfullyConnected && !node->fDisconnect; });
1448+    if (!connected) return;

dergoegge commented at 4:05 pm on July 12, 2023:

This can be dropped in favor of just checking that the CNodeState exists

glozow commented at 9:21 pm on August 14, 2023:

This is gone now, replaced with registration of ConnectionInfo when the node first connects.

in src/net_processing.cpp:1449 in 116378efc1 outdated

1440@@ -1434,6 +1441,49 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
1441     }
1442 }
1443 
1444+void PeerManagerImpl::AddOrphanAnnouncer(NodeId nodeid, const uint256& orphan_wtxid, const CTransactionRef& tx, std::chrono::microseconds current_time)
1445+{
1446+    AssertLockHeld(::cs_main); // For m_txrequest
1447+    const bool connected = m_connman.ForNode(nodeid, [](CNode* node) { return node->fSuccessfullyConnected && !node->fDisconnect; });
1448+    if (!connected) return;
1449+    if (m_txpackagetracker->Count(nodeid) + m_txrequest.Count(nodeid) >= MAX_PEER_TX_ANNOUNCEMENTS) {

dergoegge commented at 4:09 pm on July 12, 2023:

(I’ve mentioned this offline as well)

The locking assumptions around this check are weird because m_txpackagetracker has its own internal mutex where as m_txrequest is guarded by cs_main. There is nothing stopping m_txpackagetracker->Count() form returning something different right after this check.

glozow commented at 9:50 am on July 13, 2023:

Yeah good point.

It seems that, given the need to synchronize between TxRequestTracker and the package tracking stuff, we should have them both guarded by 1 lock. Looking at #26151#pullrequestreview-1116661944 it seems like we could have a m_txrequest GUARDED_BY(tx_download_mutex), m_txpackagetracker GUARDED_BY(tx_download_mutex), and lock it from these peerman functions?

Alternatively, I wonder if it makes sense to take it a step further and put this all in a TxDownloadManager module that wraps orphanage, txrequest, and package tracking.

in src/net_processing.cpp:1444 in 116378efc1 outdated

1440@@ -1434,6 +1441,49 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
1441     }
1442 }
1443 
1444+void PeerManagerImpl::AddOrphanAnnouncer(NodeId nodeid, const uint256& orphan_wtxid, const CTransactionRef& tx, std::chrono::microseconds current_time)

dergoegge commented at 4:10 pm on July 12, 2023:

0void PeerManagerImpl::AddOrphanAnnouncer(NodeId nodeid, const CTransactionRef& tx, std::chrono::microseconds current_time)

in src/txorphanage.cpp:26 in 116378efc1 outdated

13@@ -15,14 +14,35 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60;
14 /** Minimum time between orphan transactions expire time checks in seconds */
15 static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60;
16 
17+void TxOrphanage::SubtractOrphanBytes(unsigned int size, NodeId peer)

dergoegge commented at 4:11 pm on July 12, 2023:

Imo, it would be nicer to do the orphanage changes separately, including amended functional/unit/fuzz tests.

glozow commented at 3:12 pm on September 14, 2023:

Imo, it would be nicer to do the orphanage changes separately, including amended functional/unit/fuzz tests.

Opened #28481 for orphanage changes

in src/txorphanage.cpp:21 in 116378efc1 outdated

13@@ -15,14 +14,35 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60;
14 /** Minimum time between orphan transactions expire time checks in seconds */
15 static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60;
16 
17+void TxOrphanage::SubtractOrphanBytes(unsigned int size, NodeId peer)
18+{
19+    AssertLockHeld(m_mutex);
20+    Assume(m_peer_bytes_used.count(peer) > 0);
21+    Assume(m_peer_bytes_used.at(peer) >= size);

dergoegge commented at 4:13 pm on July 12, 2023:

Not a big fan of using Assume like this because we can’t test these conditions but they can happen in production (if the caller is doing something wrong).

I’d suggest just returning if these assumptions don’t hold.

glozow commented at 4:25 pm on July 24, 2023:

Agree - returning when the assumption doesn’t hold

in src/node/txpackagetracker.h:19 in 116378efc1 outdated

14+class CBlock;
15+class TxOrphanage;
16+namespace node {
17+static constexpr bool DEFAULT_ENABLE_PACKAGE_RELAY{false};
18+
19+class TxPackageTracker {

dergoegge commented at 4:16 pm on July 12, 2023:

Would be nice to add unit and fuzz tests for this

in src/node/txpackagetracker.h:56 in 116378efc1 outdated

51+    size_t OrphanageSize();
52+
53+    /** Should be called when a transaction is accepted to the mempool. If it was an orphan we were
54+     * trying to resolve, remove its entries from the orphanage and other data structures. If it is
55+     * the ancestor of an orphan, add the orphan to its associated peer's workset. */
56+    void MempoolAcceptedTx(const CTransactionRef& ptx);

dergoegge commented at 4:16 pm on July 12, 2023:

0    void MempoolAcceptedTx(const CTransactionRef& tx);

glozow commented at 4:19 pm on July 24, 2023:

I actually think ptx is better, since it’s a pointer?

in src/node/txpackagetracker.cpp:149 in 116378efc1 outdated

144+            for (const auto& txin : ptx->vin) {
145+                // We start with all parents, and then remove duplicates below.
146+                unique_parents.push_back(txin.prevout.hash);
147+            }
148+            std::sort(unique_parents.begin(), unique_parents.end());
149+            unique_parents.erase(std::unique(unique_parents.begin(), unique_parents.end()), unique_parents.end());

dergoegge commented at 4:31 pm on July 12, 2023:

0            // Add the orphan's parents. Net processing will filter out what we already have.
1            // Deduplicate parent txids, so that we don't have to loop over
2            // the same parent txid more than once down below.
3            std::set<uint256> unique_parents;
4            auto to_prevout = [](const CTxIn& in) { return in.prevout.hash; };
5            std::transform(ptx->vin.begin(), ptx->vin.end(), std::inserter(unique_parents, unique_parents.begin()), to_prevout);

in src/node/txpackagetracker.cpp:128 in 116378efc1 outdated

123+        for (const auto& entry : expired) {
124+            LogPrint(BCLog::TXPACKAGES, "\nTimeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "ancpkginfo" : "orphan parent",
125+                entry.second.GetHash().ToString(), entry.first);
126+        }
127+        std::vector<GenTxid> results;
128+        for (const auto& gtxid : tracker_requestable) {

dergoegge commented at 4:35 pm on July 12, 2023:

Would you mind adding some spacing around logical blocks in your code? I find a lot of the new code hard to read with a lack of spacing

e.g.:

 0    {
 1        AssertLockNotHeld(m_mutex);
 2        LOCK(m_mutex);
 3
 4        std::vector<std::pair<NodeId, GenTxid>> expired;
 5        auto tracker_requestable = orphan_request_tracker.GetRequestable(nodeid, current_time, &expired);
 6        for (const auto& entry : expired) {
 7            LogPrint(BCLog::TXPACKAGES, "\nTimeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "ancpkginfo" : "orphan parent",
 8                entry.second.GetHash().ToString(), entry.first);
 9        }
10
11        std::vector<GenTxid> results;
12        for (const auto& gtxid : tracker_requestable) {

in test/functional/p2p_orphan_handling.py:160 in 116378efc1 outdated

155+        orphan_wtxid, orphan_tx, parent_tx = self.create_package()
156+        orphan_inv = CInv(t=MSG_WTX, h=int(orphan_wtxid, 16))
157+
158+        peer_inbound = node.add_p2p_connection(PeerTxRelayer())
159+        peer_inbound.send_and_ping(msg_inv([orphan_inv]))
160+        self.fastforward(TXREQUEST_TIME_SKIP)

mzumsande commented at 6:56 pm on July 12, 2023:

There should be a setmocktime call with the initial time somewhere before the first fastforward, otherwise this can fail intermittently.

glozow commented at 4:42 pm on July 24, 2023:

Thanks, added at the top of run_test

in src/net_processing.cpp:2963 in 543273d96e outdated

2959@@ -2960,9 +2960,10 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
2960         const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx);
2961         const TxValidationState& state = result.m_state;
2962         const uint256& orphanHash = porphanTx->GetHash();
2963+        const uint256 orphan_wtxid = porphanTx->GetWitnessHash();

mzumsande commented at 7:35 pm on July 12, 2023:

commit 543273d96e896adf5531ed961856aa0eb70cbe57: nit: uint256& like in the line above?

glozow commented at 3:20 pm on July 24, 2023:

Done, thanks

in src/net_processing.cpp:5967 in 116378efc1 outdated

5962+        for (const auto& gtxid : requestable_orphans) {
5963+            if (AlreadyHaveTx(gtxid, /*include_orphanage=*/false)) {
5964+                // We don't know that the transaction was rejected by mempool. But if the
5965+                // transaction was added to mempool, we would have already called
5966+                // MempoolAcceptedTx().
5967+                m_txpackagetracker->MempoolRejectedTx(gtxid.GetHash());

instagibbs commented at 7:39 pm on July 12, 2023:

hmmm, this is a txid, not a wtxid, which is essentially a no-op unless it’s a non-segwit tx. What cases do we expect this MempoolRejectedTx is even needed? I added logging and will see if this case ever actually would be needed.

instagibbs commented at 3:18 pm on July 20, 2023:

also, adding wtxid logging at MEMPOOLREJ log is done would be <3

glozow commented at 9:17 pm on August 14, 2023:

Added MEMPOOLREJ log and successful AcceptToMemoryPool log for both orphan and non-orphan tx results.

mzumsande commented at 10:26 pm on July 12, 2023: contributor

some more comments, not finished yet though.

in src/net_processing.cpp:2969 in 4cbb631753 outdated

2965@@ -2966,7 +2966,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
2966             LogPrint(BCLog::MEMPOOL, "   accepted orphan tx %s\n", orphan_wtxid.ToString());
2967             RelayTransaction(orphanHash, porphanTx->GetWitnessHash());
2968             m_txpackagetracker->AddChildrenToWorkSet(*porphanTx);
2969-            m_txpackagetracker->EraseOrphanTx(orphanHash);
2970+            m_txpackagetracker->EraseOrphanTx(porphanTx->GetWitnessHash());

mzumsande commented at 10:31 pm on July 12, 2023:

nit: could use orphan_wtxid introduced in the previous commit, here and in various other places.

mzumsande commented at 8:47 pm on July 14, 2023: contributor

Some thoughts about logging:

After testing this on mainnet for a bit, I think it would be nice to be able to follow the fate of each individual orphan in BCLog::TXPACKAGES: We currently have a tx-level based log entry for addition (“stored orphan…"), but nothing for removal, so having additional log entries in MempoolAcceptedTx() for succesful resolution and MempoolRejectedTx() for failure would be nice, maybe also a tx-level message for removal due to expiration and overflow.

If this became too spammy, we could use different severities, but I think it shouldn’t be too bad except if you just started with an empty mempool?

Also, could we log wtxids whenever possible? There is currently a bit of a mix.

in src/node/txpackagetracker.cpp:87 in b5ab45e595 outdated

88+        LOCK(m_mutex);
89+        // Skip if we weren't provided the tx and can't find the wtxid in the orphanage.
90+        if (tx == nullptr && !m_orphanage.HaveTx(GenTxid::Wtxid(wtxid))) return;
91+
92+        // Even though this stores the orphan wtxid, GenTxid::Txid instead of Wtxid because we will be requesting the parents via txid.
93+        orphan_request_tracker.ReceivedInv(nodeid, GenTxid::Txid(wtxid), is_preferred, reqtime);

mzumsande commented at 9:23 pm on July 17, 2023:

(moved over from 27742): The approach to add a wtxid disguised as a txid to distinguish between legacy orphan processing and package relay seems a bit like a hack to me. I’m not strictly against it, but I guess I don’t completely understand yet why it’s necessary - the information whether a peer supports package relay does not change, so why can’t we just always use GenTxid::Wtxid(wtxid) and check again in GetOrphanRequests() and elsewhere whether we do package relay with the peer instead of checking whether it’s a txid / wtxid?

glozow commented at 1:16 pm on July 18, 2023:

Ah good point, it’s not necessary, we could just use GenTxid::Wtxid for all of them. :+1:

in src/node/txpackagetracker.h:24 in 116378efc1 outdated

19+class TxPackageTracker {
20+    class Impl;
21+    const std::unique_ptr<Impl> m_impl;
22+public:
23+    struct Options {
24+        unsigned int m_max_orphanage_count;

ariard commented at 1:16 am on July 21, 2023:

A comment can be added to precise this is the global not per-peer limit, at least on how it is used by LimitOrphans() (DEFAULT_MAX_ORPHAN_TRANSACTIONS).

glozow commented at 4:49 pm on July 24, 2023:

Added comment

in src/node/txpackagetracker.cpp:156 in 116378efc1 outdated

151+                results.emplace_back(GenTxid::Txid(txid));
152+            }
153+            // Mark the orphan as requested
154+            orphan_request_tracker.RequestedTx(nodeid, gtxid.GetHash(), current_time + ORPHAN_ANCESTOR_GETDATA_INTERVAL);
155+        }
156+        if (!results.empty()) LogPrint(BCLog::TXPACKAGES, "\nRequesting %u items from peer=%d\n", results.size(), nodeid);

instagibbs commented at 1:22 pm on July 21, 2023:

This logging should be handled by the caller, since the caller sometimes isn’t actually requesting the items!

DrahtBot added the label Needs rebase on Jul 25, 2023

in src/txorphanage.h:81 in 29d9d326d5 outdated

76+        return peer_bytes_it == m_peer_bytes_used.end() ? 0 : peer_bytes_it->second;
77+    }
78+
79+    /** Remove a peer from an orphan's announcers list, erasing the orphan if this is the only peer
80+     * who announced it. If the orphan doesn't exist or does not list this peer as an announcer, do nothing. */
81+    void EraseOrphanOfPeer(const uint256& wtxid, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);

achow101 commented at 6:39 pm on July 31, 2023:

In 29d9d326d5193bb9a410a8881eabc93de5dd6266 “[txorphanage] track size of stored orphans, total and by peer”

This function is unimplemented.

glozow commented at 9:14 pm on August 14, 2023:

Deleted from that commit

in src/net_processing.cpp:4288 in 974d864419 outdated

4284@@ -4286,6 +4285,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4285                 if (RecursiveDynamicUsage(*ptx) < 100000) {
4286                     AddToCompactExtraTransactions(ptx);
4287                 }
4288+                m_txpackagetracker->MempoolRejectedTx(tx.GetWitnessHash());

achow101 commented at 6:53 pm on July 31, 2023:

In 974d864419dd98be6e32dec3ee11f5082b060b1b “[refactor] make TxPackageTracker responsible for EraseTx and AddChildrenToWorkset”

This line seems a bit unrelated to this commit as there is no pre-existing EraseTx.

glozow commented at 9:14 pm on August 14, 2023:

Rearranged the commits so it’s more clear where the erasure is new

in src/node/txpackagetracker.cpp:15 in b5ab45e595 outdated

10+#include <txrequest.h>
11+#include <util/hasher.h>
12 
13 namespace node {
14+    /** How long to wait before requesting orphan ancpkginfo/parents from an additional peer. */
15+    static constexpr auto ORPHAN_ANCESTOR_GETDATA_INTERVAL{60s};

achow101 commented at 6:58 pm on July 31, 2023:

In b5ab45e595bbcedbd602b6385b83e9ffd983f216 “[p2p/refactor] make TxPackageTracker responsible for orphan resolution”

nit: indentation

glozow commented at 9:14 pm on August 14, 2023:

fixed

glozow marked this as a draft on Aug 3, 2023

glozow commented at 2:09 pm on August 3, 2023: member

Opened #28199 with tests that may help us ensure this PR isn’t changing specific behaviors. Marking this as draft as it depends on that one.

glozow force-pushed on Aug 14, 2023

glozow commented at 9:15 pm on August 14, 2023: member

Reworked this into a TxDownloadManager (https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1262316909) and addressed most comments.

DrahtBot removed the label Needs rebase on Aug 14, 2023

glozow force-pushed on Aug 15, 2023

DrahtBot added the label Needs rebase on Aug 21, 2023

achow101 referenced this in commit 5aa67eb365 on Aug 22, 2023

glozow force-pushed on Aug 29, 2023

DrahtBot removed the label Needs rebase on Aug 29, 2023

glozow commented at 1:35 pm on August 29, 2023: member

Some thoughts about logging:

After testing this on mainnet for a bit, I think it would be nice to be able to follow the fate of each individual orphan in BCLog::TXPACKAGES: We currently have a tx-level based log entry for addition (“stored orphan…"), but nothing for removal, so having additional log entries in MempoolAcceptedTx() for succesful resolution and MempoolRejectedTx() for failure would be nice, maybe also a tx-level message for removal due to expiration and overflow.

If this became too spammy, we could use different severities, but I think it shouldn’t be too bad except if you just started with an empty mempool?

Also, could we log wtxids whenever possible? There is currently a bit of a mix.

also, adding wtxid logging at MEMPOOLREJ log is done would be <3 @mzumsande @instagibbs I’ve split the commits adding new logs and wtxids to the logging to its own small PR, #28364

(Also, rebased.)

in src/txorphanage.cpp:229 in b34c7ba883 outdated

233@@ -226,7 +234,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block)
234             if (itByPrev == m_outpoint_to_orphan_it.end()) continue;
235             for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) {
236                 const CTransaction& orphanTx = *(*mi)->second.tx;
237-                const uint256& orphanHash = orphanTx.GetHash();

instagibbs commented at 3:31 pm on August 29, 2023:

let’s rename orphanHash while we’re here to something that doesn’t sound like txid

glozow commented at 3:02 pm on September 14, 2023:

Renamed to orphan_wtxid in #28481

in src/net_processing.cpp:4009 in 1ea66e8c2c outdated

4247-            std::vector<uint256> unique_parents;
4248-            unique_parents.reserve(tx.vin.size());
4249-            for (const CTxIn& txin : tx.vin) {
4250-                // We start with all parents, and then remove duplicates below.
4251-                unique_parents.push_back(txin.prevout.hash);
4252+            for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) {

instagibbs commented at 3:48 pm on August 29, 2023:

in 1ea66e8c2ce9e05e1346c1363196073eec554292 [refactor] consolidate invalid ATMP processing

this loop is already taken care of in ProcessValidTx, no?

in src/net_processing.cpp:2957 in 1ea66e8c2c outdated

3106+                orphan_wtxid.ToString(),
3107+                peer.m_id,
3108+                state.ToString());
3109+            // Ignoring the return value. Within orphan processing, we do not make
3110+            // additional orphan resolution requests when a transaction is missing inputs.
3111+            ProcessInvalidTx(porphanTx, peer.m_id, state);

instagibbs commented at 3:56 pm on August 29, 2023:

note: this is now being called in the case of TX_MISSING_INPUTS where it wasn’t prior

is this a behavior change? Looks like if it has rejected parents additional things can happen.

glozow commented at 3:27 pm on September 14, 2023:

Yes, behavior change. To make more concrete:

We get an orphan C with 2 missing parents, A and B
We reject A
We get B and accept it
We add C to workset and process it in ProcessOrphanTx.
Here, since we find A in recent_rejects, we also add C to recent_rejects when we wouldn’t have before.

I think this is good actually… :thinking:

instagibbs commented at 5:37 pm on September 15, 2023:

IIRC I was fine with it, just making sure I understood!

in src/txorphanage.h:71 in af6602e7ff outdated

66+    unsigned int TotalOrphanBytes() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
67+    {
68+        LOCK(m_mutex);
69+        return m_total_orphan_bytes;
70+    }
71+    /** Return total amount of orphans stored by this transaction, in bytes. */

instagibbs commented at 4:07 pm on August 29, 2023:

0    /** Return total amount of orphans stored by this peer, in bytes. */

glozow commented at 3:07 pm on September 14, 2023:

haha, fixed in #28481

in src/txorphanage.cpp:313 in 0e27954b71 outdated

298@@ -266,3 +299,24 @@ void TxOrphanage::EraseForBlock(const CBlock& block)
299         LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx included or conflicted by block\n", nErased);
300     }
301 }
302+void TxOrphanage::EraseOrphanOfPeer(const uint256& wtxid, NodeId peer)
303+{
304+    AssertLockNotHeld(m_mutex);
305+    LOCK(m_mutex);
306+    // Nothing to do if this peer isn't storing any orphans.

instagibbs commented at 4:08 pm on August 29, 2023:

is this a necessary optimization? the next check would catch this case as well

glozow commented at 3:01 pm on September 14, 2023:

You’re right, removed in #28481

in src/txorphanage.h:52 in ed557d07c6 outdated

44@@ -45,13 +45,13 @@ class TxOrphanage {
45 
46     /** Maybe erase all orphans announced by a peer (eg, after that peer disconnects). If an orphan
47      * has been announced by another peer, don't erase, just remove this peer from the list of announcers. */
48-    void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
49+    std::vector<uint256> EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);

instagibbs commented at 4:23 pm on August 29, 2023:

I don’t see it being used anywhere?

glozow commented at 3:00 pm on September 14, 2023:

Removed, I think in a past implementation I was using it but I’m not anymore

in src/txorphanage.h:99 in ed557d07c6 outdated

103@@ -101,6 +104,8 @@ class TxOrphanage {
104         int64_t nTimeExpire;
105         size_t list_pos;
106         std::set<NodeId> announcers;
107+        /** Txids of the missing parents to request. Determined by peerman. */

instagibbs commented at 4:26 pm on August 29, 2023:

in [txorphanage] store parent txids in OrphanTx

Is this just the deduplicated set of txids to possibly fetch, if still in orphanage?

glozow commented at 2:57 pm on September 14, 2023:

Yes

in src/net_processing.cpp:1883 in 94f9de3eac outdated

1879@@ -1880,6 +1880,15 @@ void PeerManagerImpl::BlockConnected(const std::shared_ptr<const CBlock>& pblock
1880         }
1881     }
1882 
1883+    if (m_chainman.ActiveChain().Tip()->GetBlockHash() != hashRecentRejectsChainTip) {

instagibbs commented at 4:52 pm on August 29, 2023:

if a block is being connected, why don’t we just unconditionally wipe m_recent_rejects? Could delete hashRecentRejectsChainTip as well

in src/node/txdownloadman.h:67 in 5d380c3b88 outdated

67@@ -68,6 +68,13 @@ class TxDownloadManager {
68 
69     /** Should be called when a notfound for a tx has been received. */
70     void ReceivedNotFound(NodeId nodeid, const std::vector<uint256>& txhashes) { m_impl->ReceivedNotFound(nodeid, txhashes); }
71+
72+    /** Add a new orphan transaction. Returns whether this orphan is going to be processed and the

instagibbs commented at 5:22 pm on August 29, 2023:

can already exist in orphanage

0    /** Add a potentially new orphan transaction. Returns whether this orphan is going to be processed and the

glozow commented at 3:43 pm on September 14, 2023:

added

in src/net_processing.cpp:4016 in 5d380c3b88 outdated

4038-                    AddKnownTx(*peer, parent_txid);
4039-                    m_txdownloadman.ReceivedTxInv(pfrom.GetId(), gtxid, current_time);
4040-                }
4041-
4042-                if (m_txdownloadman.GetOrphanageRef().AddTx(ptx, pfrom.GetId(), unique_parents)) {
4043+                const auto& [will_process_orphan, unique_parents] = m_txdownloadman.NewOrphanTx(ptx, pfrom.GetId(), current_time);

instagibbs commented at 5:28 pm on August 29, 2023:

I find the should_process/will_process interaction a bit non-obvious on the surface

It’s detecting an orphan that you may want to fetch, then attempting to add to the orphanage, and checking if it’s newly-entered?

glozow commented at 3:55 pm on September 14, 2023:

Yep. We only want to AddToCompactExtraTransactions if this is a new orphan that we’re actually keeping. We might fail to add it to orphanage if it’s full.

in src/net_processing.cpp:690 in f6e0d98b1f outdated

687@@ -690,8 +688,7 @@ class PeerManagerImpl final : public PeerManager
688     CTxMemPool& m_mempool;
689 
690     /** Protects tx download, rejection filter. */

instagibbs commented at 5:38 pm on August 29, 2023:

nothing being protected here anymore

glozow commented at 3:53 pm on September 14, 2023:

Removed comment

in src/txrequest.h:205 in 711419ffc0 outdated

200@@ -195,6 +201,9 @@ class TxRequestTracker {
201     /** Count how many announcements are being tracked in total across all peers and transaction hashes. */
202     size_t Size() const;
203 
204+    /** For some tx hash (either txid or wtxid), return all peers with non-COMPLETED announcements. */
205+    std::vector<NodeId> GetCandidatePeers(const uint256& txhash) const;

instagibbs commented at 5:41 pm on August 29, 2023:

would it make sense to have the arg be a GenTxid

glozow commented at 3:44 pm on September 14, 2023:

Probably not?

in src/node/txdownload_impl.cpp:35 in c437c48e60 outdated

31 void TxDownloadImpl::BlockConnected(const CBlock& block, const uint256& tiphash)
32     EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex)
33 {
34     LOCK(m_tx_download_mutex);
35-    m_orphanage.EraseForBlock(block);
36+    const auto erased_wtxids = m_orphanage.EraseForBlock(block);

instagibbs commented at 5:44 pm on August 29, 2023:

can you push it down to where erased_wtxids is used since it’s only used once?

glozow commented at 3:40 pm on September 14, 2023:

Done

in src/node/txdownload_impl.cpp:239 in c437c48e60 outdated

229@@ -216,6 +230,39 @@ std::vector<GenTxid> TxDownloadImpl::GetRequestsToSend(NodeId nodeid, std::chron
230     EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex)
231 {
232     LOCK(m_tx_download_mutex);
233+    // First process orphan resolution so that the tx requests can be sent asap
234+    std::vector<std::pair<NodeId, GenTxid>> expired_orphan_resolution;
235+    const auto orphans_ready = m_orphan_resolution_tracker.GetRequestable(nodeid, current_time, &expired_orphan_resolution);
236+    // Expire orphan resolution attempts
237+    for (const auto& [nodeid, orphan_gtxid] : expired_orphan_resolution) {
238+        LogPrintf("timeout of in-flight orphan resolution %s for peer=%d\n", orphan_gtxid.GetHash().ToString(), nodeid);

instagibbs commented at 5:50 pm on August 29, 2023:

do we really want to log unconditionally?

glozow commented at 3:42 pm on September 14, 2023:

oop changed

in src/node/txdownload_impl.cpp:262 in c437c48e60 outdated

255+            }
256+            m_orphan_resolution_tracker.RequestedTx(nodeid, orphan_gtxid.GetHash(),
257+                                                    current_time + ORPHAN_ANCESTOR_GETDATA_INTERVAL);
258+        } else {
259+            LogPrint(BCLog::TXPACKAGES, "couldn't find parent txids to resolve orphan %s with peer=%d\n",
260+                     nodeid, orphan_gtxid.GetHash().ToString());

instagibbs commented at 6:00 pm on August 29, 2023:

print args are flipped

glozow commented at 3:43 pm on September 14, 2023:

Fixed

in src/node/txdownload_impl.cpp:289 in 7f9db92ba3 outdated

254+                         txid.ToString(), nodeid, orphan_gtxid.GetHash().ToString());
255+            }
256+            m_orphan_resolution_tracker.RequestedTx(nodeid, orphan_gtxid.GetHash(),
257+                                                    current_time + ORPHAN_ANCESTOR_GETDATA_INTERVAL);
258+        } else {
259+            LogPrint(BCLog::TXPACKAGES, "couldn't find parent txids to resolve orphan %s with peer=%d\n",

instagibbs commented at 6:35 pm on August 29, 2023:

under what circumstances is this expected? Eviction of the orphan in the orphanage, but outstanding resolution request? A comment to the expected conditions would be helpful for documentation.

instagibbs commented at 8:05 pm on August 29, 2023:

ok, it’s happening because of: https://github.com/bitcoin/bitcoin/pull/28031/files#r1309300862

see logs:

02023-08-29T19:19:00.066912Z [mempoolrej] 259b016ce9258c536a0a222728795aa6c22de47dbd6e166f4330e33ce075fe31 (wtxid=d6dc1525169700841c605385514a6069959f86bceccd33ede3fce714fa699219) from peer=2 was not accepted: bad-txns-inputs-missingorspent
12023-08-29T19:19:00.066970Z [txpackages] stored orphan tx 259b016ce9258c536a0a222728795aa6c22de47dbd6e166f4330e33ce075fe31 (wtxid=d6dc1525169700841c605385514a6069959f86bceccd33ede3fce714fa699219) (mapsz 4 outsz 5)
22023-08-29T19:19:00.066990Z [txpackages] adding peer=2 as a candidate for resolving orphan d6dc1525169700841c605385514a6069959f86bceccd33ede3fce714fa699219
32023-08-29T19:19:00.067015Z [txpackages] adding peer=6 as a candidate for resolving orphan d6dc1525169700841c605385514a6069959f86bceccd33ede3fce714fa699219
42023-08-29T19:19:00.067027Z [txpackages] added peer=6 as announcer of orphan tx d6dc1525169700841c605385514a6069959f86bceccd33ede3fce714fa699219
52023-08-29T19:19:04.361970Z [txpackages] adding peer=21 as a candidate for resolving orphan 259b016ce9258c536a0a222728795aa6c22de47dbd6e166f4330e33ce075fe31
62023-08-29T19:19:06.399524Z [txpackages] couldn't find parent txids to resolve orphan 21 with peer=259b016ce9258c536a0a222728795aa6c22de47dbd6e166f4330e33ce075fe31
72023-08-29T19:20:02.183893Z timeout of in-flight orphan resolution d6dc1525169700841c605385514a6069959f86bceccd33ede3fce714fa699219 for peer=6
82023-08-29T19:21:02.206171Z timeout of in-flight orphan resolution d6dc1525169700841c605385514a6069959f86bceccd33ede3fce714fa699219 for peer=2
92023-08-29T19:21:02.206223Z [txpackages]    removed orphan tx 259b016ce9258c536a0a222728795aa6c22de47dbd6e166f4330e33ce075fe31 (wtxid=d6dc1525169700841c605385514a6069959f86bceccd33ede3fce714fa699219)

instagibbs commented at 6:47 pm on August 29, 2023: member

went over new-er implementation through 7f9db92ba3f483f78bd248406d921bd73b1e7054

dropping some comments

fuzzer fail:

0node/txdownload_impl.cpp:18 ConnectedPeer: Assertion `m_peer_info.count(nodeid) == 0' failed.

in src/node/txdownload_impl.cpp:199 in 7f9db92ba3 outdated

194+
195+void TxDownloadImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now)
196+    EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex)
197+{
198+    if (!Assume(m_peer_info.count(peer) > 0)) return;
199+    if (m_orphanage.HaveTx(gtxid)) AddOrphanAnnouncer(peer, gtxid.GetHash(), now);

instagibbs commented at 8:05 pm on August 29, 2023:

this adds an announcer even if the INV is txid type, even though the assumption is wtxid arg

instagibbs commented at 3:54 pm on August 30, 2023:

I think the simplest solution is just filter this addition based on wtxid-ness, since this only helps in the multi-announcer case, which wouldn’t be a regression, iiuc

glozow commented at 11:57 am on September 4, 2023:

ooh thanks :+1:

glozow commented at 7:57 am on September 13, 2023:

Fixed, just did a gate on IsWtxid. Don’t think that many non-wtxidrelay nodes are still out there anyway.

in src/node/txdownload_impl.cpp:250 in 7f9db92ba3 outdated

245+        const auto parent_txids{m_orphanage.GetParentTxids(orphan_gtxid.GetHash())};
246+        if (parent_txids.has_value()) {
247+            if (!Assume(m_peer_info.count(nodeid) > 0)) continue;
248+            const auto& info = m_peer_info.at(nodeid).m_connection_info;
249+            for (const auto& txid : *parent_txids) {
250+                // Schedule with no delay. It should be requested immediately

instagibbs commented at 3:53 pm on August 30, 2023:

0                // Schedule with no delay, i.e. not using ReceivedTxInv(). It should be requested immediately

glozow commented at 7:56 am on September 13, 2023:

Added

achow101 referenced this in commit 5666966dff on Aug 31, 2023

DrahtBot added the label Needs rebase on Aug 31, 2023

Frank-GER referenced this in commit 52cce72c8f on Sep 8, 2023

instagibbs commented at 7:35 pm on September 12, 2023: member

think this can be rebased since tests are merged

glozow force-pushed on Sep 13, 2023

glozow commented at 7:56 am on September 13, 2023: member

Rebased but not all comments addressed yet, still working on it.

DrahtBot removed the label Needs rebase on Sep 13, 2023

glozow force-pushed on Sep 14, 2023

glozow renamed this:
~~Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module~~
Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling
on Sep 15, 2023

glozow force-pushed on Sep 29, 2023

DrahtBot added the label Needs rebase on Oct 2, 2023

glozow force-pushed on Oct 3, 2023

DrahtBot removed the label Needs rebase on Oct 3, 2023

glozow force-pushed on Oct 12, 2023

in src/node/txdownload_impl.cpp:209 in 36f4d0d544 outdated

270+            const auto& info = m_peer_info.at(nodeid).m_connection_info;
271+            for (const auto& txid : *parent_txids) {
272+                // Schedule with no delay instead of using ReceivedTxInv. This means it's scheduled
273+                // for request immediately unless there is already a request out for the same txhash
274+                // (e.g. if there is another orphan that needs this parent).
275+                m_txrequest.ReceivedInv(nodeid, GenTxid::Txid(txid), info.m_preferred, current_time);

instagibbs commented at 8:56 pm on October 12, 2023:

caught by your new fuzzer: Note that the parent txid is already in the orphanage by this line, we try to re-request something we already have locally in our orphanage.

0                if (m_orphanage.HaveTxAndPeer(GenTxid::Txid(txid), nodeid)) continue;
1                m_txrequest.ReceivedInv(nodeid, GenTxid::Txid(txid), info.m_preferred, current_time);

in src/test/fuzz/txorphan.cpp:97 in 36f4d0d544 outdated

92@@ -91,8 +93,11 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
93                     {
94                         CTransactionRef ref = orphanage.GetTxToReconsider(peer_id);
95                         if (ref) {
96-                            bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetHash()));
97-                            Assert(have_tx);
98+                            Assert(orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) ||
99+                                   orphanage.HaveTx(GenTxid::Wtxid(ref->GetHash())));

instagibbs commented at 4:29 pm on October 13, 2023:

Looks like this happens in a few places? wouldn’t type safety for the constructor be nice

0                                   orphanage.HaveTx(GenTxid::Wtxid(ref->GetWitnessHash())));

glozow commented at 4:20 pm on October 16, 2023:

hahahahaha Unfortunately #28107 doesn’t help in this case since this function takes a gentxid, but I’ve rebased on top of that PR.

in src/txorphanage.cpp:157 in 36f4d0d544 outdated

219         size_t randompos = rng.randrange(m_orphan_list.size());
220-        EraseTxNoLock(m_orphan_list[randompos]->first);
221+        const auto& wtxid = m_orphan_list[randompos]->second.tx->GetWitnessHash();
222+        expired_and_evicted.emplace_back(wtxid);
223+        EraseTxNoLock(wtxid);
224         ++nEvicted;

instagibbs commented at 6:10 pm on October 13, 2023:

we should += like the above call or do ++ for both I think

glozow commented at 1:56 pm on October 17, 2023:

done

in src/txorphanage.cpp:331 in 36f4d0d544 outdated

329     if (vOrphanErase.size()) {
330         int nErased = 0;
331-        for (const uint256& orphanHash : vOrphanErase) {
332-            nErased += EraseTxNoLock(orphanHash);
333+        for (const uint256& orphan_wtxid : vOrphanErase) {
334+            nErased += EraseTxNoLock(orphan_wtxid);

instagibbs commented at 6:28 pm on October 13, 2023:

for fuzz/txorphan

0echo Kv//////////////lJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSU//////////////////////////////+A////////kEEKJf//ev///////8P///////8AAIH//wkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkB/wD8///SQP///////////////////////////////////4D///////+QQQol//96////////w/////////8KJf//ev//////////kEEKJf//1v///wP/////kEH///+QQQo1//8BPwAAv67Kf/////8u//////////////////////////////////////////////////////////////////////////96AP/1///////S0P//IP96AP////////+A////////kEEKJf//ev//////////kEEKJf//1v///wP/////kEH///+QQQo1//96/////P////+QQQol/yX//3r//////////wol///W//////////97e3t7e3t7e3t7e3t7e3t7e3t7e/+QQQol//96//////////////8s/yb/ | base64 -d > crash.txt

These call EraseTxNoLock without doing m_peer_work_set management: TxOrphanage::EraseForBlock TxOrphanage::LimitOrphans

This results in fuzzer crashes where it’s (still) in the peer’s work set, but not counted as an announcer since it’s been evicted(then later sent).

Any time EraseTx{NoLock} is called we need to make sure that m_peer_work_set is appropriately being cleaned.

Two other spots I haven’t thought through yet but I suspect need the same fix: TxDownloadImpl::MempoolAcceptedTx TxDownloadImpl::MempoolRejectedTx

edit:

latter two mempool things are called; cleanup needs to happen! working on it

instagibbs commented at 2:46 pm on October 16, 2023:

pushed another couple fixes to https://github.com/instagibbs/bitcoin/tree/orphan-resolution-module-oct-12 , was same issue as before just in

TxDownloadImpl::MempoolAcceptedTx TxDownloadImpl::MempoolRejectedTx

as noted before

glozow commented at 8:56 am on October 17, 2023:

Is it a problem to lazily handle transactions in peer work sets that have been deleted from the orphanage? It’s cleaner to remove_work_from_all_sets of course, but it iterates through every entry in m_peer_work_set so there is a performance difference but the external outcome is the same.

I think erasing proactively in EraseOrphanOfPeer is necessary because the tx isn’t deleted. Trying to avoid returning the tx in GetTxToReconsider(peer) after we’ve already decided to give up on (tx, peer).

glozow commented at 9:11 am on October 17, 2023:

TxOrphanage::EraseForBlock TxOrphanage::LimitOrphans

This results in fuzzer crashes where it’s (still) in the peer’s work set, but not counted as an announcer since it’s been evicted(then later sent).

Question: is it crashing because the tx is being returned from GetTxToReconsider? Seems weird as it should return a nullptr if it gets a tx that isn’t there anymore…

OOhhhhhhhhh could it be that it’s re-added? My guess:

AddTx(tx_child, peer)
AddChildrenToWorkSet(tx_parent) -> peer workset contains tx_child
EraseTx(tx_child) -> peer workset still contains tx_child but we expect orphanage to lazily delete that when we call GetTxToReconsider
AddTx(tx_child, other_peer) (makes no sense but we’re fuzzing)
GetTxToReconsider(peer) returns tx_child even though it’s not in orphan_resolution_tracker

instagibbs commented at 1:43 pm on October 17, 2023:

Is it a problem to lazily handle transactions in peer work sets that have been deleted from the orphanage?

Since GetTxToReconsider appears to be used in a single spot, and it really doesn’t matter that it once was announced, then erased, then reconsidered for a peer, I suppose the fuzz test should just be modified to not crash :)

glozow commented at 1:59 pm on October 17, 2023:

Removed Assert(HaveTxAndPeer()) in the fuzzer, and added comments for when we clean up the work sets and when we don’t

instagibbs commented at 6:47 pm on October 13, 2023: member

Some not snapshot-ready fixes for fuzz crashes I worked through:

https://github.com/instagibbs/bitcoin/commit/5388830c3c5bb0ed9a1d1379ec94b285de68aa9a

in src/test/fuzz/txdownloadman.cpp:428 in 36f4d0d544 outdated

381+                // nullptr, as the transaction could have been removed from orphanage without being
382+                // removed from the peer's workset.
383+                if (ptx) {
384+                    // However, if there was a non-null tx in the workset, HaveMoreWork should have
385+                    // returned true.
386+                    Assert(expect_work);

instagibbs commented at 2:21 pm on October 16, 2023:

should be able to assert that there also exists an announcement for this node?

glozow commented at 8:45 am on October 17, 2023:

an entry in orphan_resolution_tracker?

instagibbs commented at 1:46 pm on October 17, 2023:

Was going to suggest adding a check for OrphanTx.announcers, but it may not actually exist as discussed elsewhere.

glozow commented at 2:00 pm on October 17, 2023:

That, and we can’t check announcers because it’s private

glozow force-pushed on Oct 16, 2023

DrahtBot added the label Needs rebase on Oct 16, 2023

glozow force-pushed on Oct 17, 2023

instagibbs commented at 2:04 pm on October 17, 2023: member

worked through https://github.com/bitcoin/bitcoin/pull/28031/commits/e2e262fc1f6ea41d6f46b8db713c12df035464f6

All suggestions taken(or withdrawn by myself).

glozow commented at 2:44 pm on October 20, 2023: member

Note I know there’s a “needs rebase” label but that’s because I rebased on top of 28107

glozow force-pushed on Dec 18, 2023

DrahtBot removed the label Needs rebase on Dec 18, 2023

glozow force-pushed on Jan 2, 2024

DrahtBot added the label Needs rebase on Mar 9, 2024

achow101 referenced this in commit d813ba1bc4 on Apr 30, 2024

glozow force-pushed on Jul 31, 2024

DrahtBot removed the label CI failed on Jul 31, 2024

DrahtBot removed the label Needs rebase on Jul 31, 2024

DrahtBot added the label Needs rebase on Aug 1, 2024

glozow force-pushed on Aug 1, 2024

DrahtBot removed the label Needs rebase on Aug 1, 2024

DrahtBot added the label Needs rebase on Aug 5, 2024

glozow force-pushed on Aug 12, 2024

glozow force-pushed on Aug 28, 2024

DrahtBot removed the label Needs rebase on Aug 28, 2024

DrahtBot commented at 2:41 am on August 29, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29377989202

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

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

DrahtBot added the label CI failed on Aug 29, 2024

DrahtBot added the label Needs rebase on Sep 2, 2024

glozow force-pushed on Sep 6, 2024

achow101 referenced this in commit 7b66815b16 on Oct 29, 2024

[txrequest] GetCandidatePeers and ResetRequestTimeout 370e7708fb

[refactor] change type of unique_parents to Txid 938659fbd8

[txorphanage] store parent txids in OrphanTx 7badc7e004

[txorphanage] support multiple announcers

Add ability to add and track multiple announcers per orphan transaction,
erasing announcers but not the entire orphan.

The tx creation code in orphanage_tests needs to be updated so that each
tx is unique, because the CountOrphans() check assumes that calling
EraseForPeer necessarily means its orphans are deleted.

acaa7c13fa

[txorphanage] return wtxids from EraseForBlock d8bdb69652

[txorphanage] return erased wtxids from LimitOrphans 83d276c7dd

[unit test] multiple announcers fa8e543278

[fuzz] orphanage multiple announcer functions a0b1953d4f

glozow force-pushed on Nov 28, 2024

DrahtBot removed the label Needs rebase on Nov 28, 2024

glozow force-pushed on Nov 28, 2024

[txdownload] remove unique_parents that we already have

This means we no longer return parents we already have in the
m_unique_parents result from MempoolRejectedTx.

We need to separate the loop that checks AlreadyHave parents from the
loop that adds parents as announcments, because we may do the latter
loop multiple times for different peers.

b64be9ed2c

[p2p] try multiple peers for orphan resolution c04d1a876c

[functional test] orphan handling with multiple announcers 780e28cf9a

[p2p] only attempt 1p1c when both txns provided by the same peer

Now that we track all announcers of an orphan, it's not helpful to
consider an orphan provided by a peer that didn't send us this parent.
It can only hurt our chances of finding the right orphan when there are
multiple candidates.

Adapt the 2 tests in p2p_opportunistic_1p1c.py that looked at 1p1c
packages from different peers. Instead of checking that the right peer
is punished, we now check that the package is not submitted. We can't
use the functional test to see that the package was not considered
because the behavior is indistinguishable (except for the logs).

3e16a36959

[p2p] grab m_tx_download_mutex only when needed

The places where m_tx_download_mutex is held in between calls to
txdownloadman are unnecessary.

For example, m_tx_download_mutex was previously held throughout these steps:
  1. ReceivedTx(), receive should_validate=true
  2. MempoolRejectedTx(tx), receive package_to_validate={parent, child}
  3. MempoolRejectedPackage(parent, child)
  4. MempoolRejectedTx(parent)
  5. MempoolRejectedTx(child)

But this is not necessary for correctness. If, for example,
BlockConnected is called in between any of the steps, txdownloadman's
internal state will remain consistent, even if the relevant
transaction(s) confirmed. See the txdownloadman fuzzers.

This sets up the next commit which moves locking into TxDownloadManagerImpl.

51b7d4d052

[refactor] make Find1P1CPackage internal 87783ac69a

[refactor] split AlreadyHaveTx into internal and external versions 3fbc0021f3

add internal Mutex to TxDownloadManagerImpl

It is named slightly differently to make it easy to check that
m_tx_download_mutex is fully removed in the next commit.

214d893822

remove m_tx_download_mutex in net_processing 0a85e99511

glozow force-pushed on Nov 28, 2024

DrahtBot removed the label CI failed on Nov 28, 2024

glozow commented at 5:27 pm on December 1, 2024: member

Rebased. I have split the first 12 commits (the majority of this PR) into #31397. The other 5 commits are for making the module internally thread-safe, and can be opened in another PR afterward.

Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling #28031

Code Coverage & Benchmarks

Reviews

Conflicts