Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial support for KeyExpr #443

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kanishk779
Copy link
Contributor

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

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

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you manually implement Debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

use crate::MiniscriptKey;

#[derive(Debug, Clone, PartialEq, PartialOrd, Ord, Eq, Hash)]
/// Enum for representing keys in miniscript
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*representing musig keys

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

/// Single-key (e.g pk(a), here 'a' is a single key)
SingleKey(Pk),

/// Collection of keys in used for multi-signature
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

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>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should

impl FromTree for KeyExpr instead of this local function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented FromTreef for KeyExpr.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit: use _vec instead of '_vect'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, removed the if condition.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use map_err instead of match

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: name this musig_key instead of dummy_key

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some more test cases.

@kanishk779 kanishk779 force-pushed the keyexpr branch 3 times, most recently from 6b1058c to 102efbd Compare July 6, 2022 17:39
fn next(&mut self) -> Option<Self::Item> {
while !self.stack.is_empty() {
let last = self.stack.pop().expect("Size checked above");
match &*last {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, changed accordingly.

Suggested change
match &*last {
match last {

Comment on lines 59 to 72
KeyExpr::MuSig(key_vec) => {
// push the elements in reverse order
for key in key_vec.iter().rev() {
self.stack.push(key)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

}

#[derive(Debug, Clone)]
/// Iterator for keyexpr
Copy link
Contributor

@SarcasticNastik SarcasticNastik Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kanishk779 kanishk779 Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 22 to 34
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review

@@ -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())))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Revert back to &Pk.
  2. Add a test check_script_size covering all the four suggested tests.

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,
Copy link
Member

@sanket1729 sanket1729 Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. fixed as suggested.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for_each_key for a fragment containing musig.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test for this test_for_each_key in miniscript/mod.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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned earlier, we should not change this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, created the new method with the default implementation.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. changed everywhere.

Terminal::PkK(ref pk) => {
if pk.is_uncompressed() {
Terminal::PkK(ref key) => {
if key.iter().all(|pk| !pk.is_uncompressed()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. changed as suggested.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about compressed keys

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The len should always be 33

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed everywhere.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are returning an incorrect variant here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there. Some last few edits.

src/lib.rs Outdated
@@ -99,6 +99,9 @@ extern crate alloc;
#[cfg(not(feature = "std"))]
extern crate hashbrown;

#[cfg(not(feature = "std"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this line?

Copy link
Contributor Author

@kanishk779 kanishk779 Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about collect everywhere in leafpk, leafpkh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be pub?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename my_key to key

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary import

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

@kanishk779 kanishk779 force-pushed the keyexpr branch 5 times, most recently from 79f9fed to 99fe93b Compare August 23, 2022 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants