test: move remaining rand code from util/setup

jonatack commented at 6:05 PM on April 5, 2023: contributor

and drop the util/random dependency on util/setup_common. This improves code separation and allows util/setup_common to call util/random functions without creating a circular dependency, thereby addressing #26940 (comment) by glozow (thanks!)

DrahtBot commented at 6:05 PM on April 5, 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	MarcoFalke
Stale ACK	TheCharlatan

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/733 (Deniability - a tool to automatically improve coin ownership privacy by denavila)
#27792 (wallet: Deniability API (Unilateral Transaction Meta-Privacy) by denavila)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Apr 5, 2023

jonatack force-pushed on Apr 5, 2023

jonatack marked this as ready for review on Apr 5, 2023

DrahtBot added the label Needs rebase on May 10, 2023

jonatack force-pushed on May 11, 2023

DrahtBot removed the label Needs rebase on May 11, 2023

jonatack commented at 12:38 PM on May 11, 2023: contributor

Rebased 🧸

DrahtBot added the label Needs rebase on May 17, 2023

jonatack force-pushed on May 31, 2023

jonatack commented at 1:23 PM on May 31, 2023: contributor

Rebased for #27571 🥈

DrahtBot removed the label Needs rebase on May 31, 2023

DrahtBot added the label Needs rebase on Jun 12, 2023

jonatack force-pushed on Jun 13, 2023

DrahtBot removed the label Needs rebase on Jun 14, 2023

DrahtBot added the label CI failed on Jun 14, 2023

jonatack commented at 2:34 AM on June 14, 2023: contributor

Rebased for #27783 🐶 ... edit, and again for CI fix in #27886

test: move remaining random test util code from setup_common to random

and drop the util/random dependency on util/setup_common.

This improves code separation and avoids creating a circular dependency if
setup_common needs to call the util/random functions.

1b246fdd14

test: move random.h include header from setup_common.h to cpp 1cd45d4e08

jonatack force-pushed on Jun 14, 2023

DrahtBot removed the label CI failed on Jun 14, 2023

TheCharlatan approved

TheCharlatan commented at 6:51 AM on July 3, 2023: contributor

ACK 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a

in src/test/util/random.h:76 in 5b3f6a49e6 outdated

  72 | @@ -72,4 +73,9 @@ static inline CAmount InsecureRandMoneyAmount()
  73 |      return static_cast<CAmount>(InsecureRandRange(MAX_MONEY + 1));
  74 |  }
  75 |  
  76 | +static inline std::vector<unsigned char> InsecureRandBytes(int bytes)

maflcko commented at 7:49 AM on July 3, 2023:

in 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a:

Some notes:

I wonder if this should still be called "insecure", as the underlying randomness is no longer insecure, just deterministic, which is obvious in a test env. Maybe call it MockedRandBytes?
static isn't needed?
I wonder if it makes sense to wrap all member functions to global functions. Why not just call the member function of the global? (Before maybe renaming it to g_mock_random or g_test_random or so? This would allow to make use of newly added or modified member functions directly, without having to touch this header.

jonatack commented at 4:24 PM on July 7, 2023:

Good points. I'm a little hesitant to invalidate the reviews (thank you both!) but tentatively updated the name of the new helper and removed the static annotations as they are trivial to re-ACK (deferring discussion of the last point for a possible follow-up, if that's ok).

maflcko commented at 6:33 AM on July 8, 2023:

The reason why I left the comment is that randbytes is a template function and your wrapper function isn't one, so I fail to see the benefits of the wrapper function. It would be good to mention at least one benefit. Otherwise it seems odd to change code in one direction and then create a follow-up to revert the exact code changes and do some other change.

jonatack commented at 5:38 PM on July 12, 2023:

The reason why I left the comment is that randbytes is a template function

Hm, it became a template function only a few hours before the time of the comment, due to the merge of #28012.

It would be good to mention at least one benefit.

Consistent use of the rand test helpers for developer quality-of-life might be a benefit.

#28012 was merged without the template actually being used or any clear reason for the change.

What would be your suggestion regarding how to proceed here? (Thanks!)

jonatack commented at 5:44 PM on July 12, 2023:

Seems best to drop the last two commits here. Implementing the 3rd suggestion above might make the diff much larger and seems out of scope for this pull.

maflcko commented at 5:48 PM on July 12, 2023:

It was just one example. To explain it more generally: Any interface change to the underlying class will have to be done twice (or left inconsistent), which doesn't seem like an improvement (at least to me). Also there is nothing that prevents a developer from directly calling the underlying class, so adding two different ways to achieve the same or similar thing is confusing as well (at least to me).

What would be your suggestion regarding how to proceed here? (Thanks!)

My general suggestion would be what I said previously: "... just call the member function of the global? (Before maybe renaming it to g_mock_random or g_test_random or so? This would allow to make use of newly added or modified member functions directly, without having to touch this header."

jonatack commented at 6:00 PM on July 12, 2023:

Yes, but until such time that the approach is revamped, using the existing helpers consistently would have been beneficial in that it aids development and review by being more clear. That said, it's orthogonal to this pull and I'm dropping the commits that made them consistent.

maflcko commented at 6:15 PM on July 12, 2023:

For context, the functions stem from a time when they actually had a multi-line implementation body. Then the implementation was removed and replaced to be an alias, but no one bothered to resolve the simple alias.

maflcko commented at 7:49 AM on July 3, 2023: member

lgtm ACK 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a 🐤

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a 🐤
k4A0LHgzRSaeUvevA32BFKquOk+TGvs29Fj2AaHxo7IZjbAcyB7HCFnFuHrRSndSwn4fOQbjKCqKLJJqlSAEAw==

</details>

jonatack force-pushed on Jul 7, 2023

jonatack commented at 7:17 PM on July 7, 2023: contributor

Thank you for the reviews @TheCharlatan and @MarcoFalke. Updated the name of the new helper and removed the static annotations per #27425 (review), should be trivial to re-ACK.

jonatack force-pushed on Jul 12, 2023

jonatack commented at 6:08 PM on July 12, 2023: contributor

Dropped the three commits relating to the test helpers per discussion at #27425 (review).

The remaining changes were ACKed above.

maflcko commented at 1:51 PM on July 14, 2023: member

lgtm ACK 1cd45d4e08c3dfd1d6423620c79169f1404ac12b 🌂

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 1cd45d4e08c3dfd1d6423620c79169f1404ac12b 🌂
X9GvdPyim8RbDCOYc5//+G70J5uavZYmDrEIG76IqT5pUIlBCmHN8KQSlj+kUbqpgpL4x5HtzkbY8kfV5b2kAw==

</details>

DrahtBot requested review from TheCharlatan on Jul 14, 2023

fanquake merged this on Jul 19, 2023

fanquake closed this on Jul 19, 2023

maflcko commented at 11:22 AM on July 19, 2023: member

My suggestion, for future reference:

commit bfaa758ca1e690ad6d2ea34b0213f7aedc2645bb
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date:   Wed Jul 19 12:41:00 2023 +0200

    scripted-diff: Rename global test random context to g_mock_rand
    
    Instead of calling it "insecure", clarify that it is mocked, so that
    test failures due to the seed are hopefully more obviously attributed to
    it.
    
    -BEGIN VERIFY SCRIPT-
      sed -i 's/g_insecure_rand_ctx/g_mock_rand/g' $( git grep -l g_insecure_rand_ctx ./src )
    -END VERIFY SCRIPT-

diff --git a/src/test/base58_tests.cpp b/src/test/base58_tests.cpp
index 7f3ca6bf93..0a72c82a59 100644
--- a/src/test/base58_tests.cpp
+++ b/src/test/base58_tests.cpp
@@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(base58_random_encode_decode)
     for (int n = 0; n < 1000; ++n) {
         unsigned int len = 1 + InsecureRandBits(8);
         unsigned int zeroes = InsecureRandBool() ? InsecureRandRange(len + 1) : 0;
-        auto data = Cat(std::vector<unsigned char>(zeroes, '\000'), g_insecure_rand_ctx.randbytes(len - zeroes));
+        auto data = Cat(std::vector<unsigned char>(zeroes, '\000'), g_mock_rand.randbytes(len - zeroes));
         auto encoded = EncodeBase58Check(data);
         std::vector<unsigned char> decoded;
         auto ok_too_small = DecodeBase58Check(encoded, decoded, InsecureRandRange(len));
diff --git a/src/test/crypto_tests.cpp b/src/test/crypto_tests.cpp
index 8332f54591..dc19252d3b 100644
--- a/src/test/crypto_tests.cpp
+++ b/src/test/crypto_tests.cpp
@@ -1119,7 +1119,7 @@ BOOST_AUTO_TEST_CASE(muhash_tests)
         uint256 res;
         int table[4];
         for (int i = 0; i < 4; ++i) {
-            table[i] = g_insecure_rand_ctx.randbits(3);
+            table[i] = g_mock_rand.randbits(3);
         }
         for (int order = 0; order < 4; ++order) {
             MuHash3072 acc;
@@ -1139,8 +1139,8 @@ BOOST_AUTO_TEST_CASE(muhash_tests)
             }
         }
 
-        MuHash3072 x = FromInt(g_insecure_rand_ctx.randbits(4)); // x=X
-        MuHash3072 y = FromInt(g_insecure_rand_ctx.randbits(4)); // x=X, y=Y
+        MuHash3072 x = FromInt(g_mock_rand.randbits(4)); // x=X
+        MuHash3072 y = FromInt(g_mock_rand.randbits(4)); // x=X, y=Y
         MuHash3072 z; // x=X, y=Y, z=1
         z *= x; // x=X, y=Y, z=X
         z *= y; // x=X, y=Y, z=X*Y
diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index 13349329ff..3052bd65f4 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -109,7 +109,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 
 static void AddRandomOutboundPeer(NodeId& id, std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType)
 {
-    CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE);
+    CAddress addr(ip(g_mock_rand.randbits(32)), NODE_NONE);
     vNodes.emplace_back(new CNode{id++,
                                   /*sock=*/nullptr,
                                   addr,
diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index 9c811db3e9..561ba4f2ce 100644
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -277,7 +277,7 @@ void TestSatisfy(const std::string& testcase, const NodeRef& node) {
     auto challenges = FindChallenges(node); // Find all challenges in the generated miniscript.
     std::vector<Challenge> challist(challenges.begin(), challenges.end());
     for (int iter = 0; iter < 3; ++iter) {
-        Shuffle(challist.begin(), challist.end(), g_insecure_rand_ctx);
+        Shuffle(challist.begin(), challist.end(), g_mock_rand);
         Satisfier satisfier;
         TestSignatureChecker checker(satisfier);
         bool prev_mal_success = false, prev_nonmal_success = false;
diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp
index a2c4774338..ce406592a0 100644
--- a/src/test/orphanage_tests.cpp
+++ b/src/test/orphanage_tests.cpp
@@ -41,7 +41,7 @@ public:
 static void MakeNewKeyWithFastRandomContext(CKey& key)
 {
     std::vector<unsigned char> keydata;
-    keydata = g_insecure_rand_ctx.randbytes(32);
+    keydata = g_mock_rand.randbytes(32);
     key.Set(keydata.data(), keydata.data() + keydata.size(), /*fCompressedIn=*/true);
     assert(key.IsValid());
 }
@@ -54,7 +54,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
     // 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{uint256{33}};
+    g_mock_rand = FastRandomContext{uint256{33}};
 
     TxOrphanageTest orphanage;
     CKey key;
diff --git a/src/test/txrequest_tests.cpp b/src/test/txrequest_tests.cpp
index dc257a0d51..e946e8675c 100644
--- a/src/test/txrequest_tests.cpp
+++ b/src/test/txrequest_tests.cpp
@@ -392,7 +392,7 @@ void BuildBigPriorityTest(Scenario& scenario, int peers)
 
     // Determine the announcement order randomly.
     std::vector<NodeId> announce_order = request_order;
-    Shuffle(announce_order.begin(), announce_order.end(), g_insecure_rand_ctx);
+    Shuffle(announce_order.begin(), announce_order.end(), g_mock_rand);
 
     // Find a gtxid whose txhash prioritization is consistent with the required ordering within pref_peers and
     // within npref_peers.
@@ -697,7 +697,7 @@ void TestInterleavedScenarios()
         builders.emplace_back([](Scenario& scenario){ BuildWeirdRequestsTest(scenario); });
     }
     // Randomly shuffle all those functions.
-    Shuffle(builders.begin(), builders.end(), g_insecure_rand_ctx);
+    Shuffle(builders.begin(), builders.end(), g_mock_rand);
 
     Runner runner;
     auto starttime = RandomTime1y();
diff --git a/src/test/util/random.cpp b/src/test/util/random.cpp
index 4c87ab8df8..fbc0a75c37 100644
--- a/src/test/util/random.cpp
+++ b/src/test/util/random.cpp
@@ -11,7 +11,7 @@
 #include <cstdlib>
 #include <string>
 
-FastRandomContext g_insecure_rand_ctx;
+FastRandomContext g_mock_rand;
 
 /** Return the unsigned from the environment var if available, otherwise 0 */
 static uint256 GetUintFromEnv(const std::string& env_name)
diff --git a/src/test/util/random.h b/src/test/util/random.h
index c910bd6a3a..25871101c5 100644
--- a/src/test/util/random.h
+++ b/src/test/util/random.h
@@ -18,7 +18,7 @@
  * that thread_local is supported on all architectures we support) or a
  * per-thread instance could be used in the multi-threaded test.
  */
-extern FastRandomContext g_insecure_rand_ctx;
+extern FastRandomContext g_mock_rand;
 
 /**
  * Flag to make GetRand in random.h return the same number
@@ -36,35 +36,35 @@ void Seed(FastRandomContext& ctx);
 static inline void SeedInsecureRand(SeedRand seed = SeedRand::SEED)
 {
     if (seed == SeedRand::ZEROS) {
-        g_insecure_rand_ctx = FastRandomContext(/*fDeterministic=*/true);
+        g_mock_rand = FastRandomContext(/*fDeterministic=*/true);
     } else {
-        Seed(g_insecure_rand_ctx);
+        Seed(g_mock_rand);
     }
 }
 
 static inline uint32_t InsecureRand32()
 {
-    return g_insecure_rand_ctx.rand32();
+    return g_mock_rand.rand32();
 }
 
 static inline uint256 InsecureRand256()
 {
-    return g_insecure_rand_ctx.rand256();
+    return g_mock_rand.rand256();
 }
 
 static inline uint64_t InsecureRandBits(int bits)
 {
-    return g_insecure_rand_ctx.randbits(bits);
+    return g_mock_rand.randbits(bits);
 }
 
 static inline uint64_t InsecureRandRange(uint64_t range)
 {
-    return g_insecure_rand_ctx.randrange(range);
+    return g_mock_rand.randrange(range);
 }
 
 static inline bool InsecureRandBool()
 {
-    return g_insecure_rand_ctx.randbool();
+    return g_mock_rand.randbool();
 }
 
 static inline CAmount InsecureRandMoneyAmount()
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 6ae2187974..8603ab2554 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -75,8 +75,8 @@ using node::VerifyLoadedChainstate;
 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
 UrlDecodeFn* const URL_DECODE = nullptr;
 
-/** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */
-static FastRandomContext g_insecure_rand_ctx_temp_path;
+/** Random context to get unique temp data dirs. Separate from g_mock_rand, which can be seeded from a const env var */
+static FastRandomContext g_mock_rand_temp_path;
 
 std::ostream& operator<<(std::ostream& os, const uint256& num)
 {
@@ -85,7 +85,7 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
 }
 
 BasicTestingSetup::BasicTestingSetup(const ChainType chainType, const std::vector<const char*>& extra_args)
-    : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()},
+    : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_mock_rand_temp_path.rand256().ToString()},
       m_args{}
 {
     m_node.args = &gArgs;
@@ -441,7 +441,7 @@ void TestChain100Setup::MockMempoolMinFee(const CFeeRate& target_feerate)
     // Manually create an invalid transaction. Manually set the fee in the CTxMemPoolEntry to
     // achieve the exact target feerate.
     CMutableTransaction mtx = CMutableTransaction();
-    mtx.vin.push_back(CTxIn{COutPoint{g_insecure_rand_ctx.rand256(), 0}});
+    mtx.vin.push_back(CTxIn{COutPoint{g_mock_rand.rand256(), 0}});
     mtx.vout.push_back(CTxOut(1 * COIN, GetScriptForDestination(WitnessV0ScriptHash(CScript() << OP_TRUE))));
     const auto tx{MakeTransactionRef(mtx)};
     LockPoints lp;
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index dd914f159c..aeed8f7814 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -943,7 +943,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup)
 
     CMutableTransaction mtx;
     mtx.vout.push_back({COIN, GetScriptForDestination(op_dest)});
-    mtx.vin.push_back(CTxIn(g_insecure_rand_ctx.rand256(), 0));
+    mtx.vin.push_back(CTxIn(g_mock_rand.rand256(), 0));
     const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash();
 
     {

jonatack deleted the branch on Jul 19, 2023

jonatack commented at 11:50 AM on July 19, 2023: contributor

My suggestion, for future reference:

Thank you, will have a look.

sidhujag referenced this in commit 25ea716a73 on Jul 19, 2023

Fabcien referenced this in commit 4d970e2fde on May 16, 2024

bitcoin locked this on Jul 18, 2024

test: move remaining rand code from util/setup_common to util/random #27425

Reviews

Conflicts