Renable assertion

[sanket1729]

Rethink about correctness/malleability properites in covenants. See the commit description

[apoelstra]

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.

[apoelstra]

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

• Any introspection of the current input cannot be dissatisfied under any circumstances, without changing the input (but then we're executing a totally different miniscript :))
• Any introspection of other inputs, or outputs, can be dissatisfied but are only malleable if the relevant data is unsigned (i.e. the fragment is non-safe). IOW the assertion is actually wrong and you were initially correct to remove it.
• It is actually totally fine to have a threshold of covenant fragments

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.

[apoelstra]

ACK 61c2f3a

[sanket1729]

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.

[apoelstra]

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.

[sanket1729]

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.