Cleanly separate experimental/insane miniscripts from sane miniscripts #461

sanket1729 · 2022-09-19T07:46:35Z

Cleanly separates how we deal with insane and unspecified miniscripts in the library. Insane minsicripts are well-defined that could

It is unsafe(does not require a digital signature to spend it)
It contains a unspendable path because of either a) Resource limitations or b) Timelock Mixing
The script has malleable satisfaction
It has repeated public keys

There are some unspecified features like PartialDescriptors that we use to allow parsing miniscripts from scripts or inferring descriptors. For this only includes

Parsing rawpkh fragments with hash160

All of these can be individually enabled or disabled using the ExtParams builder.

As a consequence of this, we removed the Miniscript::Hash associated type and change the Semantic policy to operate only on keys.

Also fixes another issue in the usage of PkTranslator that made it impossible to implement the trait in downstream libraries

apoelstra · 2022-09-20T14:14:13Z

src/pub_macros.rs

+///
+/// See also [`crate::translate_assoc_fail`]
+#[macro_export]
+macro_rules! translate_assoc_clone {


What if we instead made this translate_assoc_into and called .into() rather than .clone()? This would work for all the hash types since they are Copy, and would potentially work more generally.

Also I think both these macros should somehow be renamed to indcate that they only implement the translate functions for hashes, not for pubkeys.

Agreed about the renaming. I was also thinking of another refactor that removes translate_pkh altogether. Now that we have Pkh(Pk) instead of Pkh(Pk::Hash), I don't think it is needed. We should not support any translation for abstract RawPkh until we have raw descriptors specified.

Yeah, agreed, let's drop translate_pkh.

sanket1729 · 2022-09-21T21:10:59Z

@apoelstra, this turned into a significantly bigger refactor than expected. We removed the Pk::Hash type altogether. It seems necessary to have abstract generic types over RawPkH types when they are not even specified yet. Users should almost never be dealing with RawTypes and lifting to raw types fails(for now). For interpreter descriptor inference purposes, we print an expr_raw_pkh(40-byte-hex) to display PkH fragments.

apoelstra · 2022-09-26T13:29:06Z

Reviewed everything except 844382f (the big one). I have a conceptual question: are you sure we shouldn't be able to parse rawpkh using from_str? Do we gain anything by not supporting this?

What if we parse a Miniscript from a Script, it contains rawpkh, and we try to print that as a string? I think it should succeed, and we should be able to parse the result.

I'd be fine if we still dropped the RawPkHash associated type and forced pkhashes to be hash160s, so I think the diff will be substantially the same, but I think there's value in having a string representation of rawpkh, if only so that you can pass "readable" renderings of existing scripts around.

As a future improvement (not in this PR) I'd also like to extend the Lift trait somehow so that you can combine lifting with filtering, and thereby avoid errors. Like, if you had the LIquid script, which you'd just parsed off the blockchain so it has rawpkhes in it, you could do lift_at_age(0) and it'd still confirm for you that it looks like a multisig plus maybe some timelock stuff. Maybe we should have a dumb Unanalyzable node which rawpkh (and eventually covenants, taproot hidden nodes, etc) maps to, which causes any analysis to error out if it's hit ... I dunno.

Create macros for writing Translator functions The trait has a general implementation of Translator like `impl Translator for PkTranslator where P::Sha256 ...`. However, this blanket implementation has constraints on associated types and makes it impossible to implement the trait for a generic type downstream. Rust compiler does not support trait specialization yet, so we should only provide a macro to ease implementation rather than a blanket implementation that causes duplicate conflicts downstream

apoelstra · 2022-10-03T01:32:01Z

src/miniscript/analyzable.rs

+use crate::{Miniscript, MiniscriptKey, ScriptContext, Terminal};
+
+/// Params for parsing miniscripts that either non-sane or non-specified(experimental) in the spec.
+/// Used as a parameter [`Miniscript::from_str_insane`] and [`Miniscript::parse_insane`].


In 89169c2:

This comment is out of date since you renamed these two methods :)

apoelstra · 2022-10-03T01:34:00Z

src/miniscript/analyzable.rs

+    /// Allow parsing of miniscripts with raw pkh fragments without the pk.
+    /// This could be obtained when parsing miniscript from script
+    pub raw_pkh: bool,
+}


I really love this struct. It's an easy reference place for basically everything weird about Miniscript :)

apoelstra · 2022-10-03T01:35:54Z

src/miniscript/analyzable.rs

+    }
+
+    /// Enable all non-sane rules and experimental rules
+    pub fn all() -> ExtParams {


In 89169c2:

Can you rename this allow_all or (somewhat snarkily) free_for_all?

apoelstra · 2022-10-03T01:37:14Z

src/miniscript/mod.rs

@@ -156,6 +157,20 @@ impl<Ctx: ScriptContext> Miniscript<Ctx::Key, Ctx> {
    /// insane scripts. In general, in a multi-party setting users should only
    /// accept sane scripts.
    pub fn parse_insane(script: &script::Script) -> Result<Miniscript<Ctx::Key, Ctx>, Error> {


In 89169c2:

I think we should deprecate this and tell users to use parse_with_ext with ExtParams::all (or whatever we call that). What do you think?

There is a small difference between insane and ext (experimental stuff). Insane things are well documented and specified in spec, but disallowed for user safety. The raw_pkh stuff is not specified anywhere yet.

I think having an insane high level API is still valuable short-hand for parse_with_ext(ExtParams::insane()) which is different from parse_with_ext(ExtParams::all())

Ok, cool, that's fair. I didn't realize that "insane" covered well-defined things outside of this library.

apoelstra · 2022-10-03T01:38:37Z

Done reviewing 89169c2. Only nits. This is a huge improvement!

I guess you should update the PR descriptor and maybe the title since so much of this PR involves introducing ExtParams.

sanket1729 · 2022-10-07T22:49:23Z

Force-pushed and updated the description.

apoelstra · 2022-10-08T14:06:55Z

Running `target/debug/examples/htlc`
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `"or(and(pk(022222222222222222222222222222222222222222222222222222222222222222),sha256(1111111111111111111111111111111111111111111111111111111111111111)),and(pk(020202020202020202020202020202020202020202020202020202020202020202),older(4444)))"`,
 right: `"or(and(pkh(4377a5acd66dc5cb67148a24818d1e51fa183bd2),sha256(1111111111111111111111111111111111111111111111111111111111111111)),and(pkh(51814f108670aced2d77c1805ddd6634bc9d4731),older(4444)))"`', examples/htlc.rs:49:5

Can you do a rebase -x and check that the htlc example passes on every commit?

Otherwise ack b72f5b1

This is one of the major breaking changes touching the parsing/display and logic of semantic policies. This should only affect the application/analysis layer of things, so it is okay to break somethings here for code clarity. My understanding is that will also help the simplicity bridge

Cleanup Translator trait by removing the translate_pkh method. RawPkh is not parsed from_str. It is only used in miniscript internally to parse scripts.

We no longer need to maintain mappings from hash160::Hash as Pkh now stores the Pk instead of Pk::Hash

Standard/Sane scripts refer to miniscripts that are described in the spec. Insane scripts refer to miniscript that are valid but can contain timelock mixes, not all paths would require signature, etc. Experimental features with ext flag current only allow parsing hash160 expr_raw_pkh scripts from string and bitcoin::Script. All regular APIs fail while parsing scripts with raw_pkh. The interpreter module relies on partial information from the blockchian without the descriptor. All the parsing in the interpreter module in done with all exprimental features. Similarly, our psbt finalizer also uses the expr features as it relies on inferring the descriptor from witness script and then puts things into the correct place.

sanket1729 · 2022-10-08T14:29:40Z

Done and force pushed. Awaiting CI

apoelstra

ACK 6a06a11

3f451db Release 8.0.0 (sanket1729) Pull request description: Draft release PR. Still blocked on reviews of 1) #418 2) #450 3) #461 ACKs for top commit: apoelstra: ACK 3f451db Tree-SHA512: aa455fd57a894b1edb49b6273126a05c9d0897ca50ee572fa8a59b4d2a96e2013f2ed9ec4b96cd5e449ba5b5ee6bd32aa57da110df69bd35b8a970ccb1440002

…022_10 d5615ac Merge rust-bitcoin/rust-miniscript#461: Cleanly separate experimental/insane miniscripts from sane miniscripts I will revisit how to combine ExtParams API with Extensions later. Right now, we only make the things compile and test

sanket1729 added this to the 8.0.0 milestone Sep 19, 2022

sanket1729 mentioned this pull request Sep 19, 2022

Release 8.0.0 #462

Merged

apoelstra reviewed Sep 20, 2022

View reviewed changes

sanket1729 force-pushed the rework_translator branch 3 times, most recently from c31de7f to c664ff0 Compare September 21, 2022 21:10

sanket1729 force-pushed the rework_translator branch 2 times, most recently from 13c655c to 844382f Compare September 23, 2022 00:24

apoelstra mentioned this pull request Sep 26, 2022

Fix inconsistent naming in the dummy key hash structs #463

Merged

sanket1729 force-pushed the rework_translator branch from 844382f to 89169c2 Compare October 3, 2022 00:23

apoelstra reviewed Oct 3, 2022

View reviewed changes

sanket1729 force-pushed the rework_translator branch from 89169c2 to b72f5b1 Compare October 7, 2022 22:40

sanket1729 changed the title ~~Remove PkTranslator trait~~ Cleanly separate experimental/insane miniscripts from sane miniscripts Oct 7, 2022

sanket1729 added 5 commits October 8, 2022 07:26

Rename the Semantic::translate_pkh to translate_pk

328d15f

Remove Hash associated type from RawPkh.

fff727c

Cleanup Translator trait by removing the translate_pkh method. RawPkh is not parsed from_str. It is only used in miniscript internally to parse scripts.

Cleanup psbt translator APIs

94d5eba

We no longer need to maintain mappings from hash160::Hash as Pkh now stores the Pk instead of Pk::Hash

sanket1729 force-pushed the rework_translator branch from b72f5b1 to 6a06a11 Compare October 8, 2022 14:29

apoelstra approved these changes Oct 8, 2022

View reviewed changes

apoelstra merged commit d5615ac into rust-bitcoin:master Oct 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanly separate experimental/insane miniscripts from sane miniscripts #461

Cleanly separate experimental/insane miniscripts from sane miniscripts #461

sanket1729 commented Sep 19, 2022 •

edited

Loading

apoelstra Sep 20, 2022

sanket1729 Sep 20, 2022 •

edited

Loading

apoelstra Sep 20, 2022

sanket1729 commented Sep 21, 2022

apoelstra commented Sep 26, 2022 •

edited

Loading

apoelstra Oct 3, 2022

apoelstra Oct 3, 2022

apoelstra Oct 3, 2022

apoelstra Oct 3, 2022

sanket1729 Oct 5, 2022

sanket1729 Oct 5, 2022

apoelstra Oct 5, 2022

apoelstra commented Oct 3, 2022

sanket1729 commented Oct 7, 2022

apoelstra commented Oct 8, 2022

sanket1729 commented Oct 8, 2022

apoelstra left a comment

Cleanly separate experimental/insane miniscripts from sane miniscripts #461

Cleanly separate experimental/insane miniscripts from sane miniscripts #461

Conversation

sanket1729 commented Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

sanket1729 Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 commented Sep 21, 2022

apoelstra commented Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Oct 3, 2022

sanket1729 commented Oct 7, 2022

apoelstra commented Oct 8, 2022

sanket1729 commented Oct 8, 2022

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Sep 19, 2022 •

edited

Loading

sanket1729 Sep 20, 2022 •

edited

Loading

apoelstra commented Sep 26, 2022 •

edited

Loading