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

Switch to descriptor wallets #457

Open
nud3l opened this issue Mar 6, 2023 · 6 comments
Open

Switch to descriptor wallets #457

nud3l opened this issue Mar 6, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@nud3l
Copy link
Member

nud3l commented Mar 6, 2023

Is your feature request related to a problem? Please describe.
The vault client uses Bitcoin legacy wallets by default.

  • Legacy wallets are harder to backup and recover due to them not having explicit derivation paths.
  • Since Bitcoin core 23, descriptor wallets are now the default.
  • Descriptor wallets make it easier to have watch-only wallets (e.g., for connecting ledger to the bitcoin core node)

For a more detailed overview, see here: https://achow101.com/2020/10/0.21-wallets and https://bitcoinmagazine.com/technical/bitcoin-core-23-0-released-whats-new

Describe the solution you'd like
Instead of using legacy wallets, the vault client should use descriptor wallets.

Context
Bitcoin core 24 supports migrating legacy wallets to descriptor wallets: https://github.com/bitcoin/bitcoin/blob/master/doc/managing-wallets.md#migrating-legacy-wallets-to-descriptor-wallets

@nud3l nud3l added the enhancement New feature or request label Mar 6, 2023
@gregdhill gregdhill changed the title Swtich to descriptor wallets Switch to descriptor wallets Mar 6, 2023
@gregdhill
Copy link
Member

This might not be feasible due to the On-chain Key Derivation (OKD) scheme we use, but worth investigating!

@nud3l
Copy link
Member Author

nud3l commented Mar 6, 2023

Yep, I think we need to check how this would work with OKD and if it's compatible/possible.

Descriptor wallets would making setting vaults up as multi-sigs a lot easier as well though.

@sander2
Copy link
Member

sander2 commented Jul 17, 2023

  • Importing addresses is not too difficult - you just add a new descriptor. The basic steps are:
    • Given the private key (for example, cPGN28WXdwXtr2cpMmVVBnBAdWBCrEfiy67WatyKfWYhx3cX4k1X),
    • Call getdescriptorinfo("wpkh(cPGN28WXdwXtr2cpMmVVBnBAdWBCrEfiy67WatyKfWYhx3cX4k1X)"). This gives the checksum, e.g. 7uwp0v72.
    • Call importdescriptors '[{"desc": "wpkh(cPGN28WXdwXtr2cpMmVVBnBAdWBCrEfiy67WatyKfWYhx3cX4k1X)#7uwp0v72", "timestamp": 0, "watchonly": false}]'
    • address is now added
    • the address is a non-ranged one
  • Descriptor does not always correlate to an address 1:1. You can use the listdescriptors rpc to observe the descriptors. When you do a getnewaddress, no new descriptors are (typically) added. Descriptors can have a range of addresses they derive. When you do getnewaddress only the next and next_index counters of that descriptor increase (as well as the range possibly).
  • (related to the above) when descriptor wallet is generating new address to use as derivation key, it's not creating a new one, but rather deriving a new one. To use the OKD scheme, we need the actual derived private key. This can be done in the following way (example for address tb1qf6rs4jyvhpjlzattndxghw0zw0h5r33reglhtf).
    • getaddressinfo tb1qf6rs4jyvhpjlzattndxghw0zw0h5r33reglhtf. This gives the derivation path (here: m/84'/1'/0'/0/0)
      • "desc": "wpkh([102b271e/84'/1'/0'/0/0]037cd790de197410fb0f420923dece21fb032ae25ab70273d12d5624ccadbe0b1c)#p6c2ufsd",
    • then do listdescriptorinfo true to get the address corresponding to that. This will give a list of all the tprv (on testnet) or xprv (on mainnet) keys in the wallet. The keys will be the 'master' keys of the descriptors. To see which one is associated to the address we're interested in, you have to iterate and use deriveaddresses <key>.
    • Once you find the xprv/tprv, run this code to get the derived private key:
      let key = bitcoincore_rpc::bitcoin::bip32::ExtendedPrivKey::from_str("tprv8ZgxMBicQKsPekhHsyavZSvfLdhUThVV5WhmAtpSvqrpptbitFrKaPcuJc5KkcdCsH641zwcPtixhCzyxffJ6qVSD8RovFdwhUp9yvVXF7s").unwrap();
      let secp = bitcoincore_rpc::bitcoin::key::Secp256k1::new();
      let derivation_path = bitcoincore_rpc::bitcoin::bip32::DerivationPath::from_str("m/84'/1'/0'/0/0").unwrap();
      warn!("wif: {}", key.derive_priv(&secp, &derivation_path).unwrap().to_priv().to_wif());
    • getdescriptorinfo "wpkh(<wif>)"
      • "descriptor": "wpkh(037cd790de197410fb0f420923dece21fb032ae25ab70273d12d5624ccadbe0b1c)#2kuqptdh"
    • Verify that is works: deriveaddresses "wpkh(037cd790de197410fb0f420923dece21fb032ae25ab70273d12d5624ccadbe0b1c)#2kuqptdh"
      • "tb1qf6rs4jyvhpjlzattndxghw0zw0h5r33reglhtf"

Note that bitcoin core intentionally made it difficult to get the private key associated with an address. Apparently this is because users assumed that the private keys are random, and that sharing them only risks the address itself, while in reality leaking one private key risks all other derived keys as well

Also, the fact that we have to iterate over all of the descriptors to find the derivation key is a point in favor of keeping a separate -master wallet (there has been talk recently to possibly merge wallets)

@nud3l
Copy link
Member Author

nud3l commented Jul 25, 2023

Summarizing my understanding:

  • Descriptor wallets work with the OKD scheme.
  • Using descriptor wallets introduces iteration over all descriptors to find the matching descriptor/UTXO.
  • Having a -master wallet would make life easier.

Questions:

  • For finding the descriptor/UTXO, does it add a lot of complexity when e.g., a vault needs to restart?
  • Is there anything blocking us from switching to descriptor wallets?

I find it quite worthwhile to make the switch as we will make it easier for vaults to backup/restore wallets, make it possible to use hardware wallets eventually and help with multisig vaults.

@sander2
Copy link
Member

sander2 commented Jul 25, 2023

For finding the descriptor/UTXO, does it add a lot of complexity when e.g., a vault needs to restart?

Well we need to do the the iteration over all descriptors that's mentioned. This iteration needs to be done once per vault start-up if we just keep it in memory.

Is there anything blocking us from switching to descriptor wallets?

No hard blockers that I'm aware of, but the bitcoin rpc library is missing some rpcs. The maintenance is not very active at all so we'll probably have to implement this ourselves on a fork.

we will make it easier for vaults to backup/restore wallets

I'm not trying to argue against this point, but why is this the case? With legacy wallets you can just copy the wallet file and that's it, right?

@nud3l
Copy link
Member Author

nud3l commented Jul 30, 2023

There's a blog post I've read somewhere that describes a bit the issues with legacy wallet recovery. IIRC, legacy wallet are not compatible with wallets other than core and core will also stop supporting them in late 2024. So while you can back them up, at some point you can only use them with outdated software. There were also some other issues where reconstructing addresses was complex but not sure if that isn't an issue if you restore just all files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

3 participants