descriptors: Allow `H` as a hardened indicator

achow101 commented at 11:14 pm on June 20, 2025: member

BIP 380 states that H is a valid hardened indicator.

descriptors: Allow H as a hardened indicator

BIP 380 states that H is a valid hardened indicator.

b6252655fe

DrahtBot added the label Descriptors on Jun 20, 2025

DrahtBot commented at 11:14 pm on June 20, 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/32788.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	davidgumberg, pablomartin4btc, janb84, w0xlt
Concept ACK	jonatack, rkrux, Sjors

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32724 (Musig2 tests by w0xlt)

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.

achow101 renamed this:
~~descriptors: Allow H as a hardened indicator~~
descriptors: Allow `H` as a hardened indicator
on Jun 20, 2025

davidgumberg commented at 1:49 am on June 21, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32788/commits/b6252655fe502ed1bbb97ab9597d9bbcf8c4484c

Conforms to BIP-380: https://github.com/bitcoin/bips/blob/master/bip-0380.mediawiki#key-expressions

jonatack commented at 1:58 am on June 21, 2025: member

Concept ACK, see https://github.com/bitcoin/bips/pull/1803

pablomartin4btc commented at 3:34 pm on June 22, 2025: member

ACK b6252655fe502ed1bbb97ab9597d9bbcf8c4484c

DrahtBot requested review from jonatack on Jun 22, 2025

in src/script/descriptor.cpp:1407 in b6252655fe

1403@@ -1404,7 +1404,7 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr
1404     bool hardened = false;
1405     if (elem.size() > 0) {
1406         const char last = elem[elem.size() - 1];
1407-        if (last == '\'' || last == 'h') {
1408+        if (last == '\'' || last == 'h' || last == 'H') {

pablomartin4btc commented at 3:42 pm on June 22, 2025:

nit: perhaps you can add a comment about the key expression specification in BIP380..

Dorex45 commented at 10:48 pm on June 22, 2025: none

Great update! Supporting both ‘H’ and ‘h’ for hardened keys makes things easier for everyone and helps avoid confusion. The extra tests are a nice touch too. Thanks for making this even more user-friendly!

janb84 commented at 8:33 am on June 23, 2025: contributor

ACK b6252655fe502ed1bbb97ab9597d9bbcf8c4484c

This PR adds capital H to the parser as a valid Hardened indicator for the descriptor. Capital H usage as an indicator is per BIP-380 spec. This makes the ParseKeyPathNum function adhere (more) to the BIP spec.

Code review ✅
Build and tested ✅
Checked that the test fails if the addition to the parser is removed. ✅

in src/test/descriptor_tests.cpp:563 in b6252655fe

559@@ -554,6 +560,7 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
560     Check("combo([01234567]xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc)", "combo([01234567]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)", "combo([01234567]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)", SIGNABLE, {{"2102d2b36900396c9282fa14628566582f206a5dd0bcc8d5e892611806cafb0301f0ac","76a91431a507b815593dfc51ffc7245ae7e5aee304246e88ac","001431a507b815593dfc51ffc7245ae7e5aee304246e","a9142aafb926eb247cb18240a7f4c07983ad1f37922687"}}, std::nullopt, /*op_desc_id=*/uint256{"7f127f7861594e3ede449eb47a7cc623c753acc0b0f0fc03fbb2dac636c20d6f"});
561     Check("pk(xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0)", "pk(xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0)", "pk(xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0)", DEFAULT, {{"210379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3ac"}}, std::nullopt, /*op_desc_id=*/uint256{"0e54cf04f2bb8d607e2241d611d169c6f7d78f0ab1f15a80642192a19fbdb7cc"}, {{0}});
562     Check("pkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/2147483647'/0)", "pkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/2147483647'/0)", "pkh([bd16bee5/2147483647h]xpub69H7F5dQzmVd3vPuLKtcXJziMEQByuDidnX3YdwgtNsecY5HRGtAAQC5mXTt4dsv9RzyjgDjAQs9VGVV6ydYCHnprc9vvaA5YtqWyL6hyds/0)", HARDENED, {{"76a914ebdc90806a9c4356c1c88e42216611e1cb4c1c1788ac"}}, OutputType::LEGACY, /*op_desc_id=*/uint256{"35a5cf511e941a35b9cb0cf515d3ef887aa4246db87d6af463265a386ad856fe"}, {{0xFFFFFFFFUL,0}});
563+    Check("pkh([deadbeef/1H/1'/1h]xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/2147483647H/0h/0'/0)", "pkh([deadbeef/1H/1'/1h]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/2147483647H/0h/0'/0)", "pkh([deadbeef/1h/1h/1h/2147483647h/0h/0h]xpub6D9zrB5MDjRMPNekz6itAEnsEXGu6vM57F8UYAkK66vUchHcACsMtvafSvTq7qS1LvB8RV9MkUiYvUrDgUTg7xYxTKAHV2ptuZSqJRoiqv8/0)", HARDENED, {{"76a914dafb9a0f0bc515b9ba064af19fb2e22c575f0b3188ac"}}, OutputType::LEGACY, /*op_desc_id=*/std::nullopt, {{0x80000001UL, 0x80000001UL, 0x80000001UL, 0xFFFFFFFFUL, 0x80000000UL, 0x80000000UL, 0}}, /*spender_nlocktime=*/0, /*spender_nsequence=*/CTxIn::SEQUENCE_FINAL, /*preimages=*/{}, "pkh([deadbeef/1'/1'/1']xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/2147483647'/0'/0'/0)", "pkh([deadbeef/1'/1'/1']xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/2147483647'/0'/0'/0)");

rkrux commented at 11:06 am on June 23, 2025:

+1 on passing the max int value in the test.

rkrux commented at 11:16 am on June 23, 2025: contributor

Concept ACK b6252655fe502ed1bbb97ab9597d9bbcf8c4484c

I feel the following site should also be updated for uniformity in this PR.

Nit: This condition can also be extracted out in a IsHardenedIndicator(char input) utility that can be reused to avoid any misses in the future.

https://github.com/bitcoin/bitcoin/blob/c5849663baa925a5291731e4e4013a08c811c646/src/test/descriptor_tests.cpp#L495

Sjors commented at 11:27 am on June 23, 2025: member

Concept ACK

Can you modify one of the functional tests to cover the use of H in importdescriptors (e.g. wallet_signer.py) and getdescriptorinfo?

scgbckbone commented at 11:41 am on June 23, 2025: none

Why is another way to specify hardened derivation needed? Is anyone using it? Seems better to remove H from BIP.

I’m sure most of the software out there rejects anything besides h '

rkrux changes_requested

rkrux commented at 12:03 pm on June 23, 2025: contributor

In addition to this comment #32788#pullrequestreview-2949654103,

this site for hardened range derivation should also be updated inside ParsePubkeyInner that is called via descriptor Parse, I think a common utility function here could prove to be robust. Maybe a functional test change as suggested in this #32788 (comment) could have caught this discrepancy. https://github.com/bitcoin/bitcoin/blob/c5849663baa925a5291731e4e4013a08c811c646/src/script/descriptor.cpp#L1567

Also, updated comments in the following places would be good to have:

0➜  bitcoin git: git grep -n "' or h" src/script/descriptor.cpp
1src/script/descriptor.cpp:374:    // Whether ' or h is used in harded derivation
2src/script/descriptor.cpp:1648: * [@param](/bitcoin-bitcoin/contributor/param/)[in] split BIP32 path string, using either ' or h for hardened derivation

rkrux commented at 12:13 pm on June 23, 2025: contributor

Why is another way to specify hardened derivation needed? Is anyone using it? Seems better to remove H from BIP.

I would prefer the removal from BIP if no other software is using it but I’m not sure if there is a way to comprehensively verify if this has not been used in the past (https://github.com/bitcoin/bips/pull/1643). And with the lack of such a verification, the allowance in core wallet seems required to me.

Sjors commented at 12:17 pm on June 23, 2025: member

My guess is that some wallets use ALL CAPS descriptors in order to generate smaller QR codes.

https://shkspr.mobi/blog/2025/02/why-are-qr-codes-with-capital-letters-smaller-than-qr-codes-with-lower-case-letters/

Not sure how useful that is when you have a mixed case xpub and other characters that are not in the limited set, e.g. ()[], though mixed-mode is possible: https://www.barcodefaq.com/2d/qr-code/qr-code-auto-encoding-mode/

scgbckbone commented at 5:30 pm on June 23, 2025: none

Why is another way to specify hardened derivation needed? Is anyone using it? Seems better to remove H from BIP.

I would prefer the removal from BIP if no other software is using it but I’m not sure if there is a way to comprehensively verify if this has not been used in the past (bitcoin/bips#1643). And with the lack of such a verification, the allowance in core wallet seems required to me.

i think the argument is circular. How those “softwares” worked up until this point?

Adding it now to ref implementation will cause that someone will start using it and then all wallet implementations will need to implement, or stay incompatible.

thanks @Sjors, I didn’t know that is possible. I thought you need just alphanumeric for that.

achow101 commented at 6:28 pm on June 23, 2025: member

How those “softwares” worked up until this point?

We don’t know, which is kind of the point of this PR.

The text of the BIP has always stated that H is valid hardened indicator, so anyone implementing from just the description of the text should have included H.

The document the BIP is based on (doc/descriptors.md) did not state that H is a valid hardened indicator.

However, there was also a test vector that had H as an invalid expression. Anyone who also implemented this test vector would have noticed that the text of the BIP did not match what this test vector stated was invalid. But this test vector was also added to the BIP about 2 years after the BIP was originally written.

Since this discrepancy was only flagged a few months ago, it seems like there are multiple possibilities of what could have happened:

Many implementations of descriptors was done prior to the test vectors, and probably prior to the BIP being written, as we know descriptors was specified long before the BIP. Such implementations may or may not have implemented it correctly.
Some implementations of descriptors were done after the test vectors were added, but did not implement the test vectors, or implemented based off of existing implementations which may or may not have allowed H.
Few implementations were done specifically from the text and tests alone, as no one flagged this discrepancy for multiple years.

The 2 primary implementations of descriptors are probably Bitcoin Core and rust-miniscript. Looking at rust-miniscript, it seems that they also did not allow H. However, even if people tested against Bitcoin Core and rust-bitcoin, it’s possible that they could be allowing H as they may not be producing descriptors with H. For example, it looks like libwally allows H as a hardened indicator, so any implementation that uses libwally for handling things like extended keys and derivation paths are probably allowing H.

Maybe I’ll ask on the mailing list again.

davidgumberg commented at 6:58 pm on June 23, 2025: contributor

Adding it now to ref implementation will cause that someone will start using it and then all wallet implementations will need to implement, or stay incompatible.

In my understanding, BIP 380 is intended to describe an implementation-independent language for describing output scripts, while Bitcoin Core is specified as a reference implementation in the BIP, isn’t it a better course of action to modify the reference implementation to more closely match the spec than to change out the spec from under existing ¹ implementations?

Sparrow: https://github.com/sparrowwallet/drongo/blob/13e1fafbe8892d7005a043ae561e09ed66f7cea6/src/main/java/com/sparrowwallet/drongo/KeyDerivation.java#L69 Libwally (used by Blockstream Green): https://github.com/ElementsProject/libwally-core/blob/12ab3fa22cbdc26040a9900d02cdb03da5164c85/src/bip32.c#L80-L87 I also checked NBitcoin (BTCPayServer), Electrum, rust-miniscript (BDK) and they don’t allow H. ↩︎

Ademan commented at 8:08 pm on June 23, 2025: none

My guess is that some wallets use ALL CAPS descriptors in order to generate smaller QR codes.

https://shkspr.mobi/blog/2025/02/why-are-qr-codes-with-capital-letters-smaller-than-qr-codes-with-lower-case-letters/

Not sure how useful that is when you have a mixed case xpub and other characters that are not in the limited set, e.g. ()[], though mixed-mode is possible: https://www.barcodefaq.com/2d/qr-code/qr-code-auto-encoding-mode/

If I did my back of the napkin math correctly, you could encode an xpub into a format like bech32, encoded into QR alphanumeric mode and save ~15% over a base58 encoded xpub in QR binary mode.

Since afaik nobody has bothered exploring this, maybe it’s of dubious value, and if you wanted to encode full descriptors you’d probably need some kind of escape sequence for (,),[,], etc... that might hamper readability to the point of “why bother?”.

Sjors commented at 7:14 am on June 24, 2025: member

you could encode an xpub into a format like bech32, encoded into QR alphanumeric mode and save ~15% over a base58 encoded xpub in QR binary mode

I think it would be nice in general to have a bech32m encoding for extended keys, because part of the reason why descriptors look weird is the mix of bech32 and base58. Like Silent Payment addresses it could just concatenate the public key and chain code. For the human readable prefix xpub, xprv, tpub, tprv would make sense. Add a version for good measure. E.g. tpub1qqfvn9pmvmz0ewpnp7w302lxqmnue2kgtpne2p38nuunun883sw36yq48ny7n2jl0nx9ljhmdnrgvpee6aufmg9wfvqfcr6c02at6r4u4xsegph7a (nonsensical testnet xpub based on https://gist.github.com/kdmukai/e6de87c90d4fedeaf866f2b10c0833a9)

https://github.com/bitcoin/bips/blob/master/bip-0352.mediawiki#address-encoding

Additionally you could allow a slightly shorter encoding without checksum, since the descriptor itself already has one, but then it wouldn’t comply with bech32m. And people and software might copy xpubs around to generate more complex wallets setups, so they should always have a checksum.

w0xlt commented at 10:04 pm on June 24, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32788/commits/b6252655fe502ed1bbb97ab9597d9bbcf8c4484c

DrahtBot requested review from Sjors on Jun 24, 2025

DrahtBot requested review from rkrux on Jun 24, 2025

glozow added the label Needs Conceptual Review on Jun 25, 2025

glozow commented at 6:30 pm on June 25, 2025: member

Not merging for now - IIUC if the approach mentioned in https://groups.google.com/u/1/g/bitcoindev/c/IAYEx4zUhHA is taken, this would be closed?

pythcoiner commented at 3:52 am on June 28, 2025: none

Is anyone using it?

support for H have been introduced in embit in this commit but afair I add H because it was in the BIP not because it was needed.

descriptors: Allow H as a hardened indicator #32788

Code Coverage & Benchmarks

Reviews

Conflicts

descriptors: Allow `H` as a hardened indicator #32788