refactor: use Span in random.* #24213

PastaPastaPasta · 2022-01-31T11:51:24Z

~~This PR does two things~~

use a Span for GetRandBytes and GetStrongRandBytes

2. make GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from GetRand(std::numeric_limits<uint64_t>::max() -> GetRand<uint64_t>()

MarcoFalke this was inspired by your comment here: #24185 (comment) about using Span, so hopefully I'll be able to get this PR done and merged 😂

~~Also, if requested I could revert the GetRand(std::numeric_limits<uint64_t>::max() -> GetRand<uint64_t>() related changes if it ends up causing too many conflicts~~

maflcko · 2022-01-31T12:21:14Z

This PR does two things

This needs to be split into two commits for review

PastaPastaPasta · 2022-01-31T12:33:24Z

This PR does two things

This needs to be split into two commits for review

Done, thanks

DrahtBot · 2022-01-31T20:49:07Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24185 (refactor: only use explicit reinterpret/const casts, not implicit by PastaPastaPasta)

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.

PastaPastaPasta · 2022-03-06T17:11:52Z

Any opinion on this? @MarcoFalke and others?

jonatack

Approach ACK, debug-builds cleanly, unit tests pass, feature_taproot.py passes (cf the CI failure)

jonatack · 2022-03-06T17:36:50Z

src/dbwrapper.cpp

@@ -227,7 +227,7 @@ const unsigned int CDBWrapper::OBFUSCATE_KEY_NUM_BYTES = 8;
 std::vector<unsigned char> CDBWrapper::CreateObfuscateKey() const
 {
    std::vector<uint8_t> ret(OBFUSCATE_KEY_NUM_BYTES);
-    GetRandBytes(ret.data(), OBFUSCATE_KEY_NUM_BYTES);
+    GetRandBytes(ret);


eeae84c missing span headers here in src/dbwrapper.cpp and in src/key.cpp, src/random.h, src/rpc/request.cpp, maybe reverify if didn't miss any others

Should it really be included in files like dbwrapper.cpp and key.cpp and request.cpp? It seems to be that it makes more sense to just explicitly include it in src/random.h.

If this is in developer notes or a strong opinion of yours, I'll happily do it, but seems better to just do it in random.h (applied and squashed random.h include)

Yes, the developer notes specify the guideline and reason:

- Every `.cpp` and `.h` file should `#include` every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers. - *Rationale*: Excluding headers because they are already indirectly included results in compilation failures when those indirect dependencies change. Furthermore, it obscures what the real code dependencies are.

I agree with the rule, but I'm not sure I agree with the application. I'm not really "using" span in these cpp files, I'm simply passing something to a function. If we change random.h to not use span, then we need to change the calling files anyway.

My local CLion for instance sees these includes are completely unused when I add them, so I see no reason to add them right now

Oh, you're right, now that you've added the header to random.h. Though key.cpp uses Span (not in your changes), so could add the header in 32b6173 if you're so inclined. Otherwise LGTM.

src/addrdb.cpp

jonatack · 2022-03-06T18:05:05Z

src/net_processing.cpp

+        uint64_t nonce;
+        do {
+            nonce = GetRand<uint64_t>();
+        } while (nonce == 0);


d350195 what is the reason for the do/while style change?

It avoids a redundant assignment, and I think it makes more sense, although I would be happy to revert this change if you would strongly want that

I wouldn't needlessly change it unless other reviewers prefer the change, but no strong opinion.

I think the old code is slightly more elegant. There's no need to distinguish the first iteration here, so let's not.

jonatack · 2022-03-14T19:18:40Z

ACK b989b5a

PastaPastaPasta · 2022-03-23T23:02:36Z

I have rebased. @jonatack please re-review.

jonatack · 2022-03-24T11:25:22Z

re-ACK 19f2c35 per git range-diff 8234cda b989b5a 19f2c35, rebase only

PastaPastaPasta · 2022-04-03T23:03:44Z

@MarcoFalke would you be able to review this PR?

laanwj · 2022-04-06T10:35:14Z

Concept and code review ACK on the span commit.
I find the other one changing GetRandInt more risky and harder to review.

Edit: might want to split this into different PRs as the changes, apart from both being related to random, are orthogonal.

maflcko · 2022-04-08T11:26:06Z

src/net.cpp

@@ -2168,7 +2168,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)

            if (fFeeler) {
                // Add small amount of random noise before connection to avoid synchronization.
-                int randsleep = GetRandInt(FEELER_SLEEP_WINDOW * 1000);
+                int randsleep = GetRand<int>(FEELER_SLEEP_WINDOW * 1000);
                if (!interruptNet.sleep_for(std::chrono::milliseconds(randsleep)))


This can probably use the exiting native chrono random stuff.

Further, I am wondering if it makes sense to add a FastRandomContext member to connman and use that for all noise generation instead of generating a new one each time. A commit like fa73e9d can then be used to provide the chrono random stuff.

maflcko · 2022-04-08T11:28:24Z

src/init.cpp

@@ -1257,7 +1257,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
    assert(!node.banman);
    node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetIntArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
    assert(!node.connman);
-    node.connman = std::make_unique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), *node.addrman, args.GetBoolArg("-networkactive", true));
+    node.connman = std::make_unique<CConnman>(GetRand<uint64_t>(), GetRand<uint64_t>(), *node.addrman, args.GetBoolArg("-networkactive", true));


Maybe the fast context could then also be used to remove those args?

maflcko · 2022-04-08T11:31:40Z

src/net_processing.cpp

-        }
+        uint64_t nonce;
+        do {
+            nonce = GetRand<uint64_t>();


I wonder if it makes sense to use a single FastRandomContext member for all entropy drawn inside PeerManagerImpl, instead of creating a new context for each call.

PastaPastaPasta · 2022-04-19T18:51:09Z

@laanwj Thanks for the review, I've dropped that other commit from this PR. Please re-ACK

laanwj · 2022-04-21T13:48:19Z

Thank you! Code review re-ACK 3ae7791

Summary: This is a backport of [[bitcoin/bitcoin#24213 | core#24213]] Depends on D14826 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D14827

DrahtBot added P2P Refactoring RPC/REST/ZMQ Utils/log/libs UTXO Db and Indexes Wallet labels Jan 31, 2022

PastaPastaPasta force-pushed the refactor-random branch 2 times, most recently from b401960 to 79deab2 Compare January 31, 2022 12:46

DrahtBot added the Needs rebase label Mar 2, 2022

PastaPastaPasta force-pushed the refactor-random branch from 79deab2 to d350195 Compare March 6, 2022 02:59

DrahtBot removed the Needs rebase label Mar 6, 2022

jonatack reviewed Mar 6, 2022

View reviewed changes

PastaPastaPasta force-pushed the refactor-random branch from d350195 to 28011cc Compare March 7, 2022 15:33

DrahtBot added the Needs rebase label Mar 14, 2022

PastaPastaPasta force-pushed the refactor-random branch from 28011cc to b989b5a Compare March 14, 2022 17:47

DrahtBot removed the Needs rebase label Mar 14, 2022

DrahtBot mentioned this pull request Mar 14, 2022

wallet: Use single FastRandomContext when creating a wallet tx #24560

Merged

maflcko removed Wallet UTXO Db and Indexes labels Mar 15, 2022

maflcko removed RPC/REST/ZMQ P2P Utils/log/libs labels Mar 15, 2022

DrahtBot added the Needs rebase label Mar 23, 2022

refactor: use Span in random.*

3ae7791

PastaPastaPasta force-pushed the refactor-random branch from b989b5a to 19f2c35 Compare March 23, 2022 23:03

DrahtBot removed the Needs rebase label Mar 24, 2022

maflcko reviewed Apr 8, 2022

View reviewed changes

PastaPastaPasta force-pushed the refactor-random branch from 19f2c35 to 3ae7791 Compare April 19, 2022 18:48

PastaPastaPasta changed the title ~~refactor: use Span in random.*; make GetRand a template, remove GetRandInt~~ refactor: use Span in random.* Apr 19, 2022

PastaPastaPasta mentioned this pull request Apr 19, 2022

refactor: make GetRand a template, remove GetRandInt #24925

Merged

laanwj added the Utils/log/libs label Apr 21, 2022

laanwj merged commit 43bb106 into bitcoin:master Apr 21, 2022

PastaPastaPasta deleted the refactor-random branch April 21, 2022 15:07

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 22, 2022

Merge bitcoin#24213: refactor: use Span in random.*

ad85007

bitcoin locked and limited conversation to collaborators Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use Span in random.* #24213

refactor: use Span in random.* #24213

PastaPastaPasta commented Jan 31, 2022 •

edited

Loading

maflcko commented Jan 31, 2022

PastaPastaPasta commented Jan 31, 2022

DrahtBot commented Jan 31, 2022 •

edited

Loading

PastaPastaPasta commented Mar 6, 2022

jonatack left a comment •

edited

Loading

jonatack Mar 6, 2022 •

edited

Loading

PastaPastaPasta Mar 7, 2022 •

edited

Loading

jonatack Mar 9, 2022

PastaPastaPasta Mar 14, 2022

jonatack Mar 14, 2022

jonatack Mar 6, 2022

PastaPastaPasta Mar 7, 2022

jonatack Mar 9, 2022 •

edited

Loading

laanwj Apr 6, 2022

jonatack commented Mar 14, 2022

PastaPastaPasta commented Mar 23, 2022

jonatack commented Mar 24, 2022

PastaPastaPasta commented Apr 3, 2022

laanwj commented Apr 6, 2022 •

edited

Loading

maflcko Apr 8, 2022

maflcko Apr 8, 2022

maflcko Apr 8, 2022

PastaPastaPasta commented Apr 19, 2022

laanwj commented Apr 21, 2022

refactor: use Span in random.* #24213

refactor: use Span in random.* #24213

Conversation

PastaPastaPasta commented Jan 31, 2022 • edited Loading

maflcko commented Jan 31, 2022

PastaPastaPasta commented Jan 31, 2022

DrahtBot commented Jan 31, 2022 • edited Loading

Conflicts

PastaPastaPasta commented Mar 6, 2022

jonatack left a comment • edited Loading

Choose a reason for hiding this comment

jonatack Mar 6, 2022 • edited Loading

Choose a reason for hiding this comment

PastaPastaPasta Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack commented Mar 14, 2022

PastaPastaPasta commented Mar 23, 2022

jonatack commented Mar 24, 2022

PastaPastaPasta commented Apr 3, 2022

laanwj commented Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PastaPastaPasta commented Apr 19, 2022

laanwj commented Apr 21, 2022

PastaPastaPasta commented Jan 31, 2022 •

edited

Loading

DrahtBot commented Jan 31, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack Mar 6, 2022 •

edited

Loading

PastaPastaPasta Mar 7, 2022 •

edited

Loading

jonatack Mar 9, 2022 •

edited

Loading

laanwj commented Apr 6, 2022 •

edited

Loading