test: [refactor] Use m

maflcko commented at 10:00 am on August 2, 2024: member

This is mostly a style-cleanup for the tests’ random generation:

g_insecure_rand_ctx in the tests is problematic, because the name is a leftover when the generator was indeed insecure. However, now the generator is deterministic, because the seed is either passed in or printed (c.f. RANDOM_CTX_SEED). Stating that deterministic randomness is insecure in the tests seems redundant at best. Fix it by just using m_rng for the name.
The global random context has many one-line aliases, such as InsecureRand32. This is problematic, because the same line of code may use the context directly and through a wrapper at the same time. For example in net_tests (see below). This inconsistency is harmless, but confusing. Fix it by just removing the one-line aliases.

0src/test/net_tests.cpp:        auto msg_data_1 = g_insecure_rand_ctx.randbytes<uint8_t>(InsecureRandRange(100000));

The wrapper for randmoney has the same problem that the same unit test uses the context directly and through a wrapper at the same time. Also, it has a single type of Rng hardcoded. Fix it by accepting any type.

DrahtBot commented at 10:00 am on August 2, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator, ryanofsky, marcofleon
Concept ACK	ismaelsadeeq

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:

#30737 (test: Fix RANDOM_CTX_SEED use with parallel tests by hodlinator)
#30673 (coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries by andrewtoth)
#30377 (refactor: Replace ParseHex with consteval “"_hex literals by hodlinator)
#29039 (versionbits refactoring by ajtowns)
#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

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 Aug 2, 2024

ismaelsadeeq commented at 12:06 pm on August 2, 2024: member

Concept ACK

fanquake requested review from dergoegge on Aug 5, 2024

fanquake requested review from marcofleon on Aug 5, 2024

marcofleon commented at 12:54 pm on August 5, 2024: contributor

Lgtm ACK fa295215f6fa3b85b0387511920f75eeb3e12b58. Reviewed the code and ran make check. Also ran the suggested test:

RANDOM_CTX_SEED=z ./src/test/test_bitcoin --log_level=all --run_test=timeoffsets_tests/timeoffsets_warning -- -printtoconsole=1 | grep RANDOM_CTX_SEED

DrahtBot requested review from ismaelsadeeq on Aug 5, 2024

in src/test/util/random.h:25 in fa295215f6 outdated

55-}
56-
57-static inline CAmount InsecureRandMoneyAmount()
58-{
59-    return static_cast<CAmount>(InsecureRandRange(MAX_MONEY + 1));
60+    return CAmount{rng.randrange(MAX_MONEY + 1)};

marcofleon commented at 12:57 pm on August 5, 2024:

Just for my own understanding, the advantage here is now that any rng in a test can be used when calling RandMoney? Before it was limited in some way?

maflcko commented at 1:25 pm on August 5, 2024:

Correct, this is one of two reasons. Generally, I try to put the motivation and explanation of the changes in the commit body. I presume you are referring to fa6685e7348903c5bcd318c8acbf85ddf7ab07c5, which lists both reasons for the change?

marcofleon commented at 1:45 pm on August 5, 2024:

Ah got it, makes sense. I need to check the individual commits. Thanks.

ryanofsky approved

ryanofsky commented at 8:29 pm on August 6, 2024: contributor

Code review ACK fa295215f6fa3b85b0387511920f75eeb3e12b58 but I think it would be nicer to add FastRandomContext& m_rng{g_rand_ctx} to BasicTestingSetup and have most test code use m_rng instead of g_rand_ctx directly.

I think this would be nicer mostly as a matter of style since most test code now doesn’t reference global variables directly, while this PR is adding hundreds of references. But it could also help down the road, for example if we wanted to remove the global, or run test cases deterministically in parallel. Current change looks ok to me though if this suggestion is too much work.

DrahtBot added the label CI failed on Aug 7, 2024

DrahtBot commented at 11:03 am on August 7, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28259682485

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko force-pushed on Aug 7, 2024

maflcko commented at 9:09 pm on August 7, 2024: member

But it could also help down the road, for example if we wanted to remove the global, or run test cases deterministically in parallel.

I looked into it. However, it seems more work. My findings:

There are ~300 lines of code touched, which can use the member alias or the global
108 lines can not use the member alias as-is, because the compiler does not see it in their scope, so they’ll need to be fixed up manually one-by-one
Excluding them (and only them) automatically leaves some unit tests in an odd state where one function of the unit test is using the global and another function is using the member alias (Something to avoid, I’d say)
Excluding the files with compile errors completely, which makes each file self-consistent is possible, but it is only touching 80 lines of code at this point.

So I did that, and will leave the the remaining 220 lines to be fixed by someone else, because they need to be done file-by-file manually. (I am happy to review a pull request doing that)

maflcko renamed this:
~~test: [refactor] Use g_rand_ctx directly~~
test: [refactor] Use g_rng/m_rng directly
on Aug 7, 2024

maflcko force-pushed on Aug 7, 2024

maflcko commented at 9:50 pm on August 7, 2024: member

Dropped one commit, because a conflicting pull has already picked it up.

DrahtBot removed the label CI failed on Aug 7, 2024

ryanofsky commented at 5:06 pm on August 14, 2024: contributor

Code review ACK facc85b766dec2481384c878ccbf1696a8386e10. But I think we can eliminate the global now, and I think would be good to do it in this PR to avoid changing affected tests more than once. I posted a branch https://github.com/ryanofsky/bitcoin/commits/pr/rng which I hope you can steal. It has the following commits:

a334a9b9bd0d8ee520f9e2769534625a5d147705 test: Add m_rng alias for the global random context
c38390fb43e40913afa2072c7986ea80da1d4d38 test: refactor: Make unsigned promotion explicit
51712d8ee0cc8b03a692f08d8a39b0e7a7fe8609 test: refactor: Give unit test functions access to test state
ce7ef02cee703e985554c7ced196684f33f96fb9 test: refactor: Pass rng parameters to test functions
678c3df2e3a2c7366646db3ec0deb7cad0ede107 test: refactor: Accept any RandomNumberGenerator in RandMoney
c1c384e57619d9e78c7af9e3ee098f3f25fd5479 scripted-diff: [test] Use g_rng/m_rng directly
8468112a8262054d71caaa90764bfa88548a82b8 test: Remove FastRandomContext global

DrahtBot requested review from marcofleon on Aug 14, 2024

maflcko force-pushed on Aug 15, 2024

in src/test/util/random.h:28 in bb67ca09c8 outdated

24@@ -25,7 +25,7 @@ enum class SeedRand {
25 };
26 
27 /** Seed the RNG for testing. This affects all randomness, except GetStrongRandBytes(). */
28-void SeedRandomForTest(SeedRand seed = SeedRand::SEED);
29+void SeedRandomForTest(FastRandomContext& rng, SeedRand seed = SeedRand::SEED);

maflcko commented at 7:15 pm on August 15, 2024:

nit in bb67ca09c88a46b99bf83e937fa761f0e25cba37: Not sure about this change. It seems overly restrictive to require a FastRandomContext& here, when a caller may not want to pass anything at all, or a different rng.

I think it would be better to drop this change.

ryanofsky commented at 9:05 pm on August 15, 2024:

re: #30571 (review)

nit in bb67ca0: Not sure about this change. It seems overly restrictive to require a FastRandomContext& here, when a caller may not want to pass anything at all, or a different rng.

Makes sense. I hadn’t really looked at this function closely and was just trying to keep behavior unchanged. Maybe it would be good to change signature to something like:

0SeedRandomForTest(SeedRand seed = SeedRand::SEED, FastRandomContext* rng = nullptr)

and change the last line of the function to

0if (rng) rng->Reseed(GetRandHash());

so it could still be used conveniently to seed m_rng. Or maybe the parameter could just be dropped like the global g_insecure_rand_ctx is dropped, and m_rng could be managed separately.

maflcko commented at 8:57 am on August 16, 2024:

so it could still be used conveniently to seed m_rng.

Not sure. The “feature” is only used in three places, one of which will be removed in the last commit.

So it just seems easier to drop this change and instead move the call to Reseed from this function to the only two places that need it.

maflcko commented at 7:16 pm on August 15, 2024: member

Sure, pushed your branch. A skim looks good. I left a nit and will do a real review later.

maflcko force-pushed on Aug 15, 2024

ryanofsky approved

ryanofsky commented at 9:07 pm on August 15, 2024: contributor

Code review ACK f1c1b37d2247eed49156c0467daa68aa38497bb8. Current version of this looks identical to the branch I pushed, except it is rebased and whitespace in the scripted diff was adjusted.

in src/test/fuzz/fuzz.cpp:109 in f1c1b37d22 outdated

101@@ -102,12 +102,6 @@ void ResetCoverageCounters() {}
102 
103 void initialize()
104 {
105-    // By default, make the RNG deterministic with a fixed seed. This will affect all
106-    // randomness during the fuzz test, except:
107-    // - GetStrongRandBytes(), which is used for the creation of private key material.
108-    // - Creating a BasicTestingSetup or derived class will switch to a random seed.
109-    SeedRandomForTest(g_insecure_rand_ctx, SeedRand::ZEROS);

maflcko commented at 8:56 am on August 16, 2024:

q in f1c1b37d2247eed49156c0467daa68aa38497bb8: Can you explain why this change is correct? SeedRandomForTest is doing two things:

Set the internal global RNGState
Reseed the passed g_insecure_rand_ctx

Removing g_insecure_rand_ctx does not remove the need to make the fuzz tests deterministic and stable.

I’ll drop this change as well.

maflcko commented at 9:50 am on August 16, 2024:

Done: Force pushed to remove this presumed bug in the last commit

ryanofsky commented at 10:47 pm on August 16, 2024:

re: #30571 (review)

q in f1c1b37: Can you explain why this change is correct?

Of course it is not correct. I only noticed that this function was updating g_insecure_rand_ctx and didn’t notice it was updating global rng state. Thanks for fixing!

maflcko force-pushed on Aug 16, 2024

ryanofsky approved

ryanofsky commented at 10:55 pm on August 16, 2024: contributor

Code review ACK 164732369fc5ecf4c7705136a2de884419023b5a. New commit replacing SeedRandomForTest is very nice and clarifies the code.

in src/test/cuckoocache_tests.cpp:313 in ac4e757bc3 outdated

308@@ -302,7 +309,8 @@ static void test_cache_generations()
309     // immediately and never uses the other half.
310     struct block_activity {
311         std::vector<uint256> reads;
312-        block_activity(uint32_t n_insert, Cache& c) : reads()
313+    };
314+    auto add_activity = [&](uint32_t n_insert, std::vector<uint256>& reads, Cache& c)

hodlinator commented at 10:56 pm on August 16, 2024:

It would leave the code in a better state after this PR if the constructor wasn’t made into this odd sidecar lambda. We already have m_rng available from prior commit so just pass it down.

Applies on top of and reverts part of ac4e757bc3633e01db973201e1633331c0815e3a “Give unit test functions access to test state”:

 0diff --git a/src/test/cuckoocache_tests.cpp b/src/test/cuckoocache_tests.cpp
 1index d2b28b9b18..ac4fe4404e 100644
 2--- a/src/test/cuckoocache_tests.cpp
 3+++ b/src/test/cuckoocache_tests.cpp
 4@@ -309,8 +309,7 @@ void test_cache_generations()
 5     // immediately and never uses the other half.
 6     struct block_activity {
 7         std::vector<uint256> reads;
 8-    };
 9-    auto add_activity = [&](uint32_t n_insert, std::vector<uint256>& reads, Cache& c)
10+        block_activity(uint32_t n_insert, FastRandomContext& rng, Cache& c)
11         {
12             std::vector<uint256> inserts;
13             inserts.resize(n_insert);
14@@ -318,7 +317,7 @@ void test_cache_generations()
15             for (uint32_t i = 0; i < n_insert; ++i) {
16                 uint32_t* ptr = (uint32_t*)inserts[i].begin();
17                 for (uint8_t j = 0; j < 8; ++j)
18-                    *(ptr++) = InsecureRand32();
19+                    *(ptr++) = rng.rand32();
20             }
21             for (uint32_t i = 0; i < n_insert / 4; ++i)
22                 reads.push_back(inserts[i]);
23@@ -326,7 +325,8 @@ void test_cache_generations()
24                 reads.push_back(inserts[i]);
25             for (const auto& h : inserts)
26                 c.insert(h);
27-        };
28+        }
29+    };
30 
31     const uint32_t BLOCK_SIZE = 1000;
32     // We expect window size 60 to perform reasonably given that each epoch
33@@ -351,8 +351,7 @@ void test_cache_generations()
34     for (uint32_t i = 0; i < total; ++i) {
35         if (last_few.size() == WINDOW_SIZE)
36             last_few.pop_front();
37-        last_few.emplace_back();
38-        add_activity(BLOCK_SIZE, last_few.back().reads, set);
39+        last_few.emplace_back(BLOCK_SIZE, m_rng, set);
40         uint32_t count = 0;
41         for (auto& act : last_few)
42             for (uint32_t k = 0; k < POP_AMOUNT; ++k) {

maflcko commented at 2:11 pm on August 20, 2024:

Thanks, done. This is a good idea to reduce the diff and make it easier to review.

maflcko renamed this:
~~test: [refactor] Use g_rng/m_rng directly~~
test: [refactor] Use m_rng directly
on Aug 17, 2024

maflcko commented at 6:42 am on August 20, 2024: member

Edited pull description and title to remove the mention of the global, now that it is removed in the last commit

in src/test/coins_tests.cpp:43 in 164732369f outdated

34@@ -35,18 +35,21 @@ bool operator==(const Coin &a, const Coin &b) {
35 
36 class CCoinsViewTest : public CCoinsView
37 {
38+    FastRandomContext& m_rng;
39     uint256 hashBestBlock_;
40     std::map<COutPoint, Coin> map_;
41 
42 public:
43+    CCoinsViewTest(FastRandomContext& rng) : m_rng{rng} {}

hodlinator commented at 12:49 pm on August 20, 2024:

nit in cb9d4712aec195f1fc5b3bc0a46bc13b46477b85:

0    CCoinsViewTest(FastRandomContext& rng LIFETIMEBOUND) : m_rng{rng} {}

maflcko commented at 2:14 pm on August 20, 2024:

Not sure. I understand that this will add compile-time checks, however I think the benefits in test code are limited, because:

Tests are deterministic and execute fast (even in valgrind and other sanitizers), so any issues are found at roughly the same CPU cost (with or without compile-time checks).
If I change it here, I open up the door to change it elsewhere in this diff, which will make it more verbose and harder to review.
The resulting code is more verbose.
The check doesn’t apply to GCC.
Passing a temporary is already forbidden by the C++ language in this context and passing another object that violates the lifetime likely won’t be detected by LIFETIMEBOUND anyway.

So I’ll leave this as-is for now.

hodlinator commented at 1:13 pm on August 20, 2024: contributor

Concept ACK 164732369fc5ecf4c7705136a2de884419023b5a

maflcko force-pushed on Aug 20, 2024

maflcko commented at 2:32 pm on August 20, 2024: member

Force pushed to make the diff (minimally) smaller and easier to review. Should be trivial to re-review.

ryanofsky approved

ryanofsky commented at 3:12 pm on August 20, 2024: contributor

Code review ACK 3836bf1e0a207a6cb659053404bcba3a44cf7bb2

Just tweaked cuckoocache_tests since last review

DrahtBot requested review from hodlinator on Aug 20, 2024

marcofleon commented at 3:45 pm on August 20, 2024: contributor

Code review ACK 3836bf1e0a207a6cb659053404bcba3a44cf7bb2

in src/bench/sign_transaction.cpp:78 in 3836bf1e0a outdated

76-    auto msg = InsecureRand256();
77-    auto merkle_root = use_null_merkle_root ? uint256() : InsecureRand256();
78-    auto aux = InsecureRand256();
79+    auto msg = rng.rand256();
80+    auto merkle_root = use_null_merkle_root ? uint256() : rng.rand256();
81+    auto aux = rng.rand256();

hodlinator commented at 8:34 am on August 21, 2024:

nit: No longer requires test/util/random.h and doesn’t get it indirectly through setup_common.h since this is a benchmark:

0-#include <test/util/random.h>
1+#include <random.h>

in src/test/util/setup_common.h:16 in 3836bf1e0a outdated

12@@ -13,6 +13,7 @@
13 #include <primitives/transaction.h>
14 #include <pubkey.h>
15 #include <stdexcept>
16+#include <test/util/random.h>

hodlinator commented at 8:44 am on August 21, 2024:

nit: No longer needed forward declaration:

0--- a/src/test/util/setup_common.h
1+++ b/src/test/util/setup_common.h
2@@ -28,7 +28,6 @@
3 class arith_uint256;
4 class CFeeRate;
5 class Chainstate;
6-class FastRandomContext;
7 
8 /** This is connected to the logger. Can be used to redirect logs to any other log */
9 extern const std::function<void(const std::string&)> G_TEST_LOG_FUN;

in src/test/prevector_tests.cpp:215 in 3836bf1e0a outdated

213-        rand_seed = InsecureRand256();
214+    prevector_tester(FastRandomContext& rng) {
215+        SeedRandomStateForTest(SeedRand::SEED);
216+        rng.Reseed(GetRandHash());
217+        rand_seed = rng.rand256();
218         rand_cache.Reseed(rand_seed);

hodlinator commented at 9:34 am on August 21, 2024:

rand_cache is dead code since 16329224e70d0525208f6b0ba00c5e1531a4f5ea. Maybe remove it in an early clean-up commit?

maflcko commented at 10:18 am on August 21, 2024:

Good catch. I’ll add a commit to this pull request, or create a follow-up, whichever happens earlier.

maflcko commented at 3:18 pm on August 21, 2024:

rand_seed is also wrong after 022cf47dd7ef8f46e32a184e84f94d1e9f3a495c, so removing rand_seed is a “bugfix” commit.

maflcko commented at 3:33 pm on August 21, 2024:

Thanks for noticing this! The unused symbols and the wrong logging should now be fixed in prevector_tester.

in src/test/util/setup_common.h:74 in 3836bf1e0a outdated

64@@ -64,6 +65,14 @@ struct BasicTestingSetup {
65     util::SignalInterrupt m_interrupt;
66     node::NodeContext m_node; // keep as first member to be destructed last
67 
68+    FastRandomContext m_rng;
69+    /** Seed the internal global RNG state and m_rng for testing. This affects all randomness, except GetStrongRandBytes(). */
70+    void SeedRandomForTest(SeedRand seed = SeedRand::SEED)
71+    {
72+        SeedRandomStateForTest(seed);
73+        m_rng.Reseed(GetRandHash());

hodlinator commented at 9:44 am on August 21, 2024:

I understand this was the behavior in the past too, but should we do something else with m_rng if the function was called with SeedRand::ZEROS?

maflcko commented at 10:06 am on August 21, 2024:

I understand this was the behavior in the past too, but should we do something else with m_rng if the function was called with SeedRand::ZEROS?

In theory one could reseed with a constant, but I don’t see the benefits, given that it:

Requires more code to handle both cases
Is not a refactor

So maybe this can be done in a follow-up if relevant?

hodlinator commented at 7:29 am on August 22, 2024:

(Thread #30571 (review))

Seems that this is not an issue (based on my fallible reading):

SeedRandomStateForTest -> MakeRandDeterministicDANGEROUS -> GetRNGState().MakeDeterministic(seed) -> which initializes the std::optional RNGState::m_deterministic_prng.

GetRandHash -> GetRandBytes -> ProcRand(..., RNGLevel::FAST, /*always_use_real_rng=*/false) -> which will end up sampling hardware-based entropy via SeedFast but then in MixExtract use RNGState::m_deterministic_prng.

in src/test/orphanage_tests.cpp:40 in 3836bf1e0a outdated

36         if (it == m_orphans.end())
37             it = m_orphans.begin();
38         return it->second.tx;
39     }
40+
41+    FastRandomContext& m_rng;

hodlinator commented at 9:51 am on August 21, 2024:

Data-members should be private by default:

 0--- a/src/test/orphanage_tests.cpp
 1+++ b/src/test/orphanage_tests.cpp
 2@@ -20,6 +20,8 @@ BOOST_FIXTURE_TEST_SUITE(orphanage_tests, TestingSetup)
 3 
 4 class TxOrphanageTest : public TxOrphanage
 5 {
 6+    FastRandomContext& m_rng;
 7+
 8 public:
 9     TxOrphanageTest(FastRandomContext& rng) : m_rng{rng} {}
10 
11@@ -36,8 +38,6 @@ public:
12             it = m_orphans.begin();
13         return it->second.tx;
14     }
15-
16-    FastRandomContext& m_rng;
17 };
18 
19 static void MakeNewKeyWithFastRandomContext(CKey& key, FastRandomContext& rand_ctx)

Same thing with CPartialMerkleTreeTester.

maflcko commented at 10:11 am on August 21, 2024:

Data-members should be private by default:

Not sure in tests, because if an object of TxOrphanageTest exists, it can be possibly useful to have access to the objects.m_rng for mocking. Whereas there are very little downsides (apart from being possibly confusing to use the objects.m_rng instead of the “global” m_rng).

I’ll leave this as-is for now, but I may change it later, because either way is fine and I don’t think it matters much.

hodlinator commented at 10:44 am on August 21, 2024:

Defaulting to private means that should something outside the class start needing access to it, that PR would need to expose it, thereby highlighting to reviewers what is actually happening.

I think it’s a good principle even if this is test code, it is new in this PR after all. Maybe if you re-touch?

maflcko commented at 11:28 am on August 21, 2024:

highlighting to reviewers what is actually happening.

Personally I think this is already happening, because the code would be orphanage.m_rng vs m_rng, clarifying that TxOrphanageTest::m_rng is used, as opposed to BasicTestingSetup::m_rng.

I’ll leave this as-is for now, but I may change it later, because either way is fine and I don’t think it matters much.

in src/test/util/random.h:12 in 3836bf1e0a outdated

10@@ -11,50 +11,18 @@
11 
12 #include <cstdint>

hodlinator commented at 9:54 am on August 21, 2024:

nit: No longer needed:

 0--- a/src/test/util/random.h
 1+++ b/src/test/util/random.h
 2@@ -7,9 +7,6 @@
 3 
 4 #include <consensus/amount.h>
 5 #include <random.h>
 6-#include <uint256.h>
 7-
 8-#include <cstdint>
 9 
10 enum class SeedRand {
11     ZEROS, //!< Seed with a compile time constant of zeros

DrahtBot requested review from hodlinator on Aug 21, 2024

maflcko commented at 10:15 am on August 21, 2024: member

Thanks for checking the includes in the tests @hodlinator! I think I’ll leave this as-is for now, because having leftover unused includes in tests should be harmless, apart from possibly being confusing, or needing more compile time. I have a follow-up to use iwyu (and enforce it), so that reviewers can use the re-review cycles to clean up includes for reviewing the code itself. However, this will be a follow-up, so I’ll leave this pull request as-is for now.

maflcko force-pushed on Aug 21, 2024

maflcko commented at 3:32 pm on August 21, 2024: member

Force pushed to add a bugfix commit to fix an incorrect test log message (bug was introduced years ago), and to remove unused symbols after a finding by @hodlinator in #30571 (review) (Thanks!).

Should be easy to re-review by looking at the new commit and then checking the other commits via:

0git range-diff bitcoin-core/master 3836bf1e0a 96cde0826e --word-diff-regex=.

(or similar)

in src/test/prevector_tests.cpp:205 in faa2608e6c outdated

201@@ -205,13 +202,11 @@ class prevector_tester {
202     }
203 
204     ~prevector_tester() {
205-        BOOST_CHECK_MESSAGE(passed, "insecure_rand: " + rand_seed.ToString());
206+        BOOST_CHECK_MESSAGE(passed, "The random seed was logged at the start of the test");

ryanofsky commented at 5:04 pm on August 21, 2024:

In commit “test: Correct the random seed log on a prevector test failure” (faa2608e6c3e1763d0991443c3ce6d9bb1466da8)

I think it would be nice to just fix the bug and print the seed when the test fails rather than requiring looking up the seed in logs. It seems like we could easily support this by adding return seed as the last line of SeedRandomForTest and this would be helpful for debugging tests that only fail with certain seeds, because they could be run without always enabling logging.

maflcko commented at 7:39 pm on August 21, 2024:

Not sure about this. To catch the seed, logging should be enabled (and it is enabled in all CI tasks). (This isn’t changed in this pull request, so the comment seems tangential)

If the burden is put on each test that uses any kind of randomness to capture and memorize the seed, and print it on failure, it will be spotty at best and confusing, verbose and inconsistent in general.

If you are worried that the seed could be missed, I share your concern, but a better or more general fix would be to add a simple patch to the cmake/ctest infra, once it is available. Such an approach would address your concern to capture the seed and print it on failure for all tests, as opposed to just one test (or a few).

However, no behavior is changed in this pull and the goal of this pull request is to be as close to a refactor as possible, so this will have to be done in a follow-up.

maflcko commented at 7:50 pm on August 21, 2024:

Personally, I like how it is done in the python functional tests:

Allow to log it to the terminal (This can be used by developers)
Always record the seed in the log in the test temp dir, which is preserved on failure, so that it can be looked up afterwards. (This is used by CI and can be used by developers as well)

Doing the same in the unit tests would be nice, but again, this seems best to be tracked, discussed and fixed in a separate issue or pull request.

ryanofsky commented at 8:14 pm on August 21, 2024:

re: #30571 (review)

That all sounds good but I think I’m proposing a smaller not bigger change in behavior by just fixing the bug instead of trying to do something grander. The change based on master is:

 0--- a/src/test/prevector_tests.cpp
 1+++ b/src/test/prevector_tests.cpp
 2@@ -209,9 +209,8 @@ public:
 3     }
 4 
 5     prevector_tester() {
 6-        SeedRandomForTest();
 7-        rand_seed = InsecureRand256();
 8-        rand_cache.Reseed(rand_seed);
 9+        rand_seed = SeedRandomForTest();
10+        rand_cache.Reseed(InsecureRand256());
11     }
12 };
13 
14--- a/src/test/util/random.cpp
15+++ b/src/test/util/random.cpp
16@@ -15,7 +15,7 @@ FastRandomContext g_insecure_rand_ctx;
17 
18 extern void MakeRandDeterministicDANGEROUS(const uint256& seed) noexcept;
19 
20-void SeedRandomForTest(SeedRand seedtype)
21+uint256 SeedRandomForTest(SeedRand seedtype)
22 {
23     static const std::string RANDOM_CTX_SEED{"RANDOM_CTX_SEED"};
24 
25@@ -35,4 +35,5 @@ void SeedRandomForTest(SeedRand seedtype)
26     LogPrintf("%s: Setting random seed for current tests to %s=%s\n", __func__, RANDOM_CTX_SEED, seed.GetHex());
27     MakeRandDeterministicDANGEROUS(seed);
28     g_insecure_rand_ctx.Reseed(GetRandHash());
29+    return seed;
30 }
31--- a/src/test/util/random.h
32+++ b/src/test/util/random.h
33@@ -25,7 +25,7 @@ enum class SeedRand {
34 };
35 
36 /** Seed the RNG for testing. This affects all randomness, except GetStrongRandBytes(). */
37-void SeedRandomForTest(SeedRand seed = SeedRand::SEED);
38+uint256 SeedRandomForTest(SeedRand seed = SeedRand::SEED);
39 
40 static inline uint32_t InsecureRand32()
41 {

This would change the test less than the current PR, provide nicer behavior, and help out with future changes that make the seed more accessible, by returning the value from the function which set it.

maflcko commented at 9:09 pm on August 21, 2024:

Yeah, your diff looks good and I understood it the first time. However I think SeedRandomForTest should remain void for now, because it wasn’t intended to put the burden of logging or storing the seed onto tests. It seems harmless at first glance, but it doesn’t help the dev much, because they’d still have to look up how to set the seed to reproduce the failure. Simply relying on the log (which states the name of the env var, as well as the value of the env var) should be enough.

There is yet another bug with the randomness in this test: Starting with commit fae43a97ca947cd0802392e9bb86d9d0572c0fba, the seed is static const per process, so a repeated call to SeedInsecureRand/SeedRandomForTest doesn’t actually generate a new seed, so running the same code in a for loop will yield the same result.

This should probably be fixed as well. I’ll try to do that tomorrow.

maflcko commented at 9:33 pm on August 21, 2024:

There is yet another bug

Fixed

ryanofsky commented at 2:03 pm on August 22, 2024:

re: #30571 (review)

Starting with commit https://github.com/bitcoin/bitcoin/commit/fae43a97ca947cd0802392e9bb86d9d0572c0fba, the seed is static const per process

Wow, I didn’t notice ctx_seed was statically caching a random value before this so I’ve been misunderstanding what the SeedRandomForTest function has been doing the whole time. I think this could be made clearer by:

Renaming SeedRand::SEED to SeedRand::FIXED_SEED so it is clearer SeedRandomForTest sets a fixed value, not one that can change between calls.
Dropping the SeedRandomForTest default argument value so if a test resets to the fixed seed the calls look like SeedRandomForTest(SeedRand::FIXED_SEED) which tells you a fixed value is being set, as opposed to SeedRandomForTest() which makes it look like it could be setting a new random seed.
Adding comments that better document what the seed functions and enum values do.

Would suggest:

 0--- a/src/test/util/random.h
 1+++ b/src/test/util/random.h
 2@@ -13,10 +13,13 @@
 3 
 4 enum class SeedRand {
 5     ZEROS, //!< Seed with a compile time constant of zeros
 6-    SEED,  //!< Use (and report) random seed from environment, or a (truly) random one.
 7+    FIXED_SEED, //!< Seed with a fixed value that never changes over the
 8+                //!< lifetime of this process. The seed is read from the
 9+                //!< RANDOM_CTX_SEED environment variable if set, otherwise
10+                //!< generated randomly once, saved, and reused.
11 };
12 
13-/** Seed the internal global RNG state for testing. This affects all randomness, except GetStrongRandBytes(). */
14+/** Seed the global RNG state for testing and log the seed value. This affects all randomness, except GetStrongRandBytes(). */
15 void SeedRandomStateForTest(SeedRand seed);
16 
17 template <RandomNumberGenerator Rng>
18--- a/src/test/util/setup_common.h
19+++ b/src/test/util/setup_common.h
20@@ -67,8 +67,8 @@ struct BasicTestingSetup {
21     node::NodeContext m_node; // keep as first member to be destructed last
22 
23     FastRandomContext m_rng;
24-    /** Seed the internal global RNG state and m_rng for testing. This affects all randomness, except GetStrongRandBytes(). */
25-    void SeedRandomForTest(SeedRand seed = SeedRand::SEED)
26+    /** Seed the global RNG state and m_rng for testing and log the seed value. This affects all randomness, except GetStrongRandBytes(). */
27+    void SeedRandomForTest(SeedRand seed)
28     {
29         SeedRandomStateForTest(seed);
30         m_rng.Reseed(GetRandHash());

maflcko commented at 3:23 pm on August 22, 2024:

Makes sense! I’ll do the documentation cleanup in a follow-up, if that is fine, because this refactoring pull request doesn’t change the behavior around this function, and I am trying to keep the scope limited.

hodlinator commented at 9:19 pm on August 23, 2024:

(Thread #30571 (review))

One could argue ZEROS is also “fixed”, but I still agree FIXED_SEED with the documentation would be a step a good direction.

maflcko commented at 9:28 am on August 26, 2024:

Ok, I’ve decided to pull in the doc update to the line of codes that I am modifying, to avoid having to touch them again. But the other refactoring and documentation will be done in the follow-up, as it needs a new commit anyway and I have a few follow-up queued up anyway.

maflcko commented at 8:01 am on August 29, 2024:

Fixed FIXED_SEED in https://github.com/bitcoin/bitcoin/pull/30748

ryanofsky approved

ryanofsky commented at 5:06 pm on August 21, 2024: contributor

Code review ACK b6b2b38aa77162420babb34e01dd510512411abf. Just simplified prevector_tests and removed misleading BOOST_CHECK_MESSAGE print since last review.

DrahtBot requested review from marcofleon on Aug 21, 2024

test: Correct the random seed log on a prevector test failure

rand_cache is unused since commit
16329224e70d0525208f6b0ba00c5e1531a4f5ea, so it can be removed

rand_seed is wrong since commit
022cf47dd7ef8f46e32a184e84f94d1e9f3a495c, because it is no longer
printing the seed that was used to seed the global random context in
tests. Instead, it prints a (random-ish) value derived from the global
random context via InsecureRand256().

Finally, the for loop creating new prevector_tester objects will always
use the same seed since commit fae43a97ca947cd0802392e9bb86d9d0572c0fba,
because repeated calls to SeedInsecureRand/SeedRandomForTest will always
reseed the global with the same "static const" seed.

Fix all issues by
* removing the unused rand_cache,
* removing the call to SeedRandomForTest which restored the same seed on
  every call in the process, and
* Reseeding the global random context with the (random-ish) rand_seed.

fae7e3791c

test: Add m_rng alias for the global random context

The two names point to the same object, but having the reference now
allows easier removal of the global in the future.

fa2cb654ec

test: refactor: Make unsigned promotion explicit

Integer promotion will already turn the `signed` into `unsigned` in
those lines. However, make the `unsigned` explicit so that the code is
clearer and a compiler warning is avoided when switching to m_rng:

| test/validation_block_tests.cpp: warning: comparison of integers of different signs: 'int' and 'const unsigned int' [-Wsign-compare]
|   136 |     bool gen_invalid = m_rng.randrange(100) < invalid_rate;
|       |                        ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
|   137 |     bool gen_fork = m_rng.randrange(100) < branch_rate;
|       |                     ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~
| 2 warnings generated.

fab023e177

test: refactor: Give unit test functions access to test state

Add unit test subclasses as needed so unit test functions that need to access
members like m_rng can reference it directly.

3dc527f460

maflcko force-pushed on Aug 21, 2024

DrahtBot commented at 9:33 pm on August 21, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29081320281

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Aug 21, 2024

maflcko force-pushed on Aug 21, 2024

DrahtBot removed the label CI failed on Aug 21, 2024

maflcko commented at 6:16 am on August 22, 2024: member

Force pushed to fix another (years old) bug in the first commit

in src/test/prevector_tests.cpp:211 in 38b03bc3af outdated

206@@ -208,84 +207,83 @@ class prevector_tester {
207         BOOST_CHECK_MESSAGE(passed, "insecure_rand: " + rand_seed.ToString());
208     }
209 
210-    prevector_tester() {
211-        SeedRandomForTest();
212-        rand_seed = InsecureRand256();
213-        rand_cache.Reseed(rand_seed);
214+    prevector_tester(FastRandomContext& rng) {
215+        rand_seed = rng.rand256();

hodlinator commented at 6:59 am on August 22, 2024:

Could make rand_seed const to clarify usage (approved by clang-format-diff.py):

 0--- a/src/test/prevector_tests.cpp
 1+++ b/src/test/prevector_tests.cpp
 2@@ -27,7 +27,7 @@ class prevector_tester {
 3 
 4     typedef typename pretype::size_type Size;
 5     bool passed = true;
 6-    uint256 rand_seed;
 7+    const uint256 rand_seed;
 8 
 9 
10     template <typename A, typename B>
11@@ -207,8 +207,8 @@ public:
12         BOOST_CHECK_MESSAGE(passed, "insecure_rand: " + rand_seed.ToString());
13     }
14 
15-    prevector_tester(FastRandomContext& rng) {
16-        rand_seed = rng.rand256();
17+    prevector_tester(FastRandomContext& rng) : rand_seed(rng.rand256())
18+    {
19         rng.Reseed(rand_seed);
20     }
21 };

maflcko commented at 7:16 am on August 22, 2024:

Could make rand_seed const to clarify usage (approved by clang-format-diff.py):

Makes sense. However, I am not modifying this line of code, so I’ll leave the style-nit as-is for now, to keep the already large pull request focussed on only the minimum diff required to fix the found bugs and apply the refactoring to achieve the stated goals in the pull motivation.

hodlinator commented at 7:06 am on August 22, 2024: contributor

Reviewed 38b03bc3af9fbf483b5a29a54a5ffb36c67b6b41 (still aiming to do some more testing).

Thanks for checking the includes in the tests @hodlinator! I think I’ll leave this as-is for now, because having leftover unused includes in tests should be harmless, apart from possibly being confusing, or needing more compile time. I have a follow-up to use iwyu (and enforce it), so that reviewers can use the re-review cycles to clean up includes for reviewing the code itself. However, this will be a follow-up, so I’ll leave this pull request as-is for now.

Agree it’s low-value cycles to check #includes/forward declarations, but until we start using iwyu everywhere, it felt fitting as you are doing major surgery in & around test/util/random.h in this case.

Great that my discovery helped spawn your additional fixes to prevector_tests.cpp!

ryanofsky approved

ryanofsky commented at 2:09 pm on August 22, 2024: contributor

Code review ACK 38b03bc3af9fbf483b5a29a54a5ffb36c67b6b41. Just another random seed bug in the prevector tests fixed since last review!

DrahtBot requested review from hodlinator on Aug 22, 2024

test: refactor: Move g_insecure_rand_ctx.Reseed out of the helper that calls MakeRandDeterministicDANGEROUS

The global g_insecure_rand_ctx will be removed in the future, so
removing it from this helper is useful.

Also, tying the two concepts of the global internal RNGState and the
global test-only rng context is a bit confusing, because tests can
simply use the m_rng, if it exists. Also, tests may seed more than one
random context, or none at all, or a random context of a different type.

Fix all issues by moving the Reseed call to the two places where it is
used.

fa19af555d

test: refactor: Pass rng parameters to test functions

Add FastRandomContext parameter to the utility function
AddTestCoin(), and a few local test functions and classes.

68f77dd21e

test: refactor: Accept any RandomNumberGenerator in RandMoney

Accepting any Rng in RandMoney makes tests more flexible to use a
different Rng. Also, passing in the Rng clarifies the call sites, so
that they all use g_rand_ctx explicitly and consistently.

fa54cab473

scripted-diff: [test] Use g_rng/m_rng directly

-BEGIN VERIFY SCRIPT-

 # Use m_rng in unit test files
 ren() { sed -i "s:\<$1\>:$2:g" $( git grep -l "$1" src/test/*.cpp src/wallet/test/*.cpp src/test/util/setup_common.cpp ) ; }
 ren InsecureRand32                m_rng.rand32
 ren InsecureRand256               m_rng.rand256
 ren InsecureRandBits              m_rng.randbits
 ren InsecureRandRange             m_rng.randrange
 ren InsecureRandBool              m_rng.randbool
 ren g_insecure_rand_ctx           m_rng
 ren g_insecure_rand_ctx_temp_path g_rng_temp_path

-END VERIFY SCRIPT-

fa0fe08eca

test: Remove FastRandomContext global

Drop g_insecure_rand_ctx

948238a683

maflcko force-pushed on Aug 26, 2024

maflcko commented at 9:28 am on August 26, 2024: member

Pushed trivial doc-only update. Should be easy to re-review.

hodlinator approved

hodlinator commented at 12:08 pm on August 26, 2024: contributor

ACK 948238a683b6c99f4e91114aa75680c6c2d73714

Gets rid of g_insecure_rand_ctx-global state along with global functions using it, in favor of local m_rng contexts.

Tested adding..

0BOOST_CHECK_EQUAL(m_rng.rand64(), 0);

..to one of the tests and verified that the random number emitted varied when re-running the test, and became constant when called repeatedly with RANDOM_CTX_SEED env var set, and different constant when modifying RANDOM_CTX_SEED value.

(Also uncovered #30696 while testing).

fae7e3791c9ed8053166773fcfb583ad19d006dd “test: Correct the random seed log on a prevector test failure”

Good addition to the PR!

nit: The tense in the commit message feels off, “rand_cache is unused since” -> “rand_cache was unused since” “rand_seed is wrong” -> “rand_seed was wrong” etc

DrahtBot requested review from ryanofsky on Aug 26, 2024

ryanofsky approved

ryanofsky commented at 3:53 pm on August 26, 2024: contributor

Code review ACK 948238a683b6c99f4e91114aa75680c6c2d73714. Only changes since last review were changing a comments a little bit.

maflcko commented at 3:02 pm on August 28, 2024: member

Is this test-only refactor (and small bugfix) with two acks rfm, or does it need more review?

marcofleon commented at 3:06 pm on August 28, 2024: contributor

In the process of reviewing now.

marcofleon approved

marcofleon commented at 3:21 pm on August 28, 2024: contributor

Code review ACK 948238a683b6c99f4e91114aa75680c6c2d73714. Only changes since my last review are the improvements in prevector_tests.

fanquake merged this on Aug 28, 2024

fanquake closed this on Aug 28, 2024

jonatack commented at 4:45 pm on August 28, 2024: member

Concept ACK. Nice to remove the global state and methods.

maflcko deleted the branch on Aug 29, 2024

test: [refactor] Use m_rng directly #30571

Code Coverage

Reviews

Conflicts

fae7e3791c9ed8053166773fcfb583ad19d006dd “test: Correct the random seed log on a prevector test failure”