wallet, rpc: Return normalized descriptor in parent

achow101 commented at 9:18 pm on May 22, 2025: member

Instead of prividing the descriptor string as stored in the db, use the normalized descriptor as is done for getaddressinfo’s parent_desc field.

Split from #32489

DrahtBot commented at 9:18 pm on May 22, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32594.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, rkrux, w0xlt
Stale ACK	janb84

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

aspostrophe -> apostrophe [misspelling]

drahtbot_id_4_m

achow101 renamed this:
~~Getaddrinfo normalized parent~~
wallet, rpc: Return normalized descriptor in parent_descs
on May 22, 2025

Sjors commented at 10:31 am on May 23, 2025: member

Concept ACK

In general normalized descriptors are more useful, because you can’t derive addresses from e.g. xpub/1h/*. So it makes sense to use them for the parent.

ACK 107517ea7d29d959e7a0b94d8217dcb894680547

I checked that the tests fail if I revert the first commit.

nit: “prividing” -> “providing”

janb84 commented at 12:34 pm on May 23, 2025: contributor

ACK 107517ea7d29d959e7a0b94d8217dcb894680547

Before change listtransactions (and others) shows un-normalized descriptors, after the descriptors are normalized

 0 {
 1    "address": "bcrt1qgymj02gx6dzrkh49d9sj0cj8rxfcrut7dyxz7r",
 2    "parent_descs": [
 3      "wpkh(tpubD6NzVbkrYhZ4YdzwuYwQLEypddJ64KVEG9tyZibLxLKMxyMCaru1c7RFX2S8NEQ7dZuNccdzk5qikw51rxzXYWdkeuREgxgdqbJy3ty68qy/84h/1h/0h/0/*)#tz7s9af2"
 4    ],
 5    "category": "immature",
 6    "amount": 50.00000000,
 7    "label": "",
 8    "vout": 0,
 9    "abandoned": false,
10    "confirmations": 1,
11    "generated": true,
12  [...]

this commit:

 0    "address": "bcrt1qgwme02jk087aessn9tht9yjkawdxys3xm36cy8",
 1    "parent_descs": [
 2      "wpkh([83869ebd/84h/1h/0h]tpubDCS1CwM385BY77ffiamcKJ4CnZc2G15YxGXeisvNhkYzocxNcSjLrPZL8KaMtYLXSCo9ng7XRrX7q7Yn7oh2652QU79ybjh4YoY7m5zZxgN/0/*)#ettrwdmf"
 3    ],
 4    "category": "immature",
 5    "amount": 50.00000000,
 6    "label": "",
 7    "vout": 0,
 8    "abandoned": false,
 9    "confirmations": 1,
10    "generated": true,
11    [...]

DrahtBot requested review from Sjors on May 23, 2025

achow101 commented at 6:26 pm on May 23, 2025: member

nit: “prividing” -> “providing”

Fixed

achow101 force-pushed on May 23, 2025

janb84 commented at 9:41 pm on May 23, 2025: contributor

re ACK a759a22d59e805834d077a28c95695e4834983a9

Changes since last ACK:

small typo change in commit

in test/functional/wallet_descriptor.py:51 in a759a22d59 outdated

46+        # Next extract the xpub
47+        desc_verify = re.sub(r"tpub\w+?(?=/)", "", desc_verify)
48+
49+        # Walk the remaining descriptor backwards looking for the hardened indicator.
50+        # It should not be found until after a ']' is found, and before a '['
51+        # The hardened indicator 'h' should not be found until after a ']' is found

rkrux commented at 12:51 pm on May 27, 2025:

Nit: Most likely this sentence was reworded midway because it ends abruptly on first line and the content is duplicated on the next.

achow101 commented at 6:36 pm on May 27, 2025:

Removed

in src/wallet/rpc/util.cpp:114 in 2bdc5aeef0 outdated

108@@ -109,7 +109,10 @@ void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey,
109 {
110     UniValue parent_descs(UniValue::VARR);
111     for (const auto& desc: wallet.GetWalletDescriptors(script_pubkey)) {
112-        parent_descs.push_back(desc.descriptor->ToString());
113+        std::string desc_str;
114+        FlatSigningProvider provider;
115+        if (!CHECK_NONFATAL(desc.descriptor->ToNormalizedString(provider, desc_str, &desc.cache))) continue;

rkrux commented at 1:44 pm on May 27, 2025:

IIUC, this is just a dummy signing provider being passed here and essentially the last hardened xpub would be retrieved from the desc cache because I don’t see this provider being populated in the call stack.

https://github.com/bitcoin/bitcoin/blob/f7cc7f6468afeb20b01ee86575c7b6329ed2faf9/src/script/descriptor.cpp#L533-L536

Ideally, passing a null for the provider arg could have been better to emphasise on this but I see the function signature accepts a reference instead.

https://github.com/bitcoin/bitcoin/blob/f7cc7f6468afeb20b01ee86575c7b6329ed2faf9/src/script/descriptor.h#L117-L118

A less ideal alternative would be to explicitly name the arg here dummy_provider to make it clear to the reader.

0FlatSigningProvider dummy_provider;

achow101 commented at 6:36 pm on May 27, 2025:

Done

rkrux commented at 1:44 pm on May 27, 2025: contributor

ACK a759a22d59e805834d077a28c95695e4834983a9

Agree that normalized descriptors are more useful as no further hardened derivation is required.

Not suggesting any change, only a question: Specific for the listunspent RPC though where both parent_descs and desc fields are present, is parent_descs field useful only when the unspent is not solvable? Because parent_descs seem like the superset of desc from few values I checked and also by its name.

achow101 commented at 6:11 pm on May 27, 2025: member

is parent_descs field useful only when the unspent is not solvable?

No. The purpose is to allow identifying the actual descriptor in the wallet that produced the address. These are supposed to be the same descriptors as output by listdescriptors false.

wallet, rpc: Push the normalized parent descriptor

Instead of providing the descriptor string as stored in the db, use the
normalized descriptor as is done for getaddressinfo's parent_desc field.

3fc9d9f241

test: Enable default wallet for wallet_descriptor.py 2554cee988

achow101 force-pushed on May 27, 2025

janb84 commented at 8:51 pm on May 27, 2025: contributor

re ACK fdabfc7b01ec2cff5f2356e1b13b5537d30dbaf8

Changes since last ACK:

removal of erroneous / double comment
rename of variable to explicit make clear it’s a dummy

DrahtBot requested review from rkrux on May 27, 2025

in test/functional/wallet_descriptor.py:59 in fdabfc7b01 outdated

55+        for c in desc_verify[::-1]:
56+            if c == "]":
57+                found_origin_end = True
58+            elif c == "[":
59+                break
60+            elif c == "h" or c == "'":

rkrux commented at 10:05 am on May 28, 2025:

0 or c == "'"

I don’t think checking for apostrophe is really required specifically in this case because as I checked in the code that only the hardened derive types are used when the SPKMs are setup & that’s what this test is using. https://github.com/bitcoin/bitcoin/blob/88b22acc3d6faeb2c0860695783a2a082d4c8098/src/wallet/walletutil.cpp#L58-L75

And the ToNormalizedString doesn’t use the compact string version as well. https://github.com/bitcoin/bitcoin/blob/88b22acc3d6faeb2c0860695783a2a082d4c8098/src/script/descriptor.cpp#L496-L502

https://github.com/bitcoin/bitcoin/blob/88b22acc3d6faeb2c0860695783a2a082d4c8098/src/script/descriptor.cpp#L470-L474

From what I looked in the importdescriptors RPC flow, the apostrophe would not be used in the normalized forms of such descriptors as well but I guess fine to keep this check - not a big thing.

achow101 commented at 7:51 pm on June 2, 2025:

Good point, dropped the apostrophe check.

in src/wallet/rpc/util.cpp:114 in fdabfc7b01 outdated

108@@ -109,7 +109,10 @@ void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey,
109 {
110     UniValue parent_descs(UniValue::VARR);
111     for (const auto& desc: wallet.GetWalletDescriptors(script_pubkey)) {
112-        parent_descs.push_back(desc.descriptor->ToString());
113+        std::string desc_str;
114+        FlatSigningProvider dummy_provider;
115+        if (!CHECK_NONFATAL(desc.descriptor->ToNormalizedString(dummy_provider, desc_str, &desc.cache))) continue;

rkrux commented at 10:09 am on May 28, 2025:

I wanted to see how the case would be handled by CHECK_NONFATAL when for some buggy reason the call here fails. I simulated a scenario and found that the error is propagated to the RPC result - good.

0➜  bitcoin git:(getaddrinfo-normalized-parent) ✗ bitcoinclireg listunspent
1error code: -1
2error message:
3Internal bug detected: false
4wallet/rpc/util.cpp:115 (PushParentDescriptors)
5Bitcoin Core v29.99.0-a759a22d59e8-dirty
6Please report this issue here: https://github.com/bitcoin/bitcoin/issues

rkrux commented at 12:19 pm on May 28, 2025: contributor

ACK fdabfc7b01ec2cff5f2356e1b13b5537d30dbaf8

Thanks for incorporating the previously suggested small changes.

in test/functional/wallet_descriptor.py:58 in fdabfc7b01 outdated

61+                if found_origin_end:
62+                    found_hardened_in_origin = True
63+                else:
64+                    found_hardened_after_origin = True
65+        assert_equal(found_hardened_in_origin, True)
66+        assert_equal(found_hardened_after_origin, False)

w0xlt commented at 0:29 am on May 29, 2025:

 0       # 1) Find the first “[ … ]”-wrapped bit and capture what’s inside it
 1        origin_match = re.search(r'\[([^\]]*)\]', desc_verify)
 2        # 2) If we got a match, pull out the inner text; otherwise use empty string
 3        origin_part = origin_match.group(1) if origin_match else ""
 4        # 3) Remove everything up to and including the first ‘]’, leaving only what follows
 5        after_origin = re.sub(r'^.*?\]', '', desc_verify) if origin_match else desc_verify
 6        # 4) Look for any hardened markers (either “h” or apostrophe) inside each piece
 7        found_hardened_in_origin = bool(re.search(r"[h']", origin_part))
 8        found_hardened_after_origin = bool(re.search(r"[h']", after_origin))
 9
10        assert_equal(found_hardened_in_origin, True)
11        assert_equal(found_hardened_after_origin, False)

w0xlt commented at 0:30 am on May 29, 2025:

nit

achow101 commented at 7:52 pm on June 2, 2025:

Taken suggestion, with some modifications.

w0xlt commented at 0:29 am on May 29, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32594/commits/fdabfc7b01ec2cff5f2356e1b13b5537d30dbaf8

test: Verify parent_desc in RPCs

getaddressinfo, listunspent, listtransactions, listsinceblock, and
gettransaction all include parent_desc(s). Make sure that these are
consistent with each other, as well as being in normalized form.

0def84d407

achow101 force-pushed on Jun 2, 2025

Sjors commented at 5:14 am on June 3, 2025: member

re-utACK 0def84d407

“vcpkg install failed” on Windows CI seems unrelated.

DrahtBot requested review from rkrux on Jun 3, 2025

DrahtBot requested review from janb84 on Jun 3, 2025

DrahtBot requested review from w0xlt on Jun 3, 2025

in test/functional/wallet_descriptor.py:47 in 0def84d407

43+
44+        # Verify that the parent descriptor is normalized
45+        # First remove the checksum
46+        desc_verify = parent_desc.split("#")[0]
47+        # Next extract the xpub
48+        desc_verify = re.sub(r"tpub\w+?(?=/)", "", desc_verify)

rkrux commented at 12:37 pm on June 3, 2025:

Nit for the comment: This removes the xpub by substituting it with emptiness, the resultant desc_verify contains everything in the descriptor but the xpub.

rkrux commented at 12:48 pm on June 3, 2025:

r"tpub\w+?(?=/)"

The \w will accept an xpub with underscore as well that is not allowed, but ok.

in test/functional/wallet_descriptor.py:49 in 0def84d407

45+        # First remove the checksum
46+        desc_verify = parent_desc.split("#")[0]
47+        # Next extract the xpub
48+        desc_verify = re.sub(r"tpub\w+?(?=/)", "", desc_verify)
49+        # Extract origin info
50+        origin_match = re.search(r'\[([\da-fh/]+)\]', desc_verify)

rkrux commented at 12:41 pm on June 3, 2025:

I checked that this Regex is fine because the origin fingerprint can only be a hex string (\da-f) and the origin_part is found even if the desc_verify doesn’t contain hardened marker such as in wpkh([58d258be/84/1/0]/0/*. That’s why I believe the subsequent "h" in origin_part check is required.

in test/functional/wallet_descriptor.py:54 in 0def84d407

50+        origin_match = re.search(r'\[([\da-fh/]+)\]', desc_verify)
51+        origin_part = origin_match.group(1) if origin_match else ""
52+        # Split on "]" for everything after the origin info
53+        after_origin = desc_verify.split("]", maxsplit=1)[-1]
54+        # Look for the hardened markers “h” inside each piece
55+        # We don't need to check for aspostrophe as normalization will not output aspostrophe

rkrux commented at 12:41 pm on June 3, 2025:

If retouched.

0- aspostrophe
1+ apostrophe

rkrux commented at 12:54 pm on June 3, 2025: contributor

ACK 0def84d407facd319b52826d013cad0d5fc8dbf5

Apparently I didn’t have the fdabfc7b01ec2cff5f2356e1b13b5537d30dbaf8 commit in my local, maybe I forgot to check it out earlier.

0git range-diff a759a22...0def84d

Good job replacing the loop in the test with the regex.

w0xlt commented at 6:30 pm on June 11, 2025: contributor

reACK https://github.com/bitcoin/bitcoin/pull/32594/commits/0def84d407facd319b52826d013cad0d5fc8dbf5

glozow merged this on Jun 13, 2025

glozow closed this on Jun 13, 2025

wallet, rpc: Return normalized descriptor in parent_descs #32594

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)