Refactor `PkH` fragment to support `Pk` #431

SarcasticNastik · 2022-06-09T17:52:19Z

As suggested in #427, this refactor helps to incorporate musig in the codebase.

SarcasticNastik · 2022-06-09T18:09:40Z

src/policy/compiler.rs

@@ -840,7 +840,14 @@ where
            insert_wrap!(AstElemExt::terminal(Terminal::True));
        }
        Concrete::Key(ref pk) => {
-            insert_wrap!(AstElemExt::terminal(Terminal::PkH(pk.to_pubkeyhash())));
+            insert_wrap!(AstElemExt::terminal(Terminal::PkH(


Ideally this insert_wrap should have been avoided (since we know Concrete::Key during compilation), but we have a change in internal representation for Terminal::PkH (found when one of tests fail in absence of this statement). As I understand, there can be 2 solutions:

Implement PartialEq for Terminal::PkH to only check equality in the field Pk::Hash. We can still access the Pk entry while having proper checks. Also, since we might require disambiguating a Terminal::PkH based on availability of Pk, Eq can check total equality by considering the first field too.

Keep both the compilations. Although I think this might give rise to ambiguity in the compilation and parsing miniscript from string can be an issue (internal representation of Terminal::PkH can't be inferred from a string representation as of now).

I'm open to any other suggestions too!

sanket1729 · 2022-06-09T18:35:47Z

@apoelstra , thinking about your comment here: bitcoin/bitcoin#24114 (comment). I am thinking it would make more sense to change Pkh(Pk::Hash) to Pk and add a new Terminal::RawPkh(Pk::Hash), just for parsing.

FromStr would not parse these. (until rawpkh is decided to supported upstream)
Compiler will always Pkh(Pk)
Parsing from the script would create a successful miniscript, but we will mark it insane. Meaning that it cannot be used in descriptors.

apoelstra · 2022-06-09T19:35:05Z

I like it. We could then also change semantic::Policy to have real public keys rather than hashes, which would simplify Simplicity interop.

sanket1729 · 2022-06-10T00:12:33Z

@SarcasticNastik, we need to modify this as follows:

Change Pkh(Pk::Hash) to Pkh(Pk)
Add a RawPkh node RawPkh(Pk::Hash) as commented

sanket1729

Will do another round tomorrow

src/interpreter/mod.rs

sanket1729 · 2022-06-13T10:19:17Z

src/miniscript/astelem.rs

@@ -86,7 +86,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
    {
        match *self {
            Terminal::PkK(ref p) => pred(ForEach(p)),
-            Terminal::PkH(ref p) => todo!("KeyHash should contain Pk"),
+            Terminal::PkH(ref p) => pred(ForEach(p)),
+            Terminal::RawPkH(ref p) => todo!("Should we require to iterate over the Hashes too?"),


We just ignore these. As these are not known keys.

src/miniscript/astelem.rs

sanket1729 · 2022-06-13T10:23:01Z

src/miniscript/astelem.rs

@@ -372,9 +376,12 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> fmt::Display for Terminal<Pk, Ctx> {
                        if let Terminal::PkK(ref pk) = sub.node {
                            // alias: pk(K) = c:pk_k(K)
                            return write!(f, "pk({})", pk);
-                        } else if let Terminal::PkH(ref pkh) = sub.node {
+                        } else if let Terminal::RawPkH(ref pkh) = sub.node {


We can print this. But we should note in comments that RawPkH is currently unsupported in descriptor spec.

src/miniscript/astelem.rs

src/miniscript/decode.rs

src/policy/compiler.rs

src/miniscript/types/extra_props.rs

sanket1729

Mostly ACK, but the history is unclean. Can you remove the previous commits?

We can do the updating the miniscript tests stuff in a follow up PR.

sanket1729 · 2022-06-16T01:46:29Z

src/miniscript/mod.rs

-        ms_attributes_test("c:andor(ripemd160(6ad07d21fd5dfc646f0b30577045ce201616b9ba),pk_h(9fc5dbe5efdce10374a4dd4053c93af540211718),and_v(v:hash256(8a35d9ca92a48eaade6f53a64985e9e2afeb74dcf8acb4c3721e0dc7e4294b25),pk_h(dd100be7d9aea5721158ebde6d6a1fd8fff93bb1)))", "82012088a6146ad07d21fd5dfc646f0b30577045ce201616b9ba876482012088aa208a35d9ca92a48eaade6f53a64985e9e2afeb74dcf8acb4c3721e0dc7e4294b258876a914dd100be7d9aea5721158ebde6d6a1fd8fff93bb1886776a9149fc5dbe5efdce10374a4dd4053c93af5402117188868ac", true, false, true, 18, 3);
-        ms_attributes_test("c:andor(u:ripemd160(6ad07d21fd5dfc646f0b30577045ce201616b9ba),pk_h(20d637c1a6404d2227f3561fdbaff5a680dba648),or_i(pk_h(9652d86bedf43ad264362e6e6eba6eb764508127),pk_h(751e76e8199196d454941c45d1b3a323f1433bd6)))", "6382012088a6146ad07d21fd5dfc646f0b30577045ce201616b9ba87670068646376a9149652d86bedf43ad264362e6e6eba6eb764508127886776a914751e76e8199196d454941c45d1b3a323f1433bd688686776a91420d637c1a6404d2227f3561fdbaff5a680dba6488868ac", true, false, true, 23, 4);
-        ms_attributes_test("c:or_i(andor(c:pk_h(fcd35ddacad9f2d5be5e464639441c6065e6955d),pk_h(9652d86bedf43ad264362e6e6eba6eb764508127),pk_h(06afd46bcdfd22ef94ac122aa11f241244a37ecc)),pk_k(02d7924d4f7d43ea965a465ae3095ff41131e5946f3c85f79e44adbcf8e27e080e))", "6376a914fcd35ddacad9f2d5be5e464639441c6065e6955d88ac6476a91406afd46bcdfd22ef94ac122aa11f241244a37ecc886776a9149652d86bedf43ad264362e6e6eba6eb7645081278868672102d7924d4f7d43ea965a465ae3095ff41131e5946f3c85f79e44adbcf8e27e080e68ac", true, true, true, 17, 5);
+        ms_attributes_test("c:or_i(and_v(v:older(16),pk_h(03daed4f2be3a8bf278e70132fb0beb7522f570e144bf615c07e996d443dee8729)),pk_h(02352bbf4a4cdd12564f93fa332ce333301d9ad40271f8107181340aef25be59d5))", "6360b26976a91420d637c1a6404d2227f3561fdbaff5a680dba648886776a9148f9dff39a81ee4abcbad2ad8bafff090415a2be88868ac", true, true, true, 12, 3);


This is okay for this PR. As a followup, we should be using the same test vectors from https://github.com/sipa/miniscript/blob/fa7ea77b19d76738addce71430cb8ff50ee846b9/bitcoin/test/miniscript_tests.cpp#L461-L475 .

Now, we don't have to manually compute the hashes here

sanket1729 · 2022-06-16T02:01:09Z

src/lib.rs

-    Hash(&'a Pk::Hash),
-}
+/// Either a key or keyhash, but both contain Pk
+pub struct ForEach<'a, Pk: MiniscriptKey>(&'a Pk);


Note to reviewers. We deliberately do not iterate over RawPkH here. In a follow-up PR, we will make Miniscript::from_ast fail when we reach a RawPkh terminal.

We can update this whenever partial descriptors are supported upstream

In 09299b0:

Shouldn't we just drop the ForEach type entirely here?

I remember @sanket1729 and I discussed about removing this later on, but I don't see any reason why I can't do it here. I'm pushing another commit on top of 5cb2bcf to remove ForEach. If accepted, I can squash the commit anytime!

sanket1729 · 2022-06-16T02:10:45Z

Follow things that we can do after this (I will do some of them):

Make the sanity_checks fail when Terminal::RawPkH is used. Partial descriptors are not specified yet and are only used internally in the codebase.
- Make sure scripts when parsed from blockchain are correctly parsed using the RawPkH terminal. The interpreter should be able to produce partial descriptors. This might need some revisiting of things that we did in the PR. We can get a better idea as we started here
- Make sure that the user cannot create RawPkH descriptors directly. 1) Compiler 2) String or 3) Script. I think we are able to do this in this PR with correct usage of sanity_checks.
Update the translation APIs to remove the pkh altogether. The translation stuff is really useful when dealing with descriptors and we do not want users to reason about rawpkh as it contains information that they don't know about.
Change the semantic Policy Pk type from Key::Hash to Key
Clean up the iterator API so that everything is covered under pk.

Remove lifetime parameter concerning `PkH(Pk::Hash)` to `PkH(Pk)` refactor.

This change allows us to allow to refactor `Terminal::PkH(Pk::Hash)` to `Terminal::PkH(Pk)`.

sanket1729 · 2022-06-16T16:04:15Z

@apoelstra, can you have a high-level look at this? I would like to merge this first amongst all the other things that would be breaking first.

src/miniscript/decode.rs

src/miniscript/types/mod.rs

apoelstra · 2022-06-17T15:07:49Z

src/policy/mod.rs

-            Terminal::PkK(ref pk) => Semantic::KeyHash(pk.to_pubkeyhash()),
-            Terminal::PkH(ref pkh) => Semantic::KeyHash(pkh.clone()),
+            Terminal::PkK(ref pk) | Terminal::PkH(ref pk) => Semantic::KeyHash(pk.to_pubkeyhash()),
+            Terminal::RawPkH(ref pkh) => Semantic::KeyHash(pkh.clone()),


In 36ff0d2:

I guess this is for a future PR, but I'd kinda like to make semantic::Policy have only Pks, not Pk::Hashes, and then refuse to lift any scripts that had RawPkH in it. We could provide a transformer that took a Miniscript<Pk> to a Miniscript<Pk::Hash> maybe, converting RawPkHs to PkHs along the way, to get the old behavior.

Yup, the future TODOs are listed #431 (comment)

apoelstra · 2022-06-17T15:25:55Z

ack 36ff0d2 aside from the comments i made

The test-cases previously containing 40-byte hash for `pk_h` fragment have been replaced with 66-byte pubkey.

Since we disregard any `Pk::Hash` as a valid iterable key, only `Pk` is considered as such. The refactor reflects directly iterating over `Pk` (a valid Miniscript Key).

apoelstra

ACK 8d0cf06

sanket1729

ACK 8d0cf06. The last commit message should be Remove ForEach struct instead of Remove ForEach trait. But won't holdoff this over that.

Thanks for keeping with the reviews

SarcasticNastik commented Jun 9, 2022

View reviewed changes

SarcasticNastik force-pushed the refactor/pk-hash branch 2 times, most recently from 4d63856 to e322327 Compare June 10, 2022 12:00

SarcasticNastik changed the title ~~Refactor PkH(Pk::Hash) -> PkH(Option<Pk>, Pk::Hash)~~ Refactor PkH fragment to support Pk Jun 10, 2022

SarcasticNastik force-pushed the refactor/pk-hash branch 3 times, most recently from 444b1fb to f170aaa Compare June 13, 2022 10:20

sanket1729 reviewed Jun 13, 2022

View reviewed changes

SarcasticNastik force-pushed the refactor/pk-hash branch 4 times, most recently from a3d2b36 to eb5b3d1 Compare June 15, 2022 23:57

SarcasticNastik marked this pull request as ready for review June 15, 2022 23:58

sanket1729 reviewed Jun 16, 2022

View reviewed changes

sanket1729 and others added 3 commits June 16, 2022 12:09

Fix formatter configuration errors with latest nightly

ca16bd8

Remove un-necessary lifetimes

701c9cc

Remove lifetime parameter concerning `PkH(Pk::Hash)` to `PkH(Pk)` refactor.

Refactor ForEach to contain only Pk.

09299b0

This change allows us to allow to refactor `Terminal::PkH(Pk::Hash)` to `Terminal::PkH(Pk)`.

SarcasticNastik force-pushed the refactor/pk-hash branch from eb5b3d1 to 36ff0d2 Compare June 16, 2022 06:39

sanket1729 mentioned this pull request Jun 17, 2022

Add Hash256 associated type #434

Merged

apoelstra reviewed Jun 17, 2022

View reviewed changes

src/miniscript/decode.rs Show resolved Hide resolved

apoelstra reviewed Jun 17, 2022

View reviewed changes

src/miniscript/types/mod.rs Outdated Show resolved Hide resolved

apoelstra reviewed Jun 17, 2022

View reviewed changes

Refactor PkH to include key.

5cb2bcf

The test-cases previously containing 40-byte hash for `pk_h` fragment have been replaced with 66-byte pubkey.

SarcasticNastik force-pushed the refactor/pk-hash branch from 36ff0d2 to 5cb2bcf Compare June 17, 2022 16:32

Remove ForEach Trait

8d0cf06

Since we disregard any `Pk::Hash` as a valid iterable key, only `Pk` is considered as such. The refactor reflects directly iterating over `Pk` (a valid Miniscript Key).

apoelstra approved these changes Jun 17, 2022

View reviewed changes

sanket1729 approved these changes Jun 17, 2022

View reviewed changes

sanket1729 merged commit 859534b into rust-bitcoin:master Jun 17, 2022

SarcasticNastik deleted the refactor/pk-hash branch June 17, 2022 21:13

apoelstra mentioned this pull request Oct 27, 2022

Add the "planning" module #481

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `PkH` fragment to support `Pk` #431

Refactor `PkH` fragment to support `Pk` #431

SarcasticNastik commented Jun 9, 2022

SarcasticNastik Jun 9, 2022 •

edited

Loading

sanket1729 commented Jun 9, 2022

apoelstra commented Jun 9, 2022

sanket1729 commented Jun 10, 2022

sanket1729 left a comment

sanket1729 Jun 13, 2022

sanket1729 Jun 13, 2022

sanket1729 left a comment

sanket1729 Jun 16, 2022

sanket1729 Jun 16, 2022

apoelstra Jun 17, 2022

SarcasticNastik Jun 17, 2022 •

edited

Loading

sanket1729 commented Jun 16, 2022

sanket1729 commented Jun 16, 2022

apoelstra Jun 17, 2022

sanket1729 Jun 17, 2022

apoelstra commented Jun 17, 2022

apoelstra left a comment

sanket1729 left a comment

Refactor PkH fragment to support Pk #431

Refactor PkH fragment to support Pk #431

Conversation

SarcasticNastik commented Jun 9, 2022

SarcasticNastik Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

sanket1729 commented Jun 9, 2022

apoelstra commented Jun 9, 2022

sanket1729 commented Jun 10, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 Jun 13, 2022

Choose a reason for hiding this comment

sanket1729 Jun 13, 2022

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 Jun 16, 2022

Choose a reason for hiding this comment

sanket1729 Jun 16, 2022

Choose a reason for hiding this comment

apoelstra Jun 17, 2022

Choose a reason for hiding this comment

SarcasticNastik Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

sanket1729 commented Jun 16, 2022

sanket1729 commented Jun 16, 2022

apoelstra Jun 17, 2022

Choose a reason for hiding this comment

sanket1729 Jun 17, 2022

Choose a reason for hiding this comment

apoelstra commented Jun 17, 2022

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

Refactor `PkH` fragment to support `Pk` #431

Refactor `PkH` fragment to support `Pk` #431

SarcasticNastik Jun 9, 2022 •

edited

Loading

SarcasticNastik Jun 17, 2022 •

edited

Loading