Request NOTFOUND transactions immediately from other outbound peers, when possible (p2p)

https://github.com/bitcoin/bitcoin/pulls/15505

Host: mzumsande  -  PR author: sdaftuar

Notes

  • The basic message flow for transaction relay is INV -> GETDATA -> TX. A node that cannot deliver a requested transaction answers the GETDATA request with NOTFOUND instead of TX.
  • NOTFOUND messages were introduced in Bitcoin Core PR 2192 and are documented in the Bitcoin developer reference.
  • Historically, Bitcoin Core would ignore NOTFOUND messages it received. This changed slightly with PR 15834, (Review Club Session), which introduced internal bookkeeping measures in response to a NOTFOUND (clearing the internal map of in-flight transactions).
  • This PR suggests to actually utilize the info of a NOTFOUND by immediately requesting the transaction from outbound peers having INV‘ed it to us before.
  • This PR touches the central P2P loop CConnman::ThreadMessageHandler() by adding a sequence counter to SendMessages(). This counter is used to deterministically select one outbound peer in each loop cycle from which we request transactions for which we have received a NOTFOUND from another peer.

Questions

  • What is the most typical situation when running a node in which NOTFOUND messages are encountered?
  • Why was the solution with the sequence counter chosen, instead of simpler alternatives like a random draw?
  • Why are only outbound peers considered for requesting transactions after receiving NOTFOUND?
  • What kind of options exist in the functional testing framework for P2P functionality? Why is it so hard to test PRs like this one with the existing framework?

Meeting Log

  113:00 <jnewbery_> hi
  213:00 <lightlike> hello
  313:00 <jkczyz> hi
  413:00 <michaelfolkson> Hey
  513:01 <davterra> hi
  613:01 <jnewbery_> lightlike is going to host today. He also prepared the notes and questions at https://bitcoin-core-review-club.github.io/15505.html . Thanks lightlike!
  713:01 <fjahr> hi
  813:01 <hugohn> hello
  913:01 <i2> hallo
 1013:01 <jonatack> hi++
 1113:01 <jnewbery_> before I hand over, my periodic reminder that the point of PR review club is to help you review PRs! Please do review the PRs on github (an if you have any questions about how to do that, feel free to ask them here)
 1213:02 <jnewbery_> ok, let's get started. Over to you, lightlike
 1313:02 <michaelfolkson> Thanks for the reminder John :)
 1413:02 <lightlike> Thanks! I found this PR interesting because it deals with some of the very basic P2P functionality (ThreadMessageHandler)
 1513:02 <lightlike> to optimize how we deal with NOTFOND message.
 1613:03 <ariard> hi
 1713:03 <lightlike> Even though it got closed by the author recently
 1813:03 <lightlike> I think it is good for learning about P2P aspects of bitcoin - definitely helped my understand some new things.
 1913:04 <lightlike> First question (some overlap with the old session on #15834):
 2013:04 <lightlike> What is the most typical situation when running a node in which NOTFOUND messages are encountered?
 2113:04 <jkczyz> When the peer no longer has the transaction in the relay set
 2213:04 <michaelfolkson> Unpruned node requesting historic transaction?
 2313:05 <michaelfolkson> *Pruned
 2413:06 <lightlike> In my experience of running a node with logging, it is dealing with orphan transactions:
 2513:06 <jnewbery_> michaelfolkson: pruning is only relevant for old blocks in the chain. This PR deals with tx relay
 2613:06 <jnewbery_> (unconfirmed txs)
 2713:07 <lightlike> when we receive an tx from a peer for which we dont have the parent, we will ask him for the parent too
 2813:07 <lightlike> sending GETDATA without him having sent an INV before.
 2913:07 <lightlike> *received
 3013:08 <michaelfolkson> But a pruned node might request a tx it no longer has?
 3113:08 <lightlike> in this case, the other node often doesn't have the parent tx (or won't send it to us) and answers with NOTFOUND.
 3213:09 <jnewbery_> michaelfolkson: pruning is only for the blockchain. tx relay is for unconfirmed txs not in the blockchain yet
 3313:09 <michaelfolkson> Ok thanks
 3413:09 <hugohn> how likely is it to receive a txn for which we don't know the parent? is that related to CPFP or no?
 3513:10 <i2> answer to typical situation for NOTFOUND: maybe: a.) blocks-only Lite client? b.) full-node that's in process of syncing to tip, c.) adversarial peer? d.) full node drops tx if tx is included in block but a reorg happens to your peer?
 3613:10 <lightlike> when firing up a node, it happens a lot (we weren't there when the parent was relayed)
 3713:13 <lightlike> i2: i dont think an adversarial peer would send a NOTFOUND currently. He could just INV a tx and never follow up with the TX if we request it.
 3813:13 <jnewbery_> i2: a) blocks-only clients will advertise that they're not relaying txs with the relay flag in the VERSION message: https://btcinformation.org/en/developer-reference#version
 3913:14 <jnewbery_> i2: d) in the case of a reorg, the disconnected txs are added back into the mempool
 4013:14 <hugohn> lightlike: I see, so when a node comes back online after a while, the INVs typically arrive out-of-order? i.e., receiving INVs for children first before receiving INVs for parent
 4113:14 <lightlike> im not entirely sure if this is typical, but in a short test-run on mainnet all of the NOTFOUNDs i got were for parents of orphan transactions.
 4213:15 <jnewbery_> lightlike: I think that's fairly typical, but I haven't actually tested it myself
 4313:15 <i2> jnewbery_ re d) so the steps are 1.) new block 2.) shed txns in block from mempool 3) reorg 4) put txn from reorg'ed blocks back into mempool?
 4413:16 <lightlike> ok, maybe move on to the next q:
 4513:16 <lightlike> Why was the solution with the sequence counter chosen, instead of simpler alternatives like a random draw?
 4613:16 <jonatack> lightlike: my testing of the NOTFOUND PR showed the same as you
 4713:16 <i2> lightlike: maybe a.) Decrease p2p churn? b.) Equalize load requested from peers (no peer wins the INV-request lottery)
 4813:17 <jkczyz> Is it to ensure a uniform distribution of peer selection? I left a comment on the review regarding the sequence number but wasn't sure if this was the reason.
 4913:17 <hugohn> I guess for robustness: we want to retry fetching the txn from all eligible outbound peers, not just one
 5013:17 <jnewbery_> i2: not quite. It's 1) get new block and see that it's got more work than tip 2) run ActivateBestChain, which tries to move to the best chain, which will 2a) rewind blocks, adding txs to mempool 2b) connect new blocks, removing txs from mempool that in the new block
 5113:19 <lightlike> yes, i think that's correct, it's to not ask more transansaction from peers earlier in the loop.
 5213:20 <michaelfolkson> How many peers is a node on average connected to? I'm assuming most would run the default config settings
 5313:20 <jnewbery_> hugohn: no, we only want to request from one peer at a time. That tripped me up too. The logic to make sure we're not requesting from more than one peer is in TryRequestTx(): https://github.com/bitcoin/bitcoin/pull/15505/files#diff-eff7adeaec73a769788bb78858815c91R773
 5413:21 <lightlike> i think it is important here that there is an "early" and "late" in the loop, even though ThreadMessageHandler loops continuously through all our peer.
 5513:21 <jnewbery_> michaelfolkson: up to 8 outbound and (for listening nodes) up to 125 total (so 117 inbound)
 5613:21 <hugohn> jnewbery_: yes I saw that, the retrying is actually one peer at a time. but the queuing up for retries happen for all outbound peers, right? to ensure that if one outbound peer fails to give us the txn, the next one still could
 5713:22 <lightlike> because the order of our peers is not random (usually there are first our outbound peers to which we connect at startup, and after that the inbound peers)
 5813:22 <jnewbery_> hugohn: right, the txid will be added to m_tx_not_found for all outbound peers that have announced the tx
 5913:22 <jonatack> perhaps a random draw would not ensure the taking of turns e.g. the same peer could be picked twice in a row
 6013:23 <michaelfolkson> Yeah especially if only 8 outbound
 6113:24 <lightlike> jonatack: i think with the random draw, the node in our current position in the loop would get more action: it has now an 1/N chance, and in case there is an entire cycle without anohter node getting the INV, it gets a second chance first
 6213:24 <i2> jonatack "peer could be picked twice in a row" = INV 'lottery' winner ;]
 6313:24 <hugohn> jonatack: yeah I think so, so if we randomly choose only one outbound peer to retry, it's less robust if that peer _also_ fails to SENDDATA . but the advantage of random draw is no extra sequence_number, which is somewhat awkward to introduce in the main processing loop...
 6413:24 <jonatack> lightlike: right, in that case random would have more bias
 6513:24 <jnewbery_> one thing we could do is shuffle the order we visit peers in the ThreadMessageHandler here: https://github.com/bitcoin/bitcoin/blob/7821821a23b68cc9ec49d69829ad4c939cb762e8/src/net.cpp#L1939
 6613:25 <lightlike> jnewbery_: yes, I thought about that too, that could be randomized
 6713:26 <jkczyz> jnewbery_: how important is maintaining determinism for sake of testing?
 6813:26 <lightlike> i actually like the proposal of jkczyz https://github.com/bitcoin/bitcoin/pull/15505#issuecomment-516636613 - on first thought I think that might work
 6913:26 <jnewbery_> jkczyz: I don't think it's important. My guess is that no tests will break.
 7013:26 <hugohn> but one consequence of queuing up the txns in all outbound peers is there could potentially be duplicate entries in each m_tx_not_found queue? I don't see any logic checking against with duplicates
 7113:27 <hugohn> do you guys think it's necessary? to check against duplicates in m_tx_not_found?
 7213:28 <jnewbery_> hugohn: I think you may be right about duplicates in m_tx_not_found
 7313:30 <lightlike> ok, next question: Why are only outbound peers considered for requesting transactions after receiving NOTFOUND?
 7413:31 <lightlike> hugohn: I think so too, not sure thought how bad duplicates would be.
 7513:32 <i2> clarify my ignorance: outbound peers = non-listen-only peers. yes?
 7613:32 <hugohn> jnewbery_ lightlike: yeah so looks like we indiscriminately queue up the txn any time we get a NOTFOUND. there's no guarantee we can't get multiple NOTFOUNDs for the same txns.
 7713:32 <hugohn> or back-to-back
 7813:32 <hugohn> retry failure
 7913:32 <lightlike> i2: no, outbound peers are when we initiate the connection, vs inbound peers that connect to us
 8013:32 <michaelfolkson> https://bitcoin.stackexchange.com/questions/42286/what-is-the-difference-between-inbound-and-outbound-connections
 8113:33 <michaelfolkson> I made same mistake
 8213:34 <michaelfolkson> Because you would rather cycle through nodes you have personally selected rather than random nodes that you haven't selected?
 8313:35 <lightlike> michaelfolkson: yes, that's true, but why not just treat everyone the same?
 8413:35 <hugohn> we generally trust the outbound peers more than inbound ones, to guard against Sybil attacks?
 8513:36 <michaelfolkson> Because nodes aren't the same. Some will go offline and won't make timely responses?
 8613:36 <lightlike> hugohn: yes. I think if we also requested from inbounds, attackers could try to withhold a tx from us by establishing many connections to us
 8713:37 <lightlike> the normal logic if a GETDATA times out also treats outbounds nodes preferentially
 8813:37 <jonatack> yes, idea is attackers could use inbound conns to prevent us from seeing a txn
 8913:38 <jonatack> thus the delay placed on inbounds giving preferance to outbounds
 9013:38 <lightlike> so an attacker could circumvent that by sending us NOTFOUND right before the GETDATA times out, and then we would be likely to ask the tx from another inbound node, that is also controlled by the attacker.
 9113:38 <jonatack> nNow + 2 sec
 9213:39 <lightlike> and so on
 9313:40 <i2> so rational for txn requests to outbound peers before inbound is to prevent an eclipse attack on a node for txn awareness (say lighnting force close)?
 9413:41 <lightlike> i2: yes, I think so. it's much harder for an attacker to control our outbound peers that we chose carefully.
 9513:42 <hugohn> the time delay attack was the motivation for the fix https://github.com/bitcoin/bitcoin/pull/14897 wasn't it? which introduced a few bugs & motivated more robustness handling of NOTFOUND
 9613:43 <jonatack> hugohn: yes, iirc
 9713:43 <hugohn> retry logic is hard :-)
 9813:44 <hugohn> so many edge cases
 9913:44 <lightlike> ok, final question:
10013:44 <lightlike> What kind of options exist in the functional testing framework for P2P functionality? Why is it so hard to test PRs like this one with the existing framework?
10113:45 <jonatack> Good read on this at https://github.com/bitcoin/bitcoin/issues/14210
10213:46 <jonatack> (current limitations and goal of an instantiable CConnman, then a MockNetworkRouter)
10313:48 <lightlike> jonatack: yes, seems like it is not possible at the moment to fully mock inbound/outbound behavior in functional tests.
10413:48 <ariard> We currently dissociate the node added from cli and from addrman
10513:49 <jonatack> and the testing can do manual conns but not spontanous ones
10613:49 <jnewbery_> IIRC there's also special logic for peers on the same subnet or IP address, which all our test nodes are for functional tests
10713:50 <jnewbery_> yeah, I think it's the IsLocal() test
10813:50 <hugohn> is the bucketing mechanism (putting nodes in new vs. tried tables) one of the main challenges? do you have to stub out the bucketing logic somehow, or create fake IP addresses?
10913:51 <jnewbery_> there are a few things that would need to change. I think they're all documented in 14210
11013:51 <ariard> I think we should create fake IP addresses for our nodes thanks to namespaces
11113:52 <jnewbery_> ariard: I don't know anything about that, but it sounds like a good idea!
11213:52 <jonatack> Ethan Deilman proposed that here https://github.com/bitcoin/bitcoin/issues/14210#issuecomment-469318300
11313:53 <jonatack> * Heilman
11413:53 <ariard> jnewbery: how IIRC that's why we conclude with dongcarl in 14210
11513:54 <jonatack> ariard: right, namespaces as you proposed https://github.com/bitcoin/bitcoin/issues/14210#issuecomment-473361633
11613:55 <ariard> not sure about all this stuff, it need more thought but first cconman encapsulation!
11713:55 <ariard> *needs
11813:55 <lightlike> do you know if the work towards an instantiable CConnman is still in an early stage, or close to being finished?
11913:55 <jonatack> before we run out of time... is anyone working on this atm?
12013:55 <hugohn> all roads point to more modular components :-)
12113:55 <ariard> I do what I can https://github.com/bitcoin/bitcoin/pull/16503 ;)
12213:56 <ariard> jonatack: dongcarl, theuni, ryanofksy
12313:56 <hugohn> ariard: yes moar pls ! :D
12413:56 <ariard> but it split between different parts of the codebase
12513:57 <jonatack> ariard: currently?
12613:57 <ariard> we'll try to remove more cconnman usages in RPC interface, don't remember what need to be done in net.cpp
12713:57 <ariard> *will
12813:57 <hugohn> also a basic q before we run out: does anyone know why g_outbound_peers_with_protect_from_disconnect is defined within net_processing.cpp’s namespace, but g_outbound_peers is defined outside of it?
12913:58 <ariard> jonatack: dunno rn, ask on core dev
13013:58 <lightlike> there is #14856, where there was not so much action recently
13113:58 <lightlike> https://github.com/bitcoin/bitcoin/pull/14856
13213:59 <ariard> lightlike: ping carl on it, he may need help
13313:59 <jnewbery_> hugohn: no idea!
13413:59 <hugohn> ha
13513:59 <jonatack> thanks, interesting. we need to be reviewing these PRs to encourage people
13613:59 <jnewbery_> Shall we wrap up there?
13714:00 <lightlike> yes, thanks everybody!
13814:00 <i2> thanks lightlike!
13914:00 <jnewbery_> that was great. Thanks so much for hosting and preparing notes lightlike!
14014:00 <jonatack> great jon, lightlike :100: would attend again
14114:00 <hugohn> thanks lightlike! learned a lot!
14214:00 <ariard> thanks lightlike!
14314:00 <jkczyz> yeah, thank you!
14414:01 <jonatack> * job :-)
14514:01 <jnewbery_> Before we go: next week PR review is looking at *two* competing PRs! https://bitcoin-core-review-club.github.io/16345.html
14614:01 <jnewbery_> notes are already up
14714:02 <jonatack> nice