Closed sanket1729 closed 1 year ago
Ooo, very interesting CI failure
thread 'descriptor::csfs_cov::tests::parse_cov' panicked at 'called `Result::unwrap()` on an `Err` value: TypeCheck("fragment «thresh(2,ver_eq(1),s:pk(C),s:pk(B))» sub-fragment 0 can not be dissatisfied and cannot be used in a threshold")', src/descriptor/csfs_cov/mod.rs:86:61
I think this is, strictly speaking, true, and we cannot sensibly/correctly use a threshold of covenant conditions! Miniscript saves us again.
This looks good, though I realize I was confused -- I thought all the covenant fragments were VERIFY
conditions and actually couldn't be dissatisfied even by modifying the transaction.
But they're not so, the situation is more subtle...
safe
). IOW the assertion is actually wrong and you were initially correct to remove it.Having said that, I'm a bit scared to remove this assertion because it reflects an underlying assumption about Miniscript that we're eliminating...I wonder if we should make all the covenant requirements verify-only and force the user to use a d:
wrapper? But then we're forcing inefficiency.
I wonder if we should make all the covenant requirements verify-only and force the user to use a d: wrapper?
This is what I am leaning towards.
Cool. It would be fine for us to later expand the language if it turned out that we need non-VERIFY
covenants in a lot of contexts, so if VERIFY
-only lets us simplify our reasoning, let's do that for now.
I'm a bit scared to remove this assertion because it reflects an underlying assumption about Miniscript that we're eliminating..
The assertion is re-enabled in this PR.
I wonder if we should make all the covenant requirements verify-only and force the user to use a d: wrapper?
Upon more investigation, there is some work in changing the miniscripts as it would require editing all the tests. IMO, the current fix with assertion enabled should be fine for now.
Rethink about correctness/malleability properites in covenants. See the commit description