BIP324 connection support by sipa · Pull Request #28196 · bitcoin/bitcoin

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.

src/test/net_tests.cpp

src/net.cpp

instagibbs

light review, comparing implementation with the BIP, did not review the tests

src/net.cpp

naumenkogs · 2023-09-07T08:32:52Z

ACK 15ea0ce587a334b647119fe65de2b305720c3eba

vasild

ACK 15ea0ce587a334b647119fe65de2b305720c3eba

Coverage report for modified lines by this PR and not covered by tests:

./src/test/test_bitcoin
./src/qt/test/test_bitcoin-qt
./test/functional/test_runner.py
FUZZ=p2p_transport_bidirectional_v2   ./src/test/fuzz/fuzz -max_total_time=1800 /tmp/empty
FUZZ=p2p_transport_bidirectional_v1v2 ./src/test/fuzz/fuzz -max_total_time=1800 /tmp/empty

src/net.cpp

Before this commit, there are only two possibly outcomes for the "more" prediction in Transport::GetBytesToSend(): * true: the transport itself has more to send, so the answer is certainly yes. * false: the transport has nothing further to send, but if vSendMsg has more message(s) left, that still will result in more wire bytes after the next SetMessageToSend(). For the BIP324 v2 transport, there will arguably be a third state: * definitely not: the transport has nothing further to send, but even if vSendMsg has more messages left, they can't be sent (right now). This happens before the handshake is complete. To implement this, we move the entire decision logic to the Transport, by adding a boolean to GetBytesToSend(), called have_next_message, which informs the transport whether more messages are available. The return values are still true and false, but they mean "definitely yes" and "definitely no", rather than "yes" and "maybe".

This introduces a V2Transport with a basic subset of BIP324 functionality: * no ability to send garbage (but receiving is supported) * no ability to send decoy packets (but receiving them is supported) * no support for short message id encoding (neither encoding or decoding) * no waiting until 12 non-V1 bytes have been received * (and thus) no detection of V1 connections on the responder side (on the sender side, detecting V1 is not supported either, but that needs to be dealt with at a higher layer, by reconnecting)

theStack

ACK 15ea0ce587a334b647119fe65de2b305720c3eba

Left some nitty follow-up material below.

theStack · 2023-09-07T12:54:40Z

src/net.cpp

small nit:

Suggested change

// Transition the sender to AWAITING_KEY state (if not already).

// Transition the sender to AWAITING_KEY state.

To my understanding, no-op state transitions are not possible anymore (otherwise, one of the assertions in SetSendState would fail), and we could only hit this path if the sender state is MAYBE_V1.

Agree. Will address if I push again.

Done in #28433.

src/test/fuzz/p2p_transport_serialization.cpp

src/net.cpp

sipa · 2023-09-07T14:09:39Z

Relevant changes since 15ea0ce587a334b647119fe65de2b305720c3eba (ignoring rebase):

--- a/src/net.cpp
+++ b/src/net.cpp
@@ -919,7 +920,7 @@ namespace {
  * Only message types that are actually implemented in this codebase need to be listed, as other
  * messages get ignored anyway - whether we know how to decode them or not.
  */
-const std::string V2_MESSAGE_IDS[] = {
+const std::array<std::string, 33> V2_MESSAGE_IDS = {
     "", // 12 bytes follow encoding the message type like in V1
     NetMsgType::ADDR,
     NetMsgType::BLOCK,
@@ -956,8 +957,6 @@ const std::string V2_MESSAGE_IDS[] = {
     ""
 };
 
-static_assert(std::size(V2_MESSAGE_IDS) == 33);
-
 class V2MessageMap
 {
     std::unordered_map<std::string, uint8_t> m_map;
--- a/src/test/fuzz/p2p_transport_serialization.cpp
+++ b/src/test/fuzz/p2p_transport_serialization.cpp
@@ -339,10 +339,7 @@ template<typename RNG>
 std::unique_ptr<Transport> MakeV2Transport(NodeId nodeid, bool initiator, RNG& rng, FuzzedDataProvider& provider)
 {
     // Retrieve key
-    auto key_data = provider.ConsumeBytes<unsigned char>(32);
-    key_data.resize(32);
-    CKey key;
-    key.Set(key_data.begin(), key_data.end(), true);
+    auto key = ConsumePrivateKey(provider);
     if (!key.IsValid()) return {};
     // Construct garbage
     size_t garb_len = provider.ConsumeIntegralInRange<size_t>(0, V2Transport::MAX_GARBAGE_LEN);

theStack

re-ACK db9888f

Sjors

Concept ACK.

Overall this looks pretty solid.

Commit c3fad1f looks good; there's no ideal way to do this, but at least it's now pretty well documented.

The state machine looks good too.

Otherwise I only shallowly reviewed up to db9888f, and without looking at test and fuzz code.

To better ensure decoy packages work, maybe you could add a (debug) rpc method to send decoy package to a given peer? Though that can wait for a followup.

Sjors · 2023-09-07T20:15:49Z

src/net.cpp

+                // to MAX_RESERVE_AHEAD above the largest allowed message contents size, and to
+                // MAX_RESERVE_AHEAD more than they've actually sent us.
+                size_t alloc_add = std::min(max_read, msg_bytes.size() + MAX_RESERVE_AHEAD);
+                m_recv_buffer.reserve(m_recv_buffer.size() + alloc_add);


297c888: since the code worked before this commit, what happens we don't call reserve? Does that result in de worst case where the operating system allocates one byte at a time?

Most std::vector implementations double their memory usage whenever they don't have enough, I believe. So without this code, you'd actually get fewer reallocations for large messages actually, but also more easily wasted (allocated but unused) memory.

sipa · 2023-09-07T20:37:39Z

To better ensure decoy packages work, maybe you could add a (debug) rpc method to send decoy package to a given peer? Though that can wait for a followup.

That's not possible in this PR, as V2Transport is not hooked up to anything except tests. This would be a possibility in #28331. Another way for testing decoys is through BIP324 support in the p2p functional test code, see #24748.

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

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

vincenzopalazzo

re-ACK db9888f

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

vasild reviewed Sep 6, 2023

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

instagibbs reviewed Sep 6, 2023

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

vasild approved these changes Sep 7, 2023

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

sipa added 10 commits September 7, 2023 08:53

net: remove unused Transport::SetReceiveVersion

5f4b2c6

crypto: Spanify EllSwiftPubKey constructor

dc2d7eb

net: make V2Transport auto-detect incoming V1 and fall back to it

8da8642

net: add short message encoding/decoding support to V2Transport

0be752d

net: make V2Transport send uniformly random number garbage bytes

3ffa5fb

net: make V2Transport preallocate receive buffer space

297c888

test: add unit tests for V2Transport

91e1ef8

net: detect wrong-network V1 talking to V2Transport

db9888f

theStack approved these changes Sep 7, 2023

View reviewed changes

Sjors reviewed Sep 7, 2023

View reviewed changes

mzumsande reviewed Sep 7, 2023

View reviewed changes

vasild reviewed Sep 8, 2023

View reviewed changes

sipa mentioned this pull request Sep 8, 2023

Follow-up to BIP324 connection support #28433

Merged

sedited 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

	// Transition the sender to AWAITING_KEY state (if not already).
	// Transition the sender to AWAITING_KEY state.

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!

Uh oh!

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Sep 7, 2023

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Sep 7, 2023

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

sipa commented Aug 1, 2023 •

edited

Loading

DrahtBot commented Aug 1, 2023 •

edited

Loading

sipa commented Sep 7, 2023 •

edited

Loading

Sjors left a comment •

edited

Loading