Add templated GetRandDuration<> #18781

maflcko · 2020-04-27T13:08:25Z

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

template <typename D>
D GetRandDur(const D& duration_max)
{
    return D{GetRand(duration_max.count())};
}

BOOST_AUTO_TEST_CASE(util_time_GetRandTime)
{
    std::chrono::seconds rand_hour = GetRandDur(std::chrono::hours{1});
    // Want seconds to be in range [0..1hour), but always get zero :((((
    BOOST_CHECK_EQUAL(rand_hour.count(), 0);
}

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.

maflcko · 2020-04-27T13:16:14Z

Conceptual suggestion by @amitiuttarwar #18038 (comment)
Approach suggestion by @promag #18751 (review)
Implementation by @sipa #18751 (comment)

maflcko · 2020-04-27T13:16:55Z

Appears to be working for me:

error: no matching function for call to 'GetRandDur'
    GetRandDur(std::chrono::hours{1});
    ^~~~~~~~~~
./random.h:74:3: note: candidate template ignored: couldn't infer template argument 'D'
D GetRandDur(typename std::common_type<D>::type max) noexcept
  ^
1 error generated.

promag

Code review ACK bbbb748.

Did the same locally yesterday.

src/random.h

maflcko · 2020-04-27T13:40:17Z

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

jonatack · 2020-04-27T15:08:10Z

ACK, could add unit tests to util_tests or random_tests? Only GetRand and GetRandInt seem to have coverage.

maflcko · 2020-04-27T15:46:13Z

Done

sipa · 2020-04-27T17:31:54Z

src/random.h

-std::chrono::microseconds GetRandMicros(std::chrono::microseconds duration_max) noexcept;
+/** Generate a random duration in the range [0..max). Precondition: max.count() > 0 */
+template <typename D>
+D GetRandomDuration(typename std::common_type<D>::type max) noexcept


Perhaps add a comment here to explain why the std::common_type construction is used here.

The idea came from here, btw: https://stackoverflow.com/a/37737627/8342274

DrahtBot · 2020-04-27T17:49:00Z

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

Conflicts

No conflicts as of last run.

promag · 2020-04-27T22:07:43Z

ACK faf2e85.

laanwj · 2020-04-30T13:10:23Z

src/random.h

@@ -67,9 +67,21 @@
 * Thread-safe.
 */
 void GetRandBytes(unsigned char* buf, int num) noexcept;
+/** Generate a random integer in the range [0..range). Precondition: range > 0 */


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

Thanks, fixed

laanwj · 2020-04-30T13:27:42Z

Code review ACK 0000ea3

promag · 2020-04-30T14:42:22Z

Code review ACK 0000ea3.

jonatack · 2020-04-30T14:53:42Z

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:

diff --git a/src/random.h b/src/random.h
index bb9b8c2a2a..0c6dc24983 100644
--- a/src/random.h
+++ b/src/random.h
@@ -67,9 +67,9 @@
  * Thread-safe.
  */
 void GetRandBytes(unsigned char* buf, int num) noexcept;
-/** Generate a random integer in the range [0..range). Precondition: range > 0 */
+/** Generate a uniform random integer in the range [0..range). Precondition: range > 0 */
 uint64_t GetRand(uint64_t nMax) noexcept;
-/** Generate a random duration in the range [0..max). Precondition: max.count() > 0 */
+/** Generate a uniform random duration in the range [0..max). Precondition: max.count() > 0 */

src/random.cpp

maflcko · 2020-04-30T22:37:19Z

Open-Close to re-run ci. See #15847 (comment)

promag · 2020-05-05T21:35:00Z

Merge?

hebasto

Concept ACK.

hebasto · 2020-05-14T07:27:03Z

src/random.h

+/** Generate a uniform random integer in the range [0..range). Precondition: range > 0 */
 uint64_t GetRand(uint64_t nMax) noexcept;


Suggested change

/** Generate a uniform random integer in the range [0..range). Precondition: range > 0 */

uint64_t GetRand(uint64_t nMax) noexcept;

/** Generate a uniform random integer in the range [0..nMax). Precondition: nMax > 0 */

uint64_t GetRand(uint64_t nMax) noexcept;

Thanks. To not invalidate all the ACKs, I am going to leave this for now.

hebasto

ACK 0000ea3 with non-blocking nit.

0000ea3 test: Add test for GetRandMillis and GetRandMicros (MarcoFalke) fa0e5b8 Add templated GetRandomDuration<> (MarcoFalke) Pull request description: A naive implementation of this template is dangerous, because the call site might accidentally omit the template parameter: ```cpp template <typename D> D GetRandDur(const D& duration_max) { return D{GetRand(duration_max.count())}; } BOOST_AUTO_TEST_CASE(util_time_GetRandTime) { std::chrono::seconds rand_hour = GetRandDur(std::chrono::hours{1}); // Want seconds to be in range [0..1hour), but always get zero :(((( BOOST_CHECK_EQUAL(rand_hour.count(), 0); } ``` 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. ACKs for top commit: laanwj: Code review ACK 0000ea3 promag: Code review ACK 0000ea3. jonatack: 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: hebasto: ACK 0000ea3 with non-blocking [nit](bitcoin#18781 (comment)). Tree-SHA512: e89d46e31452be6ea14269ecbbb2cdd9ae83b4412cd14dff7d1084283092722a2f847cb501e8054394e4a3eff852f9c87f6d694fd008b3f7e8458cb5a3068af7

Summary: > instead of implementing the function for each duration type by hand, replace it with a templated version that is safe to use. > A naive implementation of this template is dangerous, because the call site might accidentally omit the template parameter: > > ``` > template <typename D> > D GetRandDur(const D& duration_max) > { > return D{GetRand(duration_max.count())}; > } > > BOOST_AUTO_TEST_CASE(util_time_GetRandTime) > { > std::chrono::seconds rand_hour = GetRandDur(std::chrono::hours{1}); > // Want seconds to be in range [0..1hour), but always get zero :(((( > BOOST_CHECK_EQUAL(rand_hour.count(), 0); > } > ``` This is a backport of Core [[bitcoin/bitcoin#18781 | PR18781]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9118

promag reviewed Apr 27, 2020

View reviewed changes

jonatack reviewed Apr 27, 2020

View reviewed changes

src/random.h Outdated Show resolved Hide resolved

maflcko force-pushed the 2004-randDur branch from bbbb748 to fad927b Compare April 27, 2020 13:36

maflcko changed the title ~~Replace GetRandMicros with templated GetRandDur~~ Add templated GetRandDur<> Apr 27, 2020

maflcko force-pushed the 2004-randDur branch from fad927b to fa63100 Compare April 27, 2020 13:38

maflcko changed the title ~~Add templated GetRandDur<>~~ Add templated GetRandDuration<> Apr 27, 2020

sipa reviewed Apr 27, 2020

View reviewed changes

maflcko force-pushed the 2004-randDur branch from fa12f1a to faf2e85 Compare April 27, 2020 18:35

DrahtBot mentioned this pull request Apr 27, 2020

P2P: Mempool tracks locally submitted transactions to improve wallet privacy #18038

Merged

DrahtBot added the Tests label Apr 27, 2020

DrahtBot mentioned this pull request Apr 29, 2020

[doc / test / mempool] unbroadcast follow-ups #18807

Merged

DrahtBot added the Needs rebase label Apr 29, 2020

maflcko force-pushed the 2004-randDur branch from faf2e85 to fa05a4c Compare April 29, 2020 13:11

DrahtBot removed the Needs rebase label Apr 29, 2020

laanwj reviewed Apr 30, 2020

View reviewed changes

MarcoFalke added 2 commits April 30, 2020 09:19

Add templated GetRandomDuration<>

fa0e5b8

test: Add test for GetRandMillis and GetRandMicros

0000ea3

maflcko force-pushed the 2004-randDur branch from fa05a4c to 0000ea3 Compare April 30, 2020 13:20

jonatack reviewed Apr 30, 2020

View reviewed changes

src/random.cpp Show resolved Hide resolved

maflcko added the Refactoring label Apr 30, 2020

maflcko removed the Tests label Apr 30, 2020

maflcko closed this Apr 30, 2020

maflcko reopened this Apr 30, 2020

maflcko closed this Apr 30, 2020

maflcko reopened this Apr 30, 2020

hebasto requested changes May 14, 2020

View reviewed changes

hebasto approved these changes May 14, 2020

View reviewed changes

maflcko merged commit 9518708 into bitcoin:master May 15, 2020

maflcko deleted the 2004-randDur branch May 15, 2020 13:01

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		/** Generate a uniform random integer in the range [0..range). Precondition: range > 0 */
		uint64_t GetRand(uint64_t nMax) noexcept;

Add templated GetRandDuration<> #18781

Add templated GetRandDuration<> #18781

Uh oh!

Conversation

maflcko commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Apr 27, 2020

Uh oh!

maflcko commented Apr 27, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Apr 27, 2020

Uh oh!

jonatack commented Apr 27, 2020

Uh oh!

maflcko commented Apr 27, 2020

Uh oh!

sipa Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

sipa Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Apr 27, 2020

Uh oh!

laanwj Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 30, 2020

Uh oh!

promag commented Apr 30, 2020

Uh oh!

jonatack commented Apr 30, 2020

Uh oh!

Uh oh!

maflcko commented Apr 30, 2020

Uh oh!

promag commented May 5, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto May 14, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko May 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Apr 27, 2020 •

edited

Loading

DrahtBot commented Apr 27, 2020 •

edited

Loading