#15759 Add 2 outbound blocks-only connections (p2p)

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

Host: jnewbery

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

13:00 < jnewbery> hi
13:00 < dergigi> hi
13:00 < marcinja_> hi
13:00 < ccdle12> hi
13:00 < jkczyz> hi
13:00 < lightlike> hi
13:00 < michaelfolkson> Hey
13:01 < afigs> sup
13:01 < nehan> hi
13:01 < jnewbery> before we start, a couple of reminders:
13:01 < jonatack> hi
13: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
13: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!
13:03 < jnewbery> if you have any questions or thoughts about leaving review comments on PRs, they're all on topic in this channel
13:03 < jnewbery> ok, let's get started. What did you all think about the PR this week? Who had a chance to review/test?
13: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
13:04 < jnewbery> no, basic stuff is on topic
13:04 < jnewbery> please feel free to ask during the meeting
13:04 < jnewbery> IRC conversations can be multi-threaded!
13:04 < jonatack> built, ran tests, started to review and grep code
13:04 < michaelfolkson> Ok
13:05 < ariard> had a chance to review, like weeks ago
13:05 < lightlike> i tested this a couple of weeks ago, didnn't test the newest fixes.
13:05 < jkczyz> Read through comments and code yesterday/today. Added some minor comment this morning.
13:05 < jnewbery> I see review comments from ariard, lighlike and jkczyz. Great stuff!
13:06 < jnewbery> Any questions about the notes from https://bitcoin-core-review-club.github.io/15759.html ? Anything unclear?
13:06 < jnewbery> And does anyone want to have a go at the first question "What problem is this PR addressing?
13:06 < jonatack> This PR proposes to mitigate the risk of eclipse attacks by separating block
13:07 < jonatack> relay from transaction relay to occlude the network topology
13: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
13:07 < jonatack> (that was the easiest question... I *think* :D)
13: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
13:08 < jnewbery> jonatack: yeah, that's right
13: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?
13:09 < jnewbery> anyone want to expand on the motivation, or shall we move onto the next question?
13: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)?
13:10 < provoostenator> michaelfolkson: you're at risk with or without the DNS seeds
13:10 < michaelfolkson> You only need one peer to serve you correct transactions/blocks and you are able to detect attack
13:11 < jnewbery> dergigi: I haven't read the Erebus paper, but this PR was opened before that paper was released I believe
13:11 < provoostenator> You won't necessarily detect the attack, but you'll get the proper chain.
13:11 < provoostenator> The DNS seeds are used to get an initial set of IP addresses to try, you then contact those for more, etc.
13:11 < jnewbery> I don't think this PR is directly concerned with Erebus. PRs that change P2P code often conflict
13: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?
13: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".
13: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)
13:12 < ariard> lightlike: you think better management of tx dependencies and their conflicts between peers could solve it ?
13: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
13:13 < michaelfolkson> <provoostenator>: What's to stop me manually maintaining the connection to them?
13:13 < lightlike> ariard: I don't know. The author doesn't believe in that: https://github.com/bitcoin/bitcoin/pull/15759#issuecomment-480398802
13: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
13:14 < provoostenator> Only when that fails does it call the DNS seeds.
13:14 < provoostenator> michaelfolkson: DNS seeds are not nodes, the only thing they do is return a list of IP addresses.
13: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
13:15 < jnewbery> michaelfolkson: use addnode=<address> to your config
13:15 < provoostenator> Try this:
13:15 < provoostenator> dig -t A seed.bitcoin.sprovoost.nl
13:15 < jnewbery> ok, question two: Why is the tx relay inventory state moved into its own structure?
13:15 < provoostenator> DNS uses port 53 and has a completely different protocol than the Bitcoin P2P network (port 8333)
13:15 < michaelfolkson> Ah thanks
13:16 < jkczyz> To separate state that is not needed for blocks-only peers
13:16 < provoostenator> "I don't have to worry about eclipse attacks" - well, then *that* peer has to worry
13: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
13:16 < jnewbery> jkczyz: correct. Why?
13:17 < ariard> so patching all heuristics beyond TxProbe seems it to be really hard
13:17 < michaelfolkson> <provoostenator> Maybe those people named on that DNS seed list have a better understanding of the network than me ;)
13:17 < jkczyz> jnewbery: the state is not applicable to them since they are not relaying transactions
13:17 < provoostenator> I'm just good at copy-pasting. And actually had to fix a bunch of issues of time.
13:17 < jnewbery> that's right, but why not just not use that state?
13:18 < ariard> michaelfolkson: you should have multiple source of blocks beyond your ISP if so
13:18 < jkczyz> jnewbery: not sure if I parsed that one correctly
13:18 < jnewbery> why not just not initialize that state and don't use it?
13:19 < ariard> reduce memory comsumption
13:19 < jnewbery> why go to the trouble of defining a structure and adding a unique_ptr
13:19 < jkczyz> Ah, yes memory and because it is used to determine if it is blocks-only
13:19 < jnewbery> correct: the justification was to reduce memory consumption (sorry jkczyz - maybe that was just too obvious!)
13:20 < jnewbery> Does the tx relay state require a lot of memory? If so, where?
13:20 < jkczyz> jnewbery: haha, np. I also found it a bit disconcerting that CNode has over 60 member variables
13: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?
13:21 < ariard> tx relay state requires memory for inv of txid to send
13:21 < nehan> jnewbery: does it have anything to do with the bloom filter?
13: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?
13: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)
13:23 < jnewbery> There are a couple of bloom filters used by CNode. What are they and what are they for?
13:24 < nehan> jnewbery: it's unclear to my why an unused bloom filter should take up a lot ofmemory
13:24 < ariard> aj: oh yes, assuming we don't fulfill them by mistake in some part of the code
13: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
13:25 < nehan> jnewbery: i guess it's this:     vData(std::min((unsigned int)(-1  / LN2SQUARED * nElements * log(nFPRate)), MAX_BLOOM_FILTER_SIZE * 8) / 8),
13:25 < nehan> jnewbery: but another option would be not to initialize until use
13:25 < jnewbery> nehan: that's it
13: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
13: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)
13: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
13: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
13:27 < ariard> would be interesting to dig in :)
13: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
13:28 < aj> (also the idea is to eventually have 8 of these instead of just 2, so the cheaper the better)
13: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
13:28 < nehan> aj: initializing the memory on first use != reinitializing. but this is quibbling.
13:29 < jnewbery> any other questions on bloom filters, data structures or memory?
13:29 < jnewbery> Third question: How do we signal a ‘blocks-only’ connection on the P2P network?
13:29 < michaelfolkson> <aj>: I thought the simulation discussion was exploring how many additional outbound connections you should have?
13:30 < jnewbery> hint: it's in the VERSION message
13:32 < jnewbery> psst: it's defined in BIP 37
13: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
13:32 < jnewbery> There's an optional `relay` boolean at the end of the VERSION message
13:33 < jnewbery> https://btcinformation.org/en/developer-reference#version
13:33 < jnewbery> if relay is set to False, the peer shouldn't relay txs to you
13:33 < jnewbery> next question: What does aj point out in his review comment that all other reviewers missed?
13: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?
13:34 < jnewbery> what do you mean about a null object pointer? That it's a 0x00 byte or something more than that?
13: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
13:35 < nehan> jnewbery: nevermind, my question might not make sense i'll reask later if it does.
13: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)
13:36 < jnewbery> lightlike: exactly. There are peers on the network that ignore `relay`=False and will still send us tx INVs and TX messages
13:36 < ariard> spy nodes or buggy ones ?
13: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)
13: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
13:37 < lightlike> aj: did you spot this by testing in the wild, or by thinking about the PR?
13:37 < jnewbery> ariard: that's a good question. I spent some time looking into it, and I believe they're spy nodes
13: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
13: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
13: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
13:39 < michaelfolkson> <aj> So 2 was chosen due to the trade-off with resource utilization but 8 would still be plausible?
13:39 < jnewbery> moral of the story: even Suhas sometimes misses things :)
13:39 < jnewbery> great catch, aj
13:39 < jonatack> jnewbery: it might be interesting to hear, how did you look into the spy node question? code, watching node logs?
13:39 < michaelfolkson> <aj> The simulation was to see if it was worth bothering with 2 or whether to go higher at this point?
13: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.
13: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'
13:40 < aj> michaelfolkson: i took it as "seeing if 2 was already enough, or if in really bad scenarios more will be needed"
13:40 < michaelfolkson> Ok thanks
13: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
13:42 < jnewbery> (and redundant)
13: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
13:43 < jnewbery> but I agree, it's a style thing
13:44 < jnewbery> Next question: How should we handle peers that send us tx data after we’ve requested a blocks-only connection?
13:44 < lightlike> as to "increase the number later": would it be reasonable to completely separate block and tx relay peers in the future?
13:44 < jonatack> In general, these p2p changes are fascinating but ACKing them for me is held up by the difficulty of testing them.
13:44 < jkczyz> disconnect from them?
13:44 < jnewbery> lightlike: so all connections are blocks-only or tx-only?
13:44 < lightlike> yes
13: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
13:46 < jnewbery> and I connected to several other nodes on that list, and they also showed this behaviour
13:46 < michaelfolkson> <jonatack>: What difficulties specifically with regards to testing? The attack scenarios?
13: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
13:47 < jonatack> jnewbery: Thanks. I use Greg's banlist, and maybe should not use it when testing PRs then.
13:47 < ariard> michaelfolkson: thinking about all the ways code branches can be triggered due to a network event or combination of them
13: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
13: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
13:49 < jnewbery> jkczyz: yeah, I think disconnect from them is reasonable. At the moment we just log and carry on
13: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)
13: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
13: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?
13:51 < jnewbery> https://github.com/bitcoin/bitcoin/issues/14210 is the issue tracking that
13:52 < jnewbery> Any final questions in the last ten minutes?
13: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
13: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?
13:53 < jnewbery> aj: there are a few limitations:
13:53 < jkczyz> jnewbery: Is there any planned refactoring for the network code? It seems quite unwieldy/complex and perhaps increasingly so with each change.
13:53 < jnewbery> - we can't set up an outbound connection from a bitcoind node to the test framework and control the P2P message flow
13:53 < ariard> like dropping randomly some messages
13:54 < jnewbery> - all peers are considered 'local' because they're on the localhost IP, and they're treated slightly differently in the code
13:54 < jnewbery> I don't think we have any tests that pre-populate the peers db to get them to connect to each other
13:55 < michaelfolkson> Just set up a regtest network with a script?
13:55 < jnewbery> ariard: that'd be interesting testing, as would re-ordering or fuzzing P2P messages
13: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
13:57 < jonatack> jnewbery: (re: ACKING) Thanks! Good to keep in mind.
13: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?
13:57 < michaelfolkson> Any links to hand on the missing pieces of the framework?
13: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.
13:58 < michaelfolkson> I find this stuff really interesting and easier to follow than last week's
13:58 < jnewbery> aj: addnode peers are treated differently from gossiped peers
13:58 < dergigi> That was interesting indeed.
13:58 < dergigi> Oh, last question: would Signet help in testing stuff like this?
13:59 < jonatack> Great choice... Suhas' p2p ones are always good.
13:59 < ariard> jnewbery: oh yes fuzzing P2P messages by just spawning a bunch of Cconnman and let's them talking between each others
13:59 < jkczyz> jnewbery: See my question for an answer to yours ^^^
13:59 < jkczyz> jnewbery: :)
13:59 < jonatack> ariard: Someone could really go to town with p2p simulations. We need to get some funding for this :)
14:00 < michaelfolkson> <dergigi>: As you are running tests on your local machine, I don't think Signet would offer anything on top of regtest
14:00 < jnewbery> aj: yes, arguments to addnode could be useful, or an updatepeer RPC like https://github.com/bitcoin/bitcoin/pull/10160
14:00 < dergigi> michaelfolkson: I see, thank you
14:01 < michaelfolkson> <dergigi>: Maybe if you are doing tests with peers trying to attack you :)
14:01 < jnewbery> dergigi: (re signet): perhaps yes for manual testing. Our autmated tests use regtest
14: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 ?
14:02 < jnewbery> thanks everyone. Great discussion!
14:02 < jonatack> jnewbery: maybe put https://github.com/bitcoin/bitcoin/pull/10160 on the PR club list
14:02 < jnewbery> and thanks aj for dropping in!
14:02 < lightlike> thanks!
14:02 < dergigi> thanks everyone, bye!
14:03 < michaelfolkson> Thanks
14:03 < ariard> thanks!
14: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
14: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
14: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.
14: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
14:06 < aj> printf debugging is the best and it's always a privilige to add it to other people's code
14:08 < jonatack> aj: what kind of time do you spend to review/test a PR in this way?
14:08 < ariard> aj: thanks a lot, specifically the p2p stack I found hard to know if everything makes senses
14: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?
14:10 < ariard> ahah true
14:11 < aj> jonatack: not really sure, twice as long as i plan to? :)
14:12 < jonatack> aj: yeah, thanks, needed reassurance here :)