Skip to content

tests: fix incorrect assumption in v2transport_test #28489

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

Merged
merged 1 commit into from
Sep 16, 2023

Conversation

sipa
Copy link
Member

@sipa sipa commented Sep 15, 2023

One part of the current v2transport_test introduced in #28196 assumes that if a bit gets modified in a message, failure should instantly be detected after sending that message. This is not correct in case the length descriptor is modified, as that may cause the receiver to need more data first. Fix this by sending more messages until failure actually occurs.

Discovered in #27495 (comment).

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 15, 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 theStack

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

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3f4e1bb

Tested by applying the following simple patch for only damaging the first byte (being part of the length descriptor):

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 3eb7bdec62..0b623af0ef 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -1324,7 +1324,7 @@ public:
     /** Introduce a bit error in the data scheduled to be sent. */
     void Damage()
     {
-        m_to_send[InsecureRandRange(m_to_send.size())] ^= (uint8_t{1} << InsecureRandRange(8));
+        m_to_send[0] ^= (uint8_t{1} << InsecureRandRange(8));
     }
 };

which causes several failed assertions on master, e.g. (the number varies due to randomness):

$ ./src/test/test_bitcoin 
Running 523 test cases...
test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed

*** 7 failures are detected in the test module "Bitcoin Core Test Suite"

but succeeds on the PR.

@fanquake fanquake merged commit 372e7b6 into bitcoin:master Sep 16, 2023
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2023
3f4e1bb tests: fix incorrect assumption in v2transport_test (Pieter Wuille)

Pull request description:

  One part of the current `v2transport_test` introduced in bitcoin#28196 assumes that if a bit gets modified in a message, failure should instantly be detected after sending that message. This is not correct in case the length descriptor is modified, as that may cause the receiver to need more data first. Fix this by sending more messages until failure actually occurs.

  Discovered in bitcoin#27495 (comment).

ACKs for top commit:
  theStack:
    ACK 3f4e1bb

Tree-SHA512: faa90bf91996cbaaef62d764e746cb222eaf6796316b0d0e13709e528750b7c0ef09172f7fecfe814dbb8c136c5259f65ca1ac79318e6768a0bfc4e626a63249
@sipa sipa mentioned this pull request Sep 19, 2023
43 tasks
@bitcoin bitcoin locked and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants