Skip to content

Commit b763ae0

Browse files
author
MarcoFalke
committed
Merge #16878: Fix non-deterministic coverage of test DoS_mapOrphans
4455949 Make test DoS_mapOrphans deterministic (David Reikher) Pull request description: This pull request proposes a solution to make the test `DoS_mapOrphans` in denialofservice_tests.cpp have deterministic coverage. The `RandomOrphan` function in denialofservice_tests.cpp and the implicitly called function `ecdsa_signature_parse_der_lax` in pubkey.cpp were causing the non-deterministic test coverage. In the former, if a random orphan was selected the index of which is bigger than the max. orphan index in `mapOrphanTransactions`, the last orphan was returned from `RandomOrphan`. If the random number generated was never large enough, this condition would not be fulfilled and the corresponding branch wouldn't run. The proposed solution is to force one of the 50 dependant orphans to depend on the last orphan in `mapOrphanTransactions` using the newly introduced function `OrphanByIndex` (and passing it a large uint256), forcing this branch to run at least once. In the latter, if values for ECDSA `R` or `S` (or both) had no leading zeros, some code would not be executed. The solution was to find a constant signature that would be comprised of `R` and `S` values with leading zeros and calling `CPubKey::Verify` at the end of the test with this signature forcing this code to always run at least once at the end even if it hadn't throughout the test. To test that the coverage is (at least highly likely) deterministic, I ran `contrib/devtools/test_deterministic_coverage.sh denialofservice_tests/DoS_mapOrphans 1000` and the result was deterministic coverage across 1000 runs. Also - removed denialofservice_tests test entry from the list of non-deterministic tests in the coverage script. ACKs for top commit: MarcoFalke: ACK 4455949 Tree-SHA512: 987eb1f94b80d5bec4d4944e91ef43b9b8603055750362d4b4665b7f011be27045808aa9f4c6ccf8ae009b61405f9a1b8671d65a843c3328e5b8acce1f1c00a6
2 parents caf1766 + 4455949 commit b763ae0

File tree

2 files changed

+19
-2
lines changed

2 files changed

+19
-2
lines changed

contrib/devtools/test_deterministic_coverage.sh

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ GCOV_EXECUTABLE="gcov"
1616
NON_DETERMINISTIC_TESTS=(
1717
"blockfilter_index_tests/blockfilter_index_initial_sync" # src/checkqueue.h: In CCheckQueue::Loop(): while (queue.empty()) { ... }
1818
"coinselector_tests/knapsack_solver_test" # coinselector_tests.cpp: if (equal_sets(setCoinsRet, setCoinsRet2))
19-
"denialofservice_tests/DoS_mapOrphans" # denialofservice_tests.cpp: it = mapOrphanTransactions.lower_bound(InsecureRand256());
2019
"fs_tests/fsbridge_fstream" # deterministic test failure?
2120
"miner_tests/CreateNewBlock_validity" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10)
2221
"scheduler_tests/manythreads" # scheduler.cpp: CScheduler::serviceQueue()

src/test/denialofservice_tests.cpp

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@
44

55
// Unit tests for denial-of-service detection/prevention code
66

7+
#include <arith_uint256.h>
78
#include <banman.h>
89
#include <chainparams.h>
910
#include <net.h>
1011
#include <net_processing.h>
12+
#include <pubkey.h>
1113
#include <script/sign.h>
1214
#include <script/signingprovider.h>
1315
#include <script/standard.h>
@@ -314,10 +316,26 @@ static CTransactionRef RandomOrphan()
314316
return it->second.tx;
315317
}
316318

319+
static void MakeNewKeyWithFastRandomContext(CKey& key)
320+
{
321+
std::vector<unsigned char> keydata;
322+
keydata = g_insecure_rand_ctx.randbytes(32);
323+
key.Set(keydata.data(), keydata.data() + keydata.size(), /*fCompressedIn*/ true);
324+
assert(key.IsValid());
325+
}
326+
317327
BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
318328
{
329+
// This test had non-deterministic coverage due to
330+
// randomly selected seeds.
331+
// This seed is chosen so that all branches of the function
332+
// ecdsa_signature_parse_der_lax are executed during this test.
333+
// Specifically branches that run only when an ECDSA
334+
// signature's R and S values have leading zeros.
335+
g_insecure_rand_ctx = FastRandomContext(ArithToUint256(arith_uint256(33)));
336+
319337
CKey key;
320-
key.MakeNewKey(true);
338+
MakeNewKeyWithFastRandomContext(key);
321339
FillableSigningProvider keystore;
322340
BOOST_CHECK(keystore.AddKey(key));
323341

0 commit comments

Comments
 (0)