Refactor network message deserialization (p2p)

https://github.com/bitcoin/bitcoin/pull/16202

Host: jonatack  -  PR author: jonasschnelli

Notes

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

  119:00 <jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club.
  219:00 <fjahr> hi
  319:00 <pinheadmz> hi
  419:00 <sebastiavstaa> hi
  519: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!
  619:00 <gorazdko> hi
  719:01 <michaelfolkson> Holla
  819:01 <jonatack> This week, we're talking about PR16202 - "Refactor network message deserialization" by jonasschnelli.
  919:01 <jonatack> The PR is part of on-going work by jonasschnelli on BIP 324: Version 2 Peer-to-Peer Message Transport Protocol.
 1019:01 <lightlike> hi
 1119:01 <zenogais> hi all
 1219:02 <sosthene> hi
 1319: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.
 1419:03 <jonatack> Let's get started. Did you review the PR? What are your thoughts... Concept ACK, approach ACK, tested ACK, NACK?
 1519:03 <fjahr> tested ACK although I did not have much time to look at the code
 1619:04 <zenogais> Tested ACK. I was able to review the whole PR as well as test it.
 1719:04 <lightlike> concept ack, haven't understood the code yet completely.
 1819:04 <jonatack> fjahr: Excellent.
 1919:04 <jonatack> etscrivner: jkczyz: Great to see your reviews of the PR on GitHub!
 2019:04 âš¡ zenogais is etscrivner btw
 2119:04 <jonatack> zenogais: thanks!
 2219:05 <jonatack> How did you all test the changes?
 2319:05 <sebastiavstaa> built and ran the tests.
 2419: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.
 2519:05 <jonatack> Did anyone add any tests, assertions, or custom logging to test the PR?
 2619:06 <pinheadmz> built and tested, broke the test and tested the test :-)
 2719:06 <pinheadmz> just the one test that was altered, the error message is changed
 2819:06 <pinheadmz> I assume because its a refactor, the existing tests cover and protect against regression.
 2919:06 <jonatack> On my end, I added some logging for sanity checks https://gist.github.com/jonatack/d6a228878e6ef5f582ff75c974f2d6c3
 3019:06 <jonatack> and would like to run the changes through gdb tomorrow for my review, since p2p can be a risky area.
 3119: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?
 3219:06 <sosthene> (if anyone knows what I'm talking about and can link to evoskuil arguments I would be very grateful)
 3319:07 <jonatack> sosthene: I wasn't aware of controversy regarding BIP 324.
 3419:08 <zenogais> Might be useful to have some sort of P2P fuzzing also (if it doesn't already exist).
 3519:08 <pinheadmz> jonatack: that's cool - these properties i.e. `msg.m_command` are new to this PR right?
 3619:08 <sosthene> jonatack: I guess that's evoskuil vs the rest of the world then
 3719:09 <jonatack> pinheadmz: yeah, I want to see the values as a sanity check
 3819:09 <jonatack> sosthene: if you have a link please share!
 3919:09 <michaelfolkson> Nor me, sorry sosthene. Perhaps contact him?
 4019:10 <jonatack> zenogais: Fuzzing is one area I think the maintainers would be very happy to see additional tests or data for.
 4119:10 <jonatack> MarcoFalke is the primary contact for that. See also doc/fuzzing.md.
 4219:10 <michaelfolkson> I didn't stumble on any conceptual downsides to this BIP
 4319:11 <zenogais> jonatack: Thanks, will give it a look. Have been thinking about P2P fuzzing specifically for a couple of weeks now.
 4419:12 <jonasschnelli> hi
 4519:12 <jkczyz> hi
 4619:12 âš¡ zenogais waves
 4719:12 <jonatack> jonasschnelli: Hi, thanks for coming by!
 4819:13 <jonasschnelli> Sorry for being late
 4919:13 <jonatack> jkczyz: Hi, and thank you for reviewing the PR.
 5019:13 âš¡ jonasschnelli is reading back
 5119:13 âš¡ next-defection thumbs up to fuzzing
 5219:13 <jonasschnelli> Should I explain why I did this PR (PR16202)?
 5319:13 <jonatack> jonasschnelli: Would you like to say anything about this PR, or the next steps for BIP 324?
 5419:13 <jonatack> jonasschnelli: Yes, please!
 5519:13 <lightlike> here is a link to the points by evoskuil: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-March/016810.html
 5619:14 <sosthene> I found it https://twitter.com/evoskuil/status/825089930221064192
 5719:14 <jonasschnelli> PR16202 is just about adding flexibility for allowing multiple transport protocol
 5819:14 <jonatack> jkczyz: (we were discussing how people tested the PR, if you would like to share what you did to test it)
 5919:15 <sosthene> and this wrt BIP151 https://github.com/bitcoin/bips/wiki/Comments:BIP-0151
 6019: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).
 6119:15 <jonasschnelli> evoskuil's points are valid,... not sure if the overall desire (or dislike) of encryption is something we should discuss here.
 6219:15 <jonasschnelli> But we can...
 6319:15 <jonatack> jonasschnelli: What are the next BIP324 PRs we should review after this one is hopefully in?
 6419:16 <jonatack> jonasschnelli: and after PR #16562: Refactor message transport packaging https://github.com/bitcoin/bitcoin/pull/16562
 6519:16 <jonasschnelli> Yes... that one is in alignment with the deserialization.
 6619:16 <jonasschnelli> I have a branch I'd like to open after those two PRs have been merged.
 6719:17 <jonasschnelli> Both PRs are pretty much straightforward with little to no impact.
 6819:18 <michaelfolkson> What was the thinking behind separating the serialization and deserialization into two PRs? Safety or just cleaner?
 6919:18 <pinheadmz> jonasschnelli: how often would a node generate a new key for DH? every new peer connection? every restart? just once?
 7019:18 <jonasschnelli> michaelfolkson: I always try to make PRs as small as possible if they allow to be splitted.
 7119:18 <jkczyz> jonatack: Wasn't able to test, only reviewied this morning. Busy last week on the job. :)
 7219:19 <jonatack> michaelfolkson: Not to speak for jonasschnelli but it's usually easier for smaller changes to get review and merged quickly.
 7319:19 <jonasschnelli> pinheadmz: it's described in the BIP. The DH key is generated once during the encryption handshake.
 7419:19 <jonasschnelli> But we do rekey every <1GB
 7519:19 <jonatack> michaelfolkson: it's generally good to keep PRs focused.
 7619:20 <michaelfolkson> Makes sense. The deserialization PR isn't much use without the serialization PR though, is it?
 7719:20 <jonasschnelli> large PRs in risky areas (like the p2p field) tend to loop forever in rebase limbo. :)
 7819:20 <jonatack> jkczyz: I understand :)
 7919:20 <jonasschnelli> michaelfolkson: They are independent.
 8019:20 <jonasschnelli> But for BIP324, we need both in the end
 8119:20 <jonasschnelli> But they are both refactors that are valid on their own
 8219:20 <michaelfolkson> Ok thanks
 8319:21 <jonatack> jonasschnelli: Is this PR still a prerequisite for https://github.com/bitcoin/bitcoin/pull/14032
 8419:22 <jonatack> "Add p2p layer encryption with ECDH/ChaCha20Poly1305 #14032" ... on the road to implementing BIP324?
 8519: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.
 8619:22 <jonasschnelli> jonatack: yes. Somehow. But they need refactor once 16202 lands.
 8719:22 <jonatack> jonasschnelli: right.
 8819:23 <jonatack> As the review club improves over time, it would be great to see us tackle the high priority PRs more often.
 8919:23 <jonasschnelli> in general, the criticism about no need for encryption (voskuli) since it's all public anyways looks valid at first sight,... but
 9019:24 <jonasschnelli> BIP324 eliminates passive observing and adds detectability of a MITM
 9119:24 <jonasschnelli> its opportunistic encryption and therefore a building block
 9219:25 <zenogais> jonasschnelli: It sounds like MITM detection is optional and must be done manually by peers though via side-channel?
 9319: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.
 9419:26 <jonasschnelli> zenogais: yes. BIP324 has no MITM detectability next to manual comparison of session IDs...
 9519:26 <jonasschnelli> but....
 9619:26 <jonasschnelli> An attacker needs to take the risk of being detected... which is a big difference to the current status quo.
 9719:26 <zenogais> Protection against passive observability is still probably a pretty big win for most users.
 9819:26 <jonasschnelli> he cannot know if an authentication happens after he has mitm-led the encryption
 9919:27 <jonasschnelli> usually detectable observing and tampering is much less valuable for an attacker
10019:27 <next-defection> I disagree with the critiques brought up by evoskuli, P2P encryption increase observation costs (undeniable)
10119:27 <zenogais> Yeah, and optional MITM detection is still better than V1.
10219: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.
10319:28 <next-defection> even if "but MITM is still possible" "better is the enemy of good" situations are present
10419:29 <jonasschnelli> Yes. And there are schemes (Pieter Wuille's for example) building on top of BIP324, that would broad scale detect MITMs.
10519: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
10619:30 <jonasschnelli> Yes. The p2p traffic is in general not considered public.
10719:30 <jonasschnelli> But chain analysis will still be possible with BIP324.
10819:31 <jonasschnelli> (since this is very likely active listening)
10919:31 <sosthene> Does it still make sense to use Tor with this "native" encryption?
11019:31 <sosthene> I mean, does it bring any extra security or is it negligible?
11119:31 <jonasschnelli> Tor is an alternative,.. though a slightly different threat model (mostly censorship)
11219:31 <jonasschnelli> But... the great thing is, we can run BIP324 through tor
11319:31 <jonasschnelli> at no cost
11419:31 <jonasschnelli> (faster, less bandwidth)
11519: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.
11619:32 <jonasschnelli> BIP324 is our own encryption with no dependencies. Simple and effective.
11719:33 <jonasschnelli> Additionally, you can circumvent censorship or connectivity issues with tor.
11819:34 <jonasschnelli> Right now,... there are a bunch of people connecting their mobile wallets (Green, BRD, Schildbach) to their nodes with a IPv4
11919:34 <jonasschnelli> Which is _absolutely_ not secure.
12019:34 <sosthene> jonatack: (I just finished running all the tests, finally! all passed :)
12119:34 <jonasschnelli> (and those users assume to preserve privacy)
12219:35 <jonasschnelli> however, to solve that non-tor connection, we need BIP150
12319:35 <jonasschnelli> (which may follow after BIP324)
12419:36 <jonatack> BIP150: https://github.com/bitcoin/bips/blob/master/bip-0150.mediawiki
12519: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."
12619:37 <jonasschnelli> But one thing after another. :) PR16202 is a baby step forwards.
12719:38 <michaelfolkson> Re the new message structure. Variable size message is big win. You also got rid of the magic bytes?
12819:38 <jonasschnelli> Yes.
12919:39 <michaelfolkson> Why? Not needed?
13019:40 <michaelfolkson> I suppose not.
13119:40 <jonasschnelli> I don't think it's needed. We have port for specific network. And it would fail anyways quickly.
13219: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?
13319:41 <jonatack> We don't really have a framework yet for p2p simulation.
13419: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.
13519:41 <zenogais> So net magic isn't needed to avoid peering with wrong network peers?
13619:41 <jonasschnelli> zenogais: yes... I think that was the intention.
13719:42 <jonasschnelli> But the cost of 4 bytes per every message (and ~50% are less then 64 bytes) is quite high.
13819:42 <zenogais> Right, I suppose it could just be part of the handshake.
13919:43 <jonasschnelli> Yes. Maybe this is not a bad idea.
14019:44 <jonatack> Shorter INVs are a win since they make up so many of the messages.
14119:44 <jonasschnelli> Yes.
14219: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.
14319:44 <jonasschnelli> The network magic could maybe be added to the HKDF
14419:44 <jonasschnelli> https://gist.github.com/jonasschnelli/c530ea8421b8d0e80c51486325587c52#symmetric-encryption-cipher-keys
14519:44 <jonasschnelli> instead of `PRK = HKDF_EXTRACT(hash=SHA256, salt="BitcoinSharedSecret||INITIATOR_32BYTES_PUBKEY||RESPONDER_32BYTES_PUBKEY", ikm=ECDH_KEY)`
14619:44 <jonasschnelli> make `PRK = HKDF_EXTRACT(hash=SHA256, salt="NETWORK_MAGIC||BitcoinSharedSecret||INITIATOR_32BYTES_PUBKEY||RESPONDER_32BYTES_PUBKEY", ikm=ECDH_KEY)`
14719:45 <jonasschnelli> A handshake on the wrong network would just fail.
14819:45 <zenogais> Ah nice, yeah that would work perfectly.
14919:45 <jonasschnelli> Good point zenogais. I'll add others for feedback on that idea.
15019:45 <jkczyz> jonatack: High priority because it allows two separate transport implementations would be my educated guess.
15119:46 <jonasschnelli> High priority is a per-developer thing. I consider it high-prio/blocker for my work.
15219:46 <jonasschnelli> Since we have no central planning, every active developer can add stuff to the high-prio list
15319:46 <jonasschnelli> (to avoid central planning)
15419:46 <michaelfolkson> It is a blocker to other PRs yeah
15519:47 <michaelfolkson> And for a direction that seems to have broad consensus, bar Eric.
15619:48 <jonasschnelli> I think most people agree that it is a useful thing.
15719:48 <jonasschnelli> We have also already merged the cryptographic primitives (chacha20, poly1305, hkdf) as well as the AEAD
15819:48 <jonasschnelli> (which are less risky since they are only used in tests).
15919:50 <jonatack> Improving privacy is so essential. A thousand thank you's, Jonas, for your work on this.
16019:50 <jonasschnelli> Thanks for reviewing guys!
16119:50 <zenogais> Yeah, this is great work, really looking forward to the V2 transport stuff.
16219:50 <jonatack> The best way each of us can help jonasschnelli move BIP 324 forward, is to review these PRs.
16319:50 <jonatack> Let's all get our review in tomorrow for those who haven't yet.
16419:51 <sosthene> jonasschnelli: Thanks, it was great to have you here.
16519:51 <jonatack> Ten minutes left. I'm glad we've been able to discuss these important issues. Any other comments, feedback, or questions?
16619:51 <michaelfolkson> In terms of future work, jonatack you referred to 64 byte public keys and randomizing ports. Can you elaborate?
16719:52 <michaelfolkson> Also found it interesting that there could be different authentication schemes.
16819: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?
16919:52 <jonasschnelli> I think so.
17019:52 <jonatack> michaelfolkson: You mean in my slides? They were only a quick summary of jonasschnelli's complete slides.
17119:52 <jonatack> michaelfolkson:, lightlike: Great questions.
17219:53 <michaelfolkson> Ah ok. I'll look back at Jonas' slides.
17319: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
17419:54 <jonasschnelli> even with randomised ports, by looking at the package size and burst-characteristics, identifying bitcoin traffic is trivial.
17519:55 <jonasschnelli> lightlike: back to the v1/v2 question. I think, when there are enough peers supporting v2, people may disable v1
17619:55 <jonasschnelli> but sadly its a slow transition
17719:56 <jonasschnelli> (unless v1 gets exploited which is unlikely).
17819:57 <jonatack> jonasschnelli: Would you see v2 on by default in future releases? Or after a certain level of v2 adoption?
17919:57 <jonasschnelli> I hope... I guess this is the plan. Though time will show and it needs enough people willing to run v2.
18019:58 <jonatack> Privacy may turn out to be a good motivation.
18119:58 <michaelfolkson> Less systemic risk if half the network is running v1 and half is running v2 rather than everyone running v2 :)
18219:58 <jonatack> If anyone wants to continue with other questions after we wrap up, I'll hang around.
18319:59 <jkczyz> jonasschnelli: I have a suggestion regarding TransportDeserializer's interface but will leave a new comment on the PR.
18419:59 <jonasschnelli> thanks jkczyz
18519:59 <jonasschnelli> I'll try to tackle the comments by tomorrow
18620:00 <jonatack> Thanks, everyone, for participating this week!
18720:00 <jonatack> Thanks jonasschnelli for coming by!
18820:00 <michaelfolkson> Thanks both
18920:00 <lightlike> thanks jonatack, jonasschnelli!
19020:00 <jonasschnelli> thanks all
19120:00 <zenogais> Thanks all, appreciate you fielding our questions jonasschnelli
19220: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