miniscript: convert non-critical asserts to Assumes #28678
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28678. ReviewsSee 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. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
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. |
|
Are you still working on this? |
|
Converted to draft due to inactivity |
|
🤔 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. |
1 similar comment
|
🤔 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. |
|
🤔 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. |
|
Closing and marking as up for grabs. |
|
I'll open a PR on top of master which implements my suggestion. |
ff0194a miniscript: convert non-critical asserts to CHECK_NONFATAL (Antoine Poinsot) Pull request description: 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. ACKs for top commit: hodlinator: ACK ff0194a TheCharlatan: ACK ff0194a brunoerg: code review ACK ff0194a Tree-SHA512: 8ed8f7b494e46ecf7cdebe75120cd0ffe543b6bc289bf882dac631fe2ec2cae590d5f7bc2316e52db085791694b136dffbc71c40c1e16886fa53ab00bd8cabd0
It's better practice to use
Assumefor 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, useAssumes except in places where a crash or UB will be inevitable on a failedassertanyway.