net: transport abstraction by sipa · Pull Request #28165 · bitcoin/bitcoin

sipa · 2023-07-26T20:04:07Z

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.

DrahtBot · 2023-07-26T20:04:10Z

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

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.

dergoegge · 2023-07-31T10:41:11Z

Concept ACK

Merging the two means a future V2Transport can handle all this interaction without callers needing to be aware.

🚀

jonatack · 2023-08-02T21:04:45Z

Concept ACK

Sjors

Concept ACK

Mostly happy with b7a7ed70d06ab3f994ff58e3a0c99105ee88ab6e. Minor inline suggestions and/or making sure I understand what the code is doing.

I also checked that all intermediate commits compile and ran the new fuzzer for several hours (but didn't study it very carefully).

1937a5fcf795149c44b7f4f016c05000ac3adaf9: -maxsendbuffer help could be changed to say "Maximum per-connection memory use for the send buffer"
68e48a0185751d24eecb194b8efd7028c8b590f3: can you elaborate a bit in the commit description why nSendOffset can be dropped?

src/net.h

src/net.cpp

src/net.h

src/net.cpp

Sjors · 2023-08-03T16:03:02Z

src/net.cpp

68e48a0185751d24eecb194b8efd7028c8b590f3: this is much readable than the intermediate calculation introduced in 1937a5fcf795149c44b7f4f016c05000ac3adaf9. It might be worth moving that commit after here.

I don't reordering works without (even) more intermediary complexity. The issue is "net: move message serialization from PushMessage to SocketSendData" changes the data type of vSendMsg from bytes-to-be-sent to messages-to-be-sent, and the latter just don't have a known size-on-the-wire (unless an additional API to transports is added for that, or hardcoding the V1 message encoding size rules). That's why this PR first changes the notion of send buffer size: the old notion just doesn't really make sense anymore after the buffer changes introduced here.

src/net.cpp

sipa · 2023-08-03T21:09:23Z

68e48a0: can you elaborate a bit in the commit description why nSendOffset can be dropped?

In the current codebase, the send buffering (= remembering the to-be-sent bytes which we haven't managed to send yet) is done using vSendMsg (a queue of byte arrays) + nSendOffset (the position within vSendMsg[0] up to where we've sent things). After this PR, vSendMsg is turned into a queue of messages, which have not yet been converted to bytes-on-the-wire; this conversion is now handled by m_transport, and it's the transport that remembers what/how much has been sent yet.

theStack

Concept ACK

Slowly working my way through, reviewed up to f97adbf3e93e89b1e8ce1dc212e84ac6b2879463 (commit 3/8), looks good so far.
One potential code deduplication nit (though I'm not sure if it gains that much in readability): seems like for V1Transport, HaveBytesToSend() and DoneSendingMessage() are in an inverse relation (or however one would call that), so one could be expressed through the other, e.g.:

index 887669e32b..af0fa5c603 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -863,8 +863,7 @@ void V1Transport::SetMessageToSend(CSerializedNetMsg&& msg) noexcept
 
 bool V1Transport::HaveBytesToSend() const noexcept
 {
-    LOCK(m_cs_send);
-    return m_sending_header || m_bytes_sent != m_message_to_send.data.size();
+    return !DoneSendingMessage();
 }
 
 Transport::BytesToSend V1Transport::GetBytesToSend() const noexcept

vincenzopalazzo

Concept ACK till 1937a5f

I will continue later on it

mzumsande

Concept ACK

src/net.h

src/test/denialofservice_tests.cpp

mzumsande · 2023-08-14T18:15:12Z

src/test/util/net.cpp

It seems confusing and not ideal that a function called ReceiveMsgFrom has so much code dealing with the Send part, and also the side effect of flushing the send message buffer, when the goal is just to create a header for ser_msg to be able to receive that, but not to send anything. For example, it should be possible to call ReceiveMsgFrom in situations where the send buffer has unrelated contents. Maybe it'd be better to just have the relevant code (i.e. the old prepareForTransport duplicated here to extract the header instead of changing the send parts of m_transport?)

It's complicated. You're right that duplicating the old prepareForTransport here would work, but I feel that's not really the right approach, as it's kind of hiding what's really going on. It'd also be incompatible with a potential future upgrade to using v2 transports inside the affected unit and fuzz tests.

What's really going on is that we're using the CNode's own sending infrastructure to construct bytes, which are then fed to the same CNode's receive infrastructure. But there is also "normal" non-test sending logic (e.g. sending of version/verack automatically) that uses the same sending infrastucture (however, failing as there is no real socket to send anything on). The flushing here was necessary to wipe the non-test messages that enter the same transport.

I have for now tried to address your concern here by introducing a more explicit FlushSendBuffer call, and adding it where necessary to make tests pass, making it clear where and what is being flushed away, however I think the more proper solution would involve:

Somehow prevent the sending of non-test messages entirely in test connmans (rather that hacking them away after the fact).

Give the test connman dedicated per-receive-node send transports which are just used for the test messages sent there. That would make it compatible with v2 operation too.

Doing those things here feel out of scope, though.

sipa · 2023-08-15T22:47:45Z

I've made some significant changes to this PR:

DoneSendingMessage is gone (the caller can just call SetMessageToSend directly, which will fail if nothing can be sent at that time).
HaveBytesToSend is gone (the caller can just call GetBytesToSend, which will report an empty span if nothing is to be sent).
GetBytesToSend now takes a have_next_message as input, which lets its prediction for whether there are more bytes to send afterwards be more accurate (letting it take into account a future SetMessageToSend). This prediction is also tested now, through an Assume in the net code directly, and inside the transport simulation fuzz test.
A lot more comments and documentation.
More sanity checks in the added fuzz test.

Sjors

The new approach in 384c798cc3836558463e88ec7f563b236f50bf22 is a nice improvement.

I'll continue reviewing from 09972b518bed5809d37c79fb0ddef034dce27ba0 onwards after you've had a chance to comment on my more comment.

src/net.h

Sjors

ACK d9a91ba9606f828ef3846d2877ec56313653a109 just in time for the #27981 rebase :-(

src/net.cpp

The rest of net.cpp already uses Params() to determine chainparams in many places (and even V1Transport itself does so in some places). Since the only chainparams dependency is through the message start characters, just store those directly in the transport.

This adds a simulation test, with two V1Transport objects, which send messages to each other, with sending and receiving fragmented into multiple pieces that may be interleaved. It primarily verifies that the sending and receiving side are compatible with each other, plus a few sanity checks.

This more accurately captures the intent of limiting send buffer size, as many small messages can have a larger overhead that is not counted with the current approach. It also means removing the dependency on the header size (which will become a function of the transport choice) from the send buffer calculations.

…SendData This furthers transport abstraction by removing the assumption that a message can always immediately be converted to wire bytes. This assumption does not hold for the v2 transport proposed by BIP324, as no messages can be sent before the handshake completes. This is done by only keeping (complete) CSerializedNetMsg objects in vSendMsg, rather than the resulting bytes (for header and payload) that need to be sent. In SocketSendData, these objects are handed to the transport as permitted by it, and sending out the bytes the transport tells us to send. This also removes the nSendOffset member variable in CNode, as keeping track of how much has been sent is now a responsability of the transport. This is not a pure refactor, and has the following effects even for the current v1 transport: * Checksum calculation now happens in SocketSendData rather than PushMessage. For non-optimistic-send messages, that means this computation now happens in the network thread rather than the message handler thread (generally a good thing, as the message handler thread is more of a computational bottleneck). * Checksum calculation now happens while holding the cs_vSend lock. This is technically unnecessary for the v1 transport, as messages are encoded independent from one another, but is untenable for the v2 transport anyway. * Statistics updates about per-message sent bytes now happen when those bytes are actually handed to the OS, rather than at PushMessage time.

theStack

re-ACK 8a3b6f3

The only changes since my previous ACK are two nits tackled regarding spelling (s/deques/queues/) and named arguments in the simulation test.

vasild

ACK 8a3b6f3

dergoegge

Code review ACK 8a3b6f3

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

vasild · 2024-02-15T13:43:16Z

src/test/fuzz/p2p_transport_serialization.cpp

 {
    SelectParams(ChainType::REGTEST);
+    g_all_messages = getAllNetMessageTypes();
+    std::sort(g_all_messages.begin(), g_all_messages.end());


Why this sort here? Anybody remembers? It is only used like:

g_all_messages[v % g_all_messages.size()]

where v is a random number. The sort seems unnecessary?

The question arised in: #29421 (comment)

Well it's not random, it's a fuzzer input. They're random in a way, but definitely not uniformly random.

I think the sorting is there to make sure that just reorderings in the list don't affect fuzzer behavior.

DrahtBot added the P2P label Jul 26, 2023

sipa mentioned this pull request Jul 26, 2023

BIP324 tracking issue #27634

Closed

43 tasks

This was referenced Jul 27, 2023

Fix potential network stalling bug #27981

Merged

net, refactor: Privatise CNode send queue #27407

Closed

sipa force-pushed the 202307_merge_sers branch from 78c69f7 to 977f812 Compare July 29, 2023 20:56

sipa force-pushed the 202307_merge_sers branch from 977f812 to b7a7ed7 Compare July 31, 2023 16:54

sipa mentioned this pull request Aug 1, 2023

BIP324 connection support #28196

Merged

Sjors reviewed Aug 3, 2023

View reviewed changes

DrahtBot mentioned this pull request Aug 5, 2023

Use shared_ptr for CNode inside CConnman #28222

Closed

theStack reviewed Aug 8, 2023

View reviewed changes

vincenzopalazzo reviewed Aug 12, 2023

View reviewed changes

mzumsande reviewed Aug 14, 2023

View reviewed changes

sipa force-pushed the 202307_merge_sers branch from b7a7ed7 to f9f69aa Compare August 15, 2023 22:28

DrahtBot added the CI failed label Aug 16, 2023

Sjors reviewed Aug 16, 2023

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

sipa force-pushed the 202307_merge_sers branch 2 times, most recently from 51c8604 to d9a91ba Compare August 16, 2023 23:22

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

Sjors approved these changes Aug 17, 2023

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

sipa force-pushed the 202307_merge_sers branch 2 times, most recently from 3e0d044 to 47eb247 Compare August 17, 2023 19:01

DrahtBot removed the Needs rebase label Aug 17, 2023

sipa force-pushed the 202307_merge_sers branch from 47eb247 to 44bb2ba Compare August 18, 2023 02:51

DrahtBot added the CI failed label Aug 18, 2023

sipa added 5 commits August 23, 2023 19:56

refactor: make Transport::ReceivedBytes just return success/fail

8a3b6f3

sipa force-pushed the 202307_merge_sers branch from 41806a4 to 8a3b6f3 Compare August 24, 2023 00:14

sipa mentioned this pull request Aug 24, 2023

BIP324 integration #28331

Merged

theStack approved these changes Aug 24, 2023

View reviewed changes

DrahtBot requested review from Sjors, mzumsande and vincenzopalazzo August 24, 2023 09:09

vasild approved these changes Aug 24, 2023

View reviewed changes

DrahtBot requested review from vincenzopalazzo and removed request for vincenzopalazzo August 24, 2023 11:24

dergoegge approved these changes Aug 24, 2023

View reviewed changes

DrahtBot requested review from vincenzopalazzo and removed request for vincenzopalazzo August 24, 2023 12:36

fanquake merged commit 1fa6411 into bitcoin:master Aug 24, 2023

rajarshimaitra mentioned this pull request Aug 31, 2023

net: transport abstraction #28165 Bitshala/BitcoinCore-PR-Review-Club#44

Closed

vasild reviewed Feb 15, 2024

View reviewed changes

vasild mentioned this pull request Feb 15, 2024

net: make the list of known message types a compile time constant #29421

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Aug 4, 2024

merge bitcoin#28165: transport abstraction

9087679

kwvg mentioned this pull request Aug 31, 2024

backport: merge bitcoin#21148, #21327, #23970, #24021, #24543, #26844, #25325, #28165, partial bitcoin#20524, #26036, #27981 (networking backports: part 7) dashpay/dash#6067

Merged

5 tasks

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

dergoegge commented Jul 31, 2023

Uh oh!

jonatack commented Aug 2, 2023

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

sipa Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa commented Aug 3, 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!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mzumsande Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

sipa Aug 15, 2023 • 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!

sipa commented Aug 15, 2023

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

sipa Feb 15, 2024

Choose a reason for hiding this comment

sipa commented Jul 26, 2023 •

edited

Loading

DrahtBot commented Jul 26, 2023 •

edited

Loading

Sjors left a comment •

edited

Loading

sipa commented Aug 3, 2023 •

edited

Loading

sipa Aug 15, 2023 •

edited

Loading

Sjors left a comment •

edited

Loading