BIP324 connection support #28196

sipa · 2023-08-01T17:56:58Z

This is part of #27634.

This implements the BIP324 v2 transport (which implements all of what the BIP calls transport layer and application layer), though in a non-exposed way. It is tested through an extensive fuzz test, which verifies that v2 transports can talk to v2 transports, and v1 transports can talk to v2 transports, and a unit test that exercises a number of unusual scenarios. The transport is functionally complete, including:

Autodetection of incoming V1 connections.
Garbage, both sending and receiving.
Short message type IDs, both sending and receiving.
Ignore packets (receiving only, but tested in a unit test).
Session IDs are visible in getpeerinfo output (for manual comparison).

Things that are not included, left for future PRs, are:

Actually using the v2 transport for connections.
Support for the NODE_P2P_V2 service flag.
Retrying downgrade to V1 when attempted outbound V2 connections immediately fail.
P2P functional and unit tests

DrahtBot · 2023-08-01T17:57:01Z

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
ACK	theStack, mzumsande, naumenkogs
Concept ACK	Sjors
Stale ACK	vincenzopalazzo, vasild

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

Conflicts

No conflicts as of last run.

mzumsande

Code Review ACK db9888f

I reviewed the code again and did a little bit of manual mutation testing (checking that the fuzz test fails if I break things).
The nits can be ignored.

mzumsande · 2023-09-04T19:50:01Z

src/test/net_tests.cpp

+ * Each V2TransportTester encapsulates a V2Transport (the one being tested), and can be told to
+ * interact with it. To do so, it also encapsulates a BIP324Cipher to act as the other side. A
+ * second V2Transport is not used, as doing so would not permit scenarios that involve sending
+ * invalid data, or ones scenarios using BIP324 features that are not implemented on the sending


typo: either "ones" or "scenarios"

Done in #28433.

mzumsande · 2023-09-07T21:27:07Z

src/net.cpp

+    // Only wipe the buffer when everything is sent in the READY state. In the AWAITING_KEY state
+    // we still need the garbage that's in the send buffer to construct the garbage authentication
+    // packet.
+    if (m_send_state == SendState::READY && m_send_pos == m_send_buffer.size()) {


While this shortcut works, I wonder if it would be conceptually clearer to not have this special case and instead save the garbage in a m_send_garbage vector, similar to the existing m_recv_garbage on the receive side.

I'll do this if I make further changes to the PR, or as a follow-up. It would also simplify something else: getting rid of the "don't send in MABE_V1 state" exception, by only putting stuff in the send buffer when leaving that state.

Done in #28433.

naumenkogs · 2023-09-08T09:02:54Z

ACK db9888f

fanquake · 2023-09-08T09:11:51Z

Outstanding/followup comments:

#28196 (comment)
#28196 (comment)
#28196 (comment)

vasild

ACK db9888f

db9888f net: detect wrong-network V1 talking to V2Transport (Pieter Wuille) 91e1ef8 test: add unit tests for V2Transport (Pieter Wuille) 297c888 net: make V2Transport preallocate receive buffer space (Pieter Wuille) 3ffa5fb net: make V2Transport send uniformly random number garbage bytes (Pieter Wuille) 0be752d net: add short message encoding/decoding support to V2Transport (Pieter Wuille) 8da8642 net: make V2Transport auto-detect incoming V1 and fall back to it (Pieter Wuille) 13a7f01 net: add V2Transport class with subset of BIP324 functionality (Pieter Wuille) dc2d7eb crypto: Spanify EllSwiftPubKey constructor (Pieter Wuille) 5f4b2c6 net: remove unused Transport::SetReceiveVersion (Pieter Wuille) c3fad1f net: add have_next_message argument to Transport::GetBytesToSend() (Pieter Wuille) Pull request description: This is part of bitcoin#27634. This implements the BIP324 v2 transport (which implements all of what the BIP calls transport layer *and* application layer), though in a non-exposed way. It is tested through an extensive fuzz test, which verifies that v2 transports can talk to v2 transports, and v1 transports can talk to v2 transports, and a unit test that exercises a number of unusual scenarios. The transport is functionally complete, including: * Autodetection of incoming V1 connections. * Garbage, both sending and receiving. * Short message type IDs, both sending and receiving. * Ignore packets (receiving only, but tested in a unit test). * Session IDs are visible in `getpeerinfo` output (for manual comparison). Things that are not included, left for future PRs, are: * Actually using the v2 transport for connections. * Support for the `NODE_P2P_V2` service flag. * Retrying downgrade to V1 when attempted outbound V2 connections immediately fail. * P2P functional and unit tests ACKs for top commit: naumenkogs: ACK db9888f theStack: re-ACK db9888f mzumsande: Code Review ACK db9888f Tree-SHA512: 8906ac1e733a99e1f31c9111055611f706d80bbfc2edf6a07fa6e47b21bb65baacd1ff17993cbbf588063b2f5ad30b3af674a50c7bc8e8ebf4671483a21bbfeb

6470438 doc: fix typos and mistakes in BIP324 code comments (Pieter Wuille) 9bde93d net: do not use send buffer to store/cache garbage (Pieter Wuille) b6934fd net: merge V2Transport constructors, move key gen (Pieter Wuille) Pull request description: This addresses a few remaining comments on #28196: * Deduplicate the `V2Transport` constructors (bitcoin/bitcoin#28196 (comment)) * Do not use the send buffer to store garbage (bitcoin/bitcoin#28196 (comment)) * Fix typo (bitcoin/bitcoin#28196 (comment)) In addition, also fix an incorrect description in `V2Transport::SendState` (it claimed garbage was sent in the `READY` state, but it's in the `AWAITING_KEY` state). ACKs for top commit: naumenkogs: ACK 6470438 theStack: Code-review ACK 6470438 Tree-SHA512: 4bf6d2fe73c8054502d0b60e9de1722f8b3dd269c2dd6bf67197c3fb6eabcf047b6360cdab3c1fd5504215c2ac4ac2890a022780efc30ff583776242c8112451

vincenzopalazzo

re-ACK db9888f

3f4e1bb tests: fix incorrect assumption in v2transport_test (Pieter Wuille) Pull request description: One part of the current `v2transport_test` introduced in #28196 assumes that if a bit gets modified in a message, failure should instantly be detected after sending that message. This is not correct in case the length descriptor is modified, as that may cause the receiver to need more data first. Fix this by sending more messages until failure actually occurs. Discovered in #27495 (comment). ACKs for top commit: theStack: ACK 3f4e1bb Tree-SHA512: faa90bf91996cbaaef62d764e746cb222eaf6796316b0d0e13709e528750b7c0ef09172f7fecfe814dbb8c136c5259f65ca1ac79318e6768a0bfc4e626a63249

6470438 doc: fix typos and mistakes in BIP324 code comments (Pieter Wuille) 9bde93d net: do not use send buffer to store/cache garbage (Pieter Wuille) b6934fd net: merge V2Transport constructors, move key gen (Pieter Wuille) Pull request description: This addresses a few remaining comments on bitcoin#28196: * Deduplicate the `V2Transport` constructors (bitcoin#28196 (comment)) * Do not use the send buffer to store garbage (bitcoin#28196 (comment)) * Fix typo (bitcoin#28196 (comment)) In addition, also fix an incorrect description in `V2Transport::SendState` (it claimed garbage was sent in the `READY` state, but it's in the `AWAITING_KEY` state). ACKs for top commit: naumenkogs: ACK 6470438 theStack: Code-review ACK 6470438 Tree-SHA512: 4bf6d2fe73c8054502d0b60e9de1722f8b3dd269c2dd6bf67197c3fb6eabcf047b6360cdab3c1fd5504215c2ac4ac2890a022780efc30ff583776242c8112451

3f4e1bb tests: fix incorrect assumption in v2transport_test (Pieter Wuille) Pull request description: One part of the current `v2transport_test` introduced in bitcoin#28196 assumes that if a bit gets modified in a message, failure should instantly be detected after sending that message. This is not correct in case the length descriptor is modified, as that may cause the receiver to need more data first. Fix this by sending more messages until failure actually occurs. Discovered in bitcoin#27495 (comment). ACKs for top commit: theStack: ACK 3f4e1bb Tree-SHA512: faa90bf91996cbaaef62d764e746cb222eaf6796316b0d0e13709e528750b7c0ef09172f7fecfe814dbb8c136c5259f65ca1ac79318e6768a0bfc4e626a63249

626f8e3 fuzz: actually test garbage >64b in p2p transport test (Pieter Wuille) Pull request description: This fixes an oversight from #28196: in the `p2p_transport_bidirectional_v2` fuzz test, when the desired garbage length is over 64 bytes, the code would actually use garbage length 0. Fix this. ACKs for top commit: instagibbs: ACK 626f8e3 brunoerg: crACK 626f8e3 Tree-SHA512: f6346367adb10464b6c9d20aef43625531d2a4d8110887ad03214b8c1907b83560f2dd5b5415e2180a40b4cd276d51881b32b60c740471b5c6bb218aa19848d8

sipa marked this pull request as draft August 1, 2023 17:57

sipa mentioned this pull request Jul 26, 2023

BIP324 tracking issue #27634

Closed

43 tasks

sipa changed the title ~~BIP324 transport support~~ BIP324 connection support Aug 1, 2023

sipa mentioned this pull request Aug 1, 2023

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

sipa force-pushed the 202307_bip324_transport branch from 0d56e4d to ffb2f0c Compare August 1, 2023 18:12

DrahtBot added the CI failed label Aug 1, 2023

sipa force-pushed the 202307_bip324_transport branch 3 times, most recently from 90291f8 to a77b9cc Compare August 1, 2023 19:30

DrahtBot removed the CI failed label Aug 1, 2023

This was referenced Aug 13, 2023

Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305 #28263

Merged

crypto: BIP324 ciphersuite follow-up #28267

Merged

sipa force-pushed the 202307_bip324_transport branch from a77b9cc to aaa3af2 Compare August 15, 2023 00:36

DrahtBot added the CI failed label Aug 15, 2023

sipa force-pushed the 202307_bip324_transport branch 5 times, most recently from 1d0a9fb to d539d73 Compare August 17, 2023 02:08

DrahtBot added Needs rebase and removed CI failed labels Aug 17, 2023

mzumsande reviewed Sep 7, 2023

View reviewed changes

DrahtBot requested review from vincenzopalazzo and removed request for vincenzopalazzo September 7, 2023 22:58

DrahtBot requested review from vincenzopalazzo and removed request for naumenkogs and vincenzopalazzo September 8, 2023 09:03

DrahtBot requested review from vincenzopalazzo and removed request for vincenzopalazzo September 8, 2023 09:11

fanquake merged commit 4e1a38c into bitcoin:master Sep 8, 2023

vasild reviewed Sep 8, 2023

View reviewed changes

sipa mentioned this pull request Sep 8, 2023

Follow-up to BIP324 connection support #28433

Merged

TheCharlatan mentioned this pull request Sep 12, 2023

kernel: Remove protocol.h/netaddress.h/compat.h from kernel headers #28423

Merged

vasild mentioned this pull request Sep 13, 2023

Run all CI unit tests with DEBUG_LOG_OUT #28466

Closed

vincenzopalazzo reviewed Sep 13, 2023

View reviewed changes

sipa mentioned this pull request Sep 15, 2023

tests: fix incorrect assumption in v2transport_test #28489

Merged

stickies-v mentioned this pull request Oct 5, 2023

[24.x] Further backports #28535

Merged

sipa mentioned this pull request Mar 17, 2024

fuzz: actually test garbage >64b in p2p transport test #29667

Merged

bitcoin locked and limited conversation to collaborators Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

BIP324 connection support #28196

BIP324 connection support #28196

Uh oh!

sipa commented Aug 1, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Aug 1, 2023 •

edited

Loading

Uh oh!

mzumsande left a comment

Uh oh!

mzumsande Sep 4, 2023

Uh oh!

sipa Sep 8, 2023

Uh oh!

mzumsande Sep 7, 2023

Uh oh!

sipa Sep 7, 2023

Uh oh!

sipa Sep 8, 2023

Uh oh!

naumenkogs commented Sep 8, 2023

Uh oh!

fanquake commented Sep 8, 2023

Uh oh!

vasild left a comment

Uh oh!

vincenzopalazzo left a comment

Uh oh!

Uh oh!

BIP324 connection support #28196

BIP324 connection support #28196

Uh oh!

Conversation

sipa commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

sipa Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

mzumsande Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

sipa Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

sipa Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

naumenkogs commented Sep 8, 2023

Uh oh!

fanquake commented Sep 8, 2023

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sipa commented Aug 1, 2023 •

edited

Loading

DrahtBot commented Aug 1, 2023 •

edited

Loading