net: transport abstraction

[sipa]

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]

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]

Concept ACK

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

🚀

[jonatack]

Concept ACK

[Sjors]

Concept ACK

Mostly happy with b7a7ed7. 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).

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

[Sjors]

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

[sipa]

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.

[sipa]

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 f97adbf (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

[mzumsande]

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?)

[sipa]

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]

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 384c798 is a nice improvement.

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

[Sjors]

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

[vasild]

ACK 8a3b6f3

[dergoegge]

Code review ACK 8a3b6f3

[vasild]

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)

[sipa]

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.