@MarcoFalke

We should test the following cases:

1. header indicates size a, but payload is of size a-1
2. header indicates size a, but payload is of size a+1

These aren't strictly header errors. In (1), the message won't finish being read, because the deserializer is waiting for another byte. When the following message is received:

• the first byte of the header will be read as the final byte of the payload of the previous message
• that payload may or may not succeed deserialization and processing in net_processing
• the following message header will fail deserialization because it's misaligned. It'll fail the message start bytes check and be disconnected.

In (2), the following message header will be misaligned for similar reasons and will result in disconnect.

In both cases, it's the subsequent message that causes the disconnect. I don't think we necessarily need to test this, because invalid headers are tested elsewhere.

Rebased and made suggested changes. Will continue thinking about what the correct test should be for a malicious indicated size.

I don't see the downside of having a test that does:

• Send a corrupt msg with wrongly indicates size (off by one in either way)
• Send a valid msg and observe a disconnect

It should be possible to write such a test in 5-10 lines of python. Is there any reason I am missing that such a test would be inappropriate?

Regarding the situation where the payload is too large, consider the following test. A node can not tell the difference between these two "messages", they're equivalent. As opposed to testing for an oversized payload, we should test separately for:

• Checking that a node responds well to a message with an invalid checksum
• Checking that a node responds well to <24 bytes of complete junk

Sending >=24 bytes of junk (enough to fill a header or more) is covered by the rest of the tests in this module.

The situation where the payload is too small is effectively the same. The beginning of the next message will be lost, and we'll be back to receiving junk (the rest of the next message).

An incorrect payload size is IMO too high up the chain of events to be included. It's what results from an incorrect payload size that we should be checking for.

def test_equivalent_large(self):
    '''Make a message with an oversized payload
    Or, make a smaller message with the wrong checksum, then send a junk byte
    '''
    conn = self.nodes[0].add_p2p_connection(P2PDataStore())
    msg_base = msg_generic(b'badmsg', data=b'd'*5)
    msg_1 = conn.build_message(msg_base)
    cut_len = (
        4 +  # magic
        12  # msgtype
    )
    msg_1 = msg_1[:cut_len] + struct.pack("<I", len(msg_base.data) - 1) + msg_1[cut_len + 4:]
    self.nodes[0].p2p.send_raw_message(msg_1)
    with self.nodes[0].assert_debug_log(['CHECKSUM ERROR (badmsg, 4 bytes), expected a359d53a was cdbcd284']):
        # The usual connected check no longer works because the messages are misaligned
        #conn.sync_with_ping(timeout=1)
        assert self.nodes[0].p2p.is_connected
        self.nodes[0].disconnect_p2ps()
    #
    # Equivalent
    #
    conn = self.nodes[0].add_p2p_connection(P2PDataStore())
    msg_base = msg_generic(b'badmsg', data=b'd'*(5-1))
    msg_1 = conn.build_message(msg_base)
    cut_len = (
        4 +  # magic
        12 +  # msgtype
        4 # size
    )
    msg_1 = msg_1[:cut_len] + b'\xcd\xbc\xd2\x84' + msg_1[cut_len + 4:]
    self.nodes[0].p2p.send_raw_message(msg_1)
    self.nodes[0].p2p.send_raw_message(b'd')
    with self.nodes[0].assert_debug_log(['CHECKSUM ERROR (badmsg, 4 bytes), expected a359d53a was cdbcd284']):
        # The usual connected check no longer works because the messages are misaligned
        #conn.sync_with_ping(timeout=1)
        assert self.nodes[0].p2p.is_connected
        self.nodes[0].disconnect_p2ps()

Just for fun, I'll point out that we wont always disconnect on a corrupted payload size. I've made a proof of concept where the corruption neither results in a disconnect nor a misalignment. All it needs is two normal bitcoin messages and two single bit flips (one in the first message's payload size, and the other in the second message's command). It can be found here.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• p2p: Move all header verification into the network layer, extend logging #19107 (p2p: Refactor, move all header verification into the network layer, without changing behavior by troygiorshev)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Coverage

^{Updated at: 2020-06-12T18:13:45.902164.}

ACK af2a145

This is a good change and removes a lot of confused and confusing test code that isn't testing what it purports to.

Future PRs could clean this test further:

• the "resource_exhaustion" test should be removed/renamed from resource exhaustion. This was originally added to verify that receiving large messages wouldn't result in memory blowup, but that was a bad idea (tests: add invalid P2P message tests #14522 (comment)) and had to be removed.
• the "even though the node is being hammered by nonsense from one connection, it can still service other peers in a timely way." comment is wrong. All outgoing messages from the Python message thread are serial, so by the time we get to sending the pings, all of the previous large messages have been processed already.
• I can't see any reason for launching a new async task to test the magic bytes. Why not just send a message with invalid magic bytes using send_raw_message? I see this was merged without review (qa: Make swap_magic_bytes in p2p_invalid_messages atomic #15697) and there's no indication of why it's needed.
• the msg_unrecognized class can be removed and replaced with a msg_generic. The serialized length of a msg_unrecognized message seems to have caused confusion in the past (see VALID_DATA_LIMIT ) because people haven't realised the length of the length field for a serialized string

EDIT: struck out point 2. It was untrue as pointed out by Marco below.

all of the previous large messages have been processed already.

I don't think this is true. At least on travis the test failed when the ping was sent on a different connection. See #19177 (comment)

I don't think this is true. At least on travis the test failed when the ping was sent on a different connection. See #19177 (comment)

You're right. I was wrong. When I run this locally, the message sending appears to be serial because bitcoind is able to keep up with the messages as quickly as they're delivered. When bitcoind is running more slowly, such as under sanitizers, it can't keep up and Python's asyncio is able to write to both the sockets for conn and conn2 while bitcoind is processing the large invalid messages. See https://travis-ci.org/github/bitcoin/bitcoin/jobs/695869826#L3577 for example.

I don't think we necessarily need to test this, because invalid headers are tested elsewhere.

Where?

In general, I still don't understand why it is ok to remove test coverage without any replacement. We have never done this in the past and I don't see why this pull should be an exception. Don't get me wrong, I like the cleanups here and I'd like to see it merged, but I think this should and can be done without removing coverage.

If the functional tests are the wrong place to put such a test, great then move it to the unit tests, but simply removing should be the option of last resort.

For reference (master):

this pull:

Sorry, false alarm. The "red" line will only become "blue" when the socket buffer is filled in two chunks or more. This might happen non-deterministically based on network-congestion on the test machine and has nothing to do with the changes in this pull request.

re-ACK af2a145 🍦

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK af2a145e57 🍦
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiFBwv+KRk8EKRDiTcHW/1+SbEEwKl26fup1QVoLiueaW5IWuwuG6k/vgx0ZIIA
SIUVmXEXgJ/wJ+8oWtT6rR/ulh5i1S6vR38d0o1HYg6vR+sUOeuAuC/LYoKxJMHD
/AVdf9tXHk0pxIJiPOfP4DJITc/DYqcIpEDVcj22I5HJ8nrkHJcd9prMmDmYs60e
qniyKvw6c6GufEr4ziSNgs6r9Gzng7hDV7wFX/0bzy24259tedShoLOZQPdEldqa
vVyWiKufdpXb5UUhxBt68fmYDTFW5ivWhwujZGiWjNNMmiDmqwMIIglxfKjm9o3a
lEqe/19jVDf6VGcG8uo2IZap/5G319q76298GhTaLyd8qA7kvLe/f418QKeKUoVk
NZa6LW1jx6A49xQ1iwcQKWYkklNOK8OTCJi1AlmlFVwPy5+PKztvEl8+myYAfuyo
qUJ+eY0sqdCjjJJ4KsHYPcWD1uWTP3XxDgRGGA3NWbdYaQyAMLWfryc7MGj/UJU6
qm7Tuj6b
=hxKG
-----END PGP SIGNATURE-----

I can't see any reason for launching a new async task to test the magic bytes. Why not just send a message with invalid magic bytes using send_raw_message?

Done in #19264