miniscript: convert non-critical asserts to CHECK_NONFATAL #31727

pull darosior wants to merge 1 commits into bitcoin:master from darosior:2501_miniscript_nonfatal changing 2 files +54 −51
  1. darosior commented at 4:17 pm on January 23, 2025: member

    The Miniscript code contains assertions to prevent ending up in an insane state or prevent UB, but also to enforce logical invariants. For the latter it is not necessary to crash the program if they are broken. Raising an exception suffices, especially as this code is often called through the RPC interface which can in turn handle the exception and the user can report it to developers.

    This revives #28678 from Pieter Wuille.

  2. miniscript: convert non-critical asserts to CHECK_NONFATAL
    The Miniscript code contains assertions to prevent ending up in an insane state or prevent UB, but
    also to enforce logical invariants. For the latter it is not necessary to crash the program if they
    are broken. Raising an exception suffices, especially as this code is often called through the RPC
    interface which can in turn handle the exception and the user can report it to developers.
    
    This is based on previous work from Pieter Wuille.
    ff0194a7ce
  3. DrahtBot commented at 4:17 pm on January 23, 2025: contributor

    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/31727.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK brunoerg, hodlinator, kevkevinpal

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31719 (miniscript: fixes #29098 by only use first k valid signatures by tnndbtc)
    • #31713 (miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up) by hodlinator)

    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.

  4. DrahtBot added the label Descriptors on Jan 23, 2025
  5. brunoerg commented at 4:37 pm on January 23, 2025: contributor
    Concept ACK
  6. in src/script/miniscript.h:662 in ff0194a7ce
    658@@ -659,7 +659,8 @@ struct Node {
    659             stack.pop_back();
    660         }
    661         // The final remaining results element is the root result, return it.
    662-        assert(results.size() == 1);
    663+        assert(results.size() >= 1);
    


    pythcoiner commented at 5:18 pm on January 24, 2025:
    why >= if then you check against == ?

    darosior commented at 7:05 pm on January 24, 2025:
    The assert is a looser check. If this condition doesn’t hold it will trigger UB on the line right after so we’d rather crash. The CHECK_NONFATAL check is stricter and as long as the assert holds only applies to the logic, so it’s not necessary to crash the program. Raising an exception to signal the bug to the user is sufficient.

    maflcko commented at 6:14 pm on February 5, 2025:
    Just for reference, CHECK_NONFATAL is safe (from an UB perspective) to be used as a drop-in replacement for assert/Assert (and vice-versa) with only a slight difference: An assertion failure will abort the whole program, and not continue to the next statement. A nonfatal check failure will also not continue to the next statement, but it will just throw an exception and not fatally abort the whole program.
  7. DrahtBot added the label CI failed on Jan 26, 2025
  8. DrahtBot removed the label CI failed on Jan 26, 2025
  9. darosior commented at 3:33 pm on January 28, 2025: member

    Why aren’t these converted to use CHECK_NONFATAL?

    Same reason as here. I kept asserts where breaking the invariant would lead to UB. In the examples you point out it would read out of the sats vector bounds.

    Actually this should be a strict check, not an inferior or equal to!

  10. hodlinator commented at 1:56 pm on January 29, 2025: contributor

    Same reason as here. I kept asserts where breaking the invariant would lead to UB. In the examples you point out it would read out of the sats vector bounds.

    Okay, and there is no way user input from RPCs etc would currently be able to trigger the asserts?

    Actually this should be a strict check, not an inferior or equal to!

    Ah, didn’t realize, good catch indeed, = should be dropped.

  11. kevkevinpal commented at 3:44 am on February 6, 2025: contributor

    Concept ACK

    lightly reviewed the code and looks good to me


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: 2025-02-07 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me