tests: fix incorrect assumption in v2transport_test #28489

1. 
   sipa commented at 11:21 am on September 15, 2023: member

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

2. tests: fix incorrect assumption in v2transport_test 3f4e1bb9ae
3. 
   DrahtBot commented at 11:21 am on September 15, 2023: contributor

   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.

4. DrahtBot added the label Tests on Sep 15, 2023
5. fanquake requested review from theStack on Sep 15, 2023
6. theStack approved
7. 
   theStack commented at 3:09 pm on September 15, 2023: contributor

   ACK 3f4e1bb9ae5ee43da9503da37b9894037d613c6d

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

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

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

    0$ ./src/test/test_bitcoin 
    1Running 523 test cases...
    2test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
    3test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
    4test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
    5test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
    6test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
    7test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
    8test/net_tests.cpp(1364): error: in "net_tests/v2transport_test": check !ret has failed
    9
   10*** 7 failures are detected in the test module "Bitcoin Core Test Suite"
   

   but succeeds on the PR.

8. fanquake merged this on Sep 16, 2023
9. fanquake closed this on Sep 16, 2023

   ---

10. Frank-GER referenced this in commit d3181ea3ca on Sep 19, 2023
11. bitcoin locked this on Sep 15, 2024

Contributors
sipa DrahtBot theStack

Labels
Tests