Add 2 outbound blocks-only connections (
p2p) Aug 28, 2019
The Bitcoin P2P network serves 3 purposes:
relaying unconfirmed transactions
gossiping addresses of known reachable nodes on the network
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
CNodeState (defined in
netprocessing.cpp and covered by
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
An example of the
application state data that is contained in
CNode is the inventory data protected by
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.
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?
7 13:00 <michaelfolkson> Hey
10 13:01 <jnewbery> before we start, a couple of reminders:
13 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!
14 13:03 <jnewbery> if you have any questions or thoughts about leaving review comments on PRs, they're all on topic in this channel
15 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?
16 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
17 13:04 <jnewbery> no, basic stuff is on topic
18 13:04 <jnewbery> please feel free to ask during the meeting
19 13:04 <jnewbery> IRC conversations can be multi-threaded!
20 13:04 <jonatack> built, ran tests, started to review and grep code
21 13:04 <michaelfolkson> Ok
22 13:05 <ariard> had a chance to review, like weeks ago
23 13:05 <lightlike> i tested this a couple of weeks ago, didnn't test the newest fixes.
24 13:05 <jkczyz> Read through comments and code yesterday/today. Added some minor comment this morning.
25 13:05 <jnewbery> I see review comments from ariard, lighlike and jkczyz. Great stuff!
27 13:06 <jnewbery> And does anyone want to have a go at the first question "What problem is this PR addressing?
28 13:06 <jonatack> This PR proposes to mitigate the risk of eclipse attacks by separating block
29 13:07 <jonatack> relay from transaction relay to occlude the network topology
30 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
31 13:07 <jonatack> (that was the easiest question... I *think* :D)
32 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
33 13:08 <jnewbery> jonatack: yeah, that's right
34 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?
35 13:09 <jnewbery> anyone want to expand on the motivation, or shall we move onto the next question?
36 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)?
37 13:10 <provoostenator> michaelfolkson: you're at risk with or without the DNS seeds
38 13:10 <michaelfolkson> You only need one peer to serve you correct transactions/blocks and you are able to detect attack
39 13:11 <jnewbery> dergigi: I haven't read the Erebus paper, but this PR was opened before that paper was released I believe
40 13:11 <provoostenator> You won't necessarily detect the attack, but you'll get the proper chain.
41 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.
42 13:11 <jnewbery> I don't think this PR is directly concerned with Erebus. PRs that change P2P code often conflict
43 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?
44 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".
45 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)
46 13:12 <ariard> lightlike: you think better management of tx dependencies and their conflicts between peers could solve it ?
47 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
48 13:13 <michaelfolkson> <provoostenator>: What's to stop me manually maintaining the connection to them?
50 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
51 13:14 <provoostenator> Only when that fails does it call the DNS seeds.
52 13:14 <provoostenator> michaelfolkson: DNS seeds are not nodes, the only thing they do is return a list of IP addresses.
53 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
54 13:15 <jnewbery> michaelfolkson: use addnode=<address> to your config
55 13:15 <provoostenator> Try this:
56 13:15 <provoostenator> dig -t A seed.bitcoin.sprovoost.nl
57 13:15 <jnewbery> ok, question two: Why is the tx relay inventory state moved into its own structure?
58 13:15 <provoostenator> DNS uses port 53 and has a completely different protocol than the Bitcoin P2P network (port 8333)
59 13:15 <michaelfolkson> Ah thanks
60 13:16 <jkczyz> To separate state that is not needed for blocks-only peers
61 13:16 <provoostenator> "I don't have to worry about eclipse attacks" - well, then *that* peer has to worry
62 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
63 13:16 <jnewbery> jkczyz: correct. Why?
64 13:17 <ariard> so patching all heuristics beyond TxProbe seems it to be really hard
65 13:17 <michaelfolkson> <provoostenator> Maybe those people named on that DNS seed list have a better understanding of the network than me ;)
66 13:17 <jkczyz> jnewbery: the state is not applicable to them since they are not relaying transactions
67 13:17 <provoostenator> I'm just good at copy-pasting. And actually had to fix a bunch of issues of time.
68 13:17 <jnewbery> that's right, but why not just not use that state?
69 13:18 <ariard> michaelfolkson: you should have multiple source of blocks beyond your ISP if so
70 13:18 <jkczyz> jnewbery: not sure if I parsed that one correctly
71 13:18 <jnewbery> why not just not initialize that state and don't use it?
72 13:19 <ariard> reduce memory comsumption
73 13:19 <jnewbery> why go to the trouble of defining a structure and adding a unique_ptr
74 13:19 <jkczyz> Ah, yes memory and because it is used to determine if it is blocks-only
75 13:19 <jnewbery> correct: the justification was to reduce memory consumption (sorry jkczyz - maybe that was just too obvious!)
76 13:20 <jnewbery> Does the tx relay state require a lot of memory? If so, where?
77 13:20 <jkczyz> jnewbery: haha, np. I also found it a bit disconcerting that CNode has over 60 member variables
78 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?
79 13:21 <ariard> tx relay state requires memory for inv of txid to send
80 13:21 <nehan> jnewbery: does it have anything to do with the bloom filter?
81 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?
82 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)
83 13:23 <jnewbery> There are a couple of bloom filters used by CNode. What are they and what are they for?
84 13:24 <nehan> jnewbery: it's unclear to my why an unused bloom filter should take up a lot ofmemory
85 13:24 <ariard> aj: oh yes, assuming we don't fulfill them by mistake in some part of the code
86 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
87 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),
88 13:25 <nehan> jnewbery: but another option would be not to initialize until use
89 13:25 <jnewbery> nehan: that's it
90 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
91 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)
92 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
93 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
94 13:27 <ariard> would be interesting to dig in :)
95 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
96 13:28 <aj> (also the idea is to eventually have 8 of these instead of just 2, so the cheaper the better)
97 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
98 13:28 <nehan> aj: initializing the memory on first use != reinitializing. but this is quibbling.
99 13:29 <jnewbery> any other questions on bloom filters, data structures or memory?
100 13:29 <jnewbery> Third question: How do we signal a ‘blocks-only’ connection on the P2P network?
101 13:29 <michaelfolkson> <aj>: I thought the simulation discussion was exploring how many additional outbound connections you should have?
102 13:30 <jnewbery> hint: it's in the VERSION message
103 13:32 <jnewbery> psst: it's defined in BIP 37
104 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
105 13:32 <jnewbery> There's an optional `relay` boolean at the end of the VERSION message
107 13:33 <jnewbery> if relay is set to False, the peer shouldn't relay txs to you
108 13:33 <jnewbery> next question: What does aj point out in his review comment that all other reviewers missed?
109 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?
110 13:34 <jnewbery> what do you mean about a null object pointer? That it's a 0x00 byte or something more than that?
111 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
112 13:35 <nehan> jnewbery: nevermind, my question might not make sense i'll reask later if it does.
113 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)
114 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
115 13:36 <ariard> spy nodes or buggy ones ?
116 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)
117 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
118 13:37 <lightlike> aj: did you spot this by testing in the wild, or by thinking about the PR?
119 13:37 <jnewbery> ariard: that's a good question. I spent some time looking into it, and I believe they're spy nodes
120 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
121 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
122 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
123 13:39 <michaelfolkson> <aj> So 2 was chosen due to the trade-off with resource utilization but 8 would still be plausible?
124 13:39 <jnewbery> moral of the story: even Suhas sometimes misses things :)
125 13:39 <jnewbery> great catch, aj
126 13:39 <jonatack> jnewbery: it might be interesting to hear, how did you look into the spy node question? code, watching node logs?
127 13:39 <michaelfolkson> <aj> The simulation was to see if it was worth bothering with 2 or whether to go higher at this point?
128 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.
129 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'
130 13:40 <aj> michaelfolkson: i took it as "seeing if 2 was already enough, or if in really bad scenarios more will be needed"
131 13:40 <michaelfolkson> Ok thanks
132 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
133 13:42 <jnewbery> (and redundant)
134 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
135 13:43 <jnewbery> but I agree, it's a style thing
136 13:44 <jnewbery> Next question: How should we handle peers that send us tx data after we’ve requested a blocks-only connection?
137 13:44 <lightlike> as to "increase the number later": would it be reasonable to completely separate block and tx relay peers in the future?
138 13:44 <jonatack> In general, these p2p changes are fascinating but ACKing them for me is held up by the difficulty of testing them.
139 13:44 <jkczyz> disconnect from them?
140 13:44 <jnewbery> lightlike: so all connections are blocks-only or tx-only?
143 13:46 <jnewbery> and I connected to several other nodes on that list, and they also showed this behaviour
144 13:46 <michaelfolkson> <jonatack>: What difficulties specifically with regards to testing? The attack scenarios?
145 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
146 13:47 <jonatack> jnewbery: Thanks. I use Greg's banlist, and maybe should not use it when testing PRs then.
147 13:47 <ariard> michaelfolkson: thinking about all the ways code branches can be triggered due to a network event or combination of them
148 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
149 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
150 13:49 <jnewbery> jkczyz: yeah, I think disconnect from them is reasonable. At the moment we just log and carry on
152 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
153 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?
155 13:52 <jnewbery> Any final questions in the last ten minutes?
156 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
157 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?
158 13:53 <jnewbery> aj: there are a few limitations:
159 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.
160 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
161 13:53 <ariard> like dropping randomly some messages
162 13:54 <jnewbery> - all peers are considered 'local' because they're on the localhost IP, and they're treated slightly differently in the code
163 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
164 13:55 <michaelfolkson> Just set up a regtest network with a script?
165 13:55 <jnewbery> ariard: that'd be interesting testing, as would re-ordering or fuzzing P2P messages
166 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
167 13:57 <jonatack> jnewbery: (re: ACKING) Thanks! Good to keep in mind.
168 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?
169 13:57 <michaelfolkson> Any links to hand on the missing pieces of the framework?
170 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.
171 13:58 <michaelfolkson> I find this stuff really interesting and easier to follow than last week's
172 13:58 <jnewbery> aj: addnode peers are treated differently from gossiped peers
173 13:58 <dergigi> That was interesting indeed.
174 13:58 <dergigi> Oh, last question: would Signet help in testing stuff like this?
175 13:59 <jonatack> Great choice... Suhas' p2p ones are always good.
176 13:59 <ariard> jnewbery: oh yes fuzzing P2P messages by just spawning a bunch of Cconnman and let's them talking between each others
177 13:59 <jkczyz> jnewbery: See my question for an answer to yours ^^^
178 13:59 <jkczyz> jnewbery: :)
179 13:59 <jonatack> ariard: Someone could really go to town with p2p simulations. We need to get some funding for this :)
180 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
182 14:00 <dergigi> michaelfolkson: I see, thank you
183 14:01 <michaelfolkson> <dergigi>: Maybe if you are doing tests with peers trying to attack you :)
184 14:01 <jnewbery> dergigi: (re signet): perhaps yes for manual testing. Our autmated tests use regtest
185 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 ?
186 14:02 <jnewbery> thanks everyone. Great discussion!
188 14:02 <jnewbery> and thanks aj for dropping in!
189 14:02 <lightlike> thanks!
190 14:02 <dergigi> thanks everyone, bye!
191 14:03 <michaelfolkson> Thanks
192 14:03 <ariard> thanks!
193 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
194 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
195 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.
196 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
197 14:06 <aj> printf debugging is the best and it's always a privilige to add it to other people's code
198 14:08 <jonatack> aj: what kind of time do you spend to review/test a PR in this way?
199 14:08 <ariard> aj: thanks a lot, specifically the p2p stack I found hard to know if everything makes senses
200 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?
201 14:10 <ariard> ahah true
202 14:11 <aj> jonatack: not really sure, twice as long as i plan to? :)
203 14:12 <jonatack> aj: yeah, thanks, needed reassurance here :)