feat: check policy validity to prevent duplicate check

[ChrisCho-H]

• Miniscript::from_ast() only executes check_global_consensus_validity() which does not include check_global_policy_validity(). Change to use check_global_validity() to check both consensus and policy validness.
• wrap_into_miniscript() executes check_global_validity() internally after Miniscript::from_ast(). As Miniscript::from_ast() now executes check_global_validity(), it doesn't have to check duplicately.

I was originally thinking to change check_global_validity() inside wrap_into_miniscript() to check_global_policy_validity()(as consensus check is done by from_ast()). I think it's better to strengthen the validness check in from_ast(), and do nothing in wrap_into_miniscript().

[sanket1729]

The original code was intentional. One of the initials goals of the library was to enable the creation of Miniscripts that are valid according to consensus rules, though not necessarily policy-valid. Initially, we envisioned this feature for use cases such as block explorers displaying inferred Miniscripts. Which is why from_ast does not deliberately check policy rules, but from_string and compile do.

Perhaps it’s time to reconsider whether we should continue supporting non-standard but consensus-valid Miniscripts. We could simplify by limiting support to policy-valid Miniscripts only. wdyt @apoelstra?

[sanket1729]

Actually, looking at this again, we have Miniscript::from_components_unchecked that allows for creating miniscripts that are not policy valid. And maybe parsing policy non-valid should not have first class support anyways.

[sanket1729]

ACK 4fe88da.

[sanket1729]

Thank you for the detailed review. It's a not bug fix that we need to backport, it’s a valuable code cleanup that consolidates scattered checks into the constructor.

[ChrisCho-H]

Thanks for the detailed review! I wasn't not sure whether it's intentional. I've changed just commit message from fix: ... to feat: ... for the clear record.

[apoelstra]

Sure, if you think this is an improvement @sanket1729 I will review this and merge it.

But in general I want to overhaul all the validation logic (which not only includes standardness vs non-standardness but also "sane" vs "insane"), which currently is spread across the codebase in adhoc and inconsistent ways. I have #723 (comment) about this plan and have made a substantial amount of progress toward it.

But in order to do this I needed to clean up some error returns, which required I eliminate some recursion throughout the library, which is currently blocked on #745 (which is a pretty minor PR, but it's in my working branch and I use it heavily to guide my un-recursion work, so I'd like it to be merged before PR'ing more stuff).

[apoelstra]

Ok, looking at my current working branch, I have about 40 commits queued up and none of them are in conflict with this (one completely rewrites the Miniscript string parser but this part of the logic is essentially unchanged, so I'll need to rebase but it's not like this is unnecessary/wasted work).

So let's pull this in.

[apoelstra]

ACK 757daee; successfully ran local tests

[sanket1729]

which is currently blocked on #745 (which is a pretty minor PR, but it's in my working branch and I use it heavily to guide my un-recursion work, so I'd like it to be merged before PR'ing more stuff).

Sorry, I missed this and it drowned in notifications. ACKed this PR now. Excited for the cleanups. We can holdoff merging this if you want to try to opening a PR with the other changes.

[apoelstra]

No worries! I've been pretty busy with Simplicity and hadn't had time to work on this anyway, or I would've pinged you.

And I merged this one -- will rebase my work on it, changing the local checks to global ones in my refactor.