test/BIP324: disconnection scenarios during v2 handshake #29431

stratospher commented at 8:59 AM on February 14, 2024: contributor

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 commented at 8:59 AM on February 14, 2024: contributor

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	mzumsande, theStack, achow101
Concept ACK	kevkevinpal
Stale ACK	sr-gi

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.

stratospher commented at 5:27 AM on February 15, 2024: contributor

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

in test/functional/p2p_v2_misbehaving.py:60 in ec9005ca4b outdated

  55 | +            # send > 4095 bytes garbage
  56 | +            garbage_len = MAX_GARBAGE_LEN + random.randrange(1, 10)
  57 | +        elif self.test_type == TestType.WRONG_GARBAGE_TERMINATOR:
  58 | +            garbage_len = random.randrange(MAX_GARBAGE_LEN//2)
  59 | +        else:
  60 | +            garbage_len = random.randrange(MAX_GARBAGE_LEN + 1)

kevkevinpal commented at 1:03 AM on February 16, 2024:

doesn't the +1 make the garbage_len greater than 4095 which is what is being covered in TestType.EXCESS_GARBAGE

kevkevinpal commented at 1:05 AM on February 16, 2024:

never mind randrange doesn't include the value passed

stratospher commented at 2:51 AM on February 16, 2024:

yes!

in test/functional/p2p_v2_misbehaving.py:164 in ec9005ca4b outdated

 185 | +            ["V2 transport error: packet decryption failure"],  # SEND_NO_AAD
 186 | +            [],  # SEND_NON_EMPTY_VERSION_PACKET
 187 | +        ]
 188 | +        for test_type in TestType:
 189 | +            if test_type == TestType.EARLY_KEY_RESPONSE:
 190 | +                continue

kevkevinpal commented at 1:15 AM on February 16, 2024:

do you think it would make sense to use test_earlykeyresponse here and then make test_v2disconnection into run_test

stratospher commented at 7:35 AM on February 19, 2024:

test_earlykeyresponse sends handshake bytes etc.. in the MainThread and I feel it's better to isolate that behaviour in a different test.

also, if you grep for def test_ in test/functional, you'll see many examples of different tests listed in run_test() using the pattern which is currently used.

in test/functional/p2p_v2_misbehaving.py:133 in ec9005ca4b outdated

 153 | +        self.num_nodes = 1
 154 | +        self.extra_args = [["-v2transport=1", "-peertimeout=3"]]
 155 | +
 156 | +    def run_test(self):
 157 | +        self.test_earlykeyresponse()
 158 | +        self.test_v2disconnection()

kevkevinpal commented at 2:43 AM on February 16, 2024:

Are the test_type's in test_v2disconnection dependent on test_earlykeyresponse to run first?

I tried running the tests individually for each test_type but I ran into this error, using this diff a74f4ddcc50eeba04977d8eeb96ffee2a94dbbf9

2024-02-16T02:38:17.055000Z TestFramework (INFO): PRNG seed is: 8074818075718275852
2024-02-16T02:38:17.056000Z TestFramework (INFO): Initializing test directory /var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_dmwxb4ar
2024-02-16T02:38:22.936000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
    self.run_test()
  File "/Users/kevkevin/DEVDIR/bitcoin/./test/functional/p2p_v2_misbehaving.py", line 176, in run_test
    with self.nodes[0].assert_debug_log(expected_debug_message[test_type.value], timeout=5):
  File "/usr/local/Cellar/python@3.11/3.11.6_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/contextlib.py", line 144, in __exit__
    next(self.gen)
  File "/Users/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_node.py", line 492, in assert_debug_log
    self._raise_assertion_error('Expected messages "{}" does not partially match log:\n\n{}\n\n'.format(str(expected_msgs), print_log))
  File "/Users/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_node.py", line 190, in _raise_assertion_error
    raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] Expected messages "['version handshake timeout peer=2']" does not partially match log:

 - 2024-02-16T02:38:17.908176Z [net] [net.cpp:3712] [CNode] [net] Added connection peer=0
 - 2024-02-16T02:38:17.908650Z [net] [net.cpp:1820] [CreateNodeFromAcceptedSocket] [net] connection from 127.0.0.1:63849 accepted
 - 2024-02-16T02:38:21.015546Z [net] [net.cpp:1992] [InactivityCheck] [net] version handshake timeout peer=0
 - 2024-02-16T02:38:21.015844Z [net] [net.cpp:555] [CloseSocketDisconnect] [net] disconnecting peer=0
 - 2024-02-16T02:38:21.016463Z [net] [net_processing.cpp:1672] [FinalizeNode] [net] Cleared nodestate for peer=0


2024-02-16T02:38:22.999000Z TestFramework (INFO): Stopping nodes
2024-02-16T02:38:23.270000Z TestFramework (WARNING): Not cleaning up dir /var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_dmwxb4ar
2024-02-16T02:38:23.270000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_dmwxb4ar/test_framework.log
2024-02-16T02:38:23.271000Z TestFramework (ERROR):
2024-02-16T02:38:23.271000Z TestFramework (ERROR): Hint: Call /Users/kevkevin/DEVDIR/bitcoin/test/functional/combine_logs.py '/var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_dmwxb4ar' to consolidate all logs
2024-02-16T02:38:23.271000Z TestFramework (ERROR):
2024-02-16T02:38:23.271000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-02-16T02:38:23.272000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-02-16T02:38:23.272000Z TestFramework (ERROR):

stratospher commented at 7:50 AM on February 19, 2024:

yeah! expected_debug_message on L180 was written with the assumption that test_v2disconnection is run after test_earlykeyresponse. so peer=0 happens in test_earlykeyresponse and peer=1,2,3,4,5 happens in test_v2disconnection.

theStack commented at 2:35 PM on February 16, 2024: contributor

Concept ACK

kevkevinpal commented at 3:12 PM on February 16, 2024: contributor

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

DrahtBot added the label Needs rebase on Feb 27, 2024

stratospher force-pushed on Feb 28, 2024

DrahtBot removed the label Needs rebase on Feb 28, 2024

in test/functional/test_framework/test_node.py:697 in 32f9afb00a outdated

 693 | @@ -694,6 +694,9 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=Tru
 694 |          supports_v2_p2p = self.use_v2transport and supports_v2_p2p
 695 |          p2p_conn.peer_connect(**kwargs, send_version=send_version, net=self.chain, timeout_factor=self.timeout_factor, supports_v2_p2p=supports_v2_p2p)()
 696 |  
 697 | +        if wait_for_disconnect:

mzumsande commented at 10:06 PM on February 28, 2024:

It seems not ideal to me that this is done before the line self.p2ps.append(p2p_conn). I think it could lead to errors in which we expect to get disconnected but actually didn't: If we just haven't established the connected yet (i.e. the connection_made callback hasn't been called yet), we could abort here, and only afterwards establish the connection that would then not be part of self.p2ps.

A better approach in general might be to have an expect_success parameter instead that just skips the waits below and returns, and let the individual test deal with the waiting for disconnection waiting (depending on the test it could ask the TestNode, the test framework, both to wait, or do something else like checking logs for specific disconnect reasons, which the tests you add later already do).

stratospher commented at 7:45 AM on March 11, 2024:

woah more racy thread logic. good catch! i've implemented the expect_success parameter suggestion.

sr-gi commented at 4:28 PM on February 29, 2024: member

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 commented at 8:15 AM on March 7, 2024: contributor

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

in test/functional/p2p_v2_misbehaving.py:17 in af6bd3f664 outdated

  25 | -              This state is represented using `can_data_be_received` = True.
  26 | +class TestType(Enum):
  27 | +    """ Scenarios to be tested:
  28 | +
  29 | +    1. EARLY_KEY_RESPONSE - The responder needs to wait until one byte is received which does not match the 16 bytes
  30 | +    consisting of network magic followed by "version\x00\x00\x00\x00\x00" before sending out it's ellswift + garbage bytes

mzumsande commented at 8:39 PM on March 7, 2024:

nit: its

stratospher commented at 7:43 AM on March 11, 2024:

done.

in test/functional/p2p_v2_misbehaving.py:103 in af6bd3f664 outdated

 138 |          self.log.info('expected now, our custom data_received() function wouldn\'t result in assertion failure')
 139 |          ellswift_and_garbage_data = peer1.v2_state.initiate_v2_handshake()
 140 |          peer1.send_raw_message(ellswift_and_garbage_data)
 141 | -        peer1.wait_for_disconnect(timeout=5)
 142 | -        self.log.info('successful disconnection when MITM happens in the key exchange phase')
 143 | +        with self.nodes[0].assert_debug_log(['version handshake timeout peer=0']):

mzumsande commented at 9:12 PM on March 7, 2024:

nit: can use node0

stratospher commented at 7:42 AM on March 11, 2024:

done.

mzumsande commented at 10:24 PM on March 7, 2024: contributor

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

stratospher force-pushed on Mar 11, 2024

test: Rename early key response test and move random_bitflip to util

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.

bf9669af9c

test: Support disconnect waiting for add_p2p_connection

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>

86cca2cba2

stratospher force-pushed on Mar 11, 2024

stratospher commented at 7:55 AM on March 11, 2024: contributor

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

in test/functional/p2p_v2_misbehaving.py:44 in ee56293376 outdated

  67 | +            # 1. when `magic_sent` = False, send first 4 bytes of ellswift (matches network magic bytes)
  68 | +            # 2. when `magic_sent` = True, send remaining 60 bytes of ellswift
  69 | +            if not self.magic_sent:
  70 | +                self.generate_keypair_and_garbage()
  71 | +                self.magic_sent = True
  72 | +                return b"\xfa\xbf\xb5\xda"

theStack commented at 1:36 PM on March 25, 2024:

in commit ee562933765e64babe067bfcb56a65c8637df82c, magic-number-nit:

                return MAGIC_BYTES[self.net]

stratospher commented at 3:18 PM on May 17, 2024:

done.

theStack commented at 2:14 PM on March 25, 2024: contributor

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

sr-gi commented at 5:07 PM on May 6, 2024: member

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 force-pushed on May 17, 2024

stratospher commented at 5:39 AM on May 17, 2024: contributor

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.

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

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.

c642b08c4e

DrahtBot commented at 6:45 AM on May 17, 2024: contributor

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

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/25085973641</sub>

DrahtBot added the label CI failed on May 17, 2024

stratospher force-pushed on May 17, 2024

DrahtBot removed the label CI failed on May 17, 2024

stratospher commented at 3:18 PM on May 17, 2024: contributor

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:

c642b08c - logging when the garbage is sent instead of when garbage is generated (suggestion from #24748 (review) which is useful now that we have multiple child classes for EncryptedP2PState)
38eb429 - transport version as a class variable instead of a global variable

in test/functional/test_framework/v2_p2p.py:81 in 38eb429844 outdated

  77 | @@ -79,6 +78,7 @@ class EncryptedP2PState:
  78 |  
  79 |          encrypt/decrypt v2 P2P messages using v2_enc_packet() and v2_receive_packet().
  80 |      """
  81 | +    transport_version = b''

sr-gi commented at 2:46 PM on May 20, 2024:

In 38eb42984406dd9eabba0e3d197c7336aed495c7

This is not being used as a class attribute, so it should belong inside __init__()

stratospher commented at 6:58 AM on May 28, 2024:

done.

in test/functional/p2p_v2_misbehaving.py:45 in 6d9df282d0 outdated

  69 |          else:
  70 | +            # `can_data_be_received` is a variable used to assert if data is received on recvbuf.
  71 | +            # 1. v2 TestNode shouldn't respond back if we send V1_PREFIX and data shouldn't be received on recvbuf.
  72 | +            # This state is represented using `can_data_be_received` = False.
  73 | +            # 2. v2 TestNode responds back when mismatch from V1_PREFIX happens and data can be received on recvbuf.
  74 | +            # This state is represented using `can_data_be_received` = True.

sr-gi commented at 3:04 PM on May 20, 2024:

In 6d9df282d0ca925a596787df18bf88ae48deef3a

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.

stratospher commented at 7:03 AM on May 28, 2024:

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!

sr-gi commented at 4:38 PM on May 20, 2024: member

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: Make global TRANSPORT_VERSION variable an instance variable

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.

d4a1da8543

log: Add V2 handshake timeout 7d07daa623

stratospher force-pushed on May 28, 2024

stratospher commented at 7:12 AM on May 28, 2024: contributor

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 commented at 4:59 PM on May 28, 2024: member

I love it now 🚀

ACK ae528cb9e29b75d36a8018954f45bcfb886a4557

DrahtBot requested review from kevkevinpal on May 28, 2024

DrahtBot requested review from theStack on May 28, 2024

DrahtBot requested review from mzumsande on May 28, 2024

in test/functional/test_framework/test_node.py:690 in 86cca2cba2 outdated

 686 | @@ -687,14 +687,15 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=Tru
 687 |          if supports_v2_p2p is None:
 688 |              supports_v2_p2p = self.use_v2transport
 689 |  
 690 | -

mzumsande commented at 6:18 PM on June 12, 2024:

unrelated whitespace change?

stratospher commented at 5:33 AM on June 17, 2024:

PEP 8: E303 too many blank lines (2) was showing up in my code editor. so maybe we can keep it since we're touching the function here.

Mazzika1 commented at 6:35 AM on June 17, 2024:

Okay

in test/functional/p2p_v2_misbehaving.py:68 in 4a7f541054 outdated

 105 |          self.log.info('Sending ellswift bytes in parts to ensure that response from responder is received only when')
 106 |          self.log.info('ellswift bytes have a mismatch from the 16 bytes(network magic followed by "version\\x00\\x00\\x00\\x00\\x00")')
 107 |          node0 = self.nodes[0]
 108 |          self.log.info('Sending first 4 bytes of ellswift which match network magic')
 109 |          self.log.info('If a response is received, assertion failure would happen in our custom data_received() function')
 110 |          # send happens in `initiate_v2_handshake()` in `connection_made()`

mzumsande commented at 6:20 PM on June 12, 2024:

commit 4a7f541054d634a34e5f3cbda427e400dc1569f1: This comment is no longer true and should be removed - the send happens below instead.

stratospher commented at 5:45 AM on June 17, 2024:

done.

in test/functional/p2p_v2_misbehaving.py:145 in 4a7f541054 outdated

 117 | -        ellswift_and_garbage_data = peer1.v2_state.initiate_v2_handshake()
 118 | -        peer1.send_raw_message(ellswift_and_garbage_data)
 119 | -        peer1.wait_for_disconnect(timeout=5)
 120 | -        self.log.info('successful disconnection when MITM happens in the key exchange phase')
 121 | +        peer1.send_raw_message(peer1.v2_state.ellswift_ours[4:] + peer1.v2_state.sent_garbage)
 122 | +        peer1.v2_state.can_data_be_received = True

mzumsande commented at 6:29 PM on June 12, 2024:

commit 4a7f541054d634a34e5f3cbda427e400dc1569f1: I think that this could fail intermittently (can put a sleep before this line to trigger). I think we have to first set can_data_be_received, and only then send the rest of the data.

stratospher commented at 5:35 AM on June 17, 2024:

agree! done.

in test/functional/p2p_v2_misbehaving.py:54 in 12b9aa22d6 outdated

  37 | @@ -32,6 +38,15 @@ def initiate_v2_handshake(self):
  38 |          return b""
  39 |  
  40 |  
  41 | +class ExcessGarbageState(EncryptedP2PState):
  42 | +    """Generate > MAX_GARBAGE_LEN garbage bytes"""
  43 | +    def generate_keypair_and_garbage(self):
  44 | +        ellswift_garbage_bytes = super().generate_keypair_and_garbage()
  45 | +        even_more_garbage = random.randbytes(MAX_GARBAGE_LEN)

mzumsande commented at 6:46 PM on June 12, 2024:

commit 12b9aa22d6a74e4f6e9ef9672569f19d33d4f52b: I think that the test would fail if generate_keypair_and_garbage() returns 0 garbage bytes, so MAX_GARBAGE_LEN + 1 might be needed here.

stratospher commented at 5:41 AM on June 17, 2024:

oh looks like 0 garbage bytes being returned can also mess up the functions where we're tampering the garbage bytes (WRONG_GARBAGE) and SEND_NO_AAD where AAD is the garbage bytes sent. so I've change generate_keypair_and_garbage() in EncryptedP2PState to return 1 bytes garbage minimum instead.

Mazzika1 commented at 6:36 AM on June 17, 2024:

Okay

Mazzika1 commented at 6:36 AM on June 17, 2024:

Okay

mzumsande commented at 6:16 PM on June 18, 2024:

Removing the possibility of 0 garbage in EncryptedP2PState just to make this test nicer is a bit unfortunate in my opinion: If the node had a bug dealing with zero-length garbage, the test framework wouldn't be able to catch that anymore. Can't we change it locally just for the test cases who need it?

stratospher commented at 2:15 PM on June 21, 2024:

hmm good point. I've added garbage_len as an optional argument in generate_keypair_and_garbage() so that some of the test cases which don't support 0 length garbage can avoid it.

mzumsande commented at 6:47 PM on June 25, 2024:

looks good to me now!

DrahtBot requested review from mzumsande on Jun 12, 2024

test: Introduce test types and modify v2 handshake function accordingly

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.

e075fd131d

stratospher force-pushed on Jun 17, 2024

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

This test type is represented using EXCESS_GARBAGE.

e351576862

test: Check that disconnection happens when wrong garbage terminator 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.

ad1482d5a2

test: Check that disconnection happens when garbage sent/received are 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.

b5e6238fdb

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

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.

997cc00b95

test: Check that non empty version packet is ignored and no disconnection happens

This test type is represented using SEND_NON_EMPTY_VERSION_PACKET.

c9dacd958d

stratospher force-pushed on Jun 21, 2024

mzumsande commented at 6:46 PM on June 25, 2024: contributor

ACK c9dacd958d7c1e98b08a7083c299d981e4c11193

I'd prefer combining some of the first commits with the respective test commits that actually make use of the change, but that's just my opinion ad maybe a matter of taste.

DrahtBot requested review from sr-gi on Jun 25, 2024

in test/functional/p2p_v2_misbehaving.py:43 in e075fd131d outdated

  75 |  
  76 |      def connection_made(self, transport):
  77 | -        """64 bytes ellswift is sent in 2 parts during `initial_v2_handshake()`"""
  78 | -        self.v2_state = TestEncryptedP2PState()
  79 | +        if self.test_type == TestType.EARLY_KEY_RESPONSE:
  80 | +            self.v2_state = EarlyKeyResponseState(initiating=True, net='regtest')

theStack commented at 12:55 PM on June 27, 2024:

small nit, potential follow-up: could set 'regtest' as default value for the net parameter in the EncryptedP2PState class constructor (__init__), so it doesn't have to be specified repeatedly here.

theStack approved

theStack commented at 1:02 PM on June 27, 2024: contributor

Code-review ACK c9dacd958d7c1e98b08a7083c299d981e4c11193

Nice approach using a EncryptedP2PState subclass for each test scenario, I agree with other reviewers that this looks much cleaner now and is easier to follow.