miniscript: convert non-critical asserts to Assumes #28678

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

<!--174a7506f384e20aa4161008e828411d-->

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.

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?

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.

Are you still working on this?

Converted to draft due to inactivity

<!--2e250dc3d92b2c9115b66051148d6e47-->

🤔 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.

<!--2e250dc3d92b2c9115b66051148d6e47-->

🤔 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.

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

<!--2e250dc3d92b2c9115b66051148d6e47-->

🤔 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.

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?

Closing and marking as up for grabs.

I'll open a PR on top of master which implements my suggestion.