Refactor network message deserialization (p2p )
Sep 25, 2019
https://github.com/bitcoin/bitcoin/pull/16202
Host:
jonatack
PR author: jonasschnelli
Notes
Today’s PR under review is part of on-going work by
Jonas Schnelli
on BIP 324: Version 2 Peer-to-Peer Message Transport Protocol. Here are some
resources to learn more about it, or as a refresher:
BIP 324 PRs that have been merged:
A short follow-up PR you can review after this one:
If you add any tests, assertions or logs while reviewing today’s PR, please
save the diff in a public gist! We can share links to see what we tested.
Questions
Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?
Why was this PR written and considered high-priority?
Which roles and classes are being separated in the first commit?
Why is a unique_ptr used rather than a shared_ptr? What are the trade-offs?
Is this mentioned in the Bitcoin Core developer
notes ?
Describe the Adapter pattern in 1-2 sentences. What is another frequent name
for it? What are two general kinds of adapters? Which of the two kinds is more
flexible, and what might be the trade-off? Which kind is used in this PR, and
who are the participants (target, client, adaptee, adapter)?
Any other comments, feedback, or questions?
Meeting Log
1 19:00 <jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club.
4 19:00 <sebastiavstaa> hi
5 19:00 <jonatack> We usually start Bitcoin Core IRC meetings with a 'hi' so it's clear who's at keyboard. Feel free to say hi here!
7 19:01 <michaelfolkson> Holla
8 19:01 <jonatack> This week, we're talking about PR16202 - "Refactor network message deserialization" by jonasschnelli.
9 19:01 <jonatack> The PR is part of on-going work by jonasschnelli on BIP 324: Version 2 Peer-to-Peer Message Transport Protocol.
11 19:01 <zenogais> hi all
13 19:02 <jonatack> jonasschnelli: By any chance, are you here? Would you like to say anything about this PR or the next steps for BIP 324? Feel free to jump in.
14 19:03 <jonatack> Let's get started. Did you review the PR? What are your thoughts... Concept ACK, approach ACK, tested ACK, NACK?
15 19:03 <fjahr> tested ACK although I did not have much time to look at the code
16 19:04 <zenogais> Tested ACK. I was able to review the whole PR as well as test it.
17 19:04 <lightlike> concept ack, haven't understood the code yet completely.
18 19:04 <jonatack> fjahr: Excellent.
19 19:04 <jonatack> etscrivner: jkczyz: Great to see your reviews of the PR on GitHub!
20 19:04 âš¡ zenogais is etscrivner btw
21 19:04 <jonatack> zenogais: thanks!
22 19:05 <jonatack> How did you all test the changes?
23 19:05 <sebastiavstaa> built and ran the tests.
24 19:05 <zenogais> I ran the unit and functional tests. Also used my own P2P library in C to run some manual tests and make sure things worked as I expected.
25 19:05 <jonatack> Did anyone add any tests, assertions, or custom logging to test the PR?
26 19:06 <pinheadmz> built and tested, broke the test and tested the test :-)
27 19:06 <pinheadmz> just the one test that was altered, the error message is changed
28 19:06 <pinheadmz> I assume because its a refactor, the existing tests cover and protect against regression.
30 19:06 <jonatack> and would like to run the changes through gdb tomorrow for my review, since p2p can be a risky area.
31 19:06 <sosthene> I do have a question that is more about the whole BIP than this PR. I remember some time ago evoskuil criticizing the BIP on Twitter I think, but I couldn't find the thread again. So what could be controversial about it?
32 19:06 <sosthene> (if anyone knows what I'm talking about and can link to evoskuil arguments I would be very grateful)
33 19:07 <jonatack> sosthene: I wasn't aware of controversy regarding BIP 324.
34 19:08 <zenogais> Might be useful to have some sort of P2P fuzzing also (if it doesn't already exist).
35 19:08 <pinheadmz> jonatack: that's cool - these properties i.e. `msg.m_command` are new to this PR right?
36 19:08 <sosthene> jonatack: I guess that's evoskuil vs the rest of the world then
37 19:09 <jonatack> pinheadmz: yeah, I want to see the values as a sanity check
38 19:09 <jonatack> sosthene: if you have a link please share!
39 19:09 <michaelfolkson> Nor me, sorry sosthene. Perhaps contact him?
40 19:10 <jonatack> zenogais: Fuzzing is one area I think the maintainers would be very happy to see additional tests or data for.
41 19:10 <jonatack> MarcoFalke is the primary contact for that. See also doc/fuzzing.md.
42 19:10 <michaelfolkson> I didn't stumble on any conceptual downsides to this BIP
43 19:11 <zenogais> jonatack: Thanks, will give it a look. Have been thinking about P2P fuzzing specifically for a couple of weeks now.
44 19:12 <jonasschnelli> hi
46 19:12 âš¡ zenogais waves
47 19:12 <jonatack> jonasschnelli: Hi, thanks for coming by!
48 19:13 <jonasschnelli> Sorry for being late
49 19:13 <jonatack> jkczyz: Hi, and thank you for reviewing the PR.
50 19:13 âš¡ jonasschnelli is reading back
51 19:13 âš¡ next-defection thumbs up to fuzzing
52 19:13 <jonasschnelli> Should I explain why I did this PR (PR16202)?
53 19:13 <jonatack> jonasschnelli: Would you like to say anything about this PR, or the next steps for BIP 324?
54 19:13 <jonatack> jonasschnelli: Yes, please!
57 19:14 <jonasschnelli> PR16202 is just about adding flexibility for allowing multiple transport protocol
58 19:14 <jonatack> jkczyz: (we were discussing how people tested the PR, if you would like to share what you did to test it)
60 19:15 <jonasschnelli> It is necessary for any forms of protocol upgrades and also a nice refactor if nothing gets added (which we don't hope).
61 19:15 <jonasschnelli> evoskuil's points are valid,... not sure if the overall desire (or dislike) of encryption is something we should discuss here.
62 19:15 <jonasschnelli> But we can...
63 19:15 <jonatack> jonasschnelli: What are the next BIP324 PRs we should review after this one is hopefully in?
65 19:16 <jonasschnelli> Yes... that one is in alignment with the deserialization.
66 19:16 <jonasschnelli> I have a branch I'd like to open after those two PRs have been merged.
67 19:17 <jonasschnelli> Both PRs are pretty much straightforward with little to no impact.
68 19:18 <michaelfolkson> What was the thinking behind separating the serialization and deserialization into two PRs? Safety or just cleaner?
69 19:18 <pinheadmz> jonasschnelli: how often would a node generate a new key for DH? every new peer connection? every restart? just once?
70 19:18 <jonasschnelli> michaelfolkson: I always try to make PRs as small as possible if they allow to be splitted.
71 19:18 <jkczyz> jonatack: Wasn't able to test, only reviewied this morning. Busy last week on the job. :)
72 19:19 <jonatack> michaelfolkson: Not to speak for jonasschnelli but it's usually easier for smaller changes to get review and merged quickly.
73 19:19 <jonasschnelli> pinheadmz: it's described in the BIP. The DH key is generated once during the encryption handshake.
74 19:19 <jonasschnelli> But we do rekey every <1GB
75 19:19 <jonatack> michaelfolkson: it's generally good to keep PRs focused.
76 19:20 <michaelfolkson> Makes sense. The deserialization PR isn't much use without the serialization PR though, is it?
77 19:20 <jonasschnelli> large PRs in risky areas (like the p2p field) tend to loop forever in rebase limbo. :)
78 19:20 <jonatack> jkczyz: I understand :)
79 19:20 <jonasschnelli> michaelfolkson: They are independent.
80 19:20 <jonasschnelli> But for BIP324, we need both in the end
81 19:20 <jonasschnelli> But they are both refactors that are valid on their own
82 19:20 <michaelfolkson> Ok thanks
84 19:22 <jonatack> "Add p2p layer encryption with ECDH/ChaCha20Poly1305 #14032" ... on the road to implementing BIP324?
85 19:22 <zenogais> One comment: Separating serialization from message data structure is I'm thinking cleaner overall, even if this wasn't in order to add V2 serialization I still think it's a good refactor.
86 19:22 <jonasschnelli> jonatack: yes. Somehow. But they need refactor once 16202 lands.
87 19:22 <jonatack> jonasschnelli: right.
88 19:23 <jonatack> As the review club improves over time, it would be great to see us tackle the high priority PRs more often.
89 19:23 <jonasschnelli> in general, the criticism about no need for encryption (voskuli) since it's all public anyways looks valid at first sight,... but
90 19:24 <jonasschnelli> BIP324 eliminates passive observing and adds detectability of a MITM
91 19:24 <jonasschnelli> its opportunistic encryption and therefore a building block
92 19:25 <zenogais> jonasschnelli: It sounds like MITM detection is optional and must be done manually by peers though via side-channel?
93 19:25 <sosthene> It seems Eric is arguing that MITM detection would hard in practice, since nodes would need to exchange session ID over safe channels.
94 19:26 <jonasschnelli> zenogais: yes. BIP324 has no MITM detectability next to manual comparison of session IDs...
95 19:26 <jonasschnelli> but....
96 19:26 <jonasschnelli> An attacker needs to take the risk of being detected... which is a big difference to the current status quo.
97 19:26 <zenogais> Protection against passive observability is still probably a pretty big win for most users.
98 19:26 <jonasschnelli> he cannot know if an authentication happens after he has mitm-led the encryption
99 19:27 <jonasschnelli> usually detectable observing and tampering is much less valuable for an attacker
100 19:27 <next-defection> I disagree with the critiques brought up by evoskuli, P2P encryption increase observation costs (undeniable)
101 19:27 <zenogais> Yeah, and optional MITM detection is still better than V1.
102 19:28 <jonasschnelli> right now,... everyone on the network between two peers can delay a block... peers cannot detect that and the attacker can be sure of that.
103 19:28 <next-defection> even if "but MITM is still possible" "better is the enemy of good" situations are present
104 19:29 <jonasschnelli> Yes. And there are schemes (Pieter Wuille's for example) building on top of BIP324, that would broad scale detect MITMs.
105 19:29 <next-defection> the blockchain stores a limited set of information, it doesn't store the transport information which is highly valuable to well-funded attackers
106 19:30 <jonasschnelli> Yes. The p2p traffic is in general not considered public.
107 19:30 <jonasschnelli> But chain analysis will still be possible with BIP324.
108 19:31 <jonasschnelli> (since this is very likely active listening)
109 19:31 <sosthene> Does it still make sense to use Tor with this "native" encryption?
110 19:31 <sosthene> I mean, does it bring any extra security or is it negligible?
111 19:31 <jonasschnelli> Tor is an alternative,.. though a slightly different threat model (mostly censorship)
112 19:31 <jonasschnelli> But... the great thing is, we can run BIP324 through tor
113 19:31 <jonasschnelli> at no cost
114 19:31 <jonasschnelli> (faster, less bandwidth)
115 19:32 <jonatack> sosthene: IIRC, BIP 324 makes targeting more difficult for SPV nodes and those not using a VPN or Tor, but Tor is still valid with it.
116 19:32 <jonasschnelli> BIP324 is our own encryption with no dependencies. Simple and effective.
117 19:33 <jonasschnelli> Additionally, you can circumvent censorship or connectivity issues with tor.
118 19:34 <jonasschnelli> Right now,... there are a bunch of people connecting their mobile wallets (Green, BRD, Schildbach) to their nodes with a IPv4
119 19:34 <jonasschnelli> Which is _absolutely_ not secure.
120 19:34 <sosthene> jonatack: (I just finished running all the tests, finally! all passed :)
121 19:34 <jonasschnelli> (and those users assume to preserve privacy)
122 19:35 <jonasschnelli> however, to solve that non-tor connection, we need BIP150
123 19:35 <jonasschnelli> (which may follow after BIP324)
125 19:36 <jonatack> "This BIP describes a way for peers to authenticate to other peers to guarantee node ownership and/or allow peers to access additional or limited node services, without the possibility of fingerprinting."
126 19:37 <jonasschnelli> But one thing after another. :) PR16202 is a baby step forwards.
127 19:38 <michaelfolkson> Re the new message structure. Variable size message is big win. You also got rid of the magic bytes?
128 19:38 <jonasschnelli> Yes.
129 19:39 <michaelfolkson> Why? Not needed?
130 19:40 <michaelfolkson> I suppose not.
131 19:40 <jonasschnelli> I don't think it's needed. We have port for specific network. And it would fail anyways quickly.
132 19:40 <jonatack> jonasschnelli: When working on changes to the p2p network, are there any particular ways you test that the changes are working as intended?
133 19:41 <jonatack> We don't really have a framework yet for p2p simulation.
134 19:41 <jonasschnelli> jonatack: I think the test framework covers a lot. Though, running such PRs for a while (couple of days) on a node may reveal more details.
135 19:41 <zenogais> So net magic isn't needed to avoid peering with wrong network peers?
136 19:41 <jonasschnelli> zenogais: yes... I think that was the intention.
137 19:42 <jonasschnelli> But the cost of 4 bytes per every message (and ~50% are less then 64 bytes) is quite high.
138 19:42 <zenogais> Right, I suppose it could just be part of the handshake.
139 19:43 <jonasschnelli> Yes. Maybe this is not a bad idea.
140 19:44 <jonatack> Shorter INVs are a win since they make up so many of the messages.
141 19:44 <jonasschnelli> Yes.
142 19:44 <zenogais> Without seeing the network magic bytes at least once, I could see scenarios where nodes incorrectly peer - especially if they're sync from scratch.
143 19:44 <jonasschnelli> The network magic could maybe be added to the HKDF
145 19:44 <jonasschnelli> instead of `PRK = HKDF_EXTRACT(hash=SHA256, salt="BitcoinSharedSecret||INITIATOR_32BYTES_PUBKEY||RESPONDER_32BYTES_PUBKEY", ikm=ECDH_KEY)`
146 19:44 <jonasschnelli> make `PRK = HKDF_EXTRACT(hash=SHA256, salt="NETWORK_MAGIC||BitcoinSharedSecret||INITIATOR_32BYTES_PUBKEY||RESPONDER_32BYTES_PUBKEY", ikm=ECDH_KEY)`
147 19:45 <jonasschnelli> A handshake on the wrong network would just fail.
148 19:45 <zenogais> Ah nice, yeah that would work perfectly.
149 19:45 <jonasschnelli> Good point zenogais. I'll add others for feedback on that idea.
150 19:45 <jkczyz> jonatack: High priority because it allows two separate transport implementations would be my educated guess.
151 19:46 <jonasschnelli> High priority is a per-developer thing. I consider it high-prio/blocker for my work.
152 19:46 <jonasschnelli> Since we have no central planning, every active developer can add stuff to the high-prio list
153 19:46 <jonasschnelli> (to avoid central planning)
154 19:46 <michaelfolkson> It is a blocker to other PRs yeah
155 19:47 <michaelfolkson> And for a direction that seems to have broad consensus, bar Eric.
156 19:48 <jonasschnelli> I think most people agree that it is a useful thing.
157 19:48 <jonasschnelli> We have also already merged the cryptographic primitives (chacha20, poly1305, hkdf) as well as the AEAD
158 19:48 <jonasschnelli> (which are less risky since they are only used in tests).
159 19:50 <jonatack> Improving privacy is so essential. A thousand thank you's, Jonas, for your work on this.
160 19:50 <jonasschnelli> Thanks for reviewing guys!
161 19:50 <zenogais> Yeah, this is great work, really looking forward to the V2 transport stuff.
162 19:50 <jonatack> The best way each of us can help jonasschnelli move BIP 324 forward, is to review these PRs.
163 19:50 <jonatack> Let's all get our review in tomorrow for those who haven't yet.
164 19:51 <sosthene> jonasschnelli: Thanks, it was great to have you here.
165 19:51 <jonatack> Ten minutes left. I'm glad we've been able to discuss these important issues. Any other comments, feedback, or questions?
166 19:51 <michaelfolkson> In terms of future work, jonatack you referred to 64 byte public keys and randomizing ports. Can you elaborate?
167 19:52 <michaelfolkson> Also found it interesting that there could be different authentication schemes.
168 19:52 <lightlike> do you think that v2 will take over completely, or do you think both v1 and v2 will coexist for a long time?
169 19:52 <jonasschnelli> I think so.
170 19:52 <jonatack> michaelfolkson: You mean in my slides? They were only a quick summary of jonasschnelli's complete slides.
171 19:52 <jonatack> michaelfolkson:, lightlike: Great questions.
172 19:53 <michaelfolkson> Ah ok. I'll look back at Jonas' slides.
173 19:53 <jonasschnelli> I think randomising ports would go into the direction of censorship resistance which is very complicated and I think this should be done on other layers like tor
174 19:54 <jonasschnelli> even with randomised ports, by looking at the package size and burst-characteristics, identifying bitcoin traffic is trivial.
175 19:55 <jonasschnelli> lightlike: back to the v1/v2 question. I think, when there are enough peers supporting v2, people may disable v1
176 19:55 <jonasschnelli> but sadly its a slow transition
177 19:56 <jonasschnelli> (unless v1 gets exploited which is unlikely).
178 19:57 <jonatack> jonasschnelli: Would you see v2 on by default in future releases? Or after a certain level of v2 adoption?
179 19:57 <jonasschnelli> I hope... I guess this is the plan. Though time will show and it needs enough people willing to run v2.
180 19:58 <jonatack> Privacy may turn out to be a good motivation.
181 19:58 <michaelfolkson> Less systemic risk if half the network is running v1 and half is running v2 rather than everyone running v2 :)
182 19:58 <jonatack> If anyone wants to continue with other questions after we wrap up, I'll hang around.
183 19:59 <jkczyz> jonasschnelli: I have a suggestion regarding TransportDeserializer's interface but will leave a new comment on the PR.
184 19:59 <jonasschnelli> thanks jkczyz
185 19:59 <jonasschnelli> I'll try to tackle the comments by tomorrow
186 20:00 <jonatack> Thanks, everyone, for participating this week!
187 20:00 <jonatack> Thanks jonasschnelli for coming by!
188 20:00 <michaelfolkson> Thanks both
189 20:00 <lightlike> thanks jonatack, jonasschnelli!
190 20:00 <jonasschnelli> thanks all
191 20:00 <zenogais> Thanks all, appreciate you fielding our questions jonasschnelli
192 20:00 <sebastiavstaa> thanks all
Erratum
At 19:52 in the meeting log, I wrote that more information about the 64-byte
public keys and port randomisation mentioned in my presentation could be found
in Jonas Schnelli’s slides. More precisely, those discussions are from Jonas
Schnelli’s P2P Encryption presentation at
CoreDev
in June 2019. I added a link to that resource to the notes above.
Cheers - Jon Atack