-
Notifications
You must be signed in to change notification settings - Fork 38.7k
refactor: use Span in random.* #24213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This needs to be split into two commits for review |
Done, thanks |
b401960 to
79deab2
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
79deab2 to
d350195
Compare
|
Any opinion on this? @MarcoFalke and others? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK, debug-builds cleanly, unit tests pass, feature_taproot.py passes (cf the CI failure)
src/dbwrapper.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eeae84c5eb 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 32b6173e30b514437f6b16aed261d118e416ae6e if you're so inclined. Otherwise LGTM.
src/net_processing.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d35019566a what is the reason for the do/while style change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't needlessly change it unless other reviewers prefer the change, but no strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the old code is slightly more elegant. There's no need to distinguish the first iteration here, so let's not.
d350195 to
28011cc
Compare
28011cc to
b989b5a
Compare
|
ACK b989b5a43284b49c49e11d45c12c1daccc80e17a |
|
I have rebased. @jonatack please re-review. |
b989b5a to
19f2c35
Compare
|
re-ACK 19f2c35200321f0aeb728c51b75dcb47d9cca3a5 per |
|
@MarcoFalke would you be able to review this PR? |
|
Concept and code review ACK on the Edit: might want to split this into different PRs as the changes, apart from both being related to random, are orthogonal. |
src/net.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fa73e9d781d4898dbfd960aab997d99c62bc01ed can then be used to provide the chrono random stuff.
src/init.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the fast context could then also be used to remove those args?
src/net_processing.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
19f2c35 to
3ae7791
Compare
|
@laanwj Thanks for the review, I've dropped that other commit from this PR. Please re-ACK |
|
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
This PR does two things2. 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 theGetRand(std::numeric_limits<uint64_t>::max()->GetRand<uint64_t>()related changes if it ends up causing too many conflicts