Partial merge bitcoin#14624: Some simple improvements to the RNG code #3923
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
std::shuffle doesn't accept only two arguments so we use FastRandomContext()
UdjinM6
requested changes
Jan 10, 2021
There was a problem hiding this comment.
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 ingit log) - see below
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
|
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. |
xdustinface
reviewed
Jan 10, 2021
Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>
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. |
I'm using |
UdjinM6
approved these changes
Jan 10, 2021
xdustinface
approved these changes
Jan 11, 2021
PastaPastaPasta
approved these changes
Jan 14, 2021
There was a problem hiding this comment.
utACK. Be aware, your contribution may be worthy of a bounty https://trello.com/c/dbkj327J/86-dash-core-contributions
Merged
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
Mar 15, 2022
…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 <UdjinM6@users.noreply.github.com> * llmq: use inherited FastRandomContext Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> * Make the linter happy :) Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com> Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
std::random_shufflehas been deprecated and Bitcoin has opted to implement an alternative tostd::shuffleforFastRandomContextdue to a bug specific tolibstdc++namedShuffle.Unlike
std::random_shuffle,std::shuffledoes not accept only two arguments so those invocations now useFastRandomContext.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