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.

Sign up for GitHub

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 extended descriptor multisig metadata to policy extraction #626

Closed
wants to merge 1 commit into from
Closed

add extended descriptor multisig metadata to policy extraction #626

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 138 additions & 18 deletions src/descriptor/policy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ use std::collections::{BTreeMap, HashSet, VecDeque};
use std::fmt;

use serde::ser::SerializeMap;
use serde::{Serialize, Serializer};
use serde::{Deserialize, Serialize, Serializer};

use bitcoin::hashes::*;
use bitcoin::util::bip32::Fingerprint;
use bitcoin::util::bip32::{DerivationPath, ExtendedPubKey, Fingerprint};
use bitcoin::{PublicKey, XOnlyPublicKey};

use miniscript::descriptor::{
Expand Down Expand Up @@ -76,6 +76,19 @@ pub enum PkOrF {
XOnlyPubkey(XOnlyPublicKey),
/// An extended key fingerprint
Fingerprint(Fingerprint),
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we also don't need this FingerPrint anymore as its included in the XpubOrigin??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Good catch.

/// serialization for xpub origin metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call this "An XPub with origin metadata"?? As per the other item docs and sounds a bit more clear to me.

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 see the confusion. Basically because there's already something called origin in the library (the field on the DescriptorXKey struct) so it's a little confusing. I guess I'm trying to find some name that represents the full struct necessary for defining an xpub in a meaningful way (e.g. when verifying membership in a multisig wallet, which is one of the primary use cases we're building this out for).

It's been a while since I've spent time in this code, so it's possible I'm missing something, but basically what I was originally thinking was where XpubOrigin is an object that contains an Extended Public Key plus it's derivation/origin metadata.

OTOH, maybe I'm misunderstanding your comment and you're just asking to update this code comment 😅. In which case, maybe "serialization for xpub with derivation metadata" would be more clear to avoid the naming clash with origin?

XpubOrigin(XpubOrigin),
}

/// Raw public key or extended key fingerprint
#[derive(Debug, Clone, Default, Serialize, PartialEq, Eq, Hash)]
pub struct XpubOrigin {
#[serde(skip_serializing_if = "Option::is_none")]
fingerprint: Option<Fingerprint>,
#[serde(skip_serializing_if = "Option::is_none")]
derivation_path: Option<DerivationPath>,
#[serde(skip_serializing_if = "Option::is_none")]
xkey: Option<ExtendedPubKey>,
Comment on lines +83 to +91
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to me representing a full XPub, not just the origin.. So we can name it better as just an XPub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to the struct XpubOrigin or this field, xkey?

In the contexts I'm most familiar working in, an xpub origin has more information than the xpub (extended public key) itself. Namely the origin is the xpub and the information describing how it was derived: fingerprint for the root (xfp which might be a better name for that field) and the derivation path from root. The xpub only has its parent fingerprint and depth encoded (with some other data too of course.)

As for renaming xkey, I forget why I put it as this, but I feel like I remember there being a reason, maybe some sort of naming clash. If that isn't a problem though then I'm not against renaming that to xpub.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was actually referring to the struct name XpubOrigin.. The name suggests that its an origin for Xpub, but its more like an XpubWithOrigin. But I agree its mostly nomenclature and there's no right way to do it.. I think just the doc comment as something like /// Extended Publickey with origin information is good enough to clarify what this struct is for.. The struct name can remain as it is..

Copy link
Contributor

Choose a reason for hiding this comment

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

OTOH if I am not wrong, after parsing a descriptor we will always have the origin information? If no fingerprint and path is provided in the descriptor, bdk assumes it as the master key? If that's the case we can simply name this struct Xpub and it will imply that origin information is already included in it.. As we never put an Xpub into a policy without its origin..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if bdk never puts an xpub w/o origin details, if we're given one to interpret can you safely assume it's the root key? You don't need the origin information for watch-only wallets for example since you only need those details to tell the master key how to sign. We do something like this for unchained wallet configurations (which I want to use this tooling to convert to descriptors) where we only show actual origin information for keys that are yours (the clients) and blank out the origin info for those you don't have permissions to sign with in our system (for example the unchained key). We do this to preserve privacy and avoid data leaks. The xpub's relation to the master seed (origin information) should be of no concern to the other party that's signing if that key is not going to be signed with. So, in this context the origin information is optional and this would be true for all keys if you only want a watch-only descriptor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I agree.. For watch-only we don't need the origin info and its not prudent to assume its a master key.. ACK on that..

}

impl PkOrF {
Expand All @@ -89,11 +102,29 @@ impl PkOrF {
key: SinglePubKey::XOnly(pk),
..
}) => PkOrF::XOnlyPubkey(*pk),
DescriptorPublicKey::XPub(xpub) => PkOrF::Fingerprint(xpub.root_fingerprint(secp)),
DescriptorPublicKey::XPub(xpub) => PkOrF::XpubOrigin(XpubOrigin {
fingerprint: Some(xpub.root_fingerprint(secp)),
derivation_path: xpub.origin.as_ref().map(|origin| origin.clone().1),
xkey: Some(xpub.xkey),
}),
}
}
}

/// script type used to encode multisig script
#[derive(Debug, Clone, Copy, Serialize, Deserialize, Eq, PartialEq)]
pub enum MultisigScriptType {
/// P2SH
#[serde(rename = "P2SH")]
P2sh,
/// P2SH-P2WSH
#[serde(rename = "P2SH_P2WSH")]
P2shP2wsh,
/// P2WSH
#[serde(rename = "P2WSH")]
P2wsh,
}
Comment on lines +114 to +126
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use ScriptType for any type of scripts.. Not just multisigs..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Good point. I assume you're recommending the Enum be renamed to ScriptType? And maybe `P2tr should be included here now too if we're moving this to policy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup sounds good.. I think bdk can already recognize p2tr. A test with an example taproot descriptor into policy and checking if its parsing it correctly would be really nice..


/// An item that needs to be satisfied
#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
#[serde(tag = "type", rename_all = "UPPERCASE")]
Expand Down Expand Up @@ -139,6 +170,9 @@ pub enum SatisfiableItem {
keys: Vec<PkOrF>,
/// The required threshold count
threshold: usize,
/// The script type used to encode
#[serde(skip_serializing_if = "Option::is_none")]
script_type: Option<MultisigScriptType>,
Comment on lines +173 to +175
Copy link
Contributor

Choose a reason for hiding this comment

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

I am feeling like we should be able to set the ScriptType in the Policy structure itself. Not just only multisigs.. And this should not be inside SatisfiableItem..

},

// Complex item
Expand Down Expand Up @@ -578,6 +612,7 @@ impl Policy {
threshold: usize,
sorted: bool,
secp: &SecpCtx,
script_type: Option<MultisigScriptType>,
) -> Result<Option<Policy>, PolicyError> {
if threshold == 0 {
return Ok(None);
Expand Down Expand Up @@ -621,6 +656,7 @@ impl Policy {
let mut policy: Policy = SatisfiableItem::Multisig {
keys: parsed_keys,
threshold,
script_type,
}
.into();
policy.contribution = contribution;
Expand Down Expand Up @@ -690,7 +726,9 @@ impl Policy {

Ok(requirements)
}
SatisfiableItem::Multisig { keys, threshold } => {
SatisfiableItem::Multisig {
keys, threshold, ..
} => {
if selected.len() < *threshold {
return Err(PolicyError::NotEnoughItemsSelected(self.id.clone()));
}
Expand Down Expand Up @@ -951,7 +989,7 @@ impl<Ctx: ScriptContext + 'static> ExtractPolicy for Miniscript<DescriptorPublic
Some(SatisfiableItem::Hash160Preimage { hash: *hash }.into())
}
Terminal::Multi(k, pks) | Terminal::MultiA(k, pks) => {
Policy::make_multisig::<Ctx>(pks, signers, build_sat, *k, false, secp)?
Policy::make_multisig::<Ctx>(pks, signers, build_sat, *k, false, secp, None)?
}
// Identities
Terminal::Alt(inner)
Expand Down Expand Up @@ -1047,6 +1085,7 @@ impl ExtractPolicy for Descriptor<DescriptorPublicKey> {
signers: &SignersContainer,
build_sat: BuildSatisfaction,
secp: &SecpCtx,
script_type: Option<MultisigScriptType>,
) -> Result<Option<Policy>, Error> {
Ok(Policy::make_multisig::<Ctx>(
keys.pks.as_ref(),
Expand All @@ -1055,6 +1094,7 @@ impl ExtractPolicy for Descriptor<DescriptorPublicKey> {
keys.k,
true,
secp,
script_type,
)?)
}

Expand All @@ -1079,17 +1119,33 @@ impl ExtractPolicy for Descriptor<DescriptorPublicKey> {
secp,
))),
ShInner::Ms(ms) => Ok(ms.extract_policy(signers, build_sat, secp)?),
ShInner::SortedMulti(ref keys) => make_sortedmulti(keys, signers, build_sat, secp),
ShInner::SortedMulti(ref keys) => make_sortedmulti(
keys,
signers,
build_sat,
secp,
Some(MultisigScriptType::P2sh),
),
ShInner::Wsh(wsh) => match wsh.as_inner() {
WshInner::Ms(ms) => Ok(ms.extract_policy(signers, build_sat, secp)?),
WshInner::SortedMulti(ref keys) => {
make_sortedmulti(keys, signers, build_sat, secp)
}
WshInner::SortedMulti(ref keys) => make_sortedmulti(
keys,
signers,
build_sat,
secp,
Some(MultisigScriptType::P2shP2wsh),
),
},
},
Descriptor::Wsh(wsh) => match wsh.as_inner() {
WshInner::Ms(ms) => Ok(ms.extract_policy(signers, build_sat, secp)?),
WshInner::SortedMulti(ref keys) => make_sortedmulti(keys, signers, build_sat, secp),
WshInner::SortedMulti(ref keys) => make_sortedmulti(
keys,
signers,
build_sat,
secp,
Some(MultisigScriptType::P2wsh),
),
},
Descriptor::Bare(ms) => Ok(ms.as_inner().extract_policy(signers, build_sat, secp)?),
Descriptor::Tr(tr) => {
Expand Down Expand Up @@ -1172,6 +1228,7 @@ mod test {
.unwrap()
.unwrap();

println!("{:?}", policy);
assert!(matches!(&policy.item, EcdsaSignature(PkOrF::Fingerprint(f)) if f == &fingerprint));
assert!(matches!(&policy.contribution, Satisfaction::None));

Expand All @@ -1184,7 +1241,6 @@ mod test {
.extract_policy(&signers_container, BuildSatisfaction::None, &secp)
.unwrap()
.unwrap();

assert!(matches!(&policy.item, EcdsaSignature(PkOrF::Fingerprint(f)) if f == &fingerprint));
assert!(
matches!(&policy.contribution, Satisfaction::Complete {condition} if condition.csv == None && condition.timelock == None)
Expand All @@ -1208,7 +1264,7 @@ mod test {
.unwrap();

assert!(
matches!(&policy.item, Multisig { keys, threshold } if threshold == &2usize
matches!(&policy.item, Multisig { keys, threshold, .. } if threshold == &2usize
&& keys[0] == PkOrF::Fingerprint(fingerprint0)
&& keys[1] == PkOrF::Fingerprint(fingerprint1))
);
Expand Down Expand Up @@ -1238,8 +1294,9 @@ mod test {
.extract_policy(&signers_container, BuildSatisfaction::None, &secp)
.unwrap()
.unwrap();

assert!(
matches!(&policy.item, Multisig { keys, threshold } if threshold == &2usize
matches!(&policy.item, Multisig { keys, threshold, .. } if threshold == &2usize
&& keys[0] == PkOrF::Fingerprint(fingerprint0)
&& keys[1] == PkOrF::Fingerprint(fingerprint1))
);
Expand Down Expand Up @@ -1272,7 +1329,7 @@ mod test {
.unwrap();

assert!(
matches!(&policy.item, Multisig { keys, threshold } if threshold == &1
matches!(&policy.item, Multisig { keys, threshold, .. } if threshold == &1
&& keys[0] == PkOrF::Fingerprint(fingerprint0)
&& keys[1] == PkOrF::Fingerprint(fingerprint1))
);
Expand Down Expand Up @@ -1304,7 +1361,7 @@ mod test {
.unwrap();

assert!(
matches!(&policy.item, Multisig { keys, threshold } if threshold == &2
matches!(&policy.item, Multisig { keys, threshold, .. } if threshold == &2
&& keys[0] == PkOrF::Fingerprint(fingerprint0)
&& keys[1] == PkOrF::Fingerprint(fingerprint1))
);
Expand Down Expand Up @@ -1376,10 +1433,12 @@ mod test {
.unwrap();

assert!(
matches!(&policy.item, Multisig { keys, threshold } if threshold == &1
&& keys[0] == PkOrF::Fingerprint(fingerprint0)
&& keys[1] == PkOrF::Fingerprint(fingerprint1))
matches!(&policy.item, Multisig { keys, threshold, .. } if threshold == &1
&& matches!(&keys[0], PkOrF::XpubOrigin(XpubOrigin{ fingerprint, .. }) if fingerprint.unwrap() == fingerprint0)
&& matches!(&keys[1], PkOrF::XpubOrigin(XpubOrigin{ fingerprint, .. }) if fingerprint.unwrap() == fingerprint1)
)
);

assert!(
matches!(&policy.contribution, Satisfaction::PartialComplete { n, m, items, conditions, .. } if n == &2
&& m == &1
Expand Down Expand Up @@ -1910,6 +1969,67 @@ mod test {
Satisfaction::Complete {
condition: Default::default()
}
)
}

fn setup_descriptor_key<Ctx: ScriptContext>(
xpub: &str,
fingerprint: &str,
path: &str,
) -> DescriptorKey<Ctx> {
use miniscript::descriptor::{DescriptorXKey, Wildcard};
let xfp = Fingerprint::from_str(fingerprint).unwrap();
let origin_path = bip32::DerivationPath::from_str(&path).unwrap();
let origin: bip32::KeySource = (xfp, origin_path);
let xpub = bip32::ExtendedPubKey::from_str(xpub).unwrap();
let derivation_path = bip32::DerivationPath::from_str("m/0").unwrap();
let xkey = DescriptorPublicKey::XPub(DescriptorXKey {
origin: Some(origin),
xkey: xpub,
derivation_path,
wildcard: Wildcard::Unhardened,
});
xkey.into_descriptor_key().unwrap()
}

#[test]
fn test_extract_multisig_descriptor_policy() {
let secp = Secp256k1::new();
let pubkey1 = "xpub6FDrnnUsgQSwRFazYbVDs9eadQaNV13f5dtQDoWrCuMNq2qgMH7GevctMAm3PeHq3KBkh9BgA8iPfaHYACHFpfueYdeAUtjjEH3vMJWEKfu";
let pubkey2 = "xpub6EgGHjcvovyN3nK921zAGPfuB41cJXkYRdt3tLGmiMyvbgHpss4X1eRZwShbEBb1znz2e2bCkCED87QZpin3sSYKbmCzQ9Sc7LaV98ngdeX";
let xfp1 = "9d120b19";
let xfp2 = "5c9e228d";
let path = "m/48'/0'/0'/2";

let pubkey_alice = setup_descriptor_key(pubkey1, xfp1, path);
let pubkey_bob = setup_descriptor_key(pubkey2, xfp2, path);
let (desc, _, _) = descriptor!(wsh(sortedmulti(1, pubkey_alice, pubkey_bob))).unwrap();

let (wallet_desc, keymap) = desc
.into_wallet_descriptor(&secp, Network::Bitcoin)
.unwrap();
let signers_container = Arc::new(SignersContainer::build(keymap, &wallet_desc, &secp));

let policy = wallet_desc
.extract_policy(&signers_container, BuildSatisfaction::None, &secp)
.unwrap()
.unwrap();

assert!(
matches!(&policy.item, SatisfiableItem::Multisig { keys, script_type, threshold, .. } if threshold == &1
&& script_type.unwrap() == MultisigScriptType::P2wsh
&& keys.len() == 2
&& matches!(&keys[0], PkOrF::XpubOrigin(XpubOrigin{ fingerprint, derivation_path, xkey })
if fingerprint.unwrap().to_string() == xfp1
&& derivation_path.as_ref().unwrap().to_string() == path
&& xkey.unwrap().to_string() == pubkey1
)
&& matches!(&keys[1], PkOrF::XpubOrigin(XpubOrigin{ fingerprint, derivation_path, xkey })
if fingerprint.unwrap().to_string() == xfp2
&& derivation_path.as_ref().unwrap().to_string() == path
&& xkey.unwrap().to_string() == pubkey2
)
)
);
}
}