test/BIP324: disconnection scenarios during v2 handshake #29431

stratospher · 2024-02-14T08:59:18Z

Add tests for the following v2 handshake scenarios:

Disconnection happens when > MAX_GARBAGE_LEN bytes garbage is sent
Disconnection happens when incorrect garbage terminator is sent
Disconnection happens when garbage bytes are tampered with
Disconnection happens when AAD of first encrypted packet after the garbage terminator is not filled
bitcoind ignores non-empty version packet and no disconnection happens

All these tests require a modified v2 P2P class (different from EncryptedP2PState used in v2_p2p.py) to implement our custom handshake behaviour based on different scenarios and have been kept in a single test file (test/functional/p2p_v2_misbehaving.py). Shifted the test in test/functional/p2p_v2_earlykeyresponse.py which is of the same pattern to this file too.

DrahtBot · 2024-02-14T08:59:20Z

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.

Type	Reviewers
ACK	sr-gi
Concept ACK	theStack, kevkevinpal, mzumsande

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:

#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
#29500 (test: create assert_not_equal util by kevkevinpal)
#29420 (test: extend the SOCKS5 Python proxy to actually connect to a destination by vasild)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections 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.

stratospher · 2024-02-15T05:27:37Z

@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.

test/functional/p2p_v2_misbehaving.py

theStack · 2024-02-16T14:35:06Z

Concept ACK

kevkevinpal

Concept ACK

added two comments I will continue to test these changes as I am still trying to understand fully what the tests do exactly

test/functional/p2p_v2_misbehaving.py

sr-gi · 2024-02-29T16:28:44Z

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?

stratospher · 2024-03-07T08:15:46Z

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:

on master

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

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.

mzumsande

Concept ACK

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...

test/functional/test_framework/test_node.py

test/functional/p2p_v2_misbehaving.py

Early key response test is a special kind of test which requires modified v2 handshake functions. More such tests can be added where v2 handshake functions send incorrect garbage terminator, excess garbage bytes etc.. Hence, rename p2p_v2_earlykey.py to a general test file name - p2p_v2_misbehaving.py. random_bitflip function (used in signature tests prior to this commit) can be used in p2p_v2_misbehaving test to generate wrong garbage terminator, wrong garbage bytes etc.. So, move the function to util.

Adds a new boolean parameter `expect_success` to the `add_p2p_connection` method. If set, the node under test doesn't wait for connection to be established and is useful for testing scenarios when disconnection is expected. Without this parameter, intermittent test failures can happen if the disconnection happens before wait_until for is_connected is hit inside `add_p2p_connection`. Co-Authored-By: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

stratospher · 2024-03-11T07:55:34Z

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 :(

theStack · 2024-03-25T14:14:25Z

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.)

test/functional/p2p_v2_misbehaving.py

sr-gi · 2024-05-06T17:07:24Z

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.

stratospher · 2024-05-17T05:39:35Z

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.

Currently, we log the number of bytes of garbage when it is generated. The log is a better fit for when the garbage actually gets sent to the transport layer.

DrahtBot · 2024-05-17T06:45:00Z

🚧 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}

stratospher · 2024-05-17T15:18:37Z

I've updated the PR to have different child class implementations for EncryptedP2PState based on the disconnection scenario we're testing - EarlyKeyResponseState, ExcessGarbageState etc...

Also introduced 2 more commits for cleaner code:

c642b08 - logging when the garbage is sent instead of when garbage is generated (suggestion from test/BIP324: functional tests for v2 P2P encryption #24748 (comment) which is useful now that we have multiple child classes for EncryptedP2PState)
38eb429 - transport version as a class variable instead of a global variable

sr-gi

I like this approach much better, it is way easier to follow and reason about. I left some comments inline.

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.

What is weird is that the test is expecting a specific log message, which seems to also match in this case. This is still the case by the way. Is there an alternative way of making sure this does not happen?

test/functional/test_framework/v2_p2p.py

sr-gi · 2024-05-20T15:04:52Z

test/functional/p2p_v2_misbehaving.py

+        """Send ellswift and garbage bytes in 2 parts when TestType = (EARLY_KEY_RESPONSE)"""
+        # Here, the 64 bytes ellswift is assumed to have it's first 4 bytes match network magic bytes.
+        # It is sent in 2 phases:
+        # 1. when `magic_sent` = False, send first 4 bytes of ellswift (matches network magic bytes)
+        # 2. when `magic_sent` = True, send remaining 60 bytes of ellswift
+        if not self.magic_sent:
+            self.generate_keypair_and_garbage()
+            self.magic_sent = True
+            return MAGIC_BYTES[self.net]
        else:
+            # `can_data_be_received` is a variable used to assert if data is received on recvbuf.
+            # 1. v2 TestNode shouldn't respond back if we send V1_PREFIX and data shouldn't be received on recvbuf.
+            # This state is represented using `can_data_be_received` = False.
+            # 2. v2 TestNode responds back when mismatch from V1_PREFIX happens and data can be received on recvbuf.
+            # This state is represented using `can_data_be_received` = True.


In 6d9df28

I still think this can be simplified. initiate_v2_handshake is going to be called, asynchronously, when creating this object. You can reduce a lot of the boilerplate, and get rid of magic_sent, as long as you do the last bit manually on the test (or just create another method, like continue_v2_handshake).

It just feels weird to me that this initialization method is called twice (one async, once manually), it doesn't feel too intuitive.

I still think this can be simplified. initiate_v2_handshake is going to be called, asynchronously, when creating this object. You can reduce a lot of the boilerplate, and get rid of magic_sent, as long as you do the last bit manually on the test (or just create another method, like continue_v2_handshake).

yes, that can be done since the last bit is being sent from MainThread anyways.

before:

send both 4 bytes network magic (first 4 bytes of ellswift) - NetworkThread

remaining ellswift_garbage bytes - MainThread

now:

send both 4 bytes network magic (first 4 bytes of ellswift) - MainThread

remaining ellswift_garbage bytes - MainThread

realised my earlier comment is the situation in master too and a sleep in MainThread where there's no waiting for some other tasks is unlikely. 😬

I've used your original suggestion and updated the PR to send everything manually!

Currently, transport version is a global variable declared as TRANSPORT_VERSION in v2_p2p.py. Making it an instance variable would help in sending non empty transport version packets for testing purposes. It might also help EncryptedP2PState be more extensible in far future protocol upgrades.

Prior to this commit, TestEncryptedP2PState would always send initial_v2_handshake bytes in 2 parts (as required by early key response test). For generalising this test and having different v2 handshake behaviour based on the test type, special behaviours like sending initial_v2_handshake bytes in 2 parts are executed only if test_type is set to EARLY_KEY_RESPONSE.

This test type is represented using EXCESS_GARBAGE.

…is sent This test type is represented using WRONG_GARBAGE_TERMINATOR. since the wrong garbage terminator is sent to TestNode, TestNode will interpret all of the gabage bytes, wrong garbage terminator, decoy messages and version packet it receives as garbage bytes. If the length of all these is more than 4095 + 16, it will result in a missing garbage terminator error. otherwise, it will result in a V2 handshake timeout error. Send only MAX_GARBAGE_LEN//2 bytes of garbage data to TestNode so that the total length received by the TestNode is at max = (MAX_GARBAGE_LEN//2) + 16 + 10*120 + 20 = 3283 bytes (which is less than 4095 + 16 bytes) and we get a consistent V2 handshake timeout error message. If we do not limit the garbage length sent, we will intermittently get both missing garbage terminator error and V2 handshake timeout error based on the garbage length and decoy packets length which are chosen at random.

… different This test type is represented using WRONG_GARBAGE. Here, garbage bytes sent to TestNode are assumed to be tampered with and do not correspond to the garbage bytes which P2PInterface calculated and uses.

This test type is represented using SEND_NO_AAD. If AAD of the first encrypted packet sent after the garbage terminator (optional decoy packet/version packet) hasn't been filled, disconnection happens.

…tion happens This test type is represented using SEND_NON_EMPTY_VERSION_PACKET.

stratospher · 2024-05-28T07:12:30Z

thank you for the helpful reviews @sr-gi!

I've updated the PR:

EARLY_KEY_RESPONSE test doesn't have magic_sent now and the initial_v2_handshake() function is cleaner
EARLY_KEY_RESPONSE test does sending of 4 bytes ellswift (which match network magic) + remaining ellswift and garbage bytes on MainThread - both are done manually now
Added v2 handshake timeout log in 7d07daa

What is weird is that the test is expecting a specific log message, which seems to also match in this case. This is still the case by the way. Is there an alternative way of making sure this does not happen?

hmm fair point, i've added a log message in InactivityCheck() to distinguish v2 handshake timeout case from version handshake timeout.

sr-gi

I love it now 🚀

ACK ae528cb

This was referenced Feb 14, 2024

test: use v2 everywhere for P2PConnection if --v2transport is enabled #29358

Merged

p2p: Increase inbound capacity for block-relay only connections #28463

Open

kevkevinpal reviewed Feb 16, 2024

View reviewed changes

test/functional/p2p_v2_misbehaving.py Outdated Show resolved Hide resolved

kevkevinpal reviewed Feb 16, 2024

View reviewed changes

test/functional/p2p_v2_misbehaving.py Show resolved Hide resolved

test/functional/p2p_v2_misbehaving.py Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 26, 2024

test, ci: add --v1transport option, add --v2transport to a CI task #29483

Merged

DrahtBot added the Needs rebase label Feb 27, 2024

stratospher force-pushed the more-v2-tests branch from ec9005c to 22cba89 Compare February 28, 2024 13:40

DrahtBot removed the Needs rebase label Feb 28, 2024

DrahtBot mentioned this pull request Feb 28, 2024

test: create assert_not_equal util #29500

Open

mzumsande mentioned this pull request Mar 1, 2024

test: p2p: check disconnect due to lack of desirable service flags #29279

Merged

mzumsande reviewed Mar 7, 2024

View reviewed changes

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved

test/functional/p2p_v2_misbehaving.py Outdated Show resolved Hide resolved

test/functional/p2p_v2_misbehaving.py Outdated Show resolved Hide resolved

stratospher force-pushed the more-v2-tests branch from 22cba89 to 9ca0439 Compare March 11, 2024 07:25

stratospher and others added 2 commits March 11, 2024 12:58

stratospher force-pushed the more-v2-tests branch from 9ca0439 to 67708b6 Compare March 11, 2024 07:37

This was referenced Mar 11, 2024

test: extend the SOCKS5 Python proxy to actually connect to a destination #29420

Open

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Open

theStack reviewed Mar 25, 2024

View reviewed changes

test/functional/p2p_v2_misbehaving.py Outdated Show resolved Hide resolved

stratospher force-pushed the more-v2-tests branch from 67708b6 to a256640 Compare May 17, 2024 05:35

test: Log when the garbage is actually sent to transport layer

c642b08

Currently, we log the number of bytes of garbage when it is generated. The log is a better fit for when the garbage actually gets sent to the transport layer.

DrahtBot added the CI failed label May 17, 2024

stratospher force-pushed the more-v2-tests branch from a256640 to b062556 Compare May 17, 2024 07:26

DrahtBot removed the CI failed label May 17, 2024

stratospher mentioned this pull request May 17, 2024

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

sr-gi reviewed May 20, 2024

View reviewed changes

stratospher added 8 commits May 27, 2024 09:50

log: Add V2 handshake timeout

7d07daa

test: Check that disconnection happens when >4095 garbage bytes is sent

12b9aa2

This test type is represented using EXCESS_GARBAGE.

test: Check that disconnection happens when AAD isn't filled

dc2137a

This test type is represented using SEND_NO_AAD. If AAD of the first encrypted packet sent after the garbage terminator (optional decoy packet/version packet) hasn't been filled, disconnection happens.

test: Check that non empty version packet is ignored and no disconnec…

ae528cb

…tion happens This test type is represented using SEND_NON_EMPTY_VERSION_PACKET.

stratospher force-pushed the more-v2-tests branch from b062556 to ae528cb Compare May 28, 2024 06:55

sr-gi reviewed May 28, 2024

View reviewed changes

DrahtBot requested review from kevkevinpal, theStack and mzumsande May 28, 2024 16:59

This was referenced May 28, 2024

net: additional disconnect logging #28521

Open

scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint #29641

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test/BIP324: disconnection scenarios during v2 handshake #29431

test/BIP324: disconnection scenarios during v2 handshake #29431

stratospher commented Feb 14, 2024

DrahtBot commented Feb 14, 2024 •

edited

stratospher commented Feb 15, 2024

theStack commented Feb 16, 2024

kevkevinpal left a comment

sr-gi commented Feb 29, 2024 •

edited

stratospher commented Mar 7, 2024

mzumsande left a comment

stratospher commented Mar 11, 2024

theStack commented Mar 25, 2024

sr-gi commented May 6, 2024 •

edited

stratospher commented May 17, 2024

DrahtBot commented May 17, 2024

stratospher commented May 17, 2024

sr-gi left a comment

sr-gi May 20, 2024

stratospher May 28, 2024

stratospher commented May 28, 2024

sr-gi left a comment

test/BIP324: disconnection scenarios during v2 handshake #29431

Are you sure you want to change the base?

test/BIP324: disconnection scenarios during v2 handshake #29431

Conversation

stratospher commented Feb 14, 2024

DrahtBot commented Feb 14, 2024 • edited

Code Coverage

Reviews

Conflicts

stratospher commented Feb 15, 2024

theStack commented Feb 16, 2024

kevkevinpal left a comment

Choose a reason for hiding this comment

sr-gi commented Feb 29, 2024 • edited

stratospher commented Mar 7, 2024

mzumsande left a comment

Choose a reason for hiding this comment

stratospher commented Mar 11, 2024

theStack commented Mar 25, 2024

sr-gi commented May 6, 2024 • edited

stratospher commented May 17, 2024

DrahtBot commented May 17, 2024

stratospher commented May 17, 2024

sr-gi left a comment

Choose a reason for hiding this comment

sr-gi May 20, 2024

Choose a reason for hiding this comment

stratospher May 28, 2024

Choose a reason for hiding this comment

stratospher commented May 28, 2024

sr-gi left a comment

Choose a reason for hiding this comment

DrahtBot commented Feb 14, 2024 •

edited

sr-gi commented Feb 29, 2024 •

edited

sr-gi commented May 6, 2024 •

edited