miniscript: convert non-critical asserts to Assumes #28678

1. 
   sipa commented at 10:00 pm on October 18, 2023: member

   It’s better practice to use Assume for non-critical assumptions made in the code. This is especially relevant for P2P-exposed functionality, which I don’t believe is the case for miniscript, but still, use Assumes except in places where a crash or UB will be inevitable on a failed assert anyway.
2. miniscript: move satisfaction sanity check to its own function 88f8e44683
3. miniscript: convert non-critical asserts to Assumes 12ad92c882
4. 
   DrahtBot commented at 10:00 pm on October 18, 2023: 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/28678.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	laanwj, brunoerg

   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:

   • #31713 (script refactor: Remove superfluous 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.

5. DrahtBot added the label Descriptors on Oct 18, 2023
6. fanquake requested review from darosior on Oct 18, 2023
7. 
   darosior commented at 6:55 am on October 19, 2023: member

   Those checks can be reached through the RPC interface or the wallet. In the unlikely scenario they are reached in a release build it would be better if they raise an exception instead of continuing silently. For instance if the type of the miniscript descriptor you imported was miscomputed for some reason you probably don’t want to move forward and receive funds on it. Therefore i think it’s better to use CHECK_NONFATAL?
8. 
   laanwj commented at 1:53 pm on October 31, 2023: member

   Concept ACK. Not having these potentially crash the application is an improvement. Also agree with @darosior though, we don’t want error conditions that can be caused by user input to go by silently, either.
9. DrahtBot added the label CI failed on Jan 17, 2024
10. 
    maflcko commented at 5:57 pm on February 22, 2024: member

    Are you still working on this?
11. DrahtBot marked this as a draft on Apr 18, 2024
12. 
    DrahtBot commented at 7:10 am on April 18, 2024: contributor

    Converted to draft due to inactivity
13. 
    DrahtBot commented at 0:43 am on July 17, 2024: contributor

    🤔 There hasn’t been much activity lately and the CI seems to be failing.

    If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

14. 
    DrahtBot commented at 1:07 am on October 15, 2024: contributor

    🤔 There hasn’t been much activity lately and the CI seems to be failing.

    If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

15. 
    maflcko commented at 9:18 am on October 15, 2024: member

    @sipa @darosior It would be good to mark this as up-for-grabs, if you dropped working on it.
16. 
    DrahtBot commented at 10:09 am on January 13, 2025: contributor

    🤔 There hasn’t been much activity lately and the CI seems to be failing.

    If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

17. 
    brunoerg commented at 10:40 am on January 23, 2025: contributor

    Concept ACK

    @sipa @darosior It would be good to mark this as up-for-grabs, if you dropped working on it.

    Any update on it?

18. 
    sipa commented at 3:36 pm on January 23, 2025: member

    Closing and marking as up for grabs.
19. sipa closed this on Jan 23, 2025

    ---

20. sipa added the label Up for grabs on Jan 23, 2025
21. 
    darosior commented at 3:38 pm on January 23, 2025: member

    I’ll open a PR on top of master which implements my suggestion.
22. fanquake removed the label Up for grabs on Jan 23, 2025

Contributors
sipa DrahtBot darosior laanwj maflcko brunoerg

Labels
Descriptors CI failed