wallet: add parent_desc to getaddressinfo #19136
Conversation
a21fcfd to
004b17d
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Do we need a whole new RPC for this? Why not expand |
|
Agree with Sjors, unless you have plans to somehow build upon this later, I think it might as well just be added to getaddressinfo |
004b17d to
fc2f2a0
Compare
|
Changed this to be a new field |
411518b to
ff14b4f
Compare
ff14b4f to
e92546a
Compare
There was a problem hiding this comment.
I believe it's cumbersome to work with normalised descriptor represented as a string. It has been proved to be prone to errors and also less expressive. Luckily we already have a struct for this!
What if we use object representation for such a descriptor and convert it to string only at the last step.
I have something like this in my mind:
- Add
PubkeyProvider::GetLastUnhardenedKey(...). The function will be shorter and easier to understand thanToNormalizedStringwhich does two things right now: a) derives the key and b) converts it to string. - Use it in
Descriptor::GetNormalizedDescriptor(...). By usingGetLastUnhardenedKeyit's obvious now what do we mean by "normalized" without going to deep into the code. - In
DescriptorScriptPubKeyMan::GetDescriptorStringwe callGetNormalizedDescriptor()first and than use usualToPrivateSring()orToString()functions on the returned descriptor.
Another benefit of this approach is that we don't need more arguments and conditions in ToStringHelepr.
Let me know what you think, I can do a PoC if you'd endorse this idea.
|
@S3RK I originally attempted an approach like that but was not able to make it work well. The problem is largely in the same place that the problem with this PR is: nested origin pubkey providers. In both this PR and your proposal, you will still have some weirdness to handle when the bip32 pubkey provider returns an origin pubkey provider, but this itself is already within an origin pubkey provider. |
|
@achow101 I understand that it's an intrinsic part of the logic here. However my point is that we need to strive to make that weird part as easy to understand as possible. And this is why I believe working with objects/structs is better. When you work with The other benefits hold up as well: single responsibility and less cyclomatic complexity for functions, self-documented code. |
|
@S3RK If you think you have a better solution, feel free to open a PR with it. I can close this one in favor of that if it does look better. |
For history. I tried to implement alternative version, and indeed it also has its own problems. Both with nested origin/bip32 providers and also with private key management for intermediate xpub. And the diff is bigger 😞 |
GetDescriptorString returns a normalized descriptor for a DescriptorScriptPubKeyMan.
Adds parent_desc field to getaddressinfo output which contains the descriptor that was used to derive the address if it is available.
e92546a to
de6b389
Compare
|
utACK de6b389 |
| return true; | ||
| } | ||
| // Step backwards to find the last hardened step in the path | ||
| int i = (int)m_path.size() - 1; |
There was a problem hiding this comment.
9be1437 named cast per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Res-casts-named
| int i = (int)m_path.size() - 1; | |
| int i = static_cast<int>(m_path.size()) - 1; |
| end_path.push_back(m_path.at(k)); | ||
| } | ||
| // Build the string | ||
| std::string origin_str = HexStr(origin.fingerprint) + FormatHDKeypath(origin.path); |
There was a problem hiding this comment.
| std::string origin_str = HexStr(origin.fingerprint) + FormatHDKeypath(origin.path); | |
| const std::string origin_str = HexStr(origin.fingerprint) + FormatHDKeypath(origin.path); |
| @@ -243,6 +261,12 @@ class ConstPubkeyProvider final : public PubkeyProvider | |||
| ret = EncodeSecret(key); | |||
| return true; | |||
| } | |||
| bool ToNormalizedString(const SigningProvider& arg, std::string& ret, bool priv) const override | |||
There was a problem hiding this comment.
9be1437 here and L218, maybe name the arg out rather than ret like the header file declaration and the other definitions
| bool ToNormalizedString(const SigningProvider& arg, std::string& ret, bool priv) const override | |
| bool ToNormalizedString(const SigningProvider& arg, std::string& out, bool priv) const override |
| @@ -3663,6 +3663,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request) | |||
| {RPCResult::Type::BOOL, "iswatchonly", "If the address is watchonly."}, | |||
| {RPCResult::Type::BOOL, "solvable", "If we know how to spend coins sent to this address, ignoring the possible lack of private keys."}, | |||
| {RPCResult::Type::STR, "desc", /* optional */ true, "A descriptor for spending coins sent to this address (only when solvable)."}, | |||
| {RPCResult::Type::STR, "parent_desc", /* optional */ true, "The descriptor used to derive this address if this is a descriptor wallet"}, | |||
There was a problem hiding this comment.
- missing period like the neighboring sentences and maybe add a comma
| {RPCResult::Type::STR, "parent_desc", /* optional */ true, "The descriptor used to derive this address if this is a descriptor wallet"}, | |
| {RPCResult::Type::STR, "parent_desc", /* optional */ true, "The descriptor used to derive this address, if this is a descriptor wallet."}, |
-
not sure, but maybe change from:
"The descriptor used to derive this address, if this is a descriptor wallet."
to
"The descriptor used to derive this address, if generated by a descriptor wallet."
| 'internal': internal | ||
| }]) | ||
|
|
||
| for i in range(0, 10): |
There was a problem hiding this comment.
de6b389 nit, if iterator is not used in the for-loop block
| for i in range(0, 10): | |
| for _ in range(0, 10): |
| else: | ||
| addr = exp_rpc.getnewaddress(address_type=addr_type) | ||
| test_desc = exp_rpc.getaddressinfo(addr)['parent_desc'] | ||
| assert_equal(desc, test_desc) |
There was a problem hiding this comment.
de6b389 nit, the if internal...else...assert_equal logic is repeated 4 times, maybe extract to a helper
647b81b wallet, rpc: add listdescriptors command (Ivan Metlushko) Pull request description: Looking for concept ACKs **Rationale**: allow users to inspect the contents of their newly created descriptor wallets. Currently the command only returns xpubs which is not very useful in itself, but there are multiples ways to extend it: * add an option to export xprv * with #19136 it'll be possible to return normalised descriptors suitable for a watch-only purposes The output is compatible with `importdescriptors` command so it could be easily used for backup/recover purposes. **Output example:** ```json [ { "desc": "wpkh(tpubD6NzVbkrYhZ4WW6E2ZETFyNfq2hfF23SKxqSGFvUpPAY58jmmuBybwqwFihAyQPk9KnwTt5516NDZRJ7k5QPeKjy7wuVd5WvXNxwwAs5tUD/*)#nhavpr5h", "timestamp": 1296688602, "active": false, "range": [ 0, 999 ], "next": 0 } ] ``` ACKs for top commit: jonatack: re-ACK 647b81b rebased to master, debug builds cleanly, reviewed diff since last review, tested with a descriptor wallet (and with a legacy wallet) achow101: re-ACK 647b81b Tree-SHA512: 51a3620bb17c836c52cecb066d4fa9d5ff418af56809046eaee0528c4dc240a4e90fff5711ba96e399c6664e00b9ee8194e33852b1b9e75af18061296e19a8a7
647b81b wallet, rpc: add listdescriptors command (Ivan Metlushko) Pull request description: Looking for concept ACKs **Rationale**: allow users to inspect the contents of their newly created descriptor wallets. Currently the command only returns xpubs which is not very useful in itself, but there are multiples ways to extend it: * add an option to export xprv * with bitcoin#19136 it'll be possible to return normalised descriptors suitable for a watch-only purposes The output is compatible with `importdescriptors` command so it could be easily used for backup/recover purposes. **Output example:** ```json [ { "desc": "wpkh(tpubD6NzVbkrYhZ4WW6E2ZETFyNfq2hfF23SKxqSGFvUpPAY58jmmuBybwqwFihAyQPk9KnwTt5516NDZRJ7k5QPeKjy7wuVd5WvXNxwwAs5tUD/*)#nhavpr5h", "timestamp": 1296688602, "active": false, "range": [ 0, 999 ], "next": 0 } ] ``` ACKs for top commit: jonatack: re-ACK 647b81b rebased to master, debug builds cleanly, reviewed diff since last review, tested with a descriptor wallet (and with a legacy wallet) achow101: re-ACK 647b81b Tree-SHA512: 51a3620bb17c836c52cecb066d4fa9d5ff418af56809046eaee0528c4dc240a4e90fff5711ba96e399c6664e00b9ee8194e33852b1b9e75af18061296e19a8a7
| @@ -3732,6 +3733,14 @@ UniValue getaddressinfo(const JSONRPCRequest& request) | |||
| ret.pushKV("desc", InferDescriptor(scriptPubKey, *provider)->ToString()); | |||
| } | |||
|
|
|||
| DescriptorScriptPubKeyMan* desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(pwallet->GetScriptPubKeyMan(scriptPubKey)); | |||
There was a problem hiding this comment.
in e4ac869:
Below, on line 3742, there is another call to GetScriptPubKeyMan(). Maybe move that one up here to save one.
|
Tested ACK de6b389 |
de6b389 tests: Test getaddressinfo parent_desc (Andrew Chow) e4ac869 rpc: Add parent descriptor to getaddressinfo output (Andrew Chow) bbe4a36 wallet: Add GetDescriptorString to DescriptorScriptPubKeyMan (Andrew Chow) 9be1437 descriptors: Add ToNormalizedString and tests (Andrew Chow) Pull request description: Adds `parent_desc` field to the `getaddressinfo` RPC to export a public descriptor. Using the given address, `getaddressinfo` will look up which `DescriptorScriptPubKeyMan` can be used to produce that address. It will then return the descriptor for that `DescriptorScriptPubKeyMan` in the `parent_desc` field. The descriptor will be in a normalized form where the xpub at the last hardened step is derived so that the descriptor can be imported to other wallets. Tests are added to check that the correct descriptor is being returned for the wallet's addresses and that these descriptors can be imported and used in other wallets. As part of this PR, a `ToNormalizedString` function is added to the descriptor classes. This really only has an effect on `BIP32PubkeyProvider`s that have hardened derivation steps. Tests are added to check that normalized descriptors are returned. ACKs for top commit: Sjors: utACK de6b389 S3RK: Tested ACK de6b389 jonatack: Tested ACK de6b389 modulo a few minor comments fjahr: Code review ACK de6b389 meshcollider: Tested ACK de6b389 Tree-SHA512: a633e4a39f2abbd95afd7488484cfa66fdd2651dac59fe59f2b80a0940a2a4a13acf889c534a6948903d701484a2ba1218e3081feafe0b9a720dccfa9e43ca2b
a69c3b3 wallet: listdescriptors uses normalized descriptor form (Ivan Metlushko) Pull request description: Rationale: show importable descriptors with `listdescriptors` RPC It uses #19136 to derive xpub at the last hardened step. **Before**: ``` [ { "desc": "wpkh(tpubD6NzVbkrYhZ4YUQRJL49TWw1VR5v3QKUNYaGGMUfJUm19x5ZqQ2hEiPiYbAQvD2nHoPGQGPg3snLPM8sjmYpvx7XQhkmyfk8xhsUwKbXzyh/84'/1'/0'/0/*)#p4cn3erf", "timestamp": 1613982591, "active": true, "internal": false, "range": [ 0, 999 ], "next": 0 }, ... ] ``` **After**: ``` [ { "desc": "wpkh([d4ade89c/84'/1'/0']tpubDDUEYcVXy6Vh5meHvcXN3sAr4k3fWwLZGpAHbkAHL8EnkDxp4d99CjNhJHfM2fUJicANvAKnCZS6XaVAgwAeKYc1KesGCN5qbQ25qQHrRxM/0/*)#8wq8rcft", "timestamp": 1613982591, "active": true, "internal": false, "range": [ 0, 999 ], "next": 0 }, ... ] ``` ACKs for top commit: achow101: ACK a69c3b3 Tree-SHA512: 4f92e726cb8245aa0b520729cfd272945f0c66830f0555544e0618883aca516318543fa6ab1862058c64b4e4ed54ad1da953e032f4846eef7454122031c1b005
…or form a69c3b3 wallet: listdescriptors uses normalized descriptor form (Ivan Metlushko) Pull request description: Rationale: show importable descriptors with `listdescriptors` RPC It uses bitcoin#19136 to derive xpub at the last hardened step. **Before**: ``` [ { "desc": "wpkh(tpubD6NzVbkrYhZ4YUQRJL49TWw1VR5v3QKUNYaGGMUfJUm19x5ZqQ2hEiPiYbAQvD2nHoPGQGPg3snLPM8sjmYpvx7XQhkmyfk8xhsUwKbXzyh/84'/1'/0'/0/*)#p4cn3erf", "timestamp": 1613982591, "active": true, "internal": false, "range": [ 0, 999 ], "next": 0 }, ... ] ``` **After**: ``` [ { "desc": "wpkh([d4ade89c/84'/1'/0']tpubDDUEYcVXy6Vh5meHvcXN3sAr4k3fWwLZGpAHbkAHL8EnkDxp4d99CjNhJHfM2fUJicANvAKnCZS6XaVAgwAeKYc1KesGCN5qbQ25qQHrRxM/0/*)#8wq8rcft", "timestamp": 1613982591, "active": true, "internal": false, "range": [ 0, 999 ], "next": 0 }, ... ] ``` ACKs for top commit: achow101: ACK a69c3b3 Tree-SHA512: 4f92e726cb8245aa0b520729cfd272945f0c66830f0555544e0618883aca516318543fa6ab1862058c64b4e4ed54ad1da953e032f4846eef7454122031c1b005
Adds
parent_descfield to thegetaddressinfoRPC to export a public descriptor. Using the given address,getaddressinfowill look up whichDescriptorScriptPubKeyMancan be used to produce that address. It will then return the descriptor for thatDescriptorScriptPubKeyManin theparent_descfield. The descriptor will be in a normalized form where the xpub at the last hardened step is derived so that the descriptor can be imported to other wallets. Tests are added to check that the correct descriptor is being returned for the wallet's addresses and that these descriptors can be imported and used in other wallets.As part of this PR, a
ToNormalizedStringfunction is added to the descriptor classes. This really only has an effect onBIP32PubkeyProviders that have hardened derivation steps. Tests are added to check that normalized descriptors are returned.