Partial merge bitcoin#14624: Some simple improvements to the RNG code by kwvg · Pull Request #3923 · dashpay/dash

kwvg · 2021-01-09T18:12:09Z

Overview

std::random_shuffle has been deprecated and Bitcoin has opted to implement an alternative to std::shuffle for FastRandomContext due to a bug specific to libstdc++ named Shuffle.

Unlike std::random_shuffle, std::shuffle does not accept only two arguments so those invocations now use FastRandomContext.

Replicating the commit would only implement changes to Bitcoin-derived logic while leaving Dash-specific logic using the deprecated call, changes have been made for LLMQ, Governance and Privatesend accordingly.

Disclosure

Dash-specific changes have not been tested, only compilation has been ensured. Running the client on (a) testnet may be necessary.

Resources

bitcoin@3db746b (Introduce a Shuffle for FastRandomContext and use it in wallet and coinselection, Dec 13, 2018 by sipa)
https://en.cppreference.com/w/cpp/algorithm/random_shuffle
https://stackoverflow.com/questions/22915325/avoiding-self-assignment-in-stdshuffle (mentioned in sipa's commit)

bitcoin@3db746b

https://en.cppreference.com/w/cpp/algorithm/random_shuffle (deprecated in c++14)

std::shuffle doesn't accept only two arguments so we use FastRandomContext()

UdjinM6

Thanks for looking into it 👍 Those random_shuffle deprecation warnings on compilation are pretty annoying and concerning indeed.

Few thoughts/suggestions:

The original commit was included into btc 0.18 and was not backported to 0.17 afaik. Are there any real issues or is it simply to fix compilation warnings/make it C++17 compatible?
PR title should say smth like Partial merge bitcoin#14624: Some simple improvements to the RNG code (makes it easier to track backported PRs in git log)
see below

src/llmq/quorums_signing_shares.cpp

src/llmq/quorums_utils.h

Co-authored-by: UdjinM6 <[email protected]>

kwvg · 2021-01-10T03:39:27Z

I was attempting to have Dash compile with Apple Clang and it wasn't happy with the deprecated references. PR title has been changed and changes have been committed! I mixed up the Bitcoin versions, I was looking at bitcoin@f686002 instead of bitcoin@3db746b, my bad.

src/bench/bls.cpp

Co-authored-by: dustinface <[email protected]>

UdjinM6 · 2021-01-10T12:36:58Z

I was attempting to have Dash compile with Apple Clang and it wasn't happy with the deprecated references.

Does it fail to compile or does it simply complain via warnings (but succeeds otherwise)? Cause I'm on mac too (macOS 10.14.6, Apple clang version 11.0.0) and it compiles just fine for me.

kwvg · 2021-01-10T13:16:20Z

governance/governance.cpp:1023:10: error: 'random_shuffle<std::__1::__wrap_iter<uint256 *>, FastRandomContext &>' is deprecated [-Werror,-Wdeprecated-declarations]
/Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/algorithm:3116:1: note: 'random_shuffle<std::__1::__wrap_iter<uint256 *>, FastRandomContext &>' has been explicitly marked deprecated here
_LIBCPP_DEPRECATED_IN_CXX14 void
/Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/__config:1036:39: note: expanded from macro '_LIBCPP_DEPRECATED_IN_CXX14'
#  define _LIBCPP_DEPRECATED_IN_CXX14 _LIBCPP_DEPRECATED
/Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/__config:1019:48: note: expanded from macro '_LIBCPP_DEPRECATED'
#    define _LIBCPP_DEPRECATED __attribute__ ((deprecated))

I'm using clang-1200.0.32.28 (shipped with Xcode) on macOS Catalina 10.15.7

UdjinM6

Interesting. Makes sense to fix it then I guess.

utACK

xdustinface

utACK, nice that those deprecation warning will be gone now :)

PastaPastaPasta

utACK. Be aware, your contribution may be worthy of a bounty https://trello.com/c/dbkj327J/86-dash-core-contributions

…dashpay#3923) * random: Introduce std::shuffle alternative for FastRandomContext bitcoin@3db746b * random: change std::random_shuffle calls to std::shuffle https://en.cppreference.com/w/cpp/algorithm/random_shuffle (deprecated in c++14) * random: change FastRandomContext std::random_shuffle calls to shuffle * random: change last std::shuffle calls to Shuffle std::shuffle doesn't accept only two arguments so we use FastRandomContext() * llmq: use inherited FastRandomContext Co-authored-by: UdjinM6 <[email protected]> * llmq: use inherited FastRandomContext Co-authored-by: UdjinM6 <[email protected]> * Make the linter happy :) Co-authored-by: dustinface <[email protected]> Co-authored-by: UdjinM6 <[email protected]> Co-authored-by: dustinface <[email protected]>

kwvg added 4 commits January 9, 2021 23:07

random: Introduce std::shuffle alternative for FastRandomContext

1e50de5

bitcoin@3db746b

random: change std::random_shuffle calls to std::shuffle

79fe961

https://en.cppreference.com/w/cpp/algorithm/random_shuffle (deprecated in c++14)

random: change FastRandomContext std::random_shuffle calls to shuffle

643e583

random: change last std::shuffle calls to Shuffle

0e4426d

std::shuffle doesn't accept only two arguments so we use FastRandomContext()

UdjinM6 requested changes Jan 10, 2021

View reviewed changes

src/llmq/quorums_signing_shares.cpp Outdated Show resolved Hide resolved

src/llmq/quorums_utils.h Outdated Show resolved Hide resolved

kwvg and others added 2 commits January 10, 2021 09:07

llmq: use inherited FastRandomContext

eb9232f

Co-authored-by: UdjinM6 <[email protected]>

llmq: use inherited FastRandomContext

9d03a9a

Co-authored-by: UdjinM6 <[email protected]>

kwvg changed the title ~~random: backport bitcoin 0.17 random changes, apply them to dash-specific logic~~ Partial merge bitcoin#14624: Some simple improvements to the RNG code Jan 10, 2021

kwvg requested a review from UdjinM6 January 10, 2021 10:35

xdustinface reviewed Jan 10, 2021

View reviewed changes

src/bench/bls.cpp Outdated Show resolved Hide resolved

Make the linter happy :)

48b28f4

Co-authored-by: dustinface <[email protected]>

UdjinM6 approved these changes Jan 10, 2021

View reviewed changes

UdjinM6 added this to the 17 milestone Jan 10, 2021

xdustinface approved these changes Jan 11, 2021

View reviewed changes

UdjinM6 requested a review from PastaPastaPasta January 14, 2021 13:38

PastaPastaPasta approved these changes Jan 14, 2021

View reviewed changes

PastaPastaPasta merged commit 4f38f5c into dashpay:develop Jan 14, 2021

UdjinM6 mentioned this pull request Jul 19, 2021

Backport more p2p #4277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial merge bitcoin#14624: Some simple improvements to the RNG code#3923

Partial merge bitcoin#14624: Some simple improvements to the RNG code#3923
PastaPastaPasta merged 7 commits intodashpay:developfrom
kwvg:develop

kwvg commented Jan 9, 2021

Uh oh!

UdjinM6 left a comment

Uh oh!

Uh oh!

Uh oh!

kwvg commented Jan 10, 2021 •

edited

Loading

Uh oh!

Uh oh!

UdjinM6 commented Jan 10, 2021

Uh oh!

kwvg commented Jan 10, 2021

Uh oh!

UdjinM6 left a comment

Uh oh!

xdustinface left a comment

Uh oh!

PastaPastaPasta left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kwvg commented Jan 9, 2021

Overview

Disclosure

Resources

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kwvg commented Jan 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

UdjinM6 commented Jan 10, 2021

Uh oh!

kwvg commented Jan 10, 2021

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

xdustinface left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kwvg commented Jan 10, 2021 •

edited

Loading