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.
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31727.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, brunoerg, hodlinator |
| Concept ACK | kevkevinpal |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
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);
>= if then you check against == ?
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.
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.
Concept ACK ff0194a7ce9dabf1b31b64ca584e45840dce8141
Why aren’t these converted to use CHECK_NONFATAL?
https://github.com/bitcoin/bitcoin/blob/ff0194a7ce9dabf1b31b64ca584e45840dce8141/src/script/miniscript.h#L1011-L1013
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!
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
satsvector 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.
Concept ACK
lightly reviewed the code and looks good to me
ACK ff0194a7ce9dabf1b31b64ca584e45840dce8141
Makes sense to not crash when encountering invalid miniscript.
Inserted CHECK_NONFATAL(false) in two separate touched functions to see how RPCs behave:
0₿ build/bin/bitcoin-cli -regtest deriveaddresses "wsh(1)#mrg7xj7p"
1error code: -1
2error message:
3Internal bug detected: false
4../src/script/miniscript.cpp:20 (SanitizeType)
5Bitcoin Core v28.99.0-ff0194a7ce9d-dirty
6Please report this issue here: https://github.com/bitcoin/bitcoin/issues
Similar for:
0../src/script/miniscript.cpp:41 (ComputeType)
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
satsvector bounds.Okay, and there is no way user input from RPCs etc would currently be able to trigger the asserts?
(I acknowledge you drew the line at UB, but out of bounds access fits that definition AFAIK).
Seems like user input like excessive thresh k-value is guarded against at least to some extent here while string parsing:
So the asserts referenced in #31727#pullrequestreview-2577746602 will typically run after that guard. Meaning there’s little point in converting those checks to CHECK_NONFATAL.
Tested:
0₿ build/bin/bitcoin-cli -regtest deriveaddresses "wsh(thresh(1,pk(tpubDFkN51vYF36W4Yfn3wGv5fpmRo3ok7vZZjc1gmRJjumq33L776e6GkP4HGdCVjDqYiBahXCrXQKja8aUZ2xovQNS8WkF46MdY7TLHJLYD7H/0/0)))#qzdpxsqa"
1[
2 "bcrt1q2kwrzducejduqjf36zmx6qzq29l3gc5sca7z2fqcyshm5jr0v4vq7erjda"
3]
4
5₿ build/bin/bitcoin-cli -regtest deriveaddresses "wsh(thresh(2,pk(tpubDFkN51vYF36W4Yfn3wGv5fpmRo3ok7vZZjc1gmRJjumq33L776e6GkP4HGdCVjDqYiBahXCrXQKja8aUZ2xovQNS8WkF46MdY7TLHJLYD7H/0/0)))#zx6ca7mc"
6error code: -5
7error message:
8A function is needed within P2WSH
Not the clearest error message, but at least an error.
Actually this should be a strict check, not an inferior or equal to!
Ah, didn’t realize, good catch indeed,
=should be dropped.
Would still be nice if this were addressed, but bug wasn’t caused by this PR, so could be a follow-up.
Edit: Stroke out inverted logic.
@darosior discovered (https://github.com/bitcoin/bitcoin/pull/31727#issuecomment-2619342125) in response to #31727#pullrequestreview-2577746602:
Ah, didn’t realize, good catch indeed,
=should be dropped.
This is now fixed together with some more cases in follow-up #32255.
Thanks for picking it up @hodlinator.
——– Original Message ——– On 4/12/25 4:06 AM, hodlinator wrote:
@.***(https://github.com/darosior) discovered (#31727 (comment)) in response to #31727 (review):
Ah, didn’t realize, good catch indeed, = should be dropped.
This is now fixed together with some more cases in follow-up #32255.
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
hodlinator left a comment (bitcoin/bitcoin#31727)
@.***(https://github.com/darosior) discovered (#31727 (comment)) in response to #31727 (review):
Ah, didn’t realize, good catch indeed, = should be dropped.
This is now fixed together with some more cases in follow-up #32255.
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>