Add initial support for KeyExpr #443

kanishk779 · 2022-07-01T18:32:42Z

As of now, miniscript does not have support for handling Multi-signature keys. The changes introduced in this PR would be the first steps for supporting such keys.

The idea is that we will have support for legacy keys SingleKey(Pk) and Musig Keys Musig(vec<KeyExpr>)

sanket1729

Left some initial feedback apart from the no-std failures.

sanket1729 · 2022-07-01T19:24:39Z

src/miniscript/musig_key.rs

+
+#[derive(Debug, Clone, PartialEq, PartialOrd, Ord, Eq, Hash)]
+/// Enum for representing keys in miniscript
+pub enum KeyExpr<Pk: MiniscriptKey> {


Can you manually implement Debug?

Shouldn't the output of debug be same as display? If not, what should be the format while debugging ?
For example if the key is musig(A,musig(B,C),musig(D)) what should debug print for this ?

sanket1729 · 2022-07-01T19:24:58Z

src/miniscript/musig_key.rs

+use crate::MiniscriptKey;
+
+#[derive(Debug, Clone, PartialEq, PartialOrd, Ord, Eq, Hash)]
+/// Enum for representing keys in miniscript


*representing musig keys

sanket1729 · 2022-07-01T19:25:27Z

src/miniscript/musig_key.rs

+    /// Single-key (e.g pk(a), here 'a' is a single key)
+    SingleKey(Pk),
+
+    /// Collection of keys in used for multi-signature


musig signature. Don't use the term multi-signature here as it refers OP_CHECKMULSIG based constructions

sanket1729 · 2022-07-01T19:26:40Z

src/miniscript/musig_key.rs

+    type Err = KeyExprError;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let (key_tree, _) = Tree::from_slice(s).unwrap();
+        fn expr_from_tree<Pk: MiniscriptKey + FromStr>(


You should

impl FromTree for KeyExpr instead of this local function

Implemented FromTreef for KeyExpr.

sanket1729 · 2022-07-01T19:27:16Z

src/miniscript/musig_key.rs

+            tree: &Tree,
+        ) -> Result<KeyExpr<Pk>, KeyExprError> {
+            if tree.name == "musig" {
+                let mut key_expr_vect = vec![];


bit: use _vec instead of '_vect'

sanket1729 · 2022-07-01T19:27:46Z

src/miniscript/musig_key.rs

+                }
+                Ok(KeyExpr::MuSig(key_expr_vect))
+            } else {
+                if tree.name != "" {


Empty is also a valid string. There is no reason to reject it.

Ok, removed the if condition.

sanket1729 · 2022-07-01T19:28:05Z

src/miniscript/musig_key.rs

+                Ok(KeyExpr::MuSig(key_expr_vect))
+            } else {
+                if tree.name != "" {
+                    let single_key = match Pk::from_str(tree.name) {


use map_err instead of match

sanket1729 · 2022-07-01T19:29:56Z

src/miniscript/musig_key.rs

+
+    #[test]
+    fn test_one() {
+        let dummy_key = "musig(A,B,musig(C,musig(D,E)))";


The test case "musig(,,)" should also work. There should be no special rule to rule out empty strings

Yes now "musig(,,)" also passes the test.

sanket1729 · 2022-07-01T19:30:25Z

src/miniscript/musig_key.rs

+
+    #[test]
+    fn test_one() {
+        let dummy_key = "musig(A,B,musig(C,musig(D,E)))";


nit: name this musig_key instead of dummy_key

sanket1729 · 2022-07-01T19:31:00Z

src/miniscript/musig_key.rs

+        let dummy_key = "musig(A,B,musig(C,musig(D,E)))";
+        let pk = KeyExpr::<String>::from_str(dummy_key).unwrap();
+        println!("{}", pk);
+        assert_eq!(dummy_key, format!("{}", pk))


can you also more test cases here? Testing single key only. Musig with a single key.

Added some more test cases.

SarcasticNastik · 2022-07-07T19:12:00Z

src/miniscript/musig_key.rs

+    fn next(&mut self) -> Option<Self::Item> {
+        while !self.stack.is_empty() {
+            let last = self.stack.pop().expect("Size checked above");
+            match &*last {


nit: Since we don't have any overloaded Deref for KeyExpr, ig it's safe to just match it as:

Suggested change

match &*last {

match last {

Ok, changed accordingly.

Suggested change

match &*last {

match last {

SarcasticNastik · 2022-07-07T19:13:49Z

src/miniscript/musig_key.rs

+                KeyExpr::MuSig(key_vec) => {
+                    // push the elements in reverse order
+                    for key in key_vec.iter().rev() {
+                        self.stack.push(key)
+                    }
+                }


nit: Since for loops themselves are implemented using iterators, there shouldn't much difference in performance.

Suggested change

KeyExpr::MuSig(key_vec) => {

// push the elements in reverse order

for key in key_vec.iter().rev() {

self.stack.push(key)

}

}

KeyExpr::MuSig(key_vec) => key_vec.iter().rev().for_each(|key| self.stack.push(key));

SarcasticNastik · 2022-07-07T19:16:19Z

src/miniscript/musig_key.rs

+}
+
+#[derive(Debug, Clone)]
+/// Iterator for keyexpr


You can add linkable references in docstrings, ex:- for KeyExpr here. Check this out.

thanks! I didn't know that.
Fixed it now.

SarcasticNastik · 2022-07-07T19:18:40Z

src/miniscript/musig_key.rs

+impl<Pk: MiniscriptKey + FromStr> FromStr for KeyExpr<Pk> {
+    type Err = Error;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let (key_tree, _) = Tree::from_slice(s).unwrap();


I think we should propagate the error rather than panicking here. From what little I understand, we only use unwrap in cases we're sure errors shouldn't occur. In this case, s might be a malformed string and should be handled by the calling function itself.

Yes, this seems the right way of handling the error in this case.
Done.

SarcasticNastik · 2022-07-07T19:41:23Z

src/miniscript/musig_key.rs

+            let mut key_expr_vec = vec![];
+            for sub_tree in tree.args.iter() {
+                let temp_res = KeyExpr::<Pk>::from_tree(sub_tree)?;
+                key_expr_vec.push(temp_res);
+            }
+            Ok(KeyExpr::MuSig(key_expr_vec))


nit: Can be re-writtten as:

Suggested change

let mut key_expr_vec = vec![];

for sub_tree in tree.args.iter() {

let temp_res = KeyExpr::<Pk>::from_tree(sub_tree)?;

key_expr_vec.push(temp_res);

}

Ok(KeyExpr::MuSig(key_expr_vec))

let key_expr_vec = tree

.args

.iter()

.map(|subtree| KeyExpr::<Pk>::from_tree(subtree))

.collect::<Result<Vec<KeyExpr<Pk>>, Error>>()?;

Ok(KeyExpr::MuSig(key_expr_vec))

It's not the cleanest solution so really upto you if you wanna include this :p

Looks better and more readable. Changed according to the suggestion.

SarcasticNastik · 2022-07-07T19:43:27Z

src/miniscript/musig_key.rs

+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match *self {
+            KeyExpr::SingleKey(ref pk) => write!(f, "{}", pk),
+            KeyExpr::MuSig(ref my_vec) => {


I'm curious here as to why we want to print all the musig keys in Display rather than outputting the key-aggregation of the same.

I discussed this with @sanket1729 , and it seems that this is the right way for implementing display involving Musig. Because we want to show/print the individual keys and not the final aggregate key.

sanket1729

Partial review

sanket1729 · 2022-08-16T17:17:00Z

src/descriptor/bare.rs

@@ -219,7 +220,7 @@ impl<Pk: MiniscriptKey> Pkh<Pk> {
    /// sighash suffix. Includes the weight of the VarInts encoding the
    /// scriptSig and witness stack length.
    pub fn max_satisfaction_weight(&self) -> usize {
-        4 * (1 + 73 + BareCtx::pk_len(&self.pk))
+        4 * (1 + 73 + BareCtx::pk_len(&KeyExpr::SingleKey(self.pk.clone())))


Let's convert back pk_len to accept a &Pk so that we avoid the clone here

Let's add a test for Checking the len of 1) pk(uncompressedkey) in Legacy. 2) pk(compressed) in Segwitv0 3) pk(XonlyKey) in Tap and 4) pk('musig') in tap

I have posted what to change in the comment at the function definition in the context.rs file.

Revert back to &Pk.

Add a test check_script_size covering all the four suggested tests.

sanket1729 · 2022-08-16T17:22:52Z

src/interpreter/mod.rs

-                    let res = self.stack.evaluate_pk(&mut self.verify_sig, *pk);
+                    let res = match pk.single_key() {
+                        Some(pk) => self.stack.evaluate_pk(&mut self.verify_sig, *pk),
+                        None => None,


We are doing this is wrong. We should never encounter this because know the variant here is single_key. We should either use expect on single_key with a message stating "Musig keys cannot be parsed from Script"

Ok. fixed as suggested.

sanket1729 · 2022-08-16T17:23:39Z

src/interpreter/mod.rs

-                            .stack
-                            .evaluate_pk(&mut self.verify_sig, subs[node_state.n_evaluated])
-                        {
+                        let temp = match subs[node_state.n_evaluated].single_key() {


nit: Use a better name than temp. Same note about using expect here

sanket1729 · 2022-08-16T17:24:12Z

src/miniscript/astelem.rs

@@ -80,7 +81,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
        Pk::RawPkHash: 'a,
    {
        match *self {
-            Terminal::PkK(ref p) => pred(p),
+            Terminal::PkK(ref p) => p.for_each_key(pred),


Add a test for_each_key for a fragment containing musig.

Added a test for this test_for_each_key in miniscript/mod.rs

sanket1729 · 2022-08-16T17:25:44Z

src/miniscript/context.rs

@@ -332,7 +339,7 @@ where
    /// Note that this includes the serialization prefix. Returns
    /// 34/66 for Bare/Legacy based on key compressedness
    /// 34 for Segwitv0, 33 for Tap
-    fn pk_len<Pk: MiniscriptKey>(pk: &Pk) -> usize;
+    fn pk_len<Pk: MiniscriptKey>(pk: &KeyExpr<Pk>) -> usize;


As mentioned earlier, we should not change this.

But, we should add another method here with default implementation as

fn key_expr_len<Pk: MiniscriptKey>(pk: &KeyExpr) { match pk { SingleKey(pk) => Ctx::pk_len(&pk), Musig(expr) => 33, // not 32 as mentioned in another comment below } }

Ok, created the new method with the default implementation.

sanket1729 · 2022-08-16T17:31:23Z

src/miniscript/context.rs

+    fn pk_len<Pk: MiniscriptKey>(pk: &KeyExpr<Pk>) -> usize {
+        match pk {
+            KeyExpr::<Pk>::SingleKey(_) => 34,
+            KeyExpr::<Pk>::MuSig(_) => 32,


Here and everywhere, where we 32. It should actually be 33 as it also includes the push len prefix.

Done. changed everywhere.

sanket1729 · 2022-08-16T17:37:51Z

src/miniscript/context.rs

-            Terminal::PkK(ref pk) => {
-                if pk.is_uncompressed() {
+            Terminal::PkK(ref key) => {
+                if key.iter().all(|pk| !pk.is_uncompressed()) {


I know this is going to sound weird, but you would need to change to
key.iter().any(|pk| pk.is_uncompressed). Checking that some is not uncompressed is not the same as checking something is compressed.

Ok. changed as suggested.

sanket1729 · 2022-08-16T17:38:21Z

src/miniscript/context.rs

+            Terminal::MultiA(_, ref keys) => {
+                if keys
+                    .iter()
+                    .all(|keyexpr| keyexpr.iter().all(|pk| !pk.is_uncompressed()))


Same comment about compressed keys

sanket1729 · 2022-08-16T17:38:42Z

src/miniscript/context.rs

+    fn pk_len<Pk: MiniscriptKey>(pk: &KeyExpr<Pk>) -> usize {
+        match pk {
+            KeyExpr::<Pk>::SingleKey(_) => 33,
+            KeyExpr::<Pk>::MuSig(_) => 32,


The len should always be 33

changed everywhere.

sanket1729 · 2022-08-16T17:40:32Z

src/miniscript/context.rs

+                Terminal::PkK(key) => match key {
+                    KeyExpr::<Pk>::SingleKey(_pk) => Ok(()),
+                    KeyExpr::<Pk>::MuSig(_) => {
+                        return Err(Error::NonStandardBareScript);


You are returning an incorrect variant here

changed it to
Err(Error::ContextError(ScriptContextError::MusigNotAllowed(String::from(Self::name_str()))))

sanket1729

Almost there. Some last few edits.

sanket1729 · 2022-08-23T09:32:03Z

src/lib.rs

@@ -99,6 +99,9 @@ extern crate alloc;
 #[cfg(not(feature = "std"))]
 extern crate hashbrown;

+#[cfg(not(feature = "std"))]


Why this line?

I added this because the last CI check was failing. But it seems that this has nothing to do with it.
Removed it now

sanket1729 · 2022-08-23T09:42:43Z

src/miniscript/iter.rs

+                for key in keys {
+                    let temp: Vec<Pk::RawPkHash> =
+                        key.iter().map(|pk| pk.to_pubkeyhash()).collect();
+                    res.extend(temp)


Does extend take an iterator? We should not need to collect this separately and have a separate allocation for this.

Same comment about collect everywhere in leafpk, leafpkh

Done. I have removed the collect in leapk, leapkh, leapk_pkh.

sanket1729 · 2022-08-23T09:51:18Z

src/miniscript/iter.rs

            }
            _ => None,
        }
    }
 }
+/// Parent iter for all the below iters
+pub struct BaseIter<'a, Pk: MiniscriptKey, Ctx: ScriptContext> {


Does this need to be pub?

No, this is not being outside this module. I have changed it to private.

sanket1729 · 2022-08-23T09:58:08Z

src/miniscript/musig_key.rs

+        Pk: 'a,
+        Pk::RawPkHash: 'a,
+    {
+        let keys_res = self.iter().all(|my_key| pred(my_key));


nit: rename my_key to key

sanket1729 · 2022-08-23T10:08:53Z

src/policy/mod.rs

+                for key in keys {
+                    policy_vec.push((Terminal::<Pk, Ctx>::PkK(key.clone())).lift()?)
+                }
+                // Semantic::Threshold(keys.len(), policy_vec)


remove this comment

sanket1729 · 2022-08-23T10:09:06Z

src/policy/mod.rs

    use super::super::miniscript::Miniscript;
    use super::{Concrete, Liftable, Semantic};
    use crate::prelude::*;
    use crate::DummyKey;
+    // use crate::MiniscriptKey;


Unnecessary import

kanishk779 force-pushed the keyexpr branch 2 times, most recently from 62f896d to 009186f Compare July 1, 2022 19:10

sanket1729 reviewed Jul 1, 2022

View reviewed changes

kanishk779 force-pushed the keyexpr branch 3 times, most recently from 6b1058c to 102efbd Compare July 6, 2022 17:39

SarcasticNastik reviewed Jul 7, 2022

View reviewed changes

kanishk779 force-pushed the keyexpr branch from 34547af to c03e348 Compare July 21, 2022 17:51

kanishk779 force-pushed the keyexpr branch 2 times, most recently from 7d29de6 to e5ca0ae Compare August 11, 2022 19:56

sanket1729 reviewed Aug 16, 2022

View reviewed changes

kanishk779 force-pushed the keyexpr branch 6 times, most recently from 9480c72 to aef9672 Compare August 19, 2022 18:54

sanket1729 reviewed Aug 23, 2022

View reviewed changes

kanishk779 force-pushed the keyexpr branch 5 times, most recently from 79f9fed to 99fe93b Compare August 23, 2022 18:33

Add support for KeyExpr

ef43456

kanishk779 force-pushed the keyexpr branch from 99fe93b to ef43456 Compare August 24, 2022 04:09

Add initial support for KeyExpr #443

Are you sure you want to change the base?

Add initial support for KeyExpr #443

Conversation

kanishk779 commented Jul 1, 2022

sanket1729 left a 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

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

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

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

SarcasticNastik Jul 7, 2022 • edited

Choose a reason for hiding this comment

kanishk779 Aug 17, 2022 • edited

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

sanket1729 left a 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

sanket1729 Aug 16, 2022 • edited

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

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

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

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanishk779 Aug 23, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SarcasticNastik Jul 7, 2022 •

edited

kanishk779 Aug 17, 2022 •

edited

sanket1729 Aug 16, 2022 •

edited

kanishk779 Aug 23, 2022 •

edited