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:
(now #30110) Introduces TxDownloadManager, which handles all transaction downloading. Adds tests for TxDownloadManager.
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).
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.
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:
#30750 (scripted-diff: LogPrint -> LogDebug by maflcko)
#30664 (build: Remove Autotools-based build system by hebasto)
#30572 (Halt processing of unrequested transactions v2 by ariard)
#30538 (Doc: add a comment referencing past vulnerability next to where it was fixed by darosior)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
#29492 (refactor: Remove redundant definitions by Empact)
#28676 ([WIP] Cluster mempool implementation by sdaftuar)
#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
543273d96eoutdated
in
src/node/txpackagetracker.cpp:30
in
116378efc1outdated
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);
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?
in
src/node/txpackagetracker.cpp:136
in
116378efc1outdated
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);
“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 :)
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
b5ab45e595outdated
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);
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
b5ab45e595outdated
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;
Replaced the exposure of Count to CheckIsEmpty() functions
in
src/txorphanage.h:153
in
116378efc1outdated
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);
126127+ /** Map from nodeid to the amount of orphans provided by this peer, in bytes. */
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
in
src/test/orphanage_tests.cpp:366
in
b43033cfb8outdated
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.
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.
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
116378efc1outdated
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.
in
src/node/txpackagetracker.h:56
in
116378efc1outdated
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);
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 over2// the same parent txid more than once down below.3 std::set<uint256> unique_parents;
4 auto to_prevout = [](const CTxIn&in) { returnin.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
116378efc1outdated
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) {
in
src/net_processing.cpp:5967
in
116378efc1outdated
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());
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.
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
b5ab45e595outdated
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);
(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?
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).
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
29d9d326d5outdated
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);
Rearranged the commits so it’s more clear where the erasure is new
in
src/node/txpackagetracker.cpp:15
in
b5ab45e595outdated
10+#include <txrequest.h>
11+#include <util/hasher.h>
1213 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};
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
b34c7ba883outdated
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:
in
src/net_processing.cpp:4009
in
1ea66e8c2coutdated
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
1ea66e8c2coutdated
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
af6602e7ffoutdated
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:
44@@ -45,13 +45,13 @@ class TxOrphanage {
4546 /** 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:93
in
ed557d07c6outdated
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
94f9de3eacoutdated
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
5d380c3b88outdated
67@@ -68,6 +68,13 @@ class TxDownloadManager {
6869 /** 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
5d380c3b88outdated
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
f6e0d98b1foutdated
687@@ -690,8 +688,7 @@ class PeerManagerImpl final : public PeerManager
688 CTxMemPool& m_mempool;
689690 /** 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
711419ffc0outdated
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;
203204+ /** 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
c437c48e60outdated
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:
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
7f9db92ba3outdated
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
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
36f4d0d544outdated
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
36f4d0d544outdated
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).
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:
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 :)
in
src/test/fuzz/txdownloadman.cpp:409
in
36f4d0d544outdated
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?
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
[refactor] add TxDownloadManager wrapping TxOrphanage, TxRequestTracker, and bloom filters
This module is going to be responsible for managing everything related
to transaction download, including txrequest, orphan transactions and
package relay. It will be responsible for managing usage of the
TxOrphanage and instructing PeerManager:
- what tx or package-related messages to send to which peer
- whether a tx or package-related message is allowed or useful
- what transactions are available to try accepting to mempool
Future commits will consolidate the interface and re-delegate
interactions from PeerManager to TxDownloadManager.
ecbbadf0f7
[refactor] move ValidationInterface functions to TxDownloadManager
This is move-only.
a201bb8a5b
[txdownload] add read-only reference to mempool
This will become necessary in later commits that query mempool. We also
introduce the TxDownloadOptions in this commit to make the later diff
easier to review.
b88b82fc11
[refactor] move AlreadyHaveTx to TxDownload
This is move-only.
Also delete external RecentConfirmedTransactionsFilter() access since it
is no longer necessary.
58d9f83079
[refactor] move peer (dis)connection logic to TxDownload
The information stored in TxDownloadConnectionInfo isn't used until the
next commit.
f261f0731a
[refactor] rename maybe_add_extra_compact_tx to first_time_failure
The usage of this bool will increase in scope in the next commit.
For this commit, the value of this bool is accurate at each
ProcessInvalidTx callsite:
- ProcessOrphanTx -> this tx is an orphan i.e. has been rejected before
- ProcessPackageResult -> 1p1c only, each transaction is either an
orphan or in m_lazy_recent_rejects_reconsiderable
- ProcessMessage -> tx was received over p2p and validated for the first
time
31b150c915
[p2p] don't log tx invs when in IBD
These invs are ignored anyway, and this allows us to more easily move
the inv handling to TxDownloadManager in the next commit.
3bf6d430b1
[refactor] move tx inv/getdata handling to txdownload51717791c7
[refactor] move notfound processing to txdownloadc40cb85659
[refactor] move some definitions
ProcessInvalidTx will return a PackageToValidate, so it needs to be
defined afterward.
c4b94f6b98
[refactor] move new orphan handling to ProcessInvalidTxb353f5e4b8
move Find1P1CPackage into ProcessInvalidTx17a9ff4c3e
[refactor] ProcessInvalidTx logic to put peerman tasks at the end5483f1639b
[refactor] move Find1P1CPackage to txdownload
Move-only.
ec242e7b49
[refactor] move valid tx processing to TxDownloade93d7b8012
[refactor] move invalid tx processing to TxDownload
Move-only. Also delete external RecentRejectsFilter() access since it is
no longer necessary.
77c5b80374
[refactor] move invalid package processing to TxDownloadb5b4c863f2
[refactor] move new tx logic to txdownload
Also delete external RecentRejectsReconsiderableFilter() access since it
is no longer necessary.
f3cc632279
[refactor] make AlreadyHaveTx and Find1P1CPackage private to TxDownloadImpl2bc5c4fdd6
[refactor] wrap {Have,Get}TxToReconsider in txdownloadf8b2eda654
[refactor] add CheckIsEmpty and remove access to TxDownloadMan internals715caad450
[p2p] don't process orphan if in recent rejects
This should never happen normally, but just in case.
d72f66a3fa
add TxDownloadOptions bool to make deterministic TxRequestTracker
Forward this bool to the TxRequestTracker ctor. This is needed for
stablity in TxDownloadManager fuzzers
960274bd63
[fuzz] txdownloadman and txdownload_impl
The txdownload_impl is similar but allows us to check specific
invariants within its implementation. It will also change a lot more
than the external interface (txdownloadman) will, so we will add more to
this target later.
2be76e800f
[unit test] MempoolRejectedTx48bc095998
[fuzz] tx download succeeds as long as we have 1 good outbound1d4e33e7f8
DrahtBot added the label
Needs rebase
on Sep 2, 2024
DrahtBot
commented at 10:46 pm on September 2, 2024:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
[doc] fix comments in txdownloadman_one_honest_peerb39426576e
remove unnecessary mutex acquisition in notfound loop
Previously, we accessed m_txdownloadman within the loop. Now, we are
just checking and making copies of a local variable.
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
3edae62886
[txrequest] GetCandidatePeers and ResetRequestTimeout319bb0a7c7
[refactor] change type of unique_parents to Txid14af77532b
[txorphanage] store parent txids in OrphanTxc2a9baf966
[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.
7423c1dee3
[txorphanage] return wtxids from EraseForBlock9262aaa85b
[txorphanage] return erased wtxids from LimitOrphansaadc967d78
[refactor] filter unique_parents ahead of time241eab45c3
[p2p] try multiple peers for orphan resolutionde8579635f
[functional test] orphan handling with multiple announcersd80a25315f
[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).
8e893fe816
[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.
75f78bde38
[refactor] split AlreadyHaveTx into internal and external versionsc9f407cbf4
make Find1P1CPackage internal1654c8064a
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.
82d0e5fc9d
remove m_tx_download_mutex in net_processing1aee6d3eb2
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-11-21 09:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
