Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: BIP324: add checks for v1 prefix matching / wrong network magic detection #28588

Conversation

theStack
Copy link
Contributor

@theStack theStack commented Oct 4, 2023

This PR adds missing test coverage for the detection of incoming v1 connections and wrong network magic on BIP324-enabled (i.e. running with -v2transport=1) nodes. Both checks are using prefix sizes of 16 bytes (previously only 12 bytes were used for the v1 prefix matching, which was fixed by PR #28577).

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 4, 2023

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK Sjors, MarcoFalke
Stale ACK sipa

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

@sipa
Copy link
Member

sipa commented Oct 4, 2023

utACK c8581d4

Happy to rebase #28577 on this if it goes in first.

@DrahtBot DrahtBot removed the request for review from sipa October 4, 2023 15:23
@fanquake fanquake requested a review from vasild October 4, 2023 15:27
@sipa sipa mentioned this pull request Oct 4, 2023
43 tasks
@theStack theStack force-pushed the 202310-test-add_v1_prefix_detection_wrong_network_magic_check branch from c8581d4 to e130896 Compare October 4, 2023 22:01
@theStack
Copy link
Contributor Author

theStack commented Oct 4, 2023

Rebased on master, now that #28577 is merged. Diff:

diff --git a/test/functional/p2p_v2_transport.py b/test/functional/p2p_v2_transport.py
index 47828867b..dd564fed8 100755                                                                                                        
--- a/test/functional/p2p_v2_transport.py
+++ b/test/functional/p2p_v2_transport.py
@@ -130,9 +130,8 @@ class V2TransportTest(BitcoinTestFramework):                                                                         
         assert_equal(self.nodes[4].getblockcount(), 11)            
                                                                    
         # Check v1 prefix detection                                                                                                     
-        # TODO: this should have the same prefix size as the one used for wrong network magic detection (see below)
-        V1_PREFIX = MAGIC_BYTES["regtest"] + b"version\x00"
-        assert_equal(len(V1_PREFIX), 12)
+        V1_PREFIX = MAGIC_BYTES["regtest"] + b"version\x00\x00\x00\x00\x00"
+        assert_equal(len(V1_PREFIX), 16)
         with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:                                                   
             num_peers = len(self.nodes[0].getpeerinfo())                                                                                
             s.connect(("127.0.0.1", p2p_port(0)))                                                                                       
@@ -143,11 +142,11 @@ class V2TransportTest(BitcoinTestFramework):                                                                       
             self.wait_until(lambda: self.nodes[0].getpeerinfo()[-1]["transport_protocol_type"] == "v1")                                 
                                                                                                                                         
         # Check wrong network prefix detection (hits if the next 12 bytes correspond to a v1 version message)                           
-        wrong_network_magic_prefix = MAGIC_BYTES["signet"] + V1_PREFIX[4:] + b"\x00\x00\x00\00"
+        wrong_network_magic_prefix = MAGIC_BYTES["signet"] + V1_PREFIX[4:]
         with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
             s.connect(("127.0.0.1", p2p_port(0)))
             with self.nodes[0].assert_debug_log("V2 transport error: V1 peer with wrong MessageStart"):
-                s.sendall(wrong_network_magic_prefix)
+                s.sendall(wrong_network_magic_prefix + b"somepayload")

(note that some more data has to be sent after the 12-bytes v1 message type string, to trigger calling the ProcessReceivedKeyBytes method which contains the detection of the wrong network magic prefix)

@Sjors
Copy link
Member

Sjors commented Oct 5, 2023

utACK e130896

@DrahtBot DrahtBot requested a review from sipa October 5, 2023 07:30
@maflcko
Copy link
Member

maflcko commented Oct 5, 2023

lgtm ACK e130896

@fanquake fanquake merged commit 78fd3c2 into bitcoin:master Oct 5, 2023
16 checks passed
@theStack theStack deleted the 202310-test-add_v1_prefix_detection_wrong_network_magic_check branch October 5, 2023 11:03
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants