test: Check that message sends successfully when header is split across two buffers #19304

use assert_equal

Why not test the debug log output as well, e.g. Incomplete headerReceived. I think this works; maybe it can be improved on:

-        msg = conn.build_message(msg_ping(nonce=12345))
-        cut_pos = 12    # Chosen at an arbitrary position within the header
-        # Send message in two pieces
-        before = self.nodes[0].getnettotals()['totalbytesrecv']
-        conn.send_raw_message(msg[:cut_pos])
-        middle = self.nodes[0].getnettotals()['totalbytesrecv']
-        # If this assert fails, we've hit an unlikely race
-        # where the test framework sent a message in between the two halves
-        assert middle == before + cut_pos
-        conn.send_raw_message(msg[cut_pos:])
-        conn.sync_with_ping(timeout=1)
+        with self.nodes[0].assert_debug_log(['Incomplete headerReceived']):
+            msg = conn.build_message(msg_ping(nonce=12345))
+            cut_pos = 12    # Chosen at an arbitrary position within the header
+            # Send message in two pieces
+            before = self.nodes[0].getnettotals()['totalbytesrecv']
+            conn.send_raw_message(msg[:cut_pos])
+            middle = self.nodes[0].getnettotals()['totalbytesrecv']
+            # If this assert fails, we've hit an unlikely race
+            # where the test framework sent a message in between the two halves
+            assert middle == before + cut_pos
+            conn.send_raw_message(msg[cut_pos:])
+            conn.sync_with_ping(timeout=1)

with self.nodes[0].assert_debug_log(['[net] Incomplete headerReceived']):

Why not test the debug log output as well

I'd prefer not to do this. My view is that functional tests should be as black box as possible, and test the functionality rather than the implementation. Using the debug log to test that an event has happened should be used only when there's no other way to test, since doing so binds the test to the exact implementation, and any changes to control flow or logging in the product mean that the tests need to be updated.

use assert_equal

:+1:

See below

Oh right, if it's just sockets then agreed.

suggestion: perhaps describe the expected result

We don't currently log that an incomplete header was received. IMO we shouldn't, it's just an implementation detail of sockets, it doesn't really mean anything on any Bitcoin level. At the very least maybe changing bitcoind logging is out of scope for this PR?

In #19302 (review) you added LogPrintf("Incomplete header");

@jonatack are you sure? I don't see anywhere in the product code that logs "Incomplete headerReceived" (and would be surprised to, since this is purely an implementation detail)

The idea is to mention the expected result. The rest was an example, success, failure, disconnects peer, etc.

Ah I agree, I'll make this a bit more descriptive

@jonatack are you sure? I don't see anywhere in the product code that logs "Incomplete headerReceived" (and would be surprised to, since this is purely an implementation detail)

Weird. I think it was a non-consistent result. Sorry about that.

In #19302 (comment) you added LogPrintf("Incomplete header");

Hm, it's possible I didn't rebuild properly. It's late here :) will finish review of your net headers refactoring tomorrow fresh and early

If #19272 if merged first, these changes in test_large_inv() won't be needed.

+1, I'm more than happy to rebase this one if #19272 goes in first

Please join this with the import from test_framework.messages above.

I suggest you disconnect the connection at the end of this subtest, so that the node and test fixture are the same state at the end of each subtest as at the start.

Ideally, it should be possible to permute subtests in any order and have the test pass. That prevents brittleness like the asserts on peer numbers that you've had to change in an unrelated subtest here.

I don't think a sleep will consistently avoid races. Any reason why a sync_with_ping (on a different connection) or a wait_until is not working?

I think getnettotals returns information about all peers, so sync_with_ping would affect totalbytesrecv right? You could take that into account too, something like:

pinger = node.add_p2p_connection()
...
conn.send_raw_message(msg[:cut_pos])
pinger.sync_with_ping()
...
assert_equal(middle, before + cut_pos + PING_SIZE)

I don't think this guarantees that conn's message has been processed just because pinger's ping has been though...?

Actually, I think wait_until might be better:

       wait_until(lambda: self.nodes[0].getnettotals()['totalbytesrecv'] == before + cut_pos)

Wishful thinking, it'd be nice if cut_pos is a size that couldn't possibly be = another message (does this exist?). It will still fail if the node receives another message in between the halves, but can't pass if it isn't received.

Ok, I like wait_until better. More expressive at the very least. How do you like this?

        # Prevent the RPC call from going too early
        conn.wait_util(int(self.nodes[0].getnettotals()['totalbytesrecv']) != before)

Should be fine. In case it fails the assert_equal(middle, before + cut_pos) should trigger

Ah we came up with almost the same thing! I thought of this #19304 (review). Regarding cut_pos, we actually have what you're looking for. cut_pos = 12 and the size of the smallest message (any of the messages with no payload) is 24 😄

I'll think about getpeerinfo vs getnettotals

I don't think this guarantees that conn's message has been processed just because pinger's ping has been though...?

I'm pretty sure you're right. However, we're looking at a race in a test for a race inside a test at this point, so I'm going to prefer the simpler fix.

TWINSIES 👯 (If I may, I think == before + cut_pos is stronger than >= before or != before)

we actually have what you're looking for

Ooh!! Genius, should be in the comment.

== before + cut_pos is stronger, but I see three possible situations that I would rather be caught by different asserts. This is possible with != and the later assert_equal

1. Something terrible happens and the first half of the message is never processed. wait_until times out and throws an exception.
2. The node takes a while but eventually processes the first half of the message (and only that). wait_until tries getnettotals a few times and then we continue past, and the test passes.
3. The race occurs and the node processes more than just the first half of the message. The assert_equals fails and we see the exception there.

Choosing == before + cut_pos or >= before could make both 1. and 3. fail on the wait_until, which is just a little harder to debug.

Do you want to make an assertion about totalbytesrecv here too? Or is it unnecessary? Also, I know this just tests that the 2 halves are received so this might be out of scope, but do you expect a pong in return? Could be tested using conn.message_count?

We do expect a pong in return, but it doesn't really matter in this test. I've just chosen ping because it's really simple, any valid message would have worked.

MarcoFalke = faster than linter

glares at pylint

thx

nit: extra newline here?

Using PEP 257 as a basis, this could be argued either way. I like this better, and it's consistent with the 7 or so other test files that I just checked.

nit: I think the fact that cut_pos < smallest message size is pretty smart and non-arbitrary :)

        cut_pos = 12    # Smaller than any other P2P message and thus easily distinguishable

:+1:

This sends a valid message.

Er... I'm very sorry if I'm being a pain in the ass, but why is this in p2p_invalid_messages?

I'm very sorry if I'm being a pain in the ass, but why is this in p2p_invalid_messages?

You're not. Because we couldn't think of anywhere better to put it 🤷‍♂️

This would allow not changing test_large_inv():

     def run_test(self):
-        self.test_buffer()
         self.test_magic_bytes()
         self.test_checksum()
         self.test_size()
         self.test_msgtype()
         self.test_large_inv()
+        self.test_buffer()
         self.test_resource_exhaustion()