Add 2 outbound blocks-only connections (p2p)

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

Host: jnewbery  -  PR author: sdaftuar

Notes

  • The Bitcoin P2P network serves 3 purposes:
    • relaying unconfirmed transactions
    • propagating blocks
    • gossiping addresses of known reachable nodes on the network
  • (see the btcinformation developer docs for documentation on P2P messages.)
  • Although these three purposes share the same network, they have different design goals and properties. For example, transaction relay is optimized for a combination of redundancy/robustness as well as bandwidth minimization, while block relay is optimized to minimize delay.
  • There’s no reason these different purposes couldn’t be split onto different networks, and in fact there already are specialized networks for block propagation such as FIBRE and Blockstream satellite, and for transaction relay such as TxTenna.
  • Currently Bitcoin Core treats all peers as full network peers (tx, block and addr), unless you start your node in ‘blocksonly’ mode, in which case all peers are block/addr peers.
  • The defaults for numbers of connections are 8 outbound and up to 125 total (ie up to 117 inbound connections if the node is a ‘listening’ node and is reachable).
  • This PR proposes adding 2 additional blocks-only nodes, which would not relay txs or gossip addrs. The default connections would therefore be 8 full outbound, 2 blocks-only outbound, and up to 115 inbound.
  • There are two main data structures that handle peer state: CNode (defined in net.h and covered by cs_vNodes) and CNodeState (defined in netprocessing.cpp and covered by cs_main).
  • Roughly speaking, CNode is concerned with the connection to the peer, and CNodeState is concerned with application state of the peer. However, there is still some application state contained in CNode for historic reasons. This should be moved out of CNode eventually.
  • An example of the application state data that is contained in CNode is the inventory data protected by cs_inventory (see https://github.com/bitcoin/bitcoin/blob/adff8fe32101b2c007a85415c3ec565a7f137252/src/net.h#L716).
  • One change in this PR is to move the tx relay inventory state into a separate structure (TxRelay) and only initializing it if the peer is a tx relay peer.

Questions

  • What problem is this PR addressing?
  • Why is the tx relay inventory state moved into its own structure?
  • How do we signal a ‘blocks-only’ connection on the P2P network?
  • Why is the P2P behaviour of this PR difficult to test in the functional test framework?
  • What does aj point out in his review comment that all other reviewers missed?
  • How should we handle peers that send us tx data after we’ve requested a blocks-only connection?

Meeting Log

  113:00 <jnewbery> hi
  213:00 <dergigi> hi
  313:00 <marcinja_> hi
  413:00 <ccdle12> hi
  513:00 <jkczyz> hi
  613:00 <lightlike> hi
  713:00 <michaelfolkson> Hey
  813:01 <afigs> sup
  913:01 <nehan> hi
 1013:01 <jnewbery> before we start, a couple of reminders:
 1113:01 <jonatack> hi
 1213:01 <jnewbery> - I don't have any more PRs lined up on the agenda. If you have any that you want to request, please comment in https://github.com/bitcoin-core-review-club/bitcoin-core-review-club.github.io/issues/14
 1313:02 <jnewbery> - weekly reminder that PR review club exists to help you get reviewing on github. I encourage you all to leave review/test comments on the actual PR!
 1413:03 <jnewbery> if you have any questions or thoughts about leaving review comments on PRs, they're all on topic in this channel
 1513:03 <jnewbery> ok, let's get started. What did you all think about the PR this week? Who had a chance to review/test?
 1613:04 <michaelfolkson> On that I was thinking of maybe starting this 15 minutes early each week for those who want to chat about basic stuff without wasting John's time
 1713:04 <jnewbery> no, basic stuff is on topic
 1813:04 <jnewbery> please feel free to ask during the meeting
 1913:04 <jnewbery> IRC conversations can be multi-threaded!
 2013:04 <jonatack> built, ran tests, started to review and grep code
 2113:04 <michaelfolkson> Ok
 2213:05 <ariard> had a chance to review, like weeks ago
 2313:05 <lightlike> i tested this a couple of weeks ago, didnn't test the newest fixes.
 2413:05 <jkczyz> Read through comments and code yesterday/today. Added some minor comment this morning.
 2513:05 <jnewbery> I see review comments from ariard, lighlike and jkczyz. Great stuff!
 2613:06 <jnewbery> Any questions about the notes from https://bitcoin-core-review-club.github.io/15759.html ? Anything unclear?
 2713:06 <jnewbery> And does anyone want to have a go at the first question "What problem is this PR addressing?
 2813:06 <jonatack> This PR proposes to mitigate the risk of eclipse attacks by separating block
 2913:07 <jonatack> relay from transaction relay to occlude the network topology
 3013:07 <dergigi> I built it and looked through the code and the PR; don't have any comments really, still have a lot to learn and wrap my head around; wasn't sure how to test it properly; network tests from PR 14210 would be great I guess
 3113:07 <jonatack> (that was the easiest question... I *think* :D)
 3213:08 <jnewbery> dergigi: yeah, our network testing in the functional test suite is a bit deficient. It'd be nice to be able to test outbound connections
 3313:08 <jnewbery> jonatack: yeah, that's right
 3413:09 <dergigi> This PR relates to the Erebus Attack as well, right? At least it blocks/conflicts with PR 16702 which implements one of the proposed countermeasures?
 3513:09 <jnewbery> anyone want to expand on the motivation, or shall we move onto the next question?
 3613:09 <michaelfolkson> Basic question on the peer management. You initially connect to the DNS seeds but then these connections are dropped and that's where you're at risk of eclipse attack (assuming you haven't manually connected to a peer that you trust)?
 3713:10 <provoostenator> michaelfolkson: you're at risk with or without the DNS seeds
 3813:10 <michaelfolkson> You only need one peer to serve you correct transactions/blocks and you are able to detect attack
 3913:11 <jnewbery> dergigi: I haven't read the Erebus paper, but this PR was opened before that paper was released I believe
 4013:11 <provoostenator> You won't necessarily detect the attack, but you'll get the proper chain.
 4113:11 <provoostenator> The DNS seeds are used to get an initial set of IP addresses to try, you then contact those for more, etc.
 4213:11 <jnewbery> I don't think this PR is directly concerned with Erebus. PRs that change P2P code often conflict
 4313:11 <michaelfolkson> <provoostenator>: But if you maintained your connection to the DNS seeds then the transactions/blocks they provide would be different to the attacker's?
 4413:11 <lightlike> I found it interesting that fixing the root cause of topology inference (the way orphan txs are handled) is seen a "lost cause".
 4513:12 <provoostenator> michaelfolkson: you don't maintain that connection; you only poll them once and only if you can't find a peer by yourself quickly enough (11 seconds from boot)
 4613:12 <ariard> lightlike: you think better management of tx dependencies and their conflicts between peers could solve it ?
 4713:13 <jnewbery> lightlike: I think there's a tradeoff between bandwidth usage and privacy. If you're only connecting to a subset of the network and relaying txs to them, then that leaks information about your topology
 4813:13 <michaelfolkson> <provoostenator>: What's to stop me manually maintaining the connection to them?
 4913:13 <lightlike> ariard: I don't know. The author doesn't believe in that: https://github.com/bitcoin/bitcoin/pull/15759#issuecomment-480398802
 5013:14 <provoostenator> michaelfolkson: when your node starts it checks peers.dat for a list of peers it already knows about and tries to connect to some of them
 5113:14 <provoostenator> Only when that fails does it call the DNS seeds.
 5213:14 <provoostenator> michaelfolkson: DNS seeds are not nodes, the only thing they do is return a list of IP addresses.
 5313:14 <michaelfolkson> I want to maintain a connection to at least one peer I trust and then I don't have to worry about eclipse attacks
 5413:15 <jnewbery> michaelfolkson: use addnode=<address> to your config
 5513:15 <provoostenator> Try this:
 5613:15 <provoostenator> dig -t A seed.bitcoin.sprovoost.nl
 5713:15 <jnewbery> ok, question two: Why is the tx relay inventory state moved into its own structure?
 5813:15 <provoostenator> DNS uses port 53 and has a completely different protocol than the Bitcoin P2P network (port 8333)
 5913:15 <michaelfolkson> Ah thanks
 6013:16 <jkczyz> To separate state that is not needed for blocks-only peers
 6113:16 <provoostenator> "I don't have to worry about eclipse attacks" - well, then *that* peer has to worry
 6213:16 <ariard> lightlike: was thinking about this yesterday and intuitively would say as tx conflicts are solved by blocks you can always provoke conflicts between all you peers as it's unconfirmed data
 6313:16 <jnewbery> jkczyz: correct. Why?
 6413:17 <ariard> so patching all heuristics beyond TxProbe seems it to be really hard
 6513:17 <michaelfolkson> <provoostenator> Maybe those people named on that DNS seed list have a better understanding of the network than me ;)
 6613:17 <jkczyz> jnewbery: the state is not applicable to them since they are not relaying transactions
 6713:17 <provoostenator> I'm just good at copy-pasting. And actually had to fix a bunch of issues of time.
 6813:17 <jnewbery> that's right, but why not just not use that state?
 6913:18 <ariard> michaelfolkson: you should have multiple source of blocks beyond your ISP if so
 7013:18 <jkczyz> jnewbery: not sure if I parsed that one correctly
 7113:18 <jnewbery> why not just not initialize that state and don't use it?
 7213:19 <ariard> reduce memory comsumption
 7313:19 <jnewbery> why go to the trouble of defining a structure and adding a unique_ptr
 7413:19 <jkczyz> Ah, yes memory and because it is used to determine if it is blocks-only
 7513:19 <jnewbery> correct: the justification was to reduce memory consumption (sorry jkczyz - maybe that was just too obvious!)
 7613:20 <jnewbery> Does the tx relay state require a lot of memory? If so, where?
 7713:20 <jkczyz> jnewbery: haha, np. I also found it a bit disconcerting that CNode has over 60 member variables
 7813:21 <lightlike> ariard: yes, but do these conflicts necessarily have to lead to an attacker learning the entire network topology as described in the txprobe paper?
 7913:21 <ariard> tx relay state requires memory for inv of txid to send
 8013:21 <nehan> jnewbery: does it have anything to do with the bloom filter?
 8113:22 <aj> ariard: but if you don't send inv's that would just be an empty set, which shouldn't take up much memory?
 8213:23 <jnewbery> nehan: yes. It's the bloom filters that are expensive. I think without those, it wouldn't really be worth changing this (for memory saving - it might be worth it to make the code clearer)
 8313:23 <jnewbery> There are a couple of bloom filters used by CNode. What are they and what are they for?
 8413:24 <nehan> jnewbery: it's unclear to my why an unused bloom filter should take up a lot ofmemory
 8513:24 <ariard> aj: oh yes, assuming we don't fulfill them by mistake in some part of the code
 8613:24 <jnewbery> ariard: the invs are stored in a std::set, so if no invs are ever put in there, then it's not very expensive
 8713:25 <nehan> jnewbery: i guess it's this: vData(std::min((unsigned int)(-1 / LN2SQUARED * nElements * log(nFPRate)), MAX_BLOOM_FILTER_SIZE * 8) / 8),
 8813:25 <nehan> jnewbery: but another option would be not to initialize until use
 8913:25 <jnewbery> nehan: that's it
 9013:26 <jnewbery> nehan: I guess you could do that, but then I think you'd need to update all the sites that initialize and use bloom filters
 9113:27 <ariard> lightlike: don't know enough about mempool, but would say if peers have different tx dependcy graph, it's going to modify the way they relay tx (you may have other heuristics not yet discovered using RBF or CPFP)
 9213:27 <jnewbery> ok, so the PR moves the (potentially memory expensive) tx inventory state to its own struct, and only initializes that if the connection relays txs
 9313:27 <aj> i don't think our bloom filters are easy to reinitialise; if you've got a unique_ptr like pfilter you can; but if you have it in place you define the number of elements and hence memory usage at declaration
 9413:27 <ariard> would be interesting to dig in :)
 9513:27 <jnewbery> One thing we need to be careful of when making changes to defaults like this is that it doesn't adversely affect memory usage
 9613:28 <aj> (also the idea is to eventually have 8 of these instead of just 2, so the cheaper the better)
 9713:28 <jnewbery> Suhas justifies being able to add these additional peers because it doesn't cause memory usage to increase too much for the average node
 9813:28 <nehan> aj: initializing the memory on first use != reinitializing. but this is quibbling.
 9913:29 <jnewbery> any other questions on bloom filters, data structures or memory?
10013:29 <jnewbery> Third question: How do we signal a ‘blocks-only’ connection on the P2P network?
10113:29 <michaelfolkson> <aj>: I thought the simulation discussion was exploring how many additional outbound connections you should have?
10213:30 <jnewbery> hint: it's in the VERSION message
10313:32 <jnewbery> psst: it's defined in BIP 37
10413:32 <aj> nehan: when initialising the object it allocates the memory, with nElements = 50k; would have to conditionalise that or something to be able to save memory
10513:32 <jnewbery> There's an optional `relay` boolean at the end of the VERSION message
10613:33 <jnewbery> https://btcinformation.org/en/developer-reference#version
10713:33 <jnewbery> if relay is set to False, the peer shouldn't relay txs to you
10813:33 <jnewbery> next question: What does aj point out in his review comment that all other reviewers missed?
10913:33 <nehan> jnewbery: are people ok with implicitly using a null object pointer as the indicator of whether or not the node supports tx relay?
11013:34 <jnewbery> what do you mean about a null object pointer? That it's a 0x00 byte or something more than that?
11113:35 <lightlike> jnewbery: that some peers might ignore our version message and still send us INVs, in which we case we would have sent TXs even if the connection is blocks-only
11213:35 <nehan> jnewbery: nevermind, my question might not make sense i'll reask later if it does.
11313:35 <aj> michaelfolkson: the simulation looks at how many outbounds resulted in the p2p network remaining connected if you assume all the other connections everyone has are useless (because the tx stuff has allowed them to be attacked, maybe)
11413:36 <jnewbery> lightlike: exactly. There are peers on the network that ignore `relay`=False and will still send us tx INVs and TX messages
11513:36 <ariard> spy nodes or buggy ones ?
11613:36 <aj> nehan: (unique_ptr<foo> or optional<foo> are two ways to have a foo or not, but they have different memory usage when you don't have the foo)
11713:37 <jnewbery> and the new code didn't expect that, so after we receive tx INVs from those peers, we'll send them tx GETDATAs
11813:37 <lightlike> aj: did you spot this by testing in the wild, or by thinking about the PR?
11913:37 <jnewbery> ariard: that's a good question. I spent some time looking into it, and I believe they're spy nodes
12013:37 <jnewbery> because they're claiming to be v0.18 Bitcoin Core nodes, and I didn't find a bug in our VERSION relay handling
12113:38 <aj> lightlike: i thought about it then though "no, suhas wouldn't get that wrong" so ignore it; then tried it live on mainnet and about the third run saw it
12213:38 <jnewbery> nehan: sorry, I misunderstood your question. I thought you were talking about the `relay` bool in the VERSION message, not the new tx_relay struct
12313:39 <michaelfolkson> <aj> So 2 was chosen due to the trade-off with resource utilization but 8 would still be plausible?
12413:39 <jnewbery> moral of the story: even Suhas sometimes misses things :)
12513:39 <jnewbery> great catch, aj
12613:39 <jonatack> jnewbery: it might be interesting to hear, how did you look into the spy node question? code, watching node logs?
12713:39 <michaelfolkson> <aj> The simulation was to see if it was worth bothering with 2 or whether to go higher at this point?
12813:40 <nehan> jnewbery: yes, i'm curious why check node->m_tx_relay == nullptr (assuming the reader knows this is only non-null when the node supports tx relay) instead of directly including a separate bool to indicate whether or not the node supports tx relay. i guess it's a code style thing.
12913:40 <jnewbery> michaelfolkson: 2 was chosen as 'this almost certainly won't cause any harm in the short run, and we can easily increase the number later'
13013:40 <aj> michaelfolkson: i took it as "seeing if 2 was already enough, or if in really bad scenarios more will be needed"
13113:40 <michaelfolkson> Ok thanks
13213:41 <jnewbery> nehan: I think it's fine to check based on the non-nullness of the pointer. Adding an additional bool which essentially would be equivalent to saying 'this pointer is nun-null' seems unnecessary
13313:42 <jnewbery> (and redundant)
13413:43 <jnewbery> you could imagine a future change where someone adds a way to downgrade a connection to blocks-only, sets the pointer to null and forgets to update the bool, as one example of why you wouldn't want that redundancy
13513:43 <jnewbery> but I agree, it's a style thing
13613:44 <jnewbery> Next question: How should we handle peers that send us tx data after we’ve requested a blocks-only connection?
13713:44 <lightlike> as to "increase the number later": would it be reasonable to completely separate block and tx relay peers in the future?
13813:44 <jonatack> In general, these p2p changes are fascinating but ACKing them for me is held up by the difficulty of testing them.
13913:44 <jkczyz> disconnect from them?
14013:44 <jnewbery> lightlike: so all connections are blocks-only or tx-only?
14113:44 <lightlike> yes
14213:45 <jnewbery> jonatack: (sorry, dropped your question about spy nodes earlier). I noticed that the IP address for the node that was doing this was on Greg's spy node list: https://people.xiph.org/~greg/banlist.cli.txt
14313:46 <jnewbery> and I connected to several other nodes on that list, and they also showed this behaviour
14413:46 <michaelfolkson> <jonatack>: What difficulties specifically with regards to testing? The attack scenarios?
14513:47 <jnewbery> and I spent a while digging into how Bitcoin Core serializes/deserializes VERSION messages (and in particular booleans - `relay` in VERSION is the only boolean we serialize on the P2P network) and couldn't find any bugs
14613:47 <jonatack> jnewbery: Thanks. I use Greg's banlist, and maybe should not use it when testing PRs then.
14713:47 <ariard> michaelfolkson: thinking about all the ways code branches can be triggered due to a network event or combination of them
14813:48 <jnewbery> jonatack: (re:ACKing) that shouldn't discourage you from reviewing. A comment like "code looks correct to me, but I don't feel confident enough about behaviour to give an ACK" is a perfectly helpful contribution
14913:49 <jnewbery> lightlike: there is benefit to having both tx and block relay for the same peer, eg compact blocks works under the expectation that the mempool of the peers is similar
15013:49 <jnewbery> jkczyz: yeah, I think disconnect from them is reasonable. At the moment we just log and carry on
15113:50 <jnewbery> I opened a PR to do that here: https://github.com/bitcoin/bitcoin/pull/16682 (but marked as WIP, since it's less important to get merged than 15759)
15213:50 <aj> lightlike: also the overhead of relaying blocks if you're already relaying tx's seems pretty small, and the more redundancy block relay has the better
15313:51 <jnewbery> I skipped a question, but I think we already covered it: Why is the P2P behaviour of this PR difficult to test in the functional test framework?
15413:51 <jnewbery> https://github.com/bitcoin/bitcoin/issues/14210 is the issue tracking that
15513:52 <jnewbery> Any final questions in the last ten minutes?
15613:52 <jonatack> michaelfolkson: the difficulty of setting up functional tests to simulate p2p behavior, we lack p2p simulation frameworks... Suhas, Gleb, Jeremy Rubin, Giulia Fanti, have IIRC all mentioned the need for them
15713:52 <aj> why is it difficult? can't we setup a bunch of nodes, pre-populate their peers db so they know each other, and have them connect?
15813:53 <jnewbery> aj: there are a few limitations:
15913:53 <jkczyz> jnewbery: Is there any planned refactoring for the network code? It seems quite unwieldy/complex and perhaps increasingly so with each change.
16013:53 <jnewbery> - we can't set up an outbound connection from a bitcoind node to the test framework and control the P2P message flow
16113:53 <ariard> like dropping randomly some messages
16213:54 <jnewbery> - all peers are considered 'local' because they're on the localhost IP, and they're treated slightly differently in the code
16313:54 <jnewbery> I don't think we have any tests that pre-populate the peers db to get them to connect to each other
16413:55 <michaelfolkson> Just set up a regtest network with a script?
16513:55 <jnewbery> ariard: that'd be interesting testing, as would re-ordering or fuzzing P2P messages
16613:56 <jnewbery> michaelfolkson: that's what the functional tests are doing, but the framework is missing some pieces that would allow us to fully test P2P behaviour
16713:57 <jonatack> jnewbery: (re: ACKING) Thanks! Good to keep in mind.
16813:57 <aj> jnewbery: shouldn't "addnode" be able to construct whatever shape of network we want? maybe adding/removing net_permissions -- so 127.0.0.1:* defaults to LOCAL_PEER perm but that can get dropped with some arg to addnode, maybe?
16913:57 <michaelfolkson> Any links to hand on the missing pieces of the framework?
17013:58 <jnewbery> let's wrap it up there. What did people think of this one? P2P is quite tricky and there's a lot of required context so I was worried that it might be too much, but I thought this was good.
17113:58 <michaelfolkson> I find this stuff really interesting and easier to follow than last week's
17213:58 <jnewbery> aj: addnode peers are treated differently from gossiped peers
17313:58 <dergigi> That was interesting indeed.
17413:58 <dergigi> Oh, last question: would Signet help in testing stuff like this?
17513:59 <jonatack> Great choice... Suhas' p2p ones are always good.
17613:59 <ariard> jnewbery: oh yes fuzzing P2P messages by just spawning a bunch of Cconnman and let's them talking between each others
17713:59 <jkczyz> jnewbery: See my question for an answer to yours ^^^
17813:59 <jkczyz> jnewbery: :)
17913:59 <jonatack> ariard: Someone could really go to town with p2p simulations. We need to get some funding for this :)
18014:00 <michaelfolkson> <dergigi>: As you are running tests on your local machine, I don't think Signet would offer anything on top of regtest
18114:00 <jnewbery> aj: yes, arguments to addnode could be useful, or an updatepeer RPC like https://github.com/bitcoin/bitcoin/pull/10160
18214:00 <dergigi> michaelfolkson: I see, thank you
18314:01 <michaelfolkson> <dergigi>: Maybe if you are doing tests with peers trying to attack you :)
18414:01 <jnewbery> dergigi: (re signet): perhaps yes for manual testing. Our autmated tests use regtest
18514:01 <ariard> aj: what's your process to test p2p PR, throwing a node on mainet/testnet with the patch and watching logs after a week ?
18614:02 <jnewbery> thanks everyone. Great discussion!
18714:02 <jonatack> jnewbery: maybe put https://github.com/bitcoin/bitcoin/pull/10160 on the PR club list
18814:02 <jnewbery> and thanks aj for dropping in!
18914:02 <lightlike> thanks!
19014:02 <dergigi> thanks everyone, bye!
19114:03 <michaelfolkson> Thanks
19214:03 <ariard> thanks!
19314:03 <aj> jnewbery: i guess, (a) i don't get why this is hard -- i mean, it's work, but it doesn't seem really tricky, but maybe i'm missing somethng? (b) not clear to me whether it makes more sense to setup the environment (peers.dat) and have bitcoind do it's normal resolution, or have rpc commands that explicitly set stuff up makes more sense, or maybe it makes sense to do both
19414:04 <aj> ariard: read the code, read the PR comments, re-read both, run the tests, find something that doesn't make sense and try to make sense of it, repeat; once it kind-of makes sense, run it and see what happens. if things don't quite make sense, maybe add some logging, and run the tests and look through the logs, or run it on mainnet and look through the logs
19514:06 <jonatack> aj: thanks, i was curious to hear that too. I recall you were adding custom logging to test your PR to fix the NOTFOUND bug.
19614:06 <aj> ariard: sometimes i try refactoring the code to something that i think will be prettier, and discover why that's actually a huge ugly mess
19714:06 <aj> printf debugging is the best and it's always a privilige to add it to other people's code
19814:08 <jonatack> aj: what kind of time do you spend to review/test a PR in this way?
19914:08 <ariard> aj: thanks a lot, specifically the p2p stack I found hard to know if everything makes senses
20014:09 <aj> ariard: i'm not sure anyone on earth is smart enough to look at the p2p stack and honestly think everything makes sense?
20114:10 <ariard> ahah true
20214:11 <aj> jonatack: not really sure, twice as long as i plan to? :)
20314:12 <jonatack> aj: yeah, thanks, needed reassurance here :)