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)

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

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

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

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

MarcoFalke commented at 7:49 am on July 3, 2023: member

lgtm ACK 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a 🐤

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a 🐤
3k4A0LHgzRSaeUvevA32BFKquOk+TGvs29Fj2AaHxo7IZjbAcyB7HCFnFuHrRSndSwn4fOQbjKCqKLJJqlSAEAw==

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.

MarcoFalke commented at 1:51 pm on July 14, 2023: member

lgtm ACK 1cd45d4e08c3dfd1d6423620c79169f1404ac12b 🌂

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 1cd45d4e08c3dfd1d6423620c79169f1404ac12b 🌂
3X9GvdPyim8RbDCOYc5//+G70J5uavZYmDrEIG76IqT5pUIlBCmHN8KQSlj+kUbqpgpL4x5HtzkbY8kfV5b2kAw==

DrahtBot requested review from TheCharlatan on Jul 14, 2023

fanquake merged this on Jul 19, 2023

fanquake closed this on Jul 19, 2023

MarcoFalke commented at 11:22 am on July 19, 2023: member

My suggestion, for future reference:

  0commit bfaa758ca1e690ad6d2ea34b0213f7aedc2645bb
  1Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
  2Date:   Wed Jul 19 12:41:00 2023 +0200
  3
  4    scripted-diff: Rename global test random context to g_mock_rand
  5    
  6    Instead of calling it "insecure", clarify that it is mocked, so that
  7    test failures due to the seed are hopefully more obviously attributed to
  8    it.
  9    
 10    -BEGIN VERIFY SCRIPT-
 11      sed -i 's/g_insecure_rand_ctx/g_mock_rand/g' $( git grep -l g_insecure_rand_ctx ./src )
 12    -END VERIFY SCRIPT-
 13
 14diff --git a/src/test/base58_tests.cpp b/src/test/base58_tests.cpp
 15index 7f3ca6bf93..0a72c82a59 100644
 16--- a/src/test/base58_tests.cpp
 17+++ b/src/test/base58_tests.cpp
 18@@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(base58_random_encode_decode)
 19     for (int n = 0; n < 1000; ++n) {
 20         unsigned int len = 1 + InsecureRandBits(8);
 21         unsigned int zeroes = InsecureRandBool() ? InsecureRandRange(len + 1) : 0;
 22-        auto data = Cat(std::vector<unsigned char>(zeroes, '\000'), g_insecure_rand_ctx.randbytes(len - zeroes));
 23+        auto data = Cat(std::vector<unsigned char>(zeroes, '\000'), g_mock_rand.randbytes(len - zeroes));
 24         auto encoded = EncodeBase58Check(data);
 25         std::vector<unsigned char> decoded;
 26         auto ok_too_small = DecodeBase58Check(encoded, decoded, InsecureRandRange(len));
 27diff --git a/src/test/crypto_tests.cpp b/src/test/crypto_tests.cpp
 28index 8332f54591..dc19252d3b 100644
 29--- a/src/test/crypto_tests.cpp
 30+++ b/src/test/crypto_tests.cpp
 31@@ -1119,7 +1119,7 @@ BOOST_AUTO_TEST_CASE(muhash_tests)
 32         uint256 res;
 33         int table[4];
 34         for (int i = 0; i < 4; ++i) {
 35-            table[i] = g_insecure_rand_ctx.randbits(3);
 36+            table[i] = g_mock_rand.randbits(3);
 37         }
 38         for (int order = 0; order < 4; ++order) {
 39             MuHash3072 acc;
 40@@ -1139,8 +1139,8 @@ BOOST_AUTO_TEST_CASE(muhash_tests)
 41             }
 42         }
 43 
 44-        MuHash3072 x = FromInt(g_insecure_rand_ctx.randbits(4)); // x=X
 45-        MuHash3072 y = FromInt(g_insecure_rand_ctx.randbits(4)); // x=X, y=Y
 46+        MuHash3072 x = FromInt(g_mock_rand.randbits(4)); // x=X
 47+        MuHash3072 y = FromInt(g_mock_rand.randbits(4)); // x=X, y=Y
 48         MuHash3072 z; // x=X, y=Y, z=1
 49         z *= x; // x=X, y=Y, z=X
 50         z *= y; // x=X, y=Y, z=X*Y
 51diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
 52index 13349329ff..3052bd65f4 100644
 53--- a/src/test/denialofservice_tests.cpp
 54+++ b/src/test/denialofservice_tests.cpp
 55@@ -109,7 +109,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 56 
 57 static void AddRandomOutboundPeer(NodeId& id, std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType)
 58 {
 59-    CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE);
 60+    CAddress addr(ip(g_mock_rand.randbits(32)), NODE_NONE);
 61     vNodes.emplace_back(new CNode{id++,
 62                                   /*sock=*/nullptr,
 63                                   addr,
 64diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
 65index 9c811db3e9..561ba4f2ce 100644
 66--- a/src/test/miniscript_tests.cpp
 67+++ b/src/test/miniscript_tests.cpp
 68@@ -277,7 +277,7 @@ void TestSatisfy(const std::string& testcase, const NodeRef& node) {
 69     auto challenges = FindChallenges(node); // Find all challenges in the generated miniscript.
 70     std::vector<Challenge> challist(challenges.begin(), challenges.end());
 71     for (int iter = 0; iter < 3; ++iter) {
 72-        Shuffle(challist.begin(), challist.end(), g_insecure_rand_ctx);
 73+        Shuffle(challist.begin(), challist.end(), g_mock_rand);
 74         Satisfier satisfier;
 75         TestSignatureChecker checker(satisfier);
 76         bool prev_mal_success = false, prev_nonmal_success = false;
 77diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp
 78index a2c4774338..ce406592a0 100644
 79--- a/src/test/orphanage_tests.cpp
 80+++ b/src/test/orphanage_tests.cpp
 81@@ -41,7 +41,7 @@ public:
 82 static void MakeNewKeyWithFastRandomContext(CKey& key)
 83 {
 84     std::vector<unsigned char> keydata;
 85-    keydata = g_insecure_rand_ctx.randbytes(32);
 86+    keydata = g_mock_rand.randbytes(32);
 87     key.Set(keydata.data(), keydata.data() + keydata.size(), /*fCompressedIn=*/true);
 88     assert(key.IsValid());
 89 }
 90@@ -54,7 +54,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
 91     // ecdsa_signature_parse_der_lax are executed during this test.
 92     // Specifically branches that run only when an ECDSA
 93     // signature's R and S values have leading zeros.
 94-    g_insecure_rand_ctx = FastRandomContext{uint256{33}};
 95+    g_mock_rand = FastRandomContext{uint256{33}};
 96 
 97     TxOrphanageTest orphanage;
 98     CKey key;
 99diff --git a/src/test/txrequest_tests.cpp b/src/test/txrequest_tests.cpp
100index dc257a0d51..e946e8675c 100644
101--- a/src/test/txrequest_tests.cpp
102+++ b/src/test/txrequest_tests.cpp
103@@ -392,7 +392,7 @@ void BuildBigPriorityTest(Scenario& scenario, int peers)
104 
105     // Determine the announcement order randomly.
106     std::vector<NodeId> announce_order = request_order;
107-    Shuffle(announce_order.begin(), announce_order.end(), g_insecure_rand_ctx);
108+    Shuffle(announce_order.begin(), announce_order.end(), g_mock_rand);
109 
110     // Find a gtxid whose txhash prioritization is consistent with the required ordering within pref_peers and
111     // within npref_peers.
112@@ -697,7 +697,7 @@ void TestInterleavedScenarios()
113         builders.emplace_back([](Scenario& scenario){ BuildWeirdRequestsTest(scenario); });
114     }
115     // Randomly shuffle all those functions.
116-    Shuffle(builders.begin(), builders.end(), g_insecure_rand_ctx);
117+    Shuffle(builders.begin(), builders.end(), g_mock_rand);
118 
119     Runner runner;
120     auto starttime = RandomTime1y();
121diff --git a/src/test/util/random.cpp b/src/test/util/random.cpp
122index 4c87ab8df8..fbc0a75c37 100644
123--- a/src/test/util/random.cpp
124+++ b/src/test/util/random.cpp
125@@ -11,7 +11,7 @@
126 #include <cstdlib>
127 #include <string>
128 
129-FastRandomContext g_insecure_rand_ctx;
130+FastRandomContext g_mock_rand;
131 
132 /** Return the unsigned from the environment var if available, otherwise 0 */
133 static uint256 GetUintFromEnv(const std::string& env_name)
134diff --git a/src/test/util/random.h b/src/test/util/random.h
135index c910bd6a3a..25871101c5 100644
136--- a/src/test/util/random.h
137+++ b/src/test/util/random.h
138@@ -18,7 +18,7 @@
139  * that thread_local is supported on all architectures we support) or a
140  * per-thread instance could be used in the multi-threaded test.
141  */
142-extern FastRandomContext g_insecure_rand_ctx;
143+extern FastRandomContext g_mock_rand;
144 
145 /**
146  * Flag to make GetRand in random.h return the same number
147@@ -36,35 +36,35 @@ void Seed(FastRandomContext& ctx);
148 static inline void SeedInsecureRand(SeedRand seed = SeedRand::SEED)
149 {
150     if (seed == SeedRand::ZEROS) {
151-        g_insecure_rand_ctx = FastRandomContext(/*fDeterministic=*/true);
152+        g_mock_rand = FastRandomContext(/*fDeterministic=*/true);
153     } else {
154-        Seed(g_insecure_rand_ctx);
155+        Seed(g_mock_rand);
156     }
157 }
158 
159 static inline uint32_t InsecureRand32()
160 {
161-    return g_insecure_rand_ctx.rand32();
162+    return g_mock_rand.rand32();
163 }
164 
165 static inline uint256 InsecureRand256()
166 {
167-    return g_insecure_rand_ctx.rand256();
168+    return g_mock_rand.rand256();
169 }
170 
171 static inline uint64_t InsecureRandBits(int bits)
172 {
173-    return g_insecure_rand_ctx.randbits(bits);
174+    return g_mock_rand.randbits(bits);
175 }
176 
177 static inline uint64_t InsecureRandRange(uint64_t range)
178 {
179-    return g_insecure_rand_ctx.randrange(range);
180+    return g_mock_rand.randrange(range);
181 }
182 
183 static inline bool InsecureRandBool()
184 {
185-    return g_insecure_rand_ctx.randbool();
186+    return g_mock_rand.randbool();
187 }
188 
189 static inline CAmount InsecureRandMoneyAmount()
190diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
191index 6ae2187974..8603ab2554 100644
192--- a/src/test/util/setup_common.cpp
193+++ b/src/test/util/setup_common.cpp
194@@ -75,8 +75,8 @@ using node::VerifyLoadedChainstate;
195 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
196 UrlDecodeFn* const URL_DECODE = nullptr;
197 
198-/** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */
199-static FastRandomContext g_insecure_rand_ctx_temp_path;
200+/** Random context to get unique temp data dirs. Separate from g_mock_rand, which can be seeded from a const env var */
201+static FastRandomContext g_mock_rand_temp_path;
202 
203 std::ostream& operator<<(std::ostream& os, const uint256& num)
204 {
205@@ -85,7 +85,7 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
206 }
207 
208 BasicTestingSetup::BasicTestingSetup(const ChainType chainType, const std::vector<const char*>& extra_args)
209-    : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()},
210+    : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_mock_rand_temp_path.rand256().ToString()},
211       m_args{}
212 {
213     m_node.args = &gArgs;
214@@ -441,7 +441,7 @@ void TestChain100Setup::MockMempoolMinFee(const CFeeRate& target_feerate)
215     // Manually create an invalid transaction. Manually set the fee in the CTxMemPoolEntry to
216     // achieve the exact target feerate.
217     CMutableTransaction mtx = CMutableTransaction();
218-    mtx.vin.push_back(CTxIn{COutPoint{g_insecure_rand_ctx.rand256(), 0}});
219+    mtx.vin.push_back(CTxIn{COutPoint{g_mock_rand.rand256(), 0}});
220     mtx.vout.push_back(CTxOut(1 * COIN, GetScriptForDestination(WitnessV0ScriptHash(CScript() << OP_TRUE))));
221     const auto tx{MakeTransactionRef(mtx)};
222     LockPoints lp;
223diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
224index dd914f159c..aeed8f7814 100644
225--- a/src/wallet/test/wallet_tests.cpp
226+++ b/src/wallet/test/wallet_tests.cpp
227@@ -943,7 +943,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup)
228 
229     CMutableTransaction mtx;
230     mtx.vout.push_back({COIN, GetScriptForDestination(op_dest)});
231-    mtx.vin.push_back(CTxIn(g_insecure_rand_ctx.rand256(), 0));
232+    mtx.vin.push_back(CTxIn(g_mock_rand.rand256(), 0));
233     const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash();
234 
235     {

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.

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

Reviews

Conflicts