test: BIP324: add checks for v1 prefix matching / wrong network magic detection#28588
Merged
fanquake merged 1 commit intobitcoin:masterfrom Oct 5, 2023
Conversation
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee 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. |
Member
|
utACK c8581d449efcd0c7e45a0a2476a6dc356efd45dc Happy to rebase #28577 on this if it goes in first. |
theStack
force-pushed
the
202310-test-add_v1_prefix_detection_wrong_network_magic_check
branch
from
c8581d4 to
e130896
Compare
Contributor
Author
|
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 |
Member
|
utACK e130896 |
Member
|
lgtm ACK e130896 |
theStack
deleted the
202310-test-add_v1_prefix_detection_wrong_network_magic_check
branch
Frank-GER
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 13, 2023
…/ wrong network magic detection
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Oct 15, 2024
…k magic detection Co-authored-by: UdjinM6 <[email protected]>
PastaPastaPasta
added a commit
to dashpay/dash
that referenced
this pull request
Oct 16, 2024
, bitcoin#28849, bitcoin#28805, bitcoin#28951, bitcoin#29058, bitcoin#29239, partial bitcoin#28331, bitcoin#28452 (BIP324 backports: part 2) 5dd60c4 merge bitcoin#29239: Make v2transport default for addnode RPC when enabled (Kittywhiskers Van Gogh) b2ac426 merge bitcoin#29058: use v2transport for manual/addrfetch connections, add to -netinfo (Kittywhiskers Van Gogh) 92e862a merge bitcoin#28951: damage ciphertext/aad in full byte range (Kittywhiskers Van Gogh) 4e96e26 merge bitcoin#28805: Make existing functional tests compatible with --v2transport (Kittywhiskers Van Gogh) 9371e2e merge bitcoin#28849: fix node index bug when comparing peerinfo (Kittywhiskers Van Gogh) 400c9dd merge bitcoin#28634: add check for missing garbage terminator detection (Kittywhiskers Van Gogh) 65eb194 merge bitcoin#28588: add checks for v1 prefix matching / wrong network magic detection (Kittywhiskers Van Gogh) 6074974 merge bitcoin#28577: raise V1_PREFIX_LEN from 12 to 16 (Kittywhiskers Van Gogh) ff92d1a partial bitcoin#28331: BIP324 integration (Kittywhiskers Van Gogh) f9f8805 fix: drop `CConnman::mapNodesWithDataToSend`, use transport data (UdjinM6) d39d8a4 partial bitcoin#28452: Do not use std::vector = {} to release memory (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6280 * Dependent on #6276 * Dependency for #6329 * [bitcoin#28452](bitcoin#28452) was backported as the behavior it introduces is required for backports like [bitcoin#28331](bitcoin#28331). As it pre-dates earlier BIP324 backports, the `ClearShrink` usage from those backports have also been incorporated into this backport. * When backporting [bitcoin#28331](bitcoin#28331), in TestFramework's `add_nodes()`, `extra_args[i]` is not converted to a `list` like it is done upstream and avoiding duplication of `-v2transport=1` is instead done by an additional check. This is done to prevent test failures in `feature_mnehf.py`. * The local variable `args` is removed in [bitcoin#28805](bitcoin#28805) as it does nothing (the argument appending logic is removed as part of the backport) and upstream removes it anyways. Special thanks to UdjinM6 for significant contributions to this and dependent PRs! 🎉 ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: light ACK 5dd60c4 PastaPastaPasta: utACK 5dd60c4 Tree-SHA512: 7f3d0e54e1c96fc99b2d6b1e64485110aa73aeec0e4e4245ed4e6dc0529a7608e9c39103af636d5945d289775c40d3d15d7d4a75bea82462194dbf355fca48dc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).