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

pull jonatack wants to merge 3 commits into bitcoin:master from jonatack:unit-tests-CAmount-generation changing 39 files +139 −43
  1. 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.

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

  3. DrahtBot added the label Tests on Jan 21, 2023
  4. jonatack force-pushed on Jan 21, 2023
  5. jonatack marked this as ready for review on Jan 21, 2023
  6. 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.
  7. john-moffett approved
  8. 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.

  9. 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.
  10. 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
  11. jonatack force-pushed on Jan 22, 2023
  12. jonatack force-pushed on Jan 22, 2023
  13. jonatack force-pushed on Jan 22, 2023
  14. jonatack commented at 7:48 pm on January 22, 2023: contributor
    Updated with feedback from @john-moffett and @fanquake (thanks!)
  15. jonatack force-pushed on Jan 22, 2023
  16. jonatack force-pushed on Jan 22, 2023
  17. 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.
  18. jonatack force-pushed on Jan 23, 2023
  19. 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.
  20. maflcko commented at 10:38 am on January 31, 2023: member
    No objection, but not seeing the benefit over a fuzz test either?
  21. DrahtBot added the label Needs rebase on Feb 1, 2023
  22. 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
  23. jonatack force-pushed on Feb 1, 2023
  24. jonatack force-pushed on Feb 1, 2023
  25. DrahtBot removed the label Needs rebase on Feb 1, 2023
  26. 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.
  27. jonatack force-pushed on Feb 1, 2023
  28. 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.
  29. 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
  30. 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

  31. DrahtBot added the label Needs rebase on Feb 6, 2023
  32. Move random test util code from setup_common to random
    as many of the unit tests don't use this code
    81f5ade2a3
  33. jonatack force-pushed on Feb 6, 2023
  34. 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.

  35. DrahtBot removed the label Needs rebase on Feb 6, 2023
  36. 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

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

  38. jonatack force-pushed on Feb 7, 2023
  39. 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.

  40. 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
  41. De-duplicate add_coin methods to a test util helper 4275195606
  42. 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.
  43. jonatack force-pushed on Feb 9, 2023
  44. john-moffett approved
  45. john-moffett commented at 0:44 am on February 10, 2023: contributor
    ACK 4275195606e6f42466d9a8ef766b3035833df4d5
  46. DrahtBot requested review from pinheadmz on Feb 10, 2023
  47. 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

  48. DrahtBot removed review request from pinheadmz on Feb 10, 2023
  49. achow101 commented at 10:19 pm on February 17, 2023: member
    ACK 4275195606e6f42466d9a8ef766b3035833df4d5
  50. achow101 merged this on Feb 17, 2023
  51. achow101 closed this on Feb 17, 2023

  52. jonatack deleted the branch on Feb 17, 2023
  53. sidhujag referenced this in commit f40ed0e9e6 on Feb 25, 2023
  54. 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?
  55. 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.
  56. 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.

  57. fanquake referenced this in commit 24d5cf9314 on Jul 19, 2023
  58. bitcoin locked this on Apr 4, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me