Skip to content

random: move VerifyRNDRRS above InitHardwareRand #31902

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

random: move VerifyRNDRRS above InitHardwareRand #31902

wants to merge 1 commit into from

Conversation

eval-exec
Copy link
Contributor

@eval-exec eval-exec commented Feb 19, 2025 •
edited
Loading

Fix: #31826 (comment)

Friendly request @maflcko review.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 19, 2025 •
edited
Loading

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31902.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

@eval-exec eval-exec mentioned this pull request Feb 19, 2025
Merged
@eval-exec eval-exec changed the title random: move VerifyRNDRRS above InitHardwareRand, fix https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961315638 random: move VerifyRNDRRS above InitHardwareRand Feb 19, 2025
@maflcko maflcko added this to the 29.0 milestone Feb 19, 2025
@eval-exec
Copy link
Contributor Author

eval-exec commented Feb 19, 2025 •
edited
Loading

Why a CI failed label is added? I didn't notice which CI workflow is failed.

@darosior darosior mentioned this pull request Feb 19, 2025
Merged
@darosior
Copy link
Member

darosior commented Feb 19, 2025 •
edited
Loading

I think for now it's safer to just revert the original PR. See #31908. The changes can be re-considered and re-reviewed after the 29.0 branch-off.

@achow101
Copy link
Member

Still fails on AWS Graviton:

/home/ec2-user/bitcoin/src/random.cpp: In function ‘bool {anonymous}::VerifyRNDRRS()’:
/home/ec2-user/bitcoin/src/random.cpp:204:13: error: ‘GetRNDRRSInternal’ was not declared in this scope
  204 |         if (GetRNDRRSInternal(test)) {
      |             ^~~~~~~~~~~~~~~~~
gmake[2]: *** [src/util/CMakeFiles/bitcoin_util.dir/build.make:440: src/util/CMakeFiles/bitcoin_util.dir/__/random.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:940: src/util/CMakeFiles/bitcoin_util.dir/all] Error 2

achow101 added a commit that referenced this pull request Feb 19, 2025
@achow101
Merge #31908: Revert merge of PR #31826
fd14995 
3e9b12b Revert "Merge #31826: random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop" (Antoine Poinsot)

Pull request description:

  PR #31826 was merged [despite the code not compiling](#31826 (comment)).

  #31902 was opened to fix the code but since this code is only targeting a not officially supported platform, we don't have a CI in place to compile and run tests on this platform, neither apparently reviewers do (nor does the author?), don't take more risk right before 29 and revert the original broken PR.

ACKs for top commit:
  sipa:
    ACK 3e9b12b
  achow101:
    ACK 3e9b12b
  TheCharlatan:
    ACK 3e9b12b
  eval-exec:
    ACK 3e9b12b
  laanwj:
    ACK 3e9b12b

Tree-SHA512: e90f8ffb2eebe77e5b6f1c273fbeb29dd5bd6a76698d9a6048c33f3349033c56ea984dd9b64704698da01ecad4c47f98acac1a30312bf2499dbdd1931596953f
@glozow glozow removed this from the 29.0 milestone Feb 19, 2025
@glozow
Copy link
Member

glozow commented Feb 19, 2025

Closing since #31908 was merged.

@glozow glozow closed this Feb 19, 2025
@eval-exec eval-exec mentioned this pull request Feb 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@eval-exec @DrahtBot @darosior @achow101 @glozow @maflcko