IsSane() in Miniscript does not ensure a Script is actually spendable. This is an issue as we would accept any sane Miniscript when parsing a descriptor. Fix this by explicitly checking a Miniscript descriptor is both sane and spendable when parsing it.
This bug was exposed due to a check added in #22838 (https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226859880) that triggered a fuzz crash (https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1612510057).
The value is only set for satisfiable nodes, so it was undefined for
non-satisfiable nodes. Make it clear in the interface by returning
std::nullopt if the node isn't satisfiable instead of an undefined
value.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
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.
1160@@ -1161,6 +1161,9 @@ struct Node {
1161 return true;
1162 }
1163
1164+ //! Whether no satisfaction exists for this node.
1165+ bool IsNotSatisfiable() const { return !GetStackSize(); }
In d4d3fcc16ccae4755604bb3a9a6a45542b231dcc “miniscript: treat all unsatisfiable miniscripts as insane”
Since this is only used negated, I think it would be better to make this IsSatisfiable
0 bool IsSatisfiable() const { return GetStackSize().has_value(); }
This is on purpose, because:
IsSatisfiable already exists and implements the above (needs to be told what challenges are available).So to avoid the confusion i opted for the negative. But i could make it MayBeSatisfiable if that’s clearer?
——– Original Message ——–
On Jun 29, 2023, 6:45 PM, Andrew Chow wrote:
@achow101 commented on this pull request.
@@ -1161,6 +1161,9 @@ struct Node { return true; }
- //! Whether no satisfaction exists for this node.
- bool IsNotSatisfiable() const { return !GetStackSize(); }
In d4d3fcc “miniscript: treat all unsatisfiable miniscripts as insane”
Since this is only used negated, I think it would be better to make this IsSatisfiable
⬇️ Suggested change
- bool IsNotSatisfiable() const { return !GetStackSize(); }
- bool IsSatisfiable() const { return GetStackSize().has_value(); }
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>
0 is sane according to https://github.com/rust-bitcoin/rust-miniscript/blob/master/src/miniscript/analyzable.rs#L66
If we are going to change the definition can we please have some reference document somewhere about what “sane” actually means?
So I think the definition for “sane” that we want is “does the apparent policy of this miniscript match its actual semantics”. If you drop all wrappers and naively replace all fragments with their corresponding policy, is that guaranteed to match the policy of the actual script?
Arguably, this is the case for “0”, despite not having any keys, because its apparent policy is that it’s never spendable, and that is also what it actually implements. Of course, we could expand the definition and explicitly treat lack of keys as automatically non-sane, but it seems more correct to instead say that only spendable miniscripts need key to be sane.
Sure, i’ll just move the check for satifiability to the descriptor parsing logic.
Just one comment on the definition of “sane”.
I think the definition for “sane” that we want is “does the apparent policy of this miniscript match its actual semantics” - if you drop all wrappers and naively replace all fragments with their corresponding policy, is that guaranteed to match the policy of the actual script.
It’s not what we have at the moment though? For instance older(4) meets this criterion but isn’t “sane” because it does not have the s property.
It seems to me what you describe is checking that all satisfactions are within limits and there is no timelock mixes, but without the requirement for a signature?
——– Original Message ——–
On Jun 29, 2023, 9:33 PM, Pieter Wuille wrote:
So I think the definition for “sane” that we want is “does the apparent policy of this miniscript match its actual semantics” - if you drop all wrappers and naively replace all fragments with their corresponding policy, is that guaranteed to match the policy of the actual script.
Arguably, this is the case for “0”, despite not having any keys, because its apparent policy is that it’s never spendable, and that is also what it actually implements. Of course, we could expand the definition and explicitly treat lack of keys as automatically non-sane, but it seems more correct to instead say that only spendable miniscripts need key to be sane.
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>
The requirement for a signature actually stems from the same question, though it’s not as exact.
If a script can be satisfied without signatures, it means there may be no signatures at all in the whole transaction or input, and thus nothing that actually commits to the nLockTime or nSequence values respectively, and so an attacker could just modify those values without invalidating anything.
It's possible for some unsatisfiable miniscripts to be considered sane.
Make sure we refuse to import those, as they would be unspendable.
I know, my point is the definition as spelled out before would still hold (anybody would be able to spend it but the “apparent policy of this miniscript match its actual semantics”). What i was trying to get at is that the rationale for requiring sane miniscripts to contain a signature check isn’t to make the actual semantics match the apparent policy, but that it’s unusable otherwise. And it’s the same for unsatisfiable miniscripts.
Anyways, i was just nitpicking. I pushed an update to leave intact the definition of IsSane in Miniscript but instead check we don’t parse unsatisfiable miniscripts in the descriptors. Added a functional test in a separate commit to easily check it fails on master.
What i was trying to get at is that the rationale for requiring sane miniscripts to contain a signature check isn’t to make the actual semantics match the apparent policy, but that it’s unusable otherwise
No, the rationale was definitely about transaction malleability, and about transactions being spendable in surprising ways. Transactions not being spendable, given a policy which indicates non-spendability, has never been something we’ve tried to prevent or even thought much about.
I’d be fine changing our definitions or rationale, and I agree that it’d be sensible to forbid top-level unspendable policies in a wallet, but their semantics are exactly what it says on the tin, so this would be a change in the way that we’ve been thinking about these things.
No, the rationale was definitely about transaction malleability
Malleability is orthogonal. I was responding to the definition given above of “does the apparent policy of this miniscript match its actual semantics”. An older(42) script can well be malleated, its apparent policy will match its semantics.
But anyways, let’s switch to ##miniscript if we want to continue discussing this? It’s not what this PR is implementing anymore.
utACK c7db88af71b3204171f33399aa4f33b40a4f7cd9
Longer term, and not for this PR, I think it would make sense to make a clearer separation between “miniscripts (and descriptors in general) which have undesirable properties so they shouldn’t be importable without warning”, and “miniscripts that are so deficient we cannot reason about them at all”. For example, if one has a malleable (or non-IsSane(), for certain reasons) miniscript, it’d be a bad idea to import it, as assessment of its properties may be wrong. However, it may still be worth signing for if one happens to have coins encumbered by such a miniscript despite those reasons.
I forgot also checking this when parsing from Script..
EDIT: actually i don’t think we need it since we don’t let users import a descriptor from Script. And it’s always better to parse a Miniscript in some utilities (such as decodescript), even though unspendable, than to display the raw Script.