net_processing: Retry notfounds with more urgency

ajtowns commented at 7:04 am on March 2, 2020: member

Anytime we see a NOTFOUND in response to a request for a tx, look through each of our peers for anyone else who announced the tx, find one who doesn’t already have its inflight tx count maxed out, and of those, make the one who’d look at it first, look at it asap.

Anytime we see a NOTFOUND in response to a request for a tx, look through
each of our peers for anyone else who announced the tx, find one who
doesn't already have its inflight tx count maxed out, and of those,
make the one who'd look at it first, look at it asap.

a204d1586c

fanquake added the label P2P on Mar 2, 2020

ajtowns commented at 7:13 am on March 2, 2020: member

This is an alternative approach to #15505 that’s minimally invasive on the data structures. Doing something about this was suggested as a pre-req for #17303 in #17303 (comment) @sdaftuar expressed some concerns about how much of a CPU hit it could be in worst case scenarios, but it doesn’t look too bad in testing:

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 3373f7f544..1b9e66456a 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -735,8 +735,13 @@ std::chrono::microseconds CalculateTxGetDataTime(const uint256& txid, std::chron
 5     return process_time;
 6 }
 7 
 8+static uint64_t xxx_time_spent GUARDED_BY(cs_main) = 0;
 9+static uint64_t xxx_invocations GUARDED_BY(cs_main) = 0;
10+
11 static void RetryProcessTx(CConnman& connman, const uint256& txid, const std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
12 {
13+    uint64_t time_start = GetTimeMicros();
14+
15     CNodeState::TxDownloadState* best_d = nullptr;
16     std::chrono::microseconds best;
17 
18@@ -764,6 +769,12 @@ static void RetryProcessTx(CConnman& connman, const uint256& txid, const std::ch
19             }
20         }
21     }
22+
23+    xxx_time_spent += GetTimeMicros() - time_start;
24+    ++xxx_invocations;
25+    if (xxx_invocations % 1000 == 0) {
26+        LogPrintf("Time spent in RetryProcessTx %d.%03ds, %d us per call (%d calls)\n", xxx_time_spent / 1000000, (xxx_time_spent / 1000) % 1000, xxx_time_spent/xxx_invocations, xxx_invocations);
27+    }
28 }
29 
30 void RequestTx(CNodeState* state, const uint256& txid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
31diff --git a/test/functional/p2p_notfound_perf.py b/test/functional/p2p_notfound_perf.py
32new file mode 100755
33index 0000000000..970452696c
34--- /dev/null
35+++ b/test/functional/p2p_notfound_perf.py
36@@ -0,0 +1,63 @@
37+#!/usr/bin/env python3
38+# Copyright (c) 2017-2018 The Bitcoin Core developers
39+# Distributed under the MIT software license, see the accompanying
40+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
41+"""Test that we don't leak txs to inbound peers that we haven't yet announced to"""
42+
43+import time
44+from test_framework.messages import msg_notfound, msg_inv, CInv
45+from test_framework.mininode import P2PDataStore
46+from test_framework.test_framework import BitcoinTestFramework
47+from test_framework.util import (
48+    assert_equal,
49+)
50+
51+
52+class P2PNode(P2PDataStore):
53+    def on_inv(self, msg):
54+        pass
55+
56+    def on_getdata(self, msg):
57+        t = time.time()
58+        self.notfound_queue.extend(msg.inv)
59+        for inv in msg.inv:
60+            self.getdata[inv] = t
61+        while len(self.notfound_queue) >= 100:
62+            self.send_message(msg_notfound(vec=self.notfound_queue[:100]))
63+            self.notfound_queue = self.notfound_queue[100:]
64+
65+    def summary(self):
66+        return len(self.getdata), len(self.notfound_queue)
67+
68+class P2PNotFoundPerf(BitcoinTestFramework):
69+    def set_test_params(self):
70+        self.num_nodes = 1
71+
72+    def run_test(self):
73+        PEERS = 11
74+        TRANSACTIONS = 99000
75+
76+        gen_node = self.nodes[0]  # The block and tx generating node
77+        gen_node.generate(1)
78+
79+        inbound_peers = [ self.nodes[0].add_p2p_connection(P2PNode()) for _ in range(PEERS) ]
80+        for inbound in inbound_peers:
81+            inbound.getdata = {}
82+            inbound.notfound_queue = []
83+
84+        for txbatch in range(TRANSACTIONS//100):
85+            self.log.info("Doing batch %d" % (txbatch+1))
86+            ann = [CInv(t=1, h=(txbatch*1000+i)) for i in range(100)]
87+            for inbound in inbound_peers:
88+                inbound.send_message(msg_inv(inv=ann))
89+
90+        #gen_node.logging(exclude=['net'])
91+
92+
93+        for i in range(60):
94+            self.log.info("State: " + " ".join("%d:%d" % inbound.summary() for inbound in inbound_peers))
95+            time.sleep(15)
96+
97+
98+if __name__ == '__main__':
99+    P2PNotFoundPerf().main()

Gives: 2020-03-02T06:58:54.778347Z [msghand] Time spent in RetryProcessTx 18.754s, 17 us per call (1089000 calls) by the end for me, which doesn’t seem too bad. It took about 12 minutes for all 99k transactions to get requested/notfound by all 11 peers, so 19 seconds total doesn’t seem too bad. (The check for MAX_PEER_TX_IN_FLIGHT significantly cuts down on whether this actually does anything – if there’s lots of tx’s announced by each peer, then most of them will always have most tx’s in flight)

DrahtBot commented at 11:05 am on March 2, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19053 (refactor: replace CNode pointers by references within net_processing.{h,cpp} by theStack)

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.

naumenkogs commented at 5:44 pm on March 2, 2020: member

Concept ACK. Code looks good to me. Can you provide more context to understand the test? Are you basically trying to show that DoS-vector your PR opens is probably less than those which already exist?

ajtowns commented at 4:49 am on March 4, 2020: member

@naumenkogs Yeah; in a worst case scenario you could get 100 txids in a NOTFOUND message, have 125 peers to cycle through, and each peer could have announced 100k txs, so you end up with something like O( 12,500 * log(100000) ) operations as a result of a single message (albeit with a fair chunk of setup). The test indicates that 11 peers on my hardware gives a time of about 20us per txid, so per NOTFOUND with 110 peers (so a factor of 100*10) that might result in 20ms processing time on a single message. That seemed low enough to be worth a PR to me.

If that’s not good enough, could make it a bit lower in the worst case fairly easily by not retrying NOTFOUNDs as aggressively (eg, only do it for the first 10 txids in a NOTFOUND message, or ban peers if they often report a txid as NOTFOUND despite having INVed it recently, or change the retry delay from 0s-2s to 2s-4s, etc); but it’d need more rework of the data structures to get it to work efficiently, and then you have to ensure the new data structures don’t introduce different DoS possibilities.

naumenkogs commented at 4:30 pm on March 4, 2020: member

No, I think this sounds reasonable. I’d rather not introduce new data structures. As I said, we probably already have easier ways to DoS a node.

luke-jr referenced this in commit 3f41902983 on Mar 5, 2020

mzumsande commented at 3:56 pm on March 20, 2020: member

Concept ACK.

One difference to #15505 is that the retry-request there would be from outbound peers only, whereas this PR also includes inbound peers. If I understand it correctly, this does not create any additional attack surface (e.g. for InvBlocks), because we keep the exact order of retries from the case where the peer does not answer at all (not even with NOTFOUND), just ask our next peer in line earlier than we would have otherwise.

naumenkogs commented at 0:01 am on March 22, 2020: member

@mzumsande there’s a little threat that, unlike with prior behavior, we won’t have a 1 minute window between first peer announced a tx and followed up with NOTFOUND, and we execute requests for the next peers, during which more honest outbound peers can come with announcements and get prioritized against inbound dishonest peers.

But the only ways I think this can be exploited require dropping a transaction from an honest announcer’s mempool, which is probably a more critical vulnerability anyway, so this doesn’t reduce the overall security.

in src/net_processing.cpp:755 in a204d1586c

750+                 best = it->second;
751+             }
752+         }
753+    }
754+
755+    std::chrono::microseconds process_time = current_time + GetRandMicros(MAX_NOTFOUND_RETRY_RANDOM_DELAY);

naumenkogs commented at 0:05 am on March 22, 2020:

Why is 2-seconds random delay useful here? It can accidentally prioritize inbound nodes over outbound :)

ajtowns commented at 3:33 am on March 24, 2020:

It already tests that process_time < best so the 2s delay will only bring a process time forward, not push it back past an inbound peer’s process time.

I think the scenario is likely to be:

bunch of peers announce a new tx, outbound connections get polled asap, inbound connections get polled after 2s
you pick one of those, likely an outbound, and send a GETDATA
other peers announce, get a 60s-64s delay
the early announcing peers get their process time rescheduled with an additional 60-64s delay
a NOTFOUND comes back, and you pick whoever’s delay was shortest; if it came back in under 2s, you’ll next ask an inbound connection who announced early; if it came back later, you’ll probably ask an outbound connection who announced after you’d sent the first GETDATA

Which is to say, I don’t think we’re consistently prioritising outbounds after the first request anyway.

I don’t have a good rationale for the 2s random delay; instantly going from a NOTFOUND from one peer to a GETDATA to another peer seems risky to me though.

naumenkogs commented at 1:52 pm on March 24, 2020:

Yeah, I was a bit confused, now what’s on your mind makes sense to me. I’m probably fine with leaving your 2s random delay.

Which is to say, I don’t think we’re consistently prioritising outbounds after the first request anyway.

I think we do? That’s probably off-topic here, although i’d be curious why you think so.

in src/net_processing.cpp:744 in a204d1586c

739+{
740+    CNodeState::TxDownloadState* best_d = nullptr;
741+    std::chrono::microseconds best;
742+
743+    for (auto& el : mapNodeState) {
744+         CNodeState::TxDownloadState* d = &el.second.m_tx_download;

naumenkogs commented at 2:03 pm on March 24, 2020:

Could we come up with a better variable name? :( Maybe “tx_communication” or something. Yeah it sucks, but probably nothing is worse than 1-letter name for something meaningful like this.

naumenkogs commented at 2:04 pm on March 24, 2020:

also best->best_time

ariard commented at 5:24 am on March 25, 2020:

Or I renamed best to lower_process_time while reviewing, but I agree names can be more meaningful.

in src/net_processing.cpp:4079 in a204d1586c

4075@@ -4039,6 +4076,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
4076             // Erase this entry from tx_process_time (it may be added back for
4077             // processing at a later time, see below)
4078             tx_process_time.erase(tx_process_time.begin());
4079+            state.m_tx_download.m_tx_announced[txid] = std::chrono::microseconds::zero();

naumenkogs commented at 2:48 pm on March 24, 2020:

I think this line handles a tricky corner case. Can we add comment to reduce the cognitive burned on code reviewer? What I think happens is (correct me if I’m wrong):

request from X1, not found
request from X2, ignores us for 1 minute
request from X3, notfound
this line prevents us from querying X2 again (we would if this line is deleted)

Now that I wrote this, one could simply check the presence of m_tx_in_flight for that peer, and if it is in flight, not consider for re-query?

ajtowns commented at 5:26 am on March 31, 2020:

I think the idea was to stop you from doing INV x; NOTFOUND x; INV x repeatedly and making it harder for you to query other peers. But I think this introduces a bug – the zero entry never gets cleared from m_tx_announced, because that’s only cleared based on what’s in in_flight and process_time and the txid isn’t in either of those anymore.

in src/net_processing.cpp:743 in a204d1586c

734@@ -731,6 +735,37 @@ std::chrono::microseconds CalculateTxGetDataTime(const uint256& txid, std::chron
735     return process_time;
736 }
737 
738+static void RetryProcessTx(CConnman& connman, const uint256& txid, const std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
739+{
740+    CNodeState::TxDownloadState* best_d = nullptr;
741+    std::chrono::microseconds best;
742+
743+    for (auto& el : mapNodeState) {

ariard commented at 6:13 am on March 25, 2020:

Did you consider to filter by nPreferredDownload to favor outbound peers and lower DoS risk? Only querying outbound peers should be good enough to achieve goal of finding NOTFOUND transactions but even if we don’t success due to bad-connectivity of our outbounds, worst-case scenario we hit the 1-min window (and a transaction not being announced by our outbounds is less likely to be a honest announcement?)

jnewbery commented at 10:21 pm on March 25, 2020:

I think it’d be even better to change the ‘best’ definition to prefer outbound peers where we can, but fall back to inbound peers if there are no outbound peers that have announced the tx.

ajtowns commented at 5:21 am on March 31, 2020:

We already prefer outbound peers when we bump the process time (in CalculateTxGetDataTime()), and this chooses the lowest process time which will reflect that preference. I don’t think it makes sense to complicate this further unless someone wants to do some real world testing on how well/badly the process time preferencing works in practice when the first thing we try results in a notfound.

ariard commented at 6:22 am on March 25, 2020: member

Maybe commit message can be clearer and points to issue solved :

“Removing mapRelay would be a direct privacy improvement, but it may turn small-mempool peers as Dosers. If announced transactions are dropped from mempools and aren’t available anymore in mapRelay, requesters will keep sending GETDATA until download expiration. By retrying requesting NOTFOUND transactions with different peers we avoid this issue. Note a dishonest peer can still withhold NOTFOUND to trigger same behavior from requester.”?

in src/net_processing.cpp:741 in a204d1586c

734@@ -731,6 +735,37 @@ std::chrono::microseconds CalculateTxGetDataTime(const uint256& txid, std::chron
735     return process_time;
736 }
737 
738+static void RetryProcessTx(CConnman& connman, const uint256& txid, const std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
739+{
740+    CNodeState::TxDownloadState* best_d = nullptr;
741+    std::chrono::microseconds best;

jnewbery commented at 1:41 pm on March 25, 2020:

I get a compiler warning that best might be used before initialization:

0net_processing.cpp: In function ‘bool ProcessMessage(CNode*, const string&, CDataStream&, int64_t, const CChainParams&, CConnman*, BanMan*, const std::atomic<bool>&)’:
1net_processing.cpp:756:27: warning: ‘best’ may be used uninitialized in this function [-Wmaybe-uninitialized]
2     if (best_d != nullptr && process_time < best) {
3         ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
4net_processing.cpp:741:31: note: ‘best’ was declared here
5     std::chrono::microseconds best;
6                               ^~~~

The logic below means that this can’t be read before set, but I don’t see any harm in initializing to 0 or std::chrono::microseconds::max().

in src/net_processing.cpp:352 in a204d1586c

345@@ -344,8 +346,10 @@ struct CNodeState {
346          */
347         std::multimap<std::chrono::microseconds, uint256> m_tx_process_time;
348 
349-        //! Store all the transactions a peer has recently announced
350-        std::set<uint256> m_tx_announced;
351+        /* Store all the transactions a peer has recently announced,
352+         * along with their process time
353+         */
354+        std::map<uint256, std::chrono::microseconds> m_tx_announced;

jnewbery commented at 1:43 pm on March 25, 2020:

This can be an unordered map for average constant-time lookup:

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 3373f7f54..2e097d44e 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -30,6 +30,7 @@
 5 
 6 #include <memory>
 7 #include <typeinfo>
 8+#include <unordered_map>
 9 
10 #if defined(NDEBUG)
11 # error "Bitcoin cannot be compiled without assertions."
12@@ -204,6 +205,11 @@ namespace {
13     static std::vector<std::pair<uint256, CTransactionRef>> vExtraTxnForCompact GUARDED_BY(g_cs_orphans);
14 } // namespace
15 
16+struct TxHasher
17+{
18+    size_t operator()(const uint256& hash) const { return ReadLE64(hash.begin()); }
19+};
20+
21 namespace {
22 /**
23  * Maintain validation-specific state about nodes, protected by cs_main, instead
24@@ -349,7 +355,7 @@ struct CNodeState {
25         /* Store all the transactions a peer has recently announced,
26          * along with their process time
27          */
28-        std::map<uint256, std::chrono::microseconds> m_tx_announced;
29+        std::unordered_map<uint256, std::chrono::microseconds, TxHasher> m_tx_announced;
30 
31         //! Store transactions which were requested by us, with timestamp

ajtowns commented at 5:35 am on March 26, 2020:

I think you’d need that to remain constant/log time in worst-case/attack scenarios because the announced hash is completely under the control of an attacker.

in src/net_processing.cpp:350 in a204d1586c

345@@ -344,8 +346,10 @@ struct CNodeState {
346          */
347         std::multimap<std::chrono::microseconds, uint256> m_tx_process_time;
348 
349-        //! Store all the transactions a peer has recently announced
350-        std::set<uint256> m_tx_announced;
351+        /* Store all the transactions a peer has recently announced,
352+         * along with their process time

jnewbery commented at 2:11 pm on March 25, 2020:

I think this comment could be slightly improved. The time is set to:

std::chrono::microseconds::zero() if the transaction has been requested from this peer (ie exists in m_tx_in_flight)
the process time if the transaction has not yet been requested from this peer (ie exists in m_tx_process_time)

in src/net_processing.cpp:3259 in a204d1586c

3256@@ -3222,6 +3257,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
3257         std::vector<CInv> vInv;
3258         vRecv >> vInv;
3259         if (vInv.size() <= MAX_PEER_TX_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER) {

jnewbery commented at 2:51 pm on March 25, 2020:

Did you consider moving all of the NOTFOUND processing to its own function ProcessNotFound()? Splitting the logic between here and RetryProcessTx() makes it more difficult to follow than if it was all in one place.

in src/net_processing.cpp:760 in a204d1586c

755+    std::chrono::microseconds process_time = current_time + GetRandMicros(MAX_NOTFOUND_RETRY_RANDOM_DELAY);
756+    if (best_d != nullptr && process_time < best) {
757+        auto end = best_d->m_tx_process_time.end();
758+        for (auto it = best_d->m_tx_process_time.lower_bound(best); it != end && it->first == best; ++it) {
759+            if (it->second == txid) {
760+                best_d->m_tx_process_time.erase(it);

jnewbery commented at 3:02 pm on March 25, 2020:

Maybe I’m misunderstanding the logic here, but this will bring the NodeState’s m_tx_process_time forward (to now + ~1second), but does nothing to the global g_already_asked_for time, which means that next time we go round SendMessages(), we won’t actually rerequest the transaction, since last_request_time > current_time - GETDATA_TX_INTERVAL. Am I missing something?

If I’m right, I think we need to do something like reset g_already_asked_for to one minute ago.

in src/net_processing.cpp:3272 in a204d1586c

3269@@ -3234,6 +3270,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
3270                     }
3271                     state->m_tx_download.m_tx_in_flight.erase(in_flight_it);
3272                     state->m_tx_download.m_tx_announced.erase(inv.hash);

jnewbery commented at 3:06 pm on March 25, 2020:

Now that we’re potentially rerequesting txs from different peers after receiving a NOTFOUND, I think we shouldn’t erase them from a peer’s m_tx_in_flight and m_tx_announced structures in receipt of a NOTFOUND, and keep them there until the TX_EXPIRY_INTERVAL in SendMessages(), otherwise there might be an attack where an adversary has multiple connections to you, and then juggles you between them by sending an INV for a transaction, holding on to the GETDATA, and then sending a NOTFOUND immediately followed by an INV reannouncing the same tx.

ajtowns commented at 2:36 pm on March 31, 2020:

Hmm, that means a peer that sends a NOTFOUND is treated exactly the same as one that just ignores the request; so NOTFOUND is just a “fyi, I’m never going to respond, so feel free to try someone else”. That… seems like it makes a lot of sense? If we were to ever start punishing peers for (frequently) not responding to tx requests, maybe it would make sense to punish them for (equally frequently) sending notfounds too?

jnewbery commented at 3:09 pm on March 25, 2020: member

Concept ACK, but lots of questions.

My main concern is that TxDownloadState is becoming more complex, with data duplicated between member variables and only certain combinations being valid states. To reduce cognitive overload and risk of bugs, perhaps it should turned into a class with a well-defined interface for callers.

fjahr commented at 4:38 pm on March 25, 2020: member

Concept ACK but echoing jnewbery’s feedback I also still have questions and agree with most of his code comments. I also think that code is currently not tested, so adding an explicit test would be very valuable. Additionally, comments in the RetryProcessTx code would be helpful for understanding.

nit on the commit message: s/find one who doesn’t already have its inflight/find all who don’t already have their inflight/

in src/net_processing.cpp:748 in a204d1586c

743+    for (auto& el : mapNodeState) {
744+         CNodeState::TxDownloadState* d = &el.second.m_tx_download;
745+         if (d->m_tx_in_flight.size() >= MAX_PEER_TX_IN_FLIGHT) continue;
746+         auto it = d->m_tx_announced.find(txid);
747+         if (it != d->m_tx_announced.end()) {
748+             if (best_d == nullptr || (it->second != std::chrono::microseconds::zero() && it->second < best)) {

jkczyz commented at 4:48 pm on March 25, 2020:

This condition could probably be simplified considerably if (1) best is initialized appropriately and (2) the entry for the txid in m_tx_announced is removed on line 4079 rather than zeroed (like what is done on line 3272). Was zeroing chosen on line 4079 for a specific reason?

naumenkogs commented at 4:57 pm on March 25, 2020:

My guess for the reason is here :)

jkczyz commented at 4:50 pm on March 25, 2020: contributor

Concept ACK

in src/net_processing.cpp:349 in a204d1586c

345@@ -344,8 +346,10 @@ struct CNodeState {
346          */
347         std::multimap<std::chrono::microseconds, uint256> m_tx_process_time;
348 
349-        //! Store all the transactions a peer has recently announced
350-        std::set<uint256> m_tx_announced;
351+        /* Store all the transactions a peer has recently announced,

jnewbery commented at 10:36 pm on March 25, 2020:

We discussed this in PR Review Club (https://bitcoincore.reviews/18238.html#l-115 and https://bitcoincore.reviews/18238.html#l-180), and there was some agreement that even for a small struct like this, the dependencies between the fields make it difficult to reason about and potentially bug-prone.

I think from a high level, in the future we might want a TxDownloadState class which contains:

{txid, timestamp, state} objects where
- state can be ‘announced’ or ‘requested’
- timestamp refers to ‘when to request’ if state is announced, and ‘when requested’ if state is requested
a way to lookup the object by txid
a way to iterate through the ‘announced’ objects sorted by timestamp
a way to iterate through the ‘requested’ objects (doesn’t necessarily need to be sorted by timestamp since there can only be MAX_PEER_TX_IN_FLIGHT = 100 of them)
public functions to add, remove, refresh timestamps, etc

ajtowns commented at 5:00 am on March 26, 2020:

I’m +1 on this, but maybe it’d be better to get wtxid relay #18044 in first?

jnewbery commented at 1:55 am on March 31, 2020:

I’ve had a first attempt at this: https://github.com/jnewbery/bitcoin/tree/2020-03-tx-download-class

No tests or comments yet, and it could be tidied up, but do you think this is heading in the right direction?

ajtowns commented at 2:39 pm on March 31, 2020:

I was having a go at it too when I saw your comment, https://github.com/ajtowns/bitcoin/commits/202002-bump-notfound-wip – separates out the current code into the class first, before changing it, which I think works better. Might be good to have some comments/style nits on the refactor?

in src/net_processing.cpp:78 in a204d1586c

74@@ -75,6 +75,8 @@ static constexpr std::chrono::microseconds INBOUND_PEER_TX_DELAY{std::chrono::se
75 static constexpr std::chrono::microseconds GETDATA_TX_INTERVAL{std::chrono::seconds{60}};
76 /** Maximum delay (in microseconds) for transaction requests to avoid biasing some peers over others. */
77 static constexpr std::chrono::microseconds MAX_GETDATA_RANDOM_DELAY{std::chrono::seconds{2}};
78+/** Delay between receiving a NOTFOUND and trying the next peer. */

rebroad commented at 6:27 am on March 30, 2020:

why have any delay?

naumenkogs commented at 2:23 pm on March 30, 2020:

We have some discussion here

ajtowns referenced this in commit 86284f69e8 on Mar 31, 2020

DrahtBot commented at 8:36 pm on June 4, 2020: member

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

DrahtBot added the label Needs rebase on Jun 4, 2020

ajtowns commented at 3:37 am on June 15, 2020: member

Obsoleted by #19184

ajtowns closed this on Jun 15, 2020

fanquake referenced this in commit 5550fa5983 on Jul 14, 2020

sidhujag referenced this in commit 0090059f60 on Jul 14, 2020

DrahtBot locked this on Feb 15, 2022

net_processing: Retry notfounds with more urgency #18238

Conflicts