There is a discrepancy in the implementation of our p2p protocol between
bitcoind and the testing framework. The fRelay field is an optional
field at the end of a version message as of protocol version 70001.
However, when deserializing a message in bitcoind, we don't check the
version to see if it should have an fRelay field or not. Instead we
unconditionally attempt to deserialize into the field.
This commit brings the testing framework in line with the implementation
in core.
This matters for a version message with the following fields:
Version = 60000
fRelay = 1
Bitcoind would deserialize this into a version message with
Version=60000 and fRelay=1, whereas (before this commit) our testing
framework would deserialize this into a version message with
Version=60000 and fRelay=0.
This commit adds brief documentation for this feature. Included in the
justification is the purpose of this feature as well as usage and
functionality tips.
Add a functional test for CaptureMessage. This connects and then
disconnects a peer so that the handshake can be used to check if capture
is being done correctly.
Included in a docstring in the test is the following:
From the data file we'll only check the structure.
We won't care about things like:
- Deserializing the payload of the message
- This is managed by the deserialize methods in
test_framework.messages
- The order of the messages
- There's no reason why we can't, say, change the order of the
messages in the handshake
- Message Type
- We can add new message types
We're ignoring these because they're simply too brittle to test here.
This commit adds contrib/message-capture/message-capture-parser.py, a python
script to be used alongside -capturemessages to parse the captured
messages.
It is complete with arguments and will parse any file given, sorting the
messages in the files when creating the output. If an output file is
specified with -o or --output, it will dump the messages in json format
to that file, otherwise it will print to stdout.
The small change to the unused msg_generic is to bring it in line with
the other message classes, purely to avoid a bug in the future.
This commit adds the CaptureMessage function. This will later be called
when any message is sent or received. The capture directory is fixed,
in a new folder "message_capture" in the datadir. Peers will then have
their own subfolders, named with their IP address and port, replacing
colons with underscores to keep compatibility with Windows. Inside,
received and sent messages will be captured into two binary files,
msgs_recv.dat and msgs_sent.dat.
e.g.
message_capture/203.0.113.7_56072/msgs_recv.dat
message_capture/203.0.113.7_56072/msgs_sent.dat
The format has been designed as to result in a minimal performance
impact. A parsing script is added in a later commit.
This brings PushMessage and ProcessMessages further in line with the
style guide by fixing their if statements.
LogMessage is later called, inside an if statement, inside both of these
methods.
This moves header size and netmagic checking out of net_processing and
into net. This check now runs in ReadHeader, so that net can exit early
out of receiving bytes from the peer. IsValid is now slimmed down, so
it no longer needs a MessageStartChars& parameter.
Additionally this removes the rest of the m_valid_* members from
CNetMessage.
This adds a CChainParams& member to V1TransportDeserializer member, and
use it in place of many Params() calls. In addition to reducing the
number of calls to a global, this removes a parameter from GetMessage
(and will later allow us to remove one from CMessageHeader::IsValid())
This commit removes the single-parameter contructor of CMessageHeader
and replaces it with a default constructor.
The single parameter contructor isn't used anywhere except for tests.
There is no reason to initialize a CMessageHeader with a particular
messagestart. This messagestart should always be replaced when
deserializing an actual message header so that we can run checks on it.
The default constructor initializes it to zero, just like the command
and checksum.
This also removes a parameter of a V1TransportDeserializer constructor,
as it was only used for this purpose.
This removes the m_valid_checksum member from CNetMessage. Instead,
GetMessage() returns an Optional.
Additionally, GetMessage() has been given an out parameter to be used to
hold error information. For now it is specifically a uint32_t used to
hold the raw size of the corrupt message.
The checksum check is now done in GetMessage.
This is intended to only be used for logging.
This will allow log messages in the following commits to keep recording
the peer's ID, even when logging is moved into V1TransportDeserializer.
This commit clarifies the intended usage of message_count and
last_message. Additionally it changes the only usage of message_count
to using last_message instead, bringing the code further along the
intended usage.
A message can be broken across two buffers, with the split inside its
header. Usually this will occur when sending many messages, such that
the first buffer fills.
This test uses the RPC to verify that the message is actually being
received in two pieces.
There is a very rare chance of a race condition where the test framework
sends a message in between the two halves of the message under test. In
this case the peer will almost certainly disconnect and the test will
fail. An assert has been added to help debugging that rare case.
This is a simple refactor of the specified test. It is now brought in
line with the rest of the tests in the module. This should make things
easier to debug, as all of the tests are now grouped together at the
top.
This test originally made a message with an invalid stated length, and
an invalid checksum. This was because only the header was changed, but
the checksum stayed the same. This was fine for now because we check
the header first to see if it has a valid stated size, and we disconnect
if it does not, so we never end up checking for the checksum. If this
behavior was to change, this test would become a problem. (Indeed I
discovered this when playing around with this behavior). By instead
creating a message with an oversized payload from the start, we create a
message with an invalid stated length but a valid checksum, as intended.
Additionally, this takes advantage to the newly module-global
VALID_DATA_LIMIT as opposed to the magic 0x02000000. Yes, 4MB < 32MiB,
but at the moment when receiving a message we check both, so this makes
the test tighter.
As well, this renames those variables to match PEP8 and this clears up
the comment relating to VALID_DATA_LIMIT.
Admittedly, this commit is mainly to make the following ones cleaner.
Test 1 is a duplicate of test_size() later in the file. Inexplicably,
this test does not work on macOS, whereas test_size() does.
Test 2 is problematic for two reasons. First, it always fails with an
invalid checksum, which is probably not what was intended. Second, it's
not defined at this layer what the behavior should be. Hypothetically,
if this test was fixed so that it gave messages with valid checksums,
then the message would pass successfully thought the network layer and
fail only in the processing layer. A priori the network layer has no
idea what the size of a message "actually" is.
The "Why does behavior change at 78 bytes" is because of the following:
print(len(node.p2p.build_message(msg))) # 125
=> Payload size = 125 - 24 = 101
If we take 77 bytes, then there are 101 - 77 = 24 left
That's exactly the size of a header
So, bitcoind deserializes the header and rejects it for some other reason
(Almost always an invalid size (too large))
But, if we take 78 bytes, then there are 101 - 78 = 23 left
That's not enough to fill a header, so the socket stays open waiting for
more data. That's why we sometimes have to push additional data in
order for the peer to disconnect.
Additionally, both of these tests use the "conn" variable. For fun, go
look at where it's declared. (Hint: test_large_inv(). Don't we all
love python's idea of scope?)
Troy Giorshev