test: create random and coins utils, add amount helper, dedupe add

jonatack commented at 0:07 am on January 21, 2023: contributor

Move random test utilities from setup_common to a new random file, as many tests don’t use this code.
Create a helper to generate semi-random CAmounts up to MONEY_RANGE rather than only uint32, and use the helper in the unit tests.
De-duplicate a shared add_coin method by extracting it to a coins test utility.

DrahtBot commented at 0:07 am on January 21, 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	john-moffett, pinheadmz, achow101

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:

#26817 (doc: Bump copyright years to present (headers only) by MarcoFalke)
#25325 (Add pool based memory resource by martinus)

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 Jan 21, 2023

jonatack force-pushed on Jan 21, 2023

jonatack marked this as ready for review on Jan 21, 2023

in src/test/util/setup_common.cpp:102 in 3cd46094d1 outdated

 97+{
 98+    switch (InsecureRandRange(100)) {
 99+    case 0: return 0;
100+    case 1: return 1;
101+    case 2: return MAX_MONEY;
102+    default: return InsecureRandRange(InsecureRandBool() ? COIN : MAX_MONEY);

john-moffett commented at 2:19 pm on January 22, 2023:

Can you explain the motivation for restricting the range to COIN half the time? Is it just because they’re more common values?

jonatack commented at 7:35 pm on January 22, 2023:

Thanks for looking! Yes, the idea was to often generate a real-life value, while still testing the full MoneyRange in src/consensus/amount.h.

jonatack commented at 7:43 pm on January 22, 2023:

Updated its Doxygen doc with that info.

john-moffett approved

john-moffett commented at 2:20 pm on January 22, 2023: contributor

utACK 3cd46094d159373bda7efbaaf8c28bb78397947f

I like the idea of having more intelligent “random” data that includes edge cases a significant portion of the time.

in src/test/util/setup_common.cpp:96 in 3cd46094d1 outdated

92@@ -91,6 +93,16 @@ void Seed(FastRandomContext& ctx)
93     ctx = FastRandomContext(seed);
94 }
95 
96+CAmount SemiRandMoneyAmount()

fanquake commented at 2:24 pm on January 22, 2023:

No thoughts on the change, but not sure this code should be in setup_common. Probably src/test/util/something ?

jonatack commented at 7:41 pm on January 22, 2023:

Done. Didn’t see a good existing home for it, so I moved the random generation test code from setup_common to src/test/util/random, which seems beneficial as many of the unit tests don’t call it, and then added the test helper there.

jonatack renamed this:
~~test: add and use a helper for generating semi-random CAmounts~~
test: Move rand utils from setup_common to random and add a helper
on Jan 22, 2023

jonatack force-pushed on Jan 22, 2023

jonatack commented at 7:48 pm on January 22, 2023: contributor

Updated with feedback from @john-moffett and @fanquake (thanks!)

jonatack force-pushed on Jan 22, 2023

in src/Makefile.test_util.include:1 in 4c3d011a04 outdated

0@@ -1,4 +1,4 @@
1-# Copyright (c) 2013-2019 The Bitcoin Core developers
2+# Copyright (c) 2013-2023 The Bitcoin Core developers

fanquake commented at 11:06 am on January 23, 2023:

There’s no need to manually bump dates, and they’ll likely just be removed (#26817), so you can drop any changes like this.

jonatack commented at 5:28 pm on January 23, 2023:

Done.

jonatack force-pushed on Jan 23, 2023

in src/test/util/random.h:40 in 866917c2f7 outdated

39+
40+/**
41+ * Generate a semi-random CAmount up to COIN (a typical value) or MAX_MONEY (the
42+ * full money range), with occasional edge case values of 0, 1, and MAX_MONEY.
43+ */
44+static inline CAmount SemiRandMoneyAmount()

maflcko commented at 10:38 am on January 31, 2023:

Not sure if we should imitate a fuzz engine in the unit tests to pick special values. A fuzz engine is generally a lot better at finding those.

So I’d say, if you see value in testing more edge cases, I’d recommend to write a fuzz test, not a unit test with a hand written “edge-case finder”.

jonatack commented at 5:35 pm on February 1, 2023:

I thought about that, but unlike probing for a crash as fuzzers do, our unit tests are using the rand generators to check returned values, i.e. non-crash behavior, from those inputs and this change follows and improves on their intention. (Sure, adding a fuzz test would be good and provide complementary coverage.)

We could simplify the helper to just InsecureRandRange(MAX_MONEY) if reviewers prefer, but I think the proposed version provides more robust and useful coverage.

maflcko commented at 8:38 am on February 2, 2023:

unlike probing for a crash as fuzzers do, our unit tests are using the rand generators to check returned values

Conceptually unit tests and fuzz tests are no different, if you take away the fuzz engine. A fuzz test doing a check is no different than a unit test doing a check. There may be an implementation detail in the exit code (and other stuff) if one calls assert vs BOOST_REQUIRE or BOOST_CHECK, but in both cases the test exe will end unsuccessfully.

I think the proposed version provides more robust and useful coverage.

Would be good to provide an example where this leads to increased line or branch coverage

jonatack commented at 5:21 pm on February 6, 2023:

We could simplify the helper to just InsecureRandRange(MAX_MONEY) if reviewers prefer

Happy to update with a concrete suggestion within the scope here of improving the existing unit tests.

fanquake commented at 10:38 am on February 7, 2023:

Would be good to provide an example where this leads to increased line or branch coverage

I think this is the key point. If we’re going to write special randomness/edge case functions, we should be able to show they are improving test coverage. Otherwise have a fuzzer pick random values.

jonatack commented at 4:54 pm on February 7, 2023:

Removed the edge cases. It uses the money range rather than only uint32, while remaining in the same vein as the existing unit test rand helpers.

maflcko commented at 10:38 am on January 31, 2023: member

No objection, but not seeing the benefit over a fuzz test either?

DrahtBot added the label Needs rebase on Feb 1, 2023

jonatack renamed this:
~~test: Move rand utils from setup_common to random and add a helper~~
test: create random and coins utils, add amount helper, dedupe add_coin
on Feb 1, 2023

jonatack force-pushed on Feb 1, 2023

DrahtBot removed the label Needs rebase on Feb 1, 2023

jonatack commented at 6:15 pm on February 1, 2023: contributor

Rebased, squashed the money helper changes to one commit, and de-duplicated a shared add_coin method by extracting it to a test utility.

jonatack force-pushed on Feb 1, 2023

in src/test/util/blockfilter.cpp:31 in d34bc34c8c outdated

27@@ -28,4 +28,3 @@ bool ComputeFilter(BlockFilterType filter_type, const CBlockIndex* block_index,
28     filter = BlockFilter(filter_type, block, block_undo);
29     return true;
30 }
31-

pinheadmz commented at 4:16 pm on February 6, 2023:

Why remove the trailing newline?

jonatack commented at 5:10 pm on February 6, 2023:

We don’t normally have extra trailing newlines in files.

pinheadmz commented at 5:07 pm on February 6, 2023: member

concept ACK tested and reviewed, I like the deliberate introduction of edge cases into the tests and certainly makes sense to refactor out AddTestCoin() since it is duplicated. I’m not entirely sure about the move of all the random functions is a net benefit, you had to include that in 33 files when those functions were already sitting nicely in setup_common

pinheadmz commented at 5:19 pm on February 6, 2023: member

ACK d34bc34c8c69781c913faa3e572af8030d099551

The location of the random functions isn’t a blocker for me.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK d34bc34c8c69781c913faa3e572af8030d099551
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmPhNmYACgkQ5+KYS2KJ
 7yTqGYA//cUrRxVOTho65ZbiTMKbev32/NMPI4aTt+j4Jl3UtSt96kqlBgSYdGftC
 81V4B6pj2efxjFEuGQd8n1pzUt5pm8bkdg1r6JfVjEFNaz/ZqzagJS/93RU4/+3Ui
 9ffYt1jLI55ws2iy/owStLNmWqyRmh2YazTRPb+h1YAQ8FsuyIlpz75HJnKORsoA4
10gcZwJ84YxSPaYYyCLpQ/++0PcofcR7q2CFidnZ8Dlcc3LwNSbdEk7zOwSkdAbT51
11KIjIWH8Zh0LAgMbWCyXnmf1er3IdnCW/DSczQRqqbr+Mpn+0iQvyKsfqS+wJMOFj
12Yjjg7aZ8NBx4nCw9mK+Ey3/XO1oAqcNVjRJinTKW7I6WhxShBY2vr9sF9ttX4nSS
137mEQf6iYRkCPCkRDrgTCtdNMzXtVQQRwZm+vN9w5Rs7AHz7SBJFDCpxC2hnMa1ij
14FkowcK/FW1/LYYTceJmvwwbP5UuM5JN6z8j4DE6m/wztkiAOM0JSBe6IushlPplT
15xeByjBkj3HEUoRStd4bZhZindIDplqHnzBEbqMuJFY8rLXrN2Vr6pP4esuUHlmh4
16Nnm7In2py8RABm5sjeMugb5ElZR+zyDwnUElfqhwQf7iVKTqBfV+Jw05dH0Xmh9l
17dM/qOScbDnDlg1IaIkHvFTkJSTirdb5pwalG5dg1i+rjsYeDoxw=
18=2W5E
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

DrahtBot added the label Needs rebase on Feb 6, 2023

Move random test util code from setup_common to random

as many of the unit tests don't use this code

81f5ade2a3

jonatack force-pushed on Feb 6, 2023

jonatack commented at 9:21 pm on February 6, 2023: contributor

Trivial rebase per git range-diff 52ddbd5 d34bc34 e6de5ad for the merge of #26345. Thank you for the ACKs, @john-moffett and @pinheadmz, mind re-ACKing?

I don’t have a strong opinion on extracting the random utils to a helper. As most of the test files don’t need them to be a part of their compilation unit, it might be beneficial to speed up builds a bit, and it provides clearer code organization. OTOH it’s more to review so am happy also to leave them in setup_common or extract only the new helper.

DrahtBot removed the label Needs rebase on Feb 6, 2023

pinheadmz commented at 3:53 pm on February 7, 2023: member

ACK e6de5ad8dce4fea053f75de5ee0189ed5f91df41

verified rebase change is minimal, also double checked that all add_coin() calls were updated in the unit tests

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK e6de5ad8dce4fea053f75de5ee0189ed5f91df41
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmPic+UACgkQ5+KYS2KJ
 7yToqeA//Uf61CfE9JSLcqfWT2IOu2J7AsFKY09zsXG+TpQQz1NiVOUvKPkQC0RYA
 8MdPnvrXTodlsYxf1c1K9xbAkmeQ356ptlcl/sDi5w5y2eQ82Ul5kFIrfEpPMB7M8
 9qfkcIdRsz3Ctv9ps0wbfeXSeW0rw2LbokQ8qhChtyzJAeUbyTDkjvjHq9vEMNs8G
10QiGRFDEZzwxj+VkmsKOhYfJvOKUXXZJDGTzbUaATuLdXkbB17pE06n4nOAGUqbJa
11rFbSC8EZVdwBp4Ryh3jtF7+FXlDizjh1Wc+Mushx1/LvBA2vRkz4U39e97VIRTEW
12wHPvLkSyCtUX6HxuOCC2hGQY0mMfrxrtMTgUUQvTwIdu8S7JEHeEmQ2GhAGJ1T67
13Bsnih3FMpHN7VI6ugek1Egdu08CXO5ybU6hyJNgaRTsIC29XlEmYVBGqj5XuyHsO
14TYLStNA9A2cgHr/HfTrR/FB7RwaFqA47uPBvOapwmdV0dK7a1tCfJHrQxTO+SNkF
15Gdg/gBE+yYqjEgA9uqa9t6rHPrQnFtdjG3Bp0AVFJ8WSh3l8P/ZJjmtGvYEFd7xt
16LP6EEGPnt/vfmXnmViqDSmASRqnIa7sDdWcI92CG9Ds3+77SeHpFzRfPBH0zN3bi
17X/zMr8XjCxbw8hswNUxSve4yc0KW5Nu2jTiv9zfJTSfoLYdDhC8=
18=3Nhu
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

maflcko commented at 4:19 pm on February 7, 2023: member

I still don’t understand why this change is being made. #26940 (review)

It claims to add coverage, without providing any evidence or even intuition.

I am thinking that, absent that, it should be assumed that it will remove coverage. For example, sanitizers, such as the UB signed integer overflow sanitizer require large enough values to detect an overflow. If a majority of the values are now 0 and 1, where before they weren’t, it’ll have a harder time to trigger an overflow and sanitize the code.

jonatack force-pushed on Feb 7, 2023

jonatack commented at 5:02 pm on February 7, 2023: contributor

If a majority of the values are now 0 and 1

Only 1% was 0 and 1% was 1. I liked that it would check edge cases often enough to provide feedback when running make check that our current tests may miss, especially for developers running the unit tests locally to verify they are green, say, before pushing an update.

Anyway, that’s gone now; removed the edge cases in the last push. It uses the money range rather than only uint32, while remaining in the same vein as the existing unit test rand helpers, and updated the PR description.

Create InsecureRandMoneyAmount() test util helper

to generate semi-random CAmounts up to MAX_MONEY rather
than only uint32, and use it in the unit tests.

9d92c3d7f4

De-duplicate add_coin methods to a test util helper 4275195606

in src/test/util/random.h:40 in 25ea24d0a2 outdated

35+static inline bool InsecureRandBool()
36+{
37+    return g_insecure_rand_ctx.randbool();
38+}
39+
40+static inline CAmount SemiRandMoneyAmount()

john-moffett commented at 8:36 pm on February 7, 2023:

If this is now just returning an amount in the uniform [0 .. MAX_MONEY) range, maybe rename to InsecureRandMoneyAmount()? Also, bit of a nit, but maybe have it return InsecureRandRange(MAX_MONEY + CAmount(1)), since MAX_MONEY is a valid CAmount?

jonatack commented at 11:09 pm on February 9, 2023:

Done.

jonatack force-pushed on Feb 9, 2023

john-moffett approved

john-moffett commented at 0:44 am on February 10, 2023: contributor

ACK 4275195606e6f42466d9a8ef766b3035833df4d5

DrahtBot requested review from pinheadmz on Feb 10, 2023

pinheadmz commented at 3:19 pm on February 10, 2023: member

ACK 4275195606e6f42466d9a8ef766b3035833df4d5

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 4275195606e6f42466d9a8ef766b3035833df4d5
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmPmX4QACgkQ5+KYS2KJ
 7yTp9Qg/8DS+6sPz95i2whVHQhnphYYAbbF68udZnle8PqeKof7lWGHOnCF3our+/
 8m2oOGfwXdviYMtIJzB+qBpwRHWonJStIl90Tz6yWqwnM2H5djWhpRBKUsjXMSPsI
 9vpFJCL2eQNSohNcBJ3aG2rZ3SIrMRZaFBiC1cKKan5XYbZBistXFg5H7odmTTwa/
10j70qkh3dK+NINELUC5Q+exVSPK9LSwH8tmfIKjd0mb0RL7ABXISEVEQSjkdIgOG9
11tNFc9o3Nlcseyha0V7bzkDEL0cSPoxInbxlUHv/o8+6+XnPL3cn2hGrFy424OMg6
12IvZeiQacQbaHzDjDNI9bcK4nyunLqPz1G3r53ibs2CU507Mfv/TNIoToFyJzg/HF
13kgb9qRhg/KwqaZ/2bT4OatAOja+t8lnFMvVL0NB0Uhmudv1kbcGERqIQWc8/0dru
1417vdbiu3YqAstDMhtndvy/hupEuBEqEc97xNmXu9xmB8TP9FKDpRS2qWWpvbQNna
157gh49R+eBu/0kxeMhIrc2oXL1S3oaVH1kNqYwGT6cM7Qvt1nVJgGohm2JCE9PuPf
16Md+VcMm/EhQwUjl7Ja58Sv3UdkWMmQKFj/DhVowJHht4tfDPtl6hR6iBitnB3/69
17aoh/KBaiey/cSOXVv7zX5TQv21452+fdDuZPjFeGPct+vHxf/II=
18=r6pu
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

DrahtBot removed review request from pinheadmz on Feb 10, 2023

achow101 commented at 10:19 pm on February 17, 2023: member

ACK 4275195606e6f42466d9a8ef766b3035833df4d5

achow101 merged this on Feb 17, 2023

achow101 closed this on Feb 17, 2023

jonatack deleted the branch on Feb 17, 2023

sidhujag referenced this in commit f40ed0e9e6 on Feb 25, 2023

glozow commented at 10:29 am on April 5, 2023: member

What was the rationale for 81f5ade2a324167c03c5ce765a26bd42ed652723? test/util/random.h depends on test/util/setup_common because it’s using g_insecure_rand_ctx, which means setup_common can’t use any of the random functions without creating a circular dependency. Was this intentional / am I missing something? Should we be using a different set of random functions within setup_common?

jonatack commented at 2:40 pm on April 5, 2023: contributor

@glozow Thanks for bringing it up! The rationale was in this thread: #26940 (review). I noticed this as well, and that src/test/util/chainstate.h has the same dependency, and made a patch to remove those. Let me see if I can dig it up.

jonatack commented at 8:58 pm on April 5, 2023: contributor

setup_common can’t use any of the random functions without creating a circular dependency

Addressed with #27425.

fanquake referenced this in commit 24d5cf9314 on Jul 19, 2023

bitcoin locked this on Apr 4, 2024

test: create random and coins utils, add amount helper, dedupe add_coin #26940

Reviews

Conflicts