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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29500 (test: create assert_not_equal util by kevkevinpal)
• #29420 (test: extend the SOCKS5 Python proxy to actually connect to a destination by vasild)
• #28521 (net: additional disconnect logging by Sjors)

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.

@sr-gi, i tried this manually sending idea but i still think intermittent failures are possible there.

• we can't get rid of can_data_be_received variable because if we don't use this variable, test would succeed irrespective of whether we send 4 bytes network magic first or 4 bytes from ellswift bytes first and we don't want that.
• so since data_received() always happens in Network thread and send of ellswift bytes + setting can_data_be_received=True happens on MainThread, in the rare scenario that data_received() gets called before setting can_data_be_received, an intermittent failure could happen i think.

here's a branch where i tweaked the code you shared a bit with a sleep statement for making the test crash and reintroducing can_data_be_received variable.

Concept ACK

• we can't get rid of can_data_be_received variable because if we don't use this variable, test would succeed irrespective of whether we send 4 bytes network magic first or 4 bytes from ellswift bytes first and we don't want that.

I've been playing around sending anything different than the first 4 bytes as the network magic and test fail on my end (which is expected given data_received is called with magic_sent false on the first-byte mismatch).

Do you have any working example I can try to reproduce?

I've been playing around sending anything different than the first 4 bytes as the network magic and test fail on my end (which is expected given data_received is called with magic_sent false on the first-byte mismatch).

yes! that's expected behaviour and sending anything other than the first 4 bytes as network magic will make the test fail on master.

what i was trying to say was:

1. on master

• replacing return b"\xfa\xbf\xb5\xda" with any other 4 bytes will make the test fail and this is expected behaviour

3. using this patch mentioned in test: fix intermittent failure in p2p_v2_earlykeyresponse #29352

• replacing return b"\xfa\xbf\xb5\xda" with any other 4 bytes will make the test pass and this isn't behaviour we want
• this happens because can_data_be_received variable is removed in the patch

so i was just trying to say that we can't remove can_data_be_received variable.

The one thing I'm a bit unsure about is that this duplicates quite a lot of code, especially in complete_handshake - the main implementation in v2_p2p.py and the overwritten version from this test could run out of sync in the future. But I don't have a good idea how to avoid this yet...

hmm good point. will need to think about it more. simplest solution would be keeping it all in main implementation in v2_p2p.py haha but doesn't make sense to have it there just for this test file :(

The one thing I'm a bit unsure about is that this duplicates quite a lot of code, especially in complete_handshake - the main implementation in v2_p2p.py and the overwritten version from this test could run out of sync in the future. But I don't have a good idea how to avoid this yet...

hmm good point. will need to think about it more. simplest solution would be keeping it all in main implementation in v2_p2p.py haha but doesn't make sense to have it there just for this test file :(

One idea to avoid at least reimplementing generate_keypair_and_garbage would be to allow setting a fixed garbage length with a new parameter (that is None by default, meaning that a random length will be picked), e.g:

diff --git a/test/functional/test_framework/v2_p2p.py b/test/functional/test_framework/v2_p2p.py
index 8f79623bd8..c27cd5e2fe 100644
--- a/test/functional/test_framework/v2_p2p.py
+++ b/test/functional/test_framework/v2_p2p.py
@@ -111,10 +111,11 @@ class EncryptedP2PState:
             # Responding, place their public key encoding first.
             return TaggedHash("bip324_ellswift_xonly_ecdh", ellswift_theirs + ellswift_ours + ecdh_point_x32)
 
-    def generate_keypair_and_garbage(self):
+    def generate_keypair_and_garbage(self, garbage_len=None):
         """Generates ellswift keypair and 4095 bytes garbage at max"""
         self.privkey_ours, self.ellswift_ours = ellswift_create()
-        garbage_len = random.randrange(MAX_GARBAGE_LEN + 1)
+        if garbage_len is None:
+            garbage_len = random.randrange(MAX_GARBAGE_LEN + 1)
         self.sent_garbage = random.randbytes(garbage_len)
         logger.debug(f"sending {garbage_len} bytes of garbage data")
         return self.ellswift_ours + self.sent_garbage

The overruled method of initiate_v2_handshake in the test could then call this with the parameter set if needed, e.g. for the EXCESS_GARBAGE case:

     def initiate_v2_handshake(self):
         if self.test_type == TestType.EARLY_KEY_RESPONSE:
             .....
         elif self.test_type == TestType.EXCESS_GARBAGE:
             # send > 4095 bytes garbage
             return self.generate_keypair_and_garbage(garbage_len=MAX_GARBAGE_LEN + random.randrange(1, 10))

(For the WRONG_GARBAGE test, bit-flipping self.sent_garbage right after calling generate_keypair_and_garbage should still be sufficient for causing a mismatch.)

I find the approach fairly hard to follow here. Having all the logic in the constructor with if/elses based on the connection type instead of having different constructors/a test for each type of failure feels really error-prone and difficult to make sure if a test is doing what we expect. I think it'd be better to have multiple classes that build from v2_p2p and modify what's needed, even if the file gets larger.

Just an example:

In the WRONG_GARBAGE_TERMINATOR case, we are modifying the gargabe in two ways. First, we are making it smaller than expected, but also, we are randomly flipping some of its bits. Turns out if you removed both changes, the tests still passes, but I'm not sure why. I would expect this not to be the case, since removing the relevant parts should make the class equal to the expected v2_p2p.

I think it'd be better to have multiple classes that build from v2_p2p and modify what's needed, even if the file gets larger.

that's a great suggestion @sr-gi! it's possible to design those classes in such a way that it avoids code duplication and the file is almost the same size!

Turns out if you removed both changes, the tests still passes, but I'm not sure why.

because we don't send a version message to ensure that the disconnection happens in the v2 handshake phase. in your case (different from what we are testing here), we send the correct garbage terminator but because we don't send a version message - disconnection is still expected.

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/25085973641}

ACK c9dacd9

Looks like this is causing intermittent CI failures. See #30419.

Ported to the CMake-based build system in hebasto#264.