random: mark RandAddPeriodic and SeedPeriodic as noexcept #17507

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:random_followups changing 2 files +12 −22
  1. fanquake commented at 3:27 PM on November 18, 2019: member

    The usage of MilliSleep() in SeedPeriodic (previously SeedSleep) was removed in #17270, meaning it, and its users can now be marked noexcept.

    This also corrects the docs in random.h for some of the changes in #17270.

  2. doc: correct random.h docs after #17270 461e547877
  3. random: mark RandAddPeriodic and SeedPeriodic as noexcept
    The usage of MilliSleep() in SeedPeriodic (previously SeedSleep) was
removed in #17270, meaning it, and its users can now be marked noexcept.
    55b2cb199c
  4. fanquake added the label Utils/log/libs on Nov 18, 2019
  5. fanquake requested review from sipa on Nov 18, 2019
  6. in src/random.cpp:453 in 55b2cb199c 
     459 | - * this sleeping logic, so they are noexcept. The same is true for all the
 460 | - * GetRand*() functions that use GetRandBytes() indirectly.
 461 | - *
 462 | - * TODO: After moving away from interruptible boost-based thread management,
 463 | - * everything can become noexcept here.
 464 | + * None of the RNG code should ever throw any exception.
    laanwj commented at 3:35 PM on November 18, 2019:

    Very good :+1:

  7. practicalswift commented at 3:49 PM on November 18, 2019: contributor

    ACK 55b2cb199c276781b6daa5438af2da57dea3ac52

    Very nice!

    Somewhat related: it would be nice to have the same explicit guarantee for LogPrint/LogPrintf. That would allow compilers and static analyzers to treat a larger parts of our code base as "non-throwing" instead of "potentially throwing" :)

  8. laanwj commented at 4:32 PM on November 18, 2019: member

    Somewhat related: it would be nice to have the same explicit guarantee for LogPrint/LogPrintf.

    Wouldn't that be easy to do? LogPrintf already catches the only relevant exception, tinyformat::format_error, internally. I don't think any exceptions are supposed to leak out of it.

  9. practicalswift commented at 4:42 PM on November 18, 2019: contributor

    @laanwj Yes, exactly! It is only a matter of formalizing that with a noexcept :)

  10. MarcoFalke commented at 5:26 PM on November 18, 2019: member

    @practicalswift Wouldn't upstream be a better place to propose this change?

  11. practicalswift commented at 5:37 PM on November 18, 2019: contributor

    @MarcoFalke Upstream throws tinyformat::format_error which we catch: thus noexcept only makes sense on our end of things, no? :)

  12. MarcoFalke commented at 5:55 PM on November 18, 2019: member

    Fine, but LogPrintStr is also called, which might throw exceptions. In either case, this discussion should happen elsewhere.

  13. sipa commented at 7:27 PM on November 18, 2019: member

    ACK 55b2cb199c276781b6daa5438af2da57dea3ac52

  14. laanwj commented at 7:50 AM on November 19, 2019: member

    I don't think LogPrintStr is supposed to ever raise an exception, but sure, that'd need to be checked. Agree this is off topic here.

    ACK 55b2cb199c276781b6daa5438af2da57dea3ac52

  15. practicalswift commented at 9:30 AM on November 19, 2019: contributor

    @laanwj @MarcoFalke Please note that SeedPeriodic which is annotated noexcept in this PR calls LogPrintf:

    https://github.com/bitcoin/bitcoin/blob/2065ef66ee6fb2b7bb442274f860813cad85b08c/src/random.cpp#L480-L491

    Thus the question of a noexcept annotation for LogPrintf is not entirely off topic here :)

  16. laanwj commented at 10:07 AM on November 19, 2019: member

    Thus the question of a noexcept annotation for LogPrintf is not entirely off topic here :)

    Sure. I'm going to work on a PR to make logging noexcept.

  17. DrahtBot commented at 6:10 AM on December 5, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17670 (Move events_hasher into RNGState() by sipa)

    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.

  18. sipa commented at 8:41 AM on December 5, 2019: member

    ReACK. I accidentally redid part of this in #17670. The documentation improvements here should be done anyway, so let's do this PR first.

  19. laanwj referenced this in commit 6fff333c9f on Dec 5, 2019
  20. laanwj merged this on Dec 5, 2019
  21. laanwj closed this on Dec 5, 2019
  22. fanquake deleted the branch on Dec 5, 2019
  23. sidhujag referenced this in commit 9189c75128 on Dec 6, 2019
  24. deadalnix referenced this in commit fce972a3a0 on May 22, 2020
  25. sidhujag referenced this in commit 23b2206654 on Nov 10, 2020
  26. random-zebra referenced this in commit 93f43f0f81 on Apr 14, 2021
  27. DrahtBot locked this on Dec 16, 2021
Contributors
fanquakelaanwjpracticalswiftMarcoFalkesipaDrahtBot
Labels
Utils/log/libs
Linked (view graph)
#17270 Feed environment data into RNG initializers#17514 util: Make logging noexcept
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me