tests: fix incorrect assumption in v2transport_test#28489
Merged
fanquake merged 1 commit intobitcoin:masterfrom Sep 16, 2023
Merged
tests: fix incorrect assumption in v2transport_test#28489fanquake merged 1 commit intobitcoin:masterfrom
fanquake merged 1 commit intobitcoin:masterfrom
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. |
theStack
approved these changes
Sep 15, 2023
Contributor
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
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.
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
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.
4 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.
One part of the current
v2transport_testintroduced 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).