Descriptor support #37

justinmoon · 2020-06-10T00:53:00Z

Leverage descriptors to communicate your addresses to bwt.

src/hd.rs

justinmoon · 2020-06-10T00:54:22Z

src/config.rs

@@ -95,6 +95,16 @@ pub struct Config {
    )]
    pub bitcoind_cookie: Option<path::PathBuf>,

+    #[structopt(
+        short = "D",


How's -D for this?

I think this important enough to deserve getting the lowercase -d. We can change --bitcoind-dir to use -D.

Also, this should be --descriptor (singular form, not plural).

src/config.rs

src/hd.rs

justinmoon · 2020-06-10T00:57:52Z

src/hd.rs

@@ -181,111 +181,59 @@ impl HDWallet {
        }
    }

-    pub fn from_bare_xpub(


What should I do about these two? Just guess the descriptor based on extended pubkey version (xpub/ypub/zpub etc) and call from_descriptor?

Basically yes, figure out the appropriate descriptor(s) (two in the case of --xpub, one for --bare-xpub) and delegate to from_descriptor. Note that ypubs/zpubs will have to be provided in their xpub form (this is already case if encode the ExtendedPubKey to a string).

There should also be an ext_key: Option<XyzPubKey> field on the Wallet struct set for wallets that originated from an xpub, for backward compatibility. But lets leave that to later for now.

src/hd.rs

justinmoon · 2020-06-10T01:02:20Z

src/types.rs

+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.0)
+    }
+}


I expect I'm doing lots of noobish stuff in here ...

justinmoon · 2020-06-10T01:16:48Z

src/types.rs

+}
+
+#[derive(Serialize, Clone, Eq, PartialEq, Debug, Hash)]
+pub struct Descriptor(pub String);


Perhaps this should be an enum of "ranged" and "unranged" descriptors?

We could distinguish by matching * inside the string?

I think this could be a boolean field. getdescriptorinfo returns an isrange field that we could use instead of matching *.

I agree that could work, but I'm going to leave this work to a future PR

This could be From<&str> or From<String> since the conversion is not fallible. (they return T directly and not a Result<T> like FromStr.)

Though, if its just wrapping a String, this could also simply be constructed using DescriptorChecksum(cs_string) instead of DescriptorChecksum::from(cs_string).

Actually, it seems like you are in fact constructing them with DescriptorChecksum(cs_string), the conversion method could simply be removed.

shesek

Very nice! Thanks for sending this. :)

I added some comments and suggestions, let me know what you think.

shesek · 2020-06-14T04:45:48Z

src/config.rs

@@ -95,6 +95,16 @@ pub struct Config {
    )]
    pub bitcoind_cookie: Option<path::PathBuf>,

+    #[structopt(
+        short = "D",


I think this important enough to deserve getting the lowercase -d. We can change --bitcoind-dir to use -D.

Also, this should be --descriptor (singular form, not plural).

shesek · 2020-06-14T04:47:46Z

src/app.rs

-        let wallets = HDWallet::from_xpubs(
-            &config.xpubs[..],
-            &config.bare_xpubs[..],
+        let rpc = RpcClient::new(config.bitcoind_url(), config.bitcoind_auth()?)?;


You can wrap this in a Arc::new() here instead of doing this later. It'll automatically get decast to an &RpcClient when passed to from_descriptors().

src/config.rs

shesek · 2020-06-14T05:07:25Z

src/config.rs

@@ -345,6 +355,16 @@ fn parse_xpub(s: &str) -> Result<(XyzPubKey, RescanSince)> {
    Ok((xpub, rescan))
 }

+fn parse_descriptor(s: &str) -> Result<(String, RescanSince)> {
+    let mut parts = s.splitn(2, ':');


Could descriptors contains :? I can't seem to find any descriptors that do, but : appears to be included in the INPUT_CHARSET used for checksumming.

: separates the descriptor and the "rescan since" timestamp. I just copied parse_xpub (link).

Yes, I understood, I asked whether descriptors can contain : because if they do, the : separator would be ambiguous with the syntax of the descriptor itself.

But AFAICT, descriptors cannot contain :, so this shouldn't be ambiguous.

src/hd.rs

src/query.rs

shesek · 2020-06-14T05:54:43Z

src/query.rs

        let max_funded_index = wallet.max_funded_index?; // return None if this wallet has no history at all

        let gap = (0..=max_funded_index)
-            .map(|derivation_index| ScriptHash::from(&wallet.derive_address(derivation_index)))
+            .map(|derivation_index| {
+                ScriptHash::from(&wallet.derive_address(derivation_index, &self.rpc).unwrap())


Similarly to get_hd_script_info(), this now also need to be a fallible method that returns Result due to the reliance on RPC (and unwrap() also needs to be changed accordingly).

Which somewhat makes me reconsider whether we should do descriptor derivation in Rust without relying on the RPC... relying on the RPC will result in a cascade of functions that now need to be fallible, which is kind of annoying. Also, for find_hd_gap(), this is going to require many RPC calls, which is not ideal either.

But let's wait with this for now and give this some more thought.

I agree that doing the derivation in rust is preferable. But I think we need to wait until apoelstra/rust-miniscript#93 lands.

shesek · 2020-06-14T05:55:43Z

src/config.rs

@@ -345,6 +355,16 @@ fn parse_xpub(s: &str) -> Result<(XyzPubKey, RescanSince)> {
    Ok((xpub, rescan))
 }

+fn parse_descriptor(s: &str) -> Result<(String, RescanSince)> {
+    let mut parts = s.splitn(2, ':');
+    let descriptor = String::from(parts.next().unwrap());


The two unwraps here should instead return an Err, similarly to how its done in parse_xpub.

src/types.rs

shesek · 2020-06-22T05:29:15Z

src/hd.rs

    Standalone,
    // bwt never does hardended derivation itself, but can receive an hardend --bare-xpub
-    DerivedHard(Fingerprint, u32),
+    DerivedHard(DescriptorChecksum, u32),


The DerivedHard variant is no longer necessary, the descriptor itself already includes this information.

shesek · 2020-06-26T00:40:32Z

src/types.rs

+
+    pub fn checksum(&self) -> DescriptorChecksum {
+        // This assumes that the descriptor is legit ...
+        DescriptorChecksum(self.0.split("#").collect::<Vec<_>>()[1].to_string())


This would cause a panic if the descriptor is somehow invalid. How about adding a separate checksum field to the Descriptor struct and populating it in the constructor? We're already getting the checksum as a separate field from getdescriptorinfo.

- desc_check -> descr_cs - 'hd'-> 'wallet' in HTTP API - Return `Result` where appropriate - Replaced a few `.unwrap` with `?` - Fix origin mapping and remove KeyOrigin::DerivedHard - HDWallet.from_config accepts descriptors, xpubs, and bare xpubs - Swap CLI -d and -D shorthands

shesek · 2020-06-26T01:27:46Z

src/hd.rs

+                        rpc.clone(),
+                    )
+                    .with_context(|| format!("Invalid xpub-derived descriptor {}", descriptor))?
+                    .clone(),


Why is clone needed here?

Because I'm a noob. Fixed.

shesek · 2020-06-26T01:27:57Z

src/hd.rs

+        // Bare xpubs
+        for (xyz, rescan) in bare_xpubs {
+            let descriptor = match xyz.script_type {
+                ScriptType::P2pkh => format!("pkh({})", xyz.extended_pubkey.to_string()),


/* is missing here

shesek · 2020-06-26T01:29:18Z

src/http.rs

                .or_err(StatusCode::NOT_FOUND)?;
            let next_index = wallet.get_next_index();
-            let uri = format!("/hd/{}/{}", fingerprint, next_index);
+            let uri = format!("/hd/{}/{}", descr_cs, next_index);


/hd/ should be changed here too (and possibly some other places that construct a uri)

Fixed here and in some comments.

shesek · 2020-06-26T01:36:30Z

src/hd.rs

        rgb.serialize_field("network", &self.network)?;
-        rgb.serialize_field("script_type", &self.script_type)?;


We technically don't need the custom serializer for HDWallet now that it doesn't have the virtual origin field, this could simply #[derive(Serialize)].

But since this will eventually be added back for compatibility with xpub-based usage, maybe better to keep this here for now anyway.

shesek · 2020-06-26T01:41:37Z

src/hd.rs

+        // Xpubs
+        for (xyz, rescan) in xpubs {
+            // Change and receiving output descriptors
+            for change in 0..2 {


I kinda prefer the readability of inclusive ranges for such cases (0..=1), but don't feel strongly about this

I agree. Fixed.

shesek · 2020-06-26T02:08:27Z

src/hd.rs

+            for change in 0..2 {
+                let descriptor = match xyz.script_type {
+                    ScriptType::P2pkh => {
+                        format!("pkh({}/{}/*)", xyz.extended_pubkey.to_string(), change)


Perhaps this logic could be extracted from here, into something like Descriptor::from(XyzPubKey) for the bare case and Descriptor::from((XyzPubKey, usize)) for the external/internal chains case?

Then you could use descriptor: xyz.into() and descriptor: (xyz, change).into().

src/hd.rs

- Descriptor has "body" and "checksum" attributes - Forgot a /* turning bare xpubs into descriptors - Removed unnecessary clones - Rename "hd" -> "wallet" in a few more places

justinmoon · 2020-10-31T22:03:42Z

This PR is no longer relevant after this commit d305ec9#diff-30965b7e36f841dad79ecf3d968902df6d07bc7214a33ec928e77c82385fc2ca