From 23539206625936d9aa7ed73c79034777e9fa4f1d Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Tue, 21 Jul 2020 09:37:20 +0200
Subject: [PATCH] Merge #16878: Fix non-deterministic coverage of test
 DoS_mapOrphans

4455949d6f0218b40d33d7fe6de6555f8f62192f 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 4455949d6f0218b40d33d7fe6de6555f8f62192f

Tree-SHA512: 987eb1f94b80d5bec4d4944e91ef43b9b8603055750362d4b4665b7f011be27045808aa9f4c6ccf8ae009b61405f9a1b8671d65a843c3328e5b8acce1f1c00a6
---
 .../devtools/test_deterministic_coverage.sh   |  1 -
 src/test/denialofservice_tests.cpp            | 20 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/contrib/devtools/test_deterministic_coverage.sh b/contrib/devtools/test_deterministic_coverage.sh
index 1418ebb140..0ce0d3261b 100755
--- a/contrib/devtools/test_deterministic_coverage.sh
+++ b/contrib/devtools/test_deterministic_coverage.sh
@@ -17,7 +17,6 @@ GCOV_EXECUTABLE="gcov"
 NON_DETERMINISTIC_TESTS=(
     "blockfilter_index_tests/blockfilter_index_initial_sync"  # src/checkqueue.h: In CCheckQueue::Loop(): while (queue.empty()) { ... }
     "coinselector_tests/knapsack_solver_test"                 # coinselector_tests.cpp: if (equal_sets(setCoinsRet, setCoinsRet2))
-    "denialofservice_tests/DoS_mapOrphans"                    # denialofservice_tests.cpp: it = mapOrphanTransactions.lower_bound(InsecureRand256());
     "fs_tests/fsbridge_fstream"                               # deterministic test failure?
     "miner_tests/CreateNewBlock_validity"                     # validation.cpp: if (GetMainSignals().CallbacksPending() > 10)
     "scheduler_tests/manythreads"                             # scheduler.cpp: CScheduler::serviceQueue()
diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index 7c524d60fe..385032aa97 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -4,10 +4,12 @@
 
 // Unit tests for denial-of-service detection/prevention code
 
+#include <arith_uint256.h>
 #include <banman.h>
 #include <chainparams.h>
 #include <net.h>
 #include <net_processing.h>
+#include <pubkey.h>
 #include <script/sign.h>
 #include <script/signingprovider.h>
 #include <script/standard.h>
@@ -361,10 +363,26 @@ static CTransactionRef RandomOrphan()
     return it->second.tx;
 }
 
+static void MakeNewKeyWithFastRandomContext(CKey& key)
+{
+    std::vector<unsigned char> keydata;
+    keydata = g_insecure_rand_ctx.randbytes(32);
+    key.Set(keydata.data(), keydata.data() + keydata.size(), /*fCompressedIn*/ true);
+    assert(key.IsValid());
+}
+
 BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
 {
+    // This test had non-deterministic coverage due to
+    // randomly selected seeds.
+    // This seed is chosen so that all branches of the function
+    // ecdsa_signature_parse_der_lax are executed during this test.
+    // Specifically branches that run only when an ECDSA
+    // signature's R and S values have leading zeros.
+    g_insecure_rand_ctx = FastRandomContext(ArithToUint256(arith_uint256(33)));
+
     CKey key;
-    key.MakeNewKey(true);
+    MakeNewKeyWithFastRandomContext(key);
     FillableSigningProvider keystore;
     BOOST_CHECK(keystore.AddKey(key));