Bitcoin nodes “speak” a P2P protocol, which includes a way of interpreting bytes sent over
connections to one another as well as expectations for the contents of those messages peers are expected to send to each
other. Within Bitcoin Core, the implementations of these two parts of the protocol live in largely
separate areas of the code. At a high level:
The “net processing” layer includes application logic such as
responding
to ping with a pong containing the same nonce and
disconnecting
a peer that sends a transaction after we told them not to.
The “net” layer “below” net processing abstracts away the details of converting
messages to/from bytes that are sent and received on the connection.
Within the ProcessMessage function which responds to pings with pongs, The ping is a
CNetMessage retrieved from CNode::PollMessage, and the pong is sent by calling
CConnman::PushMessage.
This PR is a prerequisite of PR #28196
implementing BIP324. The complete specification of BIP324 is out of scope for this Review Club, but please make sure to read the Introduction and Goals.
BIP324 introduces a new transport protocol version. While nodes still send the same types of messages and respond to pings with pongs in the same way, the way messages are converted to/from bytes is very different.
This PR creates an abstract Transport class, combining the current TransportDeserializer and
TransportSerializer classes into a single interface for converting bytes to/from messages.
A combined interface means that V2Transport can have state shared between the
serializer and deserializer, such as the BIP324Cipher for encrypting/decrypting messages. It may
be helpful to see how V2Transport is implemented in that PR. Also see a previous
approach, which kept the serializer and deserializer
separate and
stored
state in the CNode object.
Questions
We’ll be covering the General questions on the first meeting on Wednesday, and the PR-specific questions on the second meeting on Thursday.
What is the distinction between “net” and “net processing”? What data structures and tasks might
we associate with each of them?
Does BIP324 require changes to the “net” layer, the “net processing” layer, or both? Does it affect policy or consensus?
Is {CNetMessage, CMessageHeader, CSerializedNetMsg, BytesToSend} used in sending, receiving, or both?
CNetMsgMaker and Transport both “serialize” messages. What is the difference in what they do?
In the process of turning an application object like a CTransactionRef into bytes sent over the
wire, what needs to happen? What data structures are involved? (Hint: start
here,
where a transaction is sent in response to a getdata, and trace the calls until this
line
calling Sock::Send).
The RPC getpeerinfo returns a map of bytessent_per_msg and bytesrecv_per_msg. Add a
print(self.nodes[0].getpeerinfo()[0]['bytessent_per_msg']) to one of the subtests in test/functional/p2p_sendtxrcncl.py after peers send sendtxrcncl to each other, e.g. here. BIP 330 specifies that peers send only 1 “sendtxrcncl” message to each other, and each consists of a 4B version and a 8B salt, but getpeerinfowill report a number much higher than 12B. What is the number, and what is included in those bytes?
(Hint: see
AccountForSentBytes. Where is it called?)
After PushMessage returns (e.g. on this
line),
have we sent the bytes corresponding to this message to the peer already (yes/no/maybe)? Why?
Which threads access CNode::vSendMsg? (Hint: start by searching for vSendMsg within
src/net.cpp, then search for the functions which access it, then the functions which call those
functions… until you find where the std::threads are created).
PR-specific
Can you summarize what this PR is doing?
What does it mean for
CNetMessage
to be “transport protocol agnostic”?
Prior to this PR, why are TransportSerializer and TransportDeserializer different classes?
Why should they be combined? What are the downsides to combining them, if any?
What is the difference in approach with #24545? Do you think this approach is preferable?
Commit 27f9ba23
adds an internal m_recv_mutex for protecting receiving state. The commit message also mentions
that an external mutex is needed. Why? (Hint: is it ok for vRecv to change in between calling
ReceivedMessageComplete and GetReceivedMessage?)
At a high level, what does the added fuzz test do? What kinds of bugs would it catch? What kinds of bugs would it not catch?
Commit
bb4aab90
moves the code using m_transport to convert the message to bytes from PushMessage to
SocketSendData. What are the behavior changes in this commit?
Commit
bb4aab90 mentions “removing the assumption that a message can always immediately be converted to wire bytes.” What happens in SocketSendData if m_transport isn’t able to convert messages?
Commit bb4aab90 mentions one thread being “more of a computational bottleneck” than the other. Which thread is this, and what runs on it?
<glozow> If you didn't have a chance to review the PR, today's questions are more general and you should be able to figure them out while we're going through the questions. So don't worry :) Tomorrow's questions will be PR-specific
<BrandonOdiwuor> Net sits at the bottom of the networking stack and handles low-level communication between peers while net_processing builds on top of the net layer and handles the processing and validation of messages from net layer
<glozow> I think it's also worth pointing out here that the line is somewhat blurry. `CConnman::AttemptToEvictConnection` (which is in connman/net) uses "application" logic like whether a peer provided us a valid transaction recently to decide whether we should consider evicting a peer.
<sipa> i think at some point in the codebase the serialization of data structure to bytes, and the addition of message headers, were done at the same time, so it was all call "serialization"
<glozow> The next question is about this process (and mostly an exercise to the reader): in the process of turning an application object like a `CTransactionRef` into bytes / network packets, what happens? What data structures does it turn into in the process?
<glozow> Next question is also an exercise. The RPC getpeerinfo returns a map of bytessent_per_msg and bytesrecv_per_msg. Add a print(self.nodes[0].getpeerinfo()[0]['bytessent_per_msg']) to one of the subtests in test/functional/p2p_sendtxrcncl.py after peers send sendtxrcncl to each other. What is the number of bytes sent for "sendtxrcncl" ?
<lightlike> msgMaker.Make() serializes the CTransactionRef message, calling SerializeTransaction(), then PushMessage puts the serialized msg into the vSendMsg queue, then SocketSendData adds a header/checksum (after the changes from this PR) and asks transport for the next package to send, and finally calls m_sock->Send
<instagibbs> making stuff up instead of checking: network magic + "sendtxrcncl"(padded to 12 bytes?) + 4 byte size of payload + 4 byte checksum + 4 byte version + 8 byte salt
<_aj_> yes: we(net_processing) don't have to do anything else to get it to go; no: it's extremely unlikely to have been received by the recipient by the time that function returns; maybe: if all the queues are empty it will have made it to the kernel socket layer; but if some of the queues arent, then it will still be waiting on those to drain further before getting to the OS
<sipa> one nit: even if all the queues were empty, but the message size exceeds the OS's send buffer size, only the part that fits will make it to the socket layer