Add templated GetRandDuration<> #18781

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2004-randDur changing 3 files +21 −18
  1. MarcoFalke commented at 1:08 pm on April 27, 2020: member

    A naive implementation of this template is dangerous, because the call site might accidentally omit the template parameter:

     0template <typename D>
 1D GetRandDur(const D& duration_max)
 2{
 3    return D{GetRand(duration_max.count())};
 4}
 5
 6BOOST_AUTO_TEST_CASE(util_time_GetRandTime)
 7{
 8    std::chrono::seconds rand_hour = GetRandDur(std::chrono::hours{1});
 9    // Want seconds to be in range [0..1hour), but always get zero :((((
10    BOOST_CHECK_EQUAL(rand_hour.count(), 0);
11}

    Luckily std::common_type is already specialised in the standard lib for std::chrono::duration (https://en.cppreference.com/w/cpp/chrono/duration/common_type). And its effect seem to be that the call site must always specify the template argument explicitly.

    So instead of implementing the function for each duration type by hand, replace it with a templated version that is safe to use.

  2. MarcoFalke commented at 1:16 pm on April 27, 2020: member
    Conceptual suggestion by @amitiuttarwar #18038 (review) Approach suggestion by @promag #18751#pullrequestreview-400510676 Implementation by @sipa #18751 (comment)
  3. MarcoFalke commented at 1:16 pm on April 27, 2020: member

    Appears to be working for me:

    0error: no matching function for call to 'GetRandDur'
1    GetRandDur(std::chrono::hours{1});
2    ^~~~~~~~~~
3./random.h:74:3: note: candidate template ignored: couldn't infer template argument 'D'
4D GetRandDur(typename std::common_type<D>::type max) noexcept
5  ^
61 error generated.
  4. promag commented at 1:27 pm on April 27, 2020: member

    Code review ACK bbbb7486a5a949a71e8973260c28fb1f19e2d9b7.

    Did the same locally yesterday.

  5. in src/random.h:74 in bbbb7486a5 outdated 
    66@@ -67,8 +67,15 @@
67  * Thread-safe.
68  */
69 void GetRandBytes(unsigned char* buf, int num) noexcept;
70+/** Generate a random integer in the range [0..range). Precondition: range > 0 */
71 uint64_t GetRand(uint64_t nMax) noexcept;
72-std::chrono::microseconds GetRandMicros(std::chrono::microseconds duration_max) noexcept;
73+/** Generate a random duration in the range [0..max). Precondition: max.count() > 0 */
74+template <typename D>
75+D GetRandDur(typename std::common_type<D>::type max) noexcept
    jonatack commented at 1:36 pm on April 27, 2020:
    Would GetRandDuration or GetRandomDuration be too long? It would be clearer (and fwiw “Dur” means “hard” in French :smile:)
    MarcoFalke commented at 1:39 pm on April 27, 2020:
    Thanks. Fixed
    jonatack commented at 1:41 pm on April 27, 2020:
    Thanks :)
  6. MarcoFalke force-pushed on Apr 27, 2020
  7. MarcoFalke renamed this:
    Replace GetRandMicros with templated GetRandDur
    Add templated GetRandDur<>
    on Apr 27, 2020
  8. MarcoFalke force-pushed on Apr 27, 2020
  9. MarcoFalke renamed this:
    Add templated GetRandDur<>
    Add templated GetRandDuration<>
    on Apr 27, 2020
  10. MarcoFalke commented at 1:40 pm on April 27, 2020: member

    Force pushed to

    • Make diff smaller by 2 lines, but at the same time introduce a new GetRandMillis
    • Address feedback by @jonatack to clarify the function name
  11. jonatack commented at 3:08 pm on April 27, 2020: member
    ACK, could add unit tests to util_tests or random_tests? Only GetRand and GetRandInt seem to have coverage.
  12. MarcoFalke commented at 3:46 pm on April 27, 2020: member
    Done
  13. in src/random.h:74 in fa12f1a78b outdated 
    66@@ -67,8 +67,17 @@
67  * Thread-safe.
68  */
69 void GetRandBytes(unsigned char* buf, int num) noexcept;
70+/** Generate a random integer in the range [0..range). Precondition: range > 0 */
71 uint64_t GetRand(uint64_t nMax) noexcept;
72-std::chrono::microseconds GetRandMicros(std::chrono::microseconds duration_max) noexcept;
73+/** Generate a random duration in the range [0..max). Precondition: max.count() > 0 */
74+template <typename D>
75+D GetRandomDuration(typename std::common_type<D>::type max) noexcept
    sipa commented at 5:31 pm on April 27, 2020:
    Perhaps add a comment here to explain why the std::common_type construction is used here.
    sipa commented at 5:32 pm on April 27, 2020:
    The idea came from here, btw: https://stackoverflow.com/a/37737627/8342274
    MarcoFalke commented at 6:36 pm on April 27, 2020:
    Done
  14. DrahtBot commented at 5:49 pm on April 27, 2020: member

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

    Conflicts

    No conflicts as of last run.

  15. MarcoFalke force-pushed on Apr 27, 2020
  16. DrahtBot added the label Tests on Apr 27, 2020
  17. promag commented at 10:07 pm on April 27, 2020: member
    ACK faf2e857790182a5f192cd59c5a46c5310bf9cd0.
  18. DrahtBot added the label Needs rebase on Apr 29, 2020
  19. MarcoFalke force-pushed on Apr 29, 2020
  20. DrahtBot removed the label Needs rebase on Apr 29, 2020
  21. in src/random.h:70 in fa05a4caf4 outdated 
    66@@ -67,9 +67,21 @@
67  * Thread-safe.
68  */
69 void GetRandBytes(unsigned char* buf, int num) noexcept;
70+/** Generate a random integer in the range [0..range). Precondition: range > 0 */
    laanwj commented at 1:10 pm on April 30, 2020:
    When you’re changing these comments anyway, you might want to add that the random number comes from a uniform distribution (especially for durations this is not obvious).
    MarcoFalke commented at 1:20 pm on April 30, 2020:
    Thanks, fixed
  22. Add templated GetRandomDuration<> fa0e5b89cf
  23. test: Add test for GetRandMillis and GetRandMicros 0000ea3265
  24. MarcoFalke force-pushed on Apr 30, 2020
  25. laanwj commented at 1:27 pm on April 30, 2020: member
    Code review ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0
  26. promag commented at 2:42 pm on April 30, 2020: member
    Code review ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0.
  27. jonatack commented at 2:53 pm on April 30, 2020: member

    ACK 0000ea3 thanks for the improved documentation. Code review, built, ran src/test/test_bitcoin -t random_tests -l test_suite for the new unit tests, git diff fa05a4c 0000ea3 since previous review:

     0diff --git a/src/random.h b/src/random.h
 1index bb9b8c2a2a..0c6dc24983 100644
 2--- a/src/random.h
 3+++ b/src/random.h
 4@@ -67,9 +67,9 @@
 5  * Thread-safe.
 6  */
 7 void GetRandBytes(unsigned char* buf, int num) noexcept;
 8-/** Generate a random integer in the range [0..range). Precondition: range > 0 */
 9+/** Generate a uniform random integer in the range [0..range). Precondition: range > 0 */
10 uint64_t GetRand(uint64_t nMax) noexcept;
11-/** Generate a random duration in the range [0..max). Precondition: max.count() > 0 */
12+/** Generate a uniform random duration in the range [0..max). Precondition: max.count() > 0 */
  28. in src/random.cpp:18 in 0000ea3265 
    13@@ -14,16 +14,14 @@
14 #include <wincrypt.h>
15 #endif
16 #include <logging.h>  // for LogPrintf()
17+#include <randomenv.h>
18+#include <support/allocators/secure.h>
    jonatack commented at 2:59 pm on April 30, 2020:
    fa0e5b89cf7 should these be moved to random.h?
    MarcoFalke commented at 3:14 pm on April 30, 2020:
    Why?
    jonatack commented at 3:25 pm on April 30, 2020:
    Because that is where GenRandMicros and GetRandMillis have been moved?
    jonatack commented at 3:27 pm on April 30, 2020:
    (or perhaps those are unrelated cleanups and I’m confused)
    MarcoFalke commented at 3:28 pm on April 30, 2020:
    The secure allocator is only needed for g_rng, which is in the cpp file. The lines are simply moved up a few lines. They have been sorted because I touched this file with my editor for other reasons.
    jonatack commented at 3:30 pm on April 30, 2020:
    Gotcha, thanks
  29. MarcoFalke added the label Refactoring on Apr 30, 2020
  30. MarcoFalke removed the label Tests on Apr 30, 2020
  31. MarcoFalke closed this on Apr 30, 2020
  32. MarcoFalke reopened this on Apr 30, 2020
  33. MarcoFalke commented at 10:37 pm on April 30, 2020: member
    Open-Close to re-run ci. See #15847 (comment)
  34. MarcoFalke closed this on Apr 30, 2020
  35. MarcoFalke reopened this on Apr 30, 2020
  36. promag commented at 9:35 pm on May 5, 2020: member
    Merge?
  37. in src/random.h:71 in 0000ea3265 
    66@@ -67,9 +67,21 @@
67  * Thread-safe.
68  */
69 void GetRandBytes(unsigned char* buf, int num) noexcept;
70+/** Generate a uniform random integer in the range [0..range). Precondition: range > 0 */
71 uint64_t GetRand(uint64_t nMax) noexcept;
    hebasto commented at 7:27 am on May 14, 2020: 
    0/** Generate a uniform random integer in the range [0..nMax). Precondition: nMax > 0 */
1uint64_t GetRand(uint64_t nMax) noexcept;
    MarcoFalke commented at 9:56 am on May 14, 2020:
    Thanks. To not invalidate all the ACKs, I am going to leave this for now.
  38. hebasto changes_requested
  39. hebasto commented at 7:31 am on May 14, 2020: member
    Concept ACK.
  40. hebasto approved
  41. hebasto commented at 10:01 am on May 14, 2020: member
    ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0 with non-blocking nit.
  42. MarcoFalke merged this on May 15, 2020
  43. MarcoFalke closed this on May 15, 2020
  44. MarcoFalke deleted the branch on May 15, 2020
  45. sidhujag referenced this in commit 00bc4af3a1 on May 17, 2020
  46. Fabcien referenced this in commit 6731f60abe on Feb 1, 2021
  47. MarcoFalke locked this on Feb 15, 2022


MarcoFalke promag jonatack sipa DrahtBot laanwj hebasto

Labels
Refactoring

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: 2025-04-04 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me