BIP324: Enable v2 P2P encrypted transport

[dhruv]

trafficstars

This PR brings together all other BIP324 PRs and enables v2 encrypted P2P transport.

Builds on top of open PRs: #25354, #25361, #23233, #23561, #23432. It looks like there's a lot of commits, but only the last 11 commits belong in this PR. The rest will be merged with upstream PRs.

Running a v2 node

Get the code

git remote add bip324 [email protected]:dhruv/bitcoin.git
git fetch bip324
git checkout bip324/bip324-enable

Build for your OS

Follow the appropriate instructions here

Run the node

src/bitcoind -conf=CONFIG_FILE -v2transport=1

Connect with a friend's v2 node

src/bitcoin-cli -conf=CONFIG_FILE addnode "FRIEND_IP:FRIEND_PORT" "add" true

The last parameter(p2p_v2:true) signals to your node that the peer is running a v2 supportive client and we should attempt to make an encrypted P2P connection (you're simulating the NODE_P2P_V2 service flag advertisement manually). Should that fail however (say because the peer told you mistakenly, lied, etc.), this code will downgrade the connection to unencrypted v1 transport.

Things you are helpful to test

• If your friend's node is a v2 node, you can see with wireshark that the bytes are pseudorandom (the easiest way to confirm this is that with a v1 connection, wireshark will tell you it has detected a Bitcoin connection and it'll even parse out the metadata like message type, etc; with v2, wireshark has no idea -- of course that could be because wireshark does simply not know v2, but it is because the bytestream is pseudorandom)
• Add another peer that is actually v1, but try addnode still indicating v2 support. You should see with wireshark that after a failed attempt at a v2 handshake, the connection is downgraded to unencrypted v1 and wireshark can parse it.

I've been told there are v2 nodes running at (happy to update the list as more people run persistent v2 nodes; message me and I'll add it here):

jdcoysubtxazi7dketpyb5rnjorvxad4onftveohash2pdwkgw4bvnqd.onion
rp7k2go3s5lyj3fnj6zn62ktarlrsft2ohlsxkyd7v3e3idqyptvread.onion:8333

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	jonatack, dergoegge

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27294 (refactor: Move chain names to the kernel namespace by TheCharlatan)
• #27257 (refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg by dergoegge)
• #27254 (refactor: Extract util/fs from util/system by TheCharlatan)
• #27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
• #26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
• #26684 (bench: add readblock benchmark by andrewtoth)
• #26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
• #26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
• #26366 (p2p, rpc, test: getpeerinfo improv + add test coverage for invalid command by brunoerg)
• #26283 (p2p: Fill reconciliation sets and request reconciliation (Erlay) by naumenkogs)
• #26261 (p2p: cleanup LookupIntern, Lookup and LookupHost by brunoerg)
• #25907 (wallet: rpc to add automatically generated descriptors by achow101)
• #25832 (tracing: network connection tracepoints by 0xB10C)
• #25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
• #25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild)
• #25284 (net: Use serialization parameters for CAddress serialization by MarcoFalke)
• #25268 (refactor: Introduce EvictionManager by dergoegge)
• #23352 (test: Extend stale_tip_peer_management test by MarcoFalke)
• #22341 (rpc: add getxpub by Sjors)
• #18470 (test: Extend stale tip test by MarcoFalke)
• #10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[dhruv]

Bringing in upstream changes from #20962. Ready for further review.

[dhruv]

Rebased to resolve conflicts with #21160.

Addressed comments from @mzumsande and @kallewoof in #23900.

Ready for further review.

git range-diff 171f6f269 4a38f3e be9a830

[dhruv]

Rebased. Ready for further review.

git range-diff 171f6f269 be9a830 f7cae59

[jonatack]

Concept ACK

[laanwj]

I'm testing this on one of my nodes. Feel free to message me if you want to connect.

Is there any way to see (in, say, getpeerinfo) whether a connection uses P2P v2?

[dhruv]

Thanks, @laanwj !

Is there any way to see (in, say, getpeerinfo) whether a connection uses P2P v2?

That's a good idea, I've added it here: https://github.com/bitcoin/bitcoin/projects/17#card-80335229 I'll work on it once I'm done with the basics. For now, can you tcpdump -Apn -i any -w - -U port <PORT> | wireshark -k -i - to see the differences between v1 and v2?

[dhruv]

Rebased. git range-diff e0680bbce f7cae59 d0aef7a1d

Ready for further review.

[0xB10C]

I'm running d0aef7a1d2f324aad923f734247d34baf9a50ae8 on the node behind https://bitcoind.observer. DM me for the IP.

[0xB10C]

From what I can tell, my node fails to connect to another p2p v2 peer (likely @laanwj's, we haven't fully confirmed that yet) and stops accepting inbound messages from any other peer cutting it off from the network entirely.

Logs:

(node operating as ususal)
..
2022-04-28T12:25:39Z got inv: wtx c0b2943d5e9bf180088a6683011c04bbee1e9485fdc35eae408cd30614e392a2  have peer=6
2022-04-28T12:25:39Z got inv: wtx 842fb44f4669248d8299523f6f2660c4717a11293b986973cc227ace30af8406  have peer=6
2022-04-28T12:25:39Z sending inv (289 bytes) peer=6
2022-04-28T12:25:39Z Making feeler connection to XXX.XXX.XXX.XXX:8333
2022-04-28T12:25:39Z trying connection XXX.XXX.XXX.XXX:8333 lastseen=188.3hrs
2022-04-28T12:25:39Z Added connection to XXX.XXX.XXX.XXX:8333 peer=31
2022-04-28T12:25:39Z socket closed for peer=31
2022-04-28T12:25:39Z disconnecting peer=31                      <-------- breaks here
2022-04-28T12:25:40Z sending inv (325 bytes) peer=4
2022-04-28T12:25:41Z sending inv (361 bytes) peer=27
2022-04-28T12:25:41Z sending inv (793 bytes) peer=24
2022-04-28T12:25:41Z Requesting tx 1bd8401dac5fb4b922a95c6c939cbf901d946ab3a6d2c811fd498f8c743bdd23 peer=3
2022-04-28T12:25:41Z Requesting tx ca432d826b5163bf78320eb9cecf2f47aab8cb8f176fa67e4991e659e69e7c6e peer=3
2022-04-28T12:25:41Z sending getdata (73 bytes) peer=3
2022-04-28T12:25:43Z sending inv (109 bytes) peer=3
2022-04-28T12:25:44Z sending inv (397 bytes) peer=11
2022-04-28T12:25:44Z sending inv (721 bytes) peer=18
2022-04-28T12:25:46Z sending inv (397 bytes) peer=26
2022-04-28T12:25:47Z sending inv (397 bytes) peer=13
2022-04-28T12:25:47Z sending inv (541 bytes) peer=5
..
(no more inbound messages received)

[dhruv]

@0xB10C Thanks for the logs. I'll look into it. We are making some large changes to the spec at the moment, so I'm working on updating the BIP and some of the branches are actually out of sync now. I'll be back in touch soon with updated code.

[theStack]

@dhruv: I'd have interest in testing and reviewing BIP324. Is there anything useful we can do for now or is it better to just wait for the final spec / updated BIP? (Maybe some PRs are not affected by the changes and can be already reviewed?)

[dhruv]

@theStack awesome! I'm working on changes you mentioned. In the meantime #23432 and upstream elligator squared changes in secp are unaffected and you can start review there. #23561 will have very minimal changes so you can review now and the updates will be easy to review. #20962 and #23233 will be radically different.

[laanwj]

or is it better to just wait for the final spec / updated BIP

Waiting is not a good option :smile: I think it's unavoidable that the both the BIP and implementation will still evolve a bit during review and testing, as new issues are discovered. In FOSS there's pretty much never a straightforward spec → implementation flow.

[theStack]

or is it better to just wait for the final spec / updated BIP

Waiting is not a good option 😄 I think it's unavoidable that the both the BIP and implementation will still evolve a bit during review and testing, as new issues are discovered. In FOSS there's pretty much never a straightforward spec → implementation flow.

Agree! My concern was that the undergoing spec changes are possibly so intense that the current publicly available BIP barely has any resemblance with the ~~final~~ updated version anymore and reviewing/testing current PRs would not be a well-invested time. But fortunately, that doesn't seem to be the case (https://github.com/bitcoin/bitcoin/pull/24545#issuecomment-1141408247, https://bip324.com/sections/code-review/), i.e. no excuses for waiting :)

[0xf0xx0]

From what I can tell, my node fails to connect to another p2p v2 peer (likely @laanwj's, we haven't fully confirmed that yet) and stops accepting inbound messages from any other peer cutting it off from the network entirely.

I also seem to have this issue. Relevant logs:

2022-07-22T13:55:24Z received: inv (109 bytes) peer=19
2022-07-22T13:55:24Z got inv: tx cae283722e02ae9aa0e0da00b8064b76bf05583d17d8b8b4bc8dc84578a77a3b  new peer=19
2022-07-22T13:55:24Z got inv: tx ee58f2752d52e7fc79e9e19ddc9b592c4fb105cced7f5c245175eb0615bf2139  new peer=19
2022-07-22T13:55:24Z got inv: tx 3f1a7195723b7a711aac44c43ff12a6a3e360bacf52f685d0c5f1e706c64de3f  new peer=19
2022-07-22T13:55:24Z socket closed for peer=16
2022-07-22T13:55:24Z disconnecting peer=16
2022-07-22T13:55:24Z Cleared nodestate for peer=16
2022-07-22T13:55:25Z received: version (102 bytes) peer=23
2022-07-22T13:55:25Z sending wtxidrelay (0 bytes) peer=23
2022-07-22T13:55:25Z sending sendaddrv2 (0 bytes) peer=23
2022-07-22T13:55:25Z sending verack (0 bytes) peer=23
(no more inbound)

Full log

[dhruv]

Changes in this push:

• Bring in upstream changes
• Added shapable handshake per changes developed in the working group.
• Added a functional test

Ready for further review.

Next up, I'll be looking into the review from @dergoegge and the logs from @0xB10C and @ExperiBass.

[dhruv]

@ExperiBass, I have worked with @0xB10C and @stratospher to find and fix the thread locking issue. Are you able to try again with the latest code?

This push:

• Fixed thread locking issue that happened when the Initiator tried to downgrade a v2 BIP324 connection to a v1 plaintext connection whilst the outbound slots were full.
• Fixed another bug where the Initiator would bot send the VERSION message on a v2 connection if there were no more inbound bytes to process. This wasn't caught by the functional test because there were more inbound bytes to process.

Next up, comments from @dergoegge. Ready for further review.

[0xf0xx0]

@ExperiBass, I have worked with @0xB10C and @stratospher to find and fix the thread locking issue. Are you able to try again with the latest code?

So far so good, i havent seen any crashes for the past 24 hours. Looks like its fine 👍🏾

[dhruv]

Latest push:

• Rebased
• Nomenclature update
• Bring in upstream changes
• Interpret encrypted length as length of contents rather than header + contents
• Rekey salt length is now 23 bytes instead of 32 bytes
• New test vectors, including a trivial one-byte vector and a large vector
• Authentication of garbage sent by initiator in the shapable handshake

[dhruv]

Latest push brings all the code up to date with the released BIP draft. Changes:

Handshake shapability

• Bidirectional garbage and terminators
• Early key response by responder upon first mismatching byte
• Allowing for multiple rounds in key exchange
• Remove bias away from NETWORK_MAGIC || "version\x00" for ElligatorSwift encodings
• Remove garbage terminator from the garbage authentication packet as it is implicitly authenticated

Bugfixes

• An already established v1 connection peer should not send out an ellswift key if a message has a bad v1 header
• The previous changes to test_framewotk.py made all tests fail with a small probability, that's now fixed

Test vectors

• One-sided test vectors consistent with the BIP (the BIP actually needs updating which is WIP)
• Test vectors in a json file instead of making net_tests.cpp even longer

RPC

• Expose transport protocol type via getpeerinfo
• Expose v2 session id via getpeerinfo

Also rebased the PR.

Ready for further review. Changes in this push are not backwards compatible with the previous code(handshake shapability changes) so it'd be nice if everyone running the branch could run the latest version.

[dhruv]

Latest push fixes a bug that was causing non-deterministic integration test failures. Initially, I thought it was intermittent integration test failures but it was a bug in the code for BIP324 implementation. This comment explains for interested readers and documentation.

v2 supportive inbound clients do not know when the first bytes come in whether the initiator intends on a v1 or a v2 connection. So if they have not received a VERSION message and the first message is not a VERSION message (assuming the v1 message header is valid, except for the checksum), the bytes must be interpreted as an ellswift pubkey (this is also the case if the previous bytes in the header are invalid, like say the network magic bytes are not correct). However, I was doing this by checking nVersion == 0 rather than mapRecvBytesPerMsgType[VERSION] == 0 which caused a race condition because a VERSION message could be received, and not yet processed (which happens asynchronously). The way this manifest was that a P2PConnection would send VERSION to a TestNode, TestNode would send VERSION back, and the P2PConnection would send a VERACK. When that VERACK was received at the TestNode, there was a chance that the VERSION message was queued, but not processed yet, so nVersion was still zero. The code would interpret this as a bad first message and disconnect causing add_p2p_connection() to fail with assert p2p_conn.is_connected while it was in wait_until(VERACK).

The other problem this discovered was that there is no need to change the behavior of v1 clients where the code is v2 aware, but not signalling support.

You can see the bug fix with git diff 601fb3a 4f23845

Ready for further review.

[dhruv]

Pushed again: tests against previous releases (which don't run on my personal fork for some reason) were failing because of the new optional argument to rpc:addnode not existing in older clients.

git diff 4f23845 e823a19

[dhruv]

Latest push adds a downgrade to v1 upon inactivity within the peer timeout period (the case where a peer is falsely advertised as v2 supportive, but also doesn't disconnect when it received an ellswift pubkey). Also changed the order of commits.

git diff e823a19d59 062e51c2f0

[jamesob]

Docker image (jamesob/bitcoind:bip324-enable) built for this branch as of 062e51c2f051dc68197bc21535fd06b3e01bdc21, in case that helps anyone test.

[rot13maxi]

built the dhruv:bip324-enable at commit 062e51c2f051dc68197bc21535fd06b3e01bdc21 on an m1 mac. No problems building. Started a new node with -v2transport=1 turned on. At different points of IBD, I added/removed peers to make sure I could get blocks from all v1 peers, v1 + v2 peers, v2-only peers. Once the node was synced, connected to only v2 peers and tested broadcasting transactions. Also compared mempool contents and chaintips with a v23 node running the v1 transport. So far, everything works as expected.

[jsarenik]

Thanks for doing this! I compiled it myself with one little unrelated patch on Alpine Linux (musl-libc) x86_64. It is running at be.anyone.eu.org, all three Bitcoin networks ­— mainnet, testnet and signet (and also Plebnet Playground at explorer.plebnet.fun, which is a custom signet). IPv4, IPv6, Tor, I2P. Feel free to test with my node running on residential 30 Mbps DSL connection 24/7.

UPDATE: Added IPv6 (via tunnelbroker.net) which did not reliably work before BIP324 and for some time I was assuming that unencrypted Bitcoin traffic on port 8333 is blocked by HE.net, but now it works well. May not be really related to BIP324. On all networks.

UPDATE2: Peers can come and verify the session IDs (or for signet). The addresses without square brackets and what follows after the closing square bracket are MD5 hashed, and only first 16 bytes of the hash are shown there. Example: solinxtxcc7a2qzs42tjj3zayaxjvisvskptuqcyikwi35kikota.b32.i2p:0 hashes to 992dfda709f41ad9.

[dhruv]

Latest push:

• Bringing in upstream optimizations to https://github.com/bitcoin-core/secp256k1/pull/1129

[dhruv]

Latest push:

• Bringing in upstream rebase to #26153
• Fixed a bug where a disconnection after receiving some bytes but prior to the end of the key exchange phase would result in a downgrade when it should not

[jsarenik]

Updated my nodes to 8f7714e33804b6cf96fedbe05751d1a700e1e8d1

Node1's addresses (no Tor anymore) are visible via node-details.

Node2's addresses are:

• Tor: 3xept3fmslplcm4hu4gnbpquz3d5twqg2m5hqeimbvuzykiuxan5kgad.onion
• Yggdrasil (which thinks it is IPv6): 200:40c3:3ffe:680:a5b2:9d41:3928:fb73