test: [refactor] Use m_rng directly #30571
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Lgtm ACK fa29521. 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
There was a problem hiding this comment.
Code review ACK fa29521 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.
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
I looked into it. However, it seems more work. My findings:
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
changed the title
|
Dropped one commit, because a conflicting pull has already picked it up. |
|
Code review ACK facc85b. 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:
|
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
|
Force pushed to fix another (years old) bug in the first commit |
There was a problem hiding this comment.
Reviewed 38b03bc (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!
src/test/prevector_tests.cpp
Outdated
|
|
||
| ~prevector_tester() { | ||
| BOOST_CHECK_MESSAGE(passed, "insecure_rand: " + rand_seed.ToString()); | ||
| BOOST_CHECK_MESSAGE(passed, "The random seed was logged at the start of the test"); |
There was a problem hiding this comment.
re: #30571 (comment)
Starting with commit fae43a9, 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::SEEDtoSeedRand::FIXED_SEEDso it is clearerSeedRandomForTestsets a fixed value, not one that can change between calls. - Dropping the
SeedRandomForTestdefault argument value so if a test resets to the fixed seed the calls look likeSeedRandomForTest(SeedRand::FIXED_SEED)which tells you a fixed value is being set, as opposed toSeedRandomForTest()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:
--- a/src/test/util/random.h
+++ b/src/test/util/random.h
@@ -13,10 +13,13 @@
enum class SeedRand {
ZEROS, //!< Seed with a compile time constant of zeros
- SEED, //!< Use (and report) random seed from environment, or a (truly) random one.
+ FIXED_SEED, //!< Seed with a fixed value that never changes over the
+ //!< lifetime of this process. The seed is read from the
+ //!< RANDOM_CTX_SEED environment variable if set, otherwise
+ //!< generated randomly once, saved, and reused.
};
-/** Seed the internal global RNG state for testing. This affects all randomness, except GetStrongRandBytes(). */
+/** Seed the global RNG state for testing and log the seed value. This affects all randomness, except GetStrongRandBytes(). */
void SeedRandomStateForTest(SeedRand seed);
template <RandomNumberGenerator Rng>
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -67,8 +67,8 @@ struct BasicTestingSetup {
node::NodeContext m_node; // keep as first member to be destructed last
FastRandomContext m_rng;
- /** Seed the internal global RNG state and m_rng for testing. This affects all randomness, except GetStrongRandBytes(). */
- void SeedRandomForTest(SeedRand seed = SeedRand::SEED)
+ /** Seed the global RNG state and m_rng for testing and log the seed value. This affects all randomness, except GetStrongRandBytes(). */
+ void SeedRandomForTest(SeedRand seed)
{
SeedRandomStateForTest(seed);
m_rng.Reseed(GetRandHash());
…t 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.
Add FastRandomContext parameter to the utility function AddTestCoin(), and a few local test functions and classes.
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.
-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-
Drop g_insecure_rand_ctx
|
Pushed trivial doc-only update. Should be easy to re-review. |
There was a problem hiding this comment.
ACK 948238a
Gets rid of g_insecure_rand_ctx-global state along with global functions using it, in favor of local m_rng contexts.
Tested adding..
BOOST_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).
fae7e37 "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
src/test/prevector_tests.cpp
Outdated
|
|
||
| ~prevector_tester() { | ||
| BOOST_CHECK_MESSAGE(passed, "insecure_rand: " + rand_seed.ToString()); | ||
| BOOST_CHECK_MESSAGE(passed, "The random seed was logged at the start of the test"); |
There was a problem hiding this comment.
(Thread #30571 (comment))
One could argue ZEROS is also "fixed", but I still agree FIXED_SEED with the documentation would be a step a good direction.
|
Is this test-only refactor (and small bugfix) with two acks rfm, or does it need more review? |
|
In the process of reviewing now. |
There was a problem hiding this comment.
Code review ACK 948238a. Only changes since my last review are the improvements in prevector_tests.
|
Concept ACK. Nice to remove the global state and methods. |
8 participants
This is mostly a style-cleanup for the tests' random generation:
g_insecure_rand_ctxin 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 usingm_rngfor 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.