add extended descriptor multisig metadata to policy extraction #626

bucko13 · 2022-06-07T03:14:07Z

Description

This is a first attempt at resolving #597. When using the policy module to parse descriptors, it can be useful to return additional information about the descriptor. This PR adds some of that in, namely key origin details about each key involved and the script type (p2sh, p2sh-p2wsh, or p2wsh).

Notes to the reviewers

An earlier version of this took advantage of PkOrF being a struct, but unfortunately just as I was ready to push up for a PR a bunch of changes came in that impacted this, specifically using an enum for PkOrF instead which changed a lot of the assumptions and tests. The enum makes most sense when these objects are relatively one dimensional (also makes for more straightforward tests), but when the key item can have more properties such as including more of the key origin, it got a little trickier.

I only updated the tests for the test I added for this feature plus one other I saw breaking. I thought I'd put it up as a draft first to get any feedback before tweaking any further. It also wasn't clear if some of the tests were breaking for other reasons.

The keys also come out a little weirder when serialized now then when they were done as a struct. Since they are basically tagged with the struct name, each key is an object with a single key xpub_origin whose value is an object with the origin details: fingerprint (which might be better called xfp or root_fingerprint?), derivation_path, and xkey).

I also considered whether or not to add the full_derivation_path or trailing path which you can sometimes have in a descriptor including wildcard (e.g. /0/*), but figured I could wait for that too.

Note, I'm very new at Rust (this is my first production Rust code written), so I'm sure there's a lot that could be more idiomatic or that I've just done wrong completely. All suggestions on how to improve welcome!

Checklists

fix remaining tests
add full_derivation_path (or similar) (?)

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

rajarshimaitra

I went through the code briefly, and I have some comments on the MultisigScriptType stuffs..

The script type should not multisig specific. We should be able to get the script type for any generic descriptors. So its should be a field inside the Policy structure itself.

But the problem of doing that is our Policy structure is recursive.. So we cannot just add an extra field inside the Policy itself, as it will then get recursively added in the internal policy nodes too..

So Instead one way to do that would be to have a GlobalPolicy structure, which includes the global ScriptType and a Policy. Then the remaining logic of Policy structure remains the same, which can recursively include more policies in it.

Concept ACK on the XPub data..

rajarshimaitra · 2022-08-18T17:02:54Z

src/descriptor/policy.rs

@@ -76,6 +76,19 @@ pub enum PkOrF {
    XOnlyPubkey(XOnlyPublicKey),
    /// An extended key fingerprint
    Fingerprint(Fingerprint),
+    /// serialization for xpub origin metadata


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

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?

rajarshimaitra · 2022-08-18T17:04:08Z

src/descriptor/policy.rs

@@ -76,6 +76,19 @@ pub enum PkOrF {
    XOnlyPubkey(XOnlyPublicKey),
    /// An extended key fingerprint
    Fingerprint(Fingerprint),


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

Yep! Good catch.

rajarshimaitra · 2022-08-18T17:05:48Z

src/descriptor/policy.rs

+/// 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>,


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

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.

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

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

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.

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

rajarshimaitra · 2022-08-18T17:07:09Z

src/descriptor/policy.rs

+/// 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,
+}


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

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?

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

rajarshimaitra · 2022-08-18T17:12:51Z

src/descriptor/policy.rs

+        /// The script type used to encode
+        #[serde(skip_serializing_if = "Option::is_none")]
+        script_type: Option<MultisigScriptType>,


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

bucko13 · 2022-08-21T15:37:31Z

Thanks for the review @rajarshimaitra! I’m afk for a little bit but at a high level this makes sense to me. Hopefully I can get some time to refactor when I’m back in September

rajarshimaitra · 2022-08-22T17:18:21Z

No issues.. This is a valuable PR IMO.. It will be helpful to get more info out from our policy language..

bucko13 · 2022-09-12T22:25:53Z

Thanks again for the review @rajarshimaitra. Just to be clear, the request for updates is basically two parts:

We want a new GlobalPolicy struct that has two parts: ScriptType and Policy, where Policy is the normal Policy as it is now.
Xpub origin data concept is good as is, but sounds like we want to clear up the language/naming a bit (see my comments in the PR on that)

danielabrozzoni · 2023-03-16T17:10:34Z

Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open!

(More specifically, we're going to use the miniscript "planning module" instead of Policies)

bucko13 · 2023-03-16T17:46:01Z

Got it. Thanks for the update! We do want to revisit this soon but sounds like it’ll make sense to do it on the new API. Largely if we can export the policy info (or whatever it’s called 😅) and have it accessible via wasm bindings than that is really all we were trying to get from this.

add extended descriptor multisig metadata to policy extraction

658f60b

notmandatory assigned bucko13 Jun 7, 2022

notmandatory mentioned this pull request Jun 21, 2022

W25 BDK Library Team Call bitcoindevkit/.github#9

Closed

14 tasks

notmandatory added the new feature New feature or request label Jul 4, 2022

rajarshimaitra suggested changes Aug 18, 2022

View reviewed changes

notmandatory mentioned this pull request Sep 13, 2022

W37 BDK Library Team Call bitcoindevkit/.github#22

Closed

26 tasks

danielabrozzoni closed this Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add extended descriptor multisig metadata to policy extraction #626

add extended descriptor multisig metadata to policy extraction #626

bucko13 commented Jun 7, 2022

rajarshimaitra left a comment •

edited

Loading

rajarshimaitra Aug 18, 2022

bucko13 Sep 12, 2022

rajarshimaitra Aug 18, 2022

bucko13 Sep 12, 2022

rajarshimaitra Aug 18, 2022

bucko13 Sep 12, 2022

rajarshimaitra Sep 14, 2022

rajarshimaitra Sep 14, 2022

bucko13 Sep 20, 2022

rajarshimaitra Sep 21, 2022

rajarshimaitra Aug 18, 2022

bucko13 Sep 12, 2022

rajarshimaitra Sep 14, 2022

rajarshimaitra Aug 18, 2022

bucko13 commented Aug 21, 2022 •

edited

Loading

rajarshimaitra commented Aug 22, 2022

bucko13 commented Sep 12, 2022

danielabrozzoni commented Mar 16, 2023

bucko13 commented Mar 16, 2023

add extended descriptor multisig metadata to policy extraction #626

add extended descriptor multisig metadata to policy extraction #626

Conversation

bucko13 commented Jun 7, 2022

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

rajarshimaitra left a comment • 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

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

bucko13 commented Aug 21, 2022 • edited Loading

rajarshimaitra commented Aug 22, 2022

bucko13 commented Sep 12, 2022

danielabrozzoni commented Mar 16, 2023

bucko13 commented Mar 16, 2023

rajarshimaitra left a comment •

edited

Loading

bucko13 commented Aug 21, 2022 •

edited

Loading