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

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	laanwj

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

   Conflicts

   No conflicts as of last run.

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.

Contributors
sipa DrahtBot darosior laanwj maflcko

Labels
Descriptors CI failed