Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BIP324 tracking issue #27634

Closed
42 of 43 tasks
sipa opened this issue May 12, 2023 · 5 comments
Closed
42 of 43 tasks

BIP324 tracking issue #27634

sipa opened this issue May 12, 2023 · 5 comments

Comments

@sipa
Copy link
Member

sipa commented May 12, 2023

This issue will be updated to reflect the current state of BIP324 integration.

PRs ready for review:

Overall plan:

Older stuff:
@instagibbs instagibbs moved this to Priority Projects in High-priority for review May 12, 2023
@fanquake fanquake pinned this issue May 26, 2023
achow101 added a commit that referenced this issue Jul 12, 2023
0bf8747 test: add ChaCha20 test triggering 32-bit block counter overflow (Sebastian Falbesoner)
7f2a985 tests: improve ChaCha20 unit tests (Pieter Wuille)
511a8d4 crypto: Implement RFC8439-compatible variant of ChaCha20 (Pieter Wuille)

Pull request description:

  Based on and replaces part of #25361, part of the BIP324 project (#27634). See also #19225 for background.

  There are two variants of ChaCha20 in use. The currently implemented one uses a 64-bit nonce and a 64-bit block counter, while the one used in RFC8439 (and thus BIP324) uses a 96-bit nonce and 32-bit block counter. This PR changes the logic to use the 96-bit nonce variant, though in a way that's compatible with >256 GiB output (by automatically incrementing the first 32-bit part of the nonce when the block counter overflows).

  For those who reviewed the original PR, the biggest change is here that the 96-bit nonce is passed as a Nonce96 type (pair of 32-bit + 64-bit integer) rather than a 12-byte array.

ACKs for top commit:
  achow101:
    ACK 0bf8747
  theStack:
    Code-review ACK 0bf8747

Tree-SHA512: 62e4cbd5388b8d50ef1a0dc99b6f4ad36c7b4419032035f8e622dda63a62311dd923032217e20054bcd836865d4be5c074f9e5538ca158f94f08eab75c5519c1
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Jul 12, 2023
0bf8747 test: add ChaCha20 test triggering 32-bit block counter overflow (Sebastian Falbesoner)
7f2a985 tests: improve ChaCha20 unit tests (Pieter Wuille)
511a8d4 crypto: Implement RFC8439-compatible variant of ChaCha20 (Pieter Wuille)

Pull request description:

  Based on and replaces part of bitcoin#25361, part of the BIP324 project (bitcoin#27634). See also bitcoin#19225 for background.

  There are two variants of ChaCha20 in use. The currently implemented one uses a 64-bit nonce and a 64-bit block counter, while the one used in RFC8439 (and thus BIP324) uses a 96-bit nonce and 32-bit block counter. This PR changes the logic to use the 96-bit nonce variant, though in a way that's compatible with >256 GiB output (by automatically incrementing the first 32-bit part of the nonce when the block counter overflows).

  For those who reviewed the original PR, the biggest change is here that the 96-bit nonce is passed as a Nonce96 type (pair of 32-bit + 64-bit integer) rather than a 12-byte array.

ACKs for top commit:
  achow101:
    ACK 0bf8747
  theStack:
    Code-review ACK 0bf8747

Tree-SHA512: 62e4cbd5388b8d50ef1a0dc99b6f4ad36c7b4419032035f8e622dda63a62311dd923032217e20054bcd836865d4be5c074f9e5538ca158f94f08eab75c5519c1
@bitcoin bitcoin deleted a comment from doperiddle Jul 17, 2023
achow101 added a commit that referenced this issue Jul 17, 2023
4e5c933 Switch all callers from poly1305_auth to Poly1305 class (Pieter Wuille)
8871f7d tests: add more Poly1305 test vectors (Pieter Wuille)
40e6c5b crypto: add Poly1305 class with std::byte Span interface (Pieter Wuille)
50269b3 crypto: switch poly1305 to incremental implementation (Pieter Wuille)

Pull request description:

  Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see #27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces:
  * The additionally authenticated data (AAD), padded to 16 bytes.
  * The ciphertext, padded to 16 bytes.
  * The length of the AAD and the length of the ciphertext, together another 16 bytes.

  Implementing RFC8439 using the existing `poly1305_auth` function requires creating a temporary copy with all these pieces of data concatenated just for the purpose of computing the tag (the approach used in #25361).

  This PR replaces the poly1305 code with new code from https://github.com/floodyberry/poly1305-donna (with minor adjustments to make it match our coding style and use our utility functions, documented in the commit) which supports incremental operation, and then adds a C++ wrapper interface using std::byte Spans around it, and adds tests that incremental and all-at-once computation match.

ACKs for top commit:
  achow101:
    ACK 4e5c933
  theStack:
    ACK 4e5c933
  stratospher:
    tested ACK 4e5c933.

Tree-SHA512: df6e9a2a4a38a480f9e4360d3e3def5311673a727a4a85b008a084cf6843b260dc82cec7c73e1cecaaccbf10f3521a0ae7dba388b65d0b086770f7fbc5223e2a
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Jul 19, 2023
…modernize

4e5c933 Switch all callers from poly1305_auth to Poly1305 class (Pieter Wuille)
8871f7d tests: add more Poly1305 test vectors (Pieter Wuille)
40e6c5b crypto: add Poly1305 class with std::byte Span interface (Pieter Wuille)
50269b3 crypto: switch poly1305 to incremental implementation (Pieter Wuille)

Pull request description:

  Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see bitcoin#27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces:
  * The additionally authenticated data (AAD), padded to 16 bytes.
  * The ciphertext, padded to 16 bytes.
  * The length of the AAD and the length of the ciphertext, together another 16 bytes.

  Implementing RFC8439 using the existing `poly1305_auth` function requires creating a temporary copy with all these pieces of data concatenated just for the purpose of computing the tag (the approach used in bitcoin#25361).

  This PR replaces the poly1305 code with new code from https://github.com/floodyberry/poly1305-donna (with minor adjustments to make it match our coding style and use our utility functions, documented in the commit) which supports incremental operation, and then adds a C++ wrapper interface using std::byte Spans around it, and adds tests that incremental and all-at-once computation match.

ACKs for top commit:
  achow101:
    ACK 4e5c933
  theStack:
    ACK 4e5c933
  stratospher:
    tested ACK 4e5c933.

Tree-SHA512: df6e9a2a4a38a480f9e4360d3e3def5311673a727a4a85b008a084cf6843b260dc82cec7c73e1cecaaccbf10f3521a0ae7dba388b65d0b086770f7fbc5223e2a
This was referenced Jul 26, 2023
fanquake added a commit that referenced this issue Aug 10, 2023
1c7582e tests: add decryption test to bip324_tests (Pieter Wuille)
990f0f8 Add BIP324Cipher, encapsulating key agreement, derivation, and stream/AEAD ciphers (Pieter Wuille)
c91cedf crypto: support split plaintext in ChaCha20Poly1305 Encrypt/Decrypt (Pieter Wuille)
af2b44c bench: add benchmark for FSChaCha20Poly1305 (Pieter Wuille)
aa8cee9 crypto: add FSChaCha20Poly1305, rekeying wrapper around ChaCha20Poly1305 (Pieter Wuille)
0fee267 crypto: add FSChaCha20, a rekeying wrapper around ChaCha20 (Pieter Wuille)
9ff0768 crypto: add the ChaCha20Poly1305 AEAD as specified in RFC8439 (Pieter Wuille)
9fd085a crypto: remove outdated variant of ChaCha20Poly1305 AEAD (Pieter Wuille)

Pull request description:

  Depends on #27985 and #27993, based on and partially replaces #25361, part of #27634. Draft while dependencies are not merged.

  This adds implementations of:
  * The ChaCha20Poly1305 AEAD from [RFC8439 section 2.8](https://datatracker.ietf.org/doc/html/rfc8439#section-2.8), including test vectors.
  * The FSChaCha20 stream cipher as specified in [BIP324](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#rekeying-wrappers-fschacha20poly1305-and-fschacha20), a rekeying wrapper around ChaCha20.
  * The FSChaCha20Poly1305 AEAD as specified in [BIP324](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#rekeying-wrappers-fschacha20poly1305-and-fschacha20), a rekeying wrapper around ChaCha20Poly1305.
  * A BIP324Cipher class that encapsulates key agreement, key derivation, and stream ciphers and AEADs for [BIP324 packet encoding](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#overall-packet-encryption-and-decryption-pseudocode).

  The ChaCha20Poly1305 and FSChaCha20Poly1305 implementations are new, taking advance of the improvements in #27993.

ACKs for top commit:
  jamesob:
    reACK 1c7582e
  theStack:
    ACK 1c7582e
  stratospher:
    tested ACK 1c7582e.

Tree-SHA512: 06728b4b95b21c5b732ed08faf40e94d0583f9d86ff4db3b92dd519dcd9fbfa0f310bc66ef1e59c9e49dd844ba8c5ac06e2001762a804fb5aa97027816045a46
@fanquake
Copy link
Member

fanquake commented Aug 10, 2023

Review comments from #28008 that could be incorporated into future changes:

fanquake added a commit that referenced this issue Aug 24, 2023
8a3b6f3 refactor: make Transport::ReceivedBytes just return success/fail (Pieter Wuille)
bb4aab9 net: move message conversion to wire bytes from PushMessage to SocketSendData (Pieter Wuille)
a1a1060 net: measure send buffer fullness based on memory usage (Pieter Wuille)
009ff8d fuzz: add bidirectional fragmented transport test (Pieter Wuille)
fb2c5ed net: make V1Transport implicitly use current chainparams (Pieter Wuille)
0de48fe net: abstract sending side of transport serialization further (Pieter Wuille)
649a83c refactor: rename Transport class receive functions (Pieter Wuille)
27f9ba2 net: add V1Transport lock protecting receive state (Pieter Wuille)
93594e4 refactor: merge transport serializer and deserializer into Transport class (Pieter Wuille)

Pull request description:

  This PR furthers the P2P message serialization/deserialization abstraction introduced in #16202 and #16562, in preparation for introducing the BIP324 v2 transport (making this part of #27634). However, nothing in this PR is BIP324-specific, and it contains a number of independently useful improvements.

  The overall idea is to have a single object in every `CNode` (called `m_transport`) that is responsible for converting sent messages to wire bytes, and for converting received wire bytes back to messages, while having as little as possible knowledge about this conversion process in higher-level net code. To accomplish that, there is an abstract `Transport` class with (currently) a single `V1Transport` implementation.

  Structurally, the above is accomplished by:
  * Merging the `TransportDeserializer` and `TransportSerializer` classes into a single `Transport` class, which encompasses both the sending and receiving side. For `V1Transport` these two sides are entirely separate, but this assumption doesn't hold for the BIP324 transport where e.g. the sending encryption key depends on the DH key negotiation data received from the other side. Merging the two means a future `V2Transport` can handle all this interaction without callers needing to be aware.
  * Removing the assumption that each message is sent using a computed header followed by (unmodified) data bytes. To achieve that, the sending side of `Transport` mirrors what the receiver side does: callers can set a message to be sent, then ask what bytes must be sent out, and then allowing them to transition to the next message.
  * Adding internal locks to protect the sending and receiving state of the `V1Transport` implementation. I believe these aren't strictly needed (opinions welcome) as there is no real way to use `Transport` objects in a multi-threaded fashion without some form of external synchronization (e.g. "get next bytes to send" isn't meaningful to call from multiple threads at the same time without mechanism to control the order they'll actually get sent). Still, I feel it's cleaner to make the object responsible for its own consistency (as we definitely do not want the entire object to be under a single external GUARDED_BY, as that'd prevent simultaneous sending and receiving).
  * Moving the conversion of messages to bytes on the sending side from `PushMessage` to `SocketSendData`, which is needed to deal with the fact that a transport may not immediately be able to send messages.

  This PR is not a refactor, though some commits are. Among the semantic changes are:
  * Changing the send buffer pushback mechanism to trigger based on the memory usage of the buffer rather than the amount of bytes to be sent. This is both closer to the desired behavior, and makes the buffering independent from transport details (which is why it's included here).
  * When optimistic send is not applicable, the V1 message checksum calculation now runs in the net thread rather than the message handling thread. I believe that's generally an improvement, as the message handling thread is far more computationally bottlenecked already.
  * The checksum calculation now runs under the `CNode::cs_vSend` lock, which does mean no two checksum calculations for messages sent to the same node can run in parallel, even if running in separate threads. Despite that limitation, having the checksum for non-optimistic sends moved in the net thread is still an improvement, I believe.
  * Statistics for per-message-type sent bytes are now updated when the bytes are actually handed to the OS rather than in `PushMessage`. This is because the actual serialized sizes aren't known until they've gone through the transport object.

  A fuzz test of the entire `V1Transport` is included. More elaborate rationale for each of the changes can be found in the commit messages.

ACKs for top commit:
  theStack:
    re-ACK 8a3b6f3
  vasild:
    ACK 8a3b6f3
  dergoegge:
    Code review ACK 8a3b6f3

Tree-SHA512: 26e9a6df47f1dd3e3f3edb4874edf365728e5a8bbc9d0d4d71fb6000cb2dfde5574902c47ffcf825af6743922f2ff9d31a5a38942a196f4ca6669122e15e42e4
fanquake added a commit that referenced this issue Sep 8, 2023
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 #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
Frank-GER pushed a commit to syscoin/syscoin that referenced this issue Sep 8, 2023
8a3b6f3 refactor: make Transport::ReceivedBytes just return success/fail (Pieter Wuille)
bb4aab9 net: move message conversion to wire bytes from PushMessage to SocketSendData (Pieter Wuille)
a1a1060 net: measure send buffer fullness based on memory usage (Pieter Wuille)
009ff8d fuzz: add bidirectional fragmented transport test (Pieter Wuille)
fb2c5ed net: make V1Transport implicitly use current chainparams (Pieter Wuille)
0de48fe net: abstract sending side of transport serialization further (Pieter Wuille)
649a83c refactor: rename Transport class receive functions (Pieter Wuille)
27f9ba2 net: add V1Transport lock protecting receive state (Pieter Wuille)
93594e4 refactor: merge transport serializer and deserializer into Transport class (Pieter Wuille)

Pull request description:

  This PR furthers the P2P message serialization/deserialization abstraction introduced in bitcoin#16202 and bitcoin#16562, in preparation for introducing the BIP324 v2 transport (making this part of bitcoin#27634). However, nothing in this PR is BIP324-specific, and it contains a number of independently useful improvements.

  The overall idea is to have a single object in every `CNode` (called `m_transport`) that is responsible for converting sent messages to wire bytes, and for converting received wire bytes back to messages, while having as little as possible knowledge about this conversion process in higher-level net code. To accomplish that, there is an abstract `Transport` class with (currently) a single `V1Transport` implementation.

  Structurally, the above is accomplished by:
  * Merging the `TransportDeserializer` and `TransportSerializer` classes into a single `Transport` class, which encompasses both the sending and receiving side. For `V1Transport` these two sides are entirely separate, but this assumption doesn't hold for the BIP324 transport where e.g. the sending encryption key depends on the DH key negotiation data received from the other side. Merging the two means a future `V2Transport` can handle all this interaction without callers needing to be aware.
  * Removing the assumption that each message is sent using a computed header followed by (unmodified) data bytes. To achieve that, the sending side of `Transport` mirrors what the receiver side does: callers can set a message to be sent, then ask what bytes must be sent out, and then allowing them to transition to the next message.
  * Adding internal locks to protect the sending and receiving state of the `V1Transport` implementation. I believe these aren't strictly needed (opinions welcome) as there is no real way to use `Transport` objects in a multi-threaded fashion without some form of external synchronization (e.g. "get next bytes to send" isn't meaningful to call from multiple threads at the same time without mechanism to control the order they'll actually get sent). Still, I feel it's cleaner to make the object responsible for its own consistency (as we definitely do not want the entire object to be under a single external GUARDED_BY, as that'd prevent simultaneous sending and receiving).
  * Moving the conversion of messages to bytes on the sending side from `PushMessage` to `SocketSendData`, which is needed to deal with the fact that a transport may not immediately be able to send messages.

  This PR is not a refactor, though some commits are. Among the semantic changes are:
  * Changing the send buffer pushback mechanism to trigger based on the memory usage of the buffer rather than the amount of bytes to be sent. This is both closer to the desired behavior, and makes the buffering independent from transport details (which is why it's included here).
  * When optimistic send is not applicable, the V1 message checksum calculation now runs in the net thread rather than the message handling thread. I believe that's generally an improvement, as the message handling thread is far more computationally bottlenecked already.
  * The checksum calculation now runs under the `CNode::cs_vSend` lock, which does mean no two checksum calculations for messages sent to the same node can run in parallel, even if running in separate threads. Despite that limitation, having the checksum for non-optimistic sends moved in the net thread is still an improvement, I believe.
  * Statistics for per-message-type sent bytes are now updated when the bytes are actually handed to the OS rather than in `PushMessage`. This is because the actual serialized sizes aren't known until they've gone through the transport object.

  A fuzz test of the entire `V1Transport` is included. More elaborate rationale for each of the changes can be found in the commit messages.

ACKs for top commit:
  theStack:
    re-ACK 8a3b6f3
  vasild:
    ACK 8a3b6f3
  dergoegge:
    Code review ACK 8a3b6f3

Tree-SHA512: 26e9a6df47f1dd3e3f3edb4874edf365728e5a8bbc9d0d4d71fb6000cb2dfde5574902c47ffcf825af6743922f2ff9d31a5a38942a196f4ca6669122e15e42e4
Frank-GER pushed a commit to syscoin/syscoin that referenced this issue Sep 8, 2023
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
@real-or-random
Copy link
Contributor

@sipa Want to add an item for mentioning BIP324 in https://github.com/bitcoin/bitcoin/blob/master/doc/bips.md?

@sipa
Copy link
Member Author

sipa commented Sep 27, 2023

@real-or-random Done in #28331.

fanquake added a commit that referenced this issue Oct 3, 2023
75a3291 doc: mention BIP324 support in bips.md (Pieter Wuille)
64ca721 test: enable v2 transport between nodes in some functional tests (Pieter Wuille)
05d19fb test: Functional test for opportunistic encryption (dhruv)
b815cce net: expose transport types/session IDs of connections in RPC and logs (Pieter Wuille)
432a62c net: reconnect with V1Transport under certain conditions (Pieter Wuille)
4d265d0 sync: modernize CSemaphore / CSemaphoreGrant (Pieter Wuille)
c73cd42 rpc: addnode arg to use BIP324 v2 p2p (dhruv)
62d21ee net: use V2Transport when NODE_P2P_V2 service flag is present (Pieter Wuille)
a4706bc rpc: don't report v2 handshake bytes in the per-type sent byte statistics (Sebastian Falbesoner)
abf343b net: advertise NODE_P2P_V2 if CLI arg -v2transport is on (Pieter Wuille)

Pull request description:

  Part of #27634.

  This makes BIP324 support feature complete, through a (default off) `-v2transport` option for enabling V2 connections. If it is enabled:
  * The `NODE_P2P_V2` service flag (*1 << 11*) is advertized.
  * Inbound connections can use V1 or V2 (automatically detected based on the protocol used by the peer)
  * V2 connections are used on outbound when the `NODE_P2P_V2` service is available (or the new `use_v2` parameter is set on the `addnode` RPC).
  * V2 outbound connections that instantly fail get retried as V1.

  There are two new RPC fields, `"transport_protocol_type"` and `"session_id"`, in `getpeerinfo`.

ACKs for top commit:
  mzumsande:
    re-ACK 75a3291
  theStack:
    re-ACK 75a3291

Tree-SHA512: 90ea1cd37f3dce410a59ff5de1c2405891e8aa62318d0e06dcb68b21603fb0c061631526633f3d4fb630e63d2b8db407eed48e246befcbef3503bea893a4ff15
@vasild
Copy link
Contributor

vasild commented Oct 3, 2023

Minor thing that was missed, can be included in some of the followups:

s/TransportDeserializer/Transport class/
diff --git i/src/net.cpp w/src/net.cpp
index 6b2ef5f43d..5b0efc16aa 100644
--- i/src/net.cpp
+++ w/src/net.cpp
@@ -812,13 +812,13 @@ const uint256& V1Transport::GetMessageHash() const
 
 CNetMessage V1Transport::GetReceivedMessage(const std::chrono::microseconds time, bool& reject_message)
 {
     AssertLockNotHeld(m_recv_mutex);
     // Initialize out parameter
     reject_message = false;
-    // decompose a single CNetMessage from the TransportDeserializer
+    // decompose a single CNetMessage from the Transport class
     LOCK(m_recv_mutex);
     CNetMessage msg(std::move(vRecv));
 
     // store message type string, time, and sizes
     msg.m_type = hdr.GetCommand();
     msg.m_time = time;
@@ -3724,13 +3724,13 @@ void CNode::MarkReceivedMsgsForProcessing()
 {
     AssertLockNotHeld(m_msg_process_queue_mutex);
 
     size_t nSizeAdded = 0;
     for (const auto& msg : vRecvMsg) {
         // vRecvMsg contains only completed CNetMessage
-        // the single possible partially deserialized message are held by TransportDeserializer
+        // the single possible partially deserialized message are held by Transport class
         nSizeAdded += msg.m_raw_message_size;
     }
 
     LOCK(m_msg_process_queue_mutex);
     m_msg_process_queue.splice(m_msg_process_queue.end(), vRecvMsg);
     m_msg_process_queue_size += nSizeAdded;

achow101 added a commit that referenced this issue Jan 31, 2024
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see #27634):
  * Inbound connections will auto-sense whether v1 or v2 is in use.
  * Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure.
  * Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.

  It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK 0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
@sipa
Copy link
Member Author

sipa commented Feb 4, 2024

There are a few minor follow-ups to consider still, but I believe this issue can be closed. Thanks everyone for getting us this far!

@sipa sipa closed this as completed Feb 4, 2024
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Oct 24, 2024
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see bitcoin#27634):
  * Inbound connections will auto-sense whether v1 or v2 is in use.
  * Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure.
  * Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.

  It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK bitcoin@0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Oct 24, 2024
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see bitcoin#27634):
  * Inbound connections will auto-sense whether v1 or v2 is in use.
  * Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure.
  * Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.

  It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK bitcoin@0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Oct 24, 2024
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see bitcoin#27634):
  * Inbound connections will auto-sense whether v1 or v2 is in use.
  * Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure.
  * Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.

  It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK bitcoin@0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Oct 24, 2024
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see bitcoin#27634):
  * Inbound connections will auto-sense whether v1 or v2 is in use.
  * Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure.
  * Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.

  It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK bitcoin@0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Oct 25, 2024
0bef104 net: enable v2transport by default (Pieter Wuille)

Pull request description:

  This enables BIP324's v2 transport by default (see bitcoin#27634):
  * Inbound connections will auto-sense whether v1 or v2 is in use.
  * Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure.
  * Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.

  It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node.

ACKs for top commit:
  stratospher:
    ACK 0bef104.
  josibake:
    reACK bitcoin@0bef104
  achow101:
    ACK 0bef104
  naumenkogs:
    ACK 0bef104
  theStack:
    ACK 0bef104
  willcl-ark:
    crACK 0bef104
  BrandonOdiwuor:
    utACK 0bef104
  pablomartin4btc:
    re ACK 0bef104
  kristapsk:
    utACK 0bef104

Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants