Switch hardened derivation marker to h by Sjors · Pull Request #26076 · bitcoin/bitcoin

Sjors · 2022-09-13T13:19:21Z

This makes it easier to handle descriptor strings manually, especially when importing from another Bitcoin Core wallet.

For example the importdescriptors RPC call is easiest to use h as the marker: '["desc": ".../0h/..."]', avoiding the need for escape characters. With this change listdescriptors will use h, so you can copy-paste the result, without having to add escape characters or switch ' to 'h' manually.

Both markers can still be parsed.

The hdkeypath field in getaddressinfo is also impacted by this change, except for legacy wallets. The latter is to prevent accidentally breaking ancient software that uses our legacy wallet.

See discussion in #15740

furszy

Concept ACK

DrahtBot · 2022-09-14T09:30:27Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	darosior, achow101
Stale ACK	sipa, w0xlt, furszy

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:

#26627 (wallet: Migrate non-HD keys with single combo containing a list of keys by achow101)
#26626 (descriptors: Add a KEY expression representing a list of individual keys by achow101)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

luke-jr · 2022-09-19T23:36:00Z

doc/release-notes-26076.md

The enclosing single quotes are shell escaping, not part of the actual RPC call (which has no issue at present).

By "manually" I mean using bitcoin-cli from the shell. Afaik the GUI console - probably used more often - has the same problem.

jarolrod · 2022-09-23T07:11:03Z

concept ack

achow101 · 2022-10-27T18:33:44Z

ACK fc5594d036a47434ea301f5e6adff06c59649fd2

sipa · 2022-10-27T21:37:32Z

I'm in favor of using h instead of ' as default, but I worry that just rendering every descriptor with h instead will break things. Right now, ' roundtrips in descriptor parsing/serializing, and this PR breaks that.

Would it be unreasonable to make the descriptor classes remember which of the two was used when parsing, and reuse the same one when serializing? That guarantees roundtripping, but defaults could still prefer h for new and/or inferred descriptors.

Not a NACK, but I'd be more comfortable with just honoring whatever was parsed.

sipa · 2022-10-27T21:38:01Z

My apologies for the accidental close. That was unintentional.

Sjors · 2022-10-31T13:57:28Z

Made a slight documentation change to the first commit and then pushed three more which change the following behaviour:

Normalized descriptors use h (this is useful when using listwalletdescriptors with older wallets)
Non-normalized descriptors (including the private string) use whathever was provided at creation time, as @sipa requested
New wallets use h instead of \'

I could squash the new behavior into the original commit, but I'll wait for concept ACK.

achow101 · 2022-11-01T21:28:45Z

Concept ACK

Please squash

Sjors · 2022-11-03T08:23:10Z

@achow101 squashed

achow101 · 2022-11-03T16:47:08Z

src/script/descriptor.cpp

Should this consider normalized as well?

I intentionally didn't normalize the private-key versions. Though I could.

What was the rationale for only normalizing the public-key version with h?

We don't normalize the private key version of descriptors in general. I'm not sure if it matters a lot, but would prefer to keep such a change to followup.

achow101 · 2022-11-03T16:51:36Z

src/script/descriptor.cpp

Does apostrophe need to be optional? Either the path uses an apostrophe, or it doesn't. And it doesn't matter if the path is not hardened at all.

In one of the call sites it first checks the origin and then the rest of the path. The optional is necessary to combine those results, especially in the case where the origin doesn't have any hardened derivation (one of the test cases).

I'm not seeing how it is necessary. It could initialized to false in ParsePubKey and I think that would achieve the same result. Nothing is checking whether the optional has a value, except at the very end with value_or(false), and that's equivalent to initializing as false.

If I initialise to false, it would be set to true when checking the origin and then back to false when checking the path.

(at minimum this confusion suggests I should add more comments)

This seems to work:

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index e639e494582..877d2538b2c 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -1056,7 +1056,7 @@ enum class ParseScriptContext { }; /** Parse a key path, being passed a split list of elements (the first element is ignored). */ -[[nodiscard]] bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out, std::optional<bool>& apostrophe, std::string& error) +[[nodiscard]] bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out, bool& apostrophe, std::string& error) { for (size_t i = 1; i < split.size(); ++i) { Span<const char> elem = split[i]; @@ -1083,7 +1083,7 @@ enum class ParseScriptContext { } /** Parse a public key that excludes origin information. */ -std::unique_ptr<PubkeyProvider> ParsePubkeyInner(uint32_t key_exp_index, const Span<const char>& sp, ParseScriptContext ctx, FlatSigningProvider& out, std::optional<bool>& apostrophe, std::string& error) +std::unique_ptr<PubkeyProvider> ParsePubkeyInner(uint32_t key_exp_index, const Span<const char>& sp, ParseScriptContext ctx, FlatSigningProvider& out, bool& apostrophe, std::string& error) { using namespace spanparsing; @@ -1149,7 +1149,7 @@ std::unique_ptr<PubkeyProvider> ParsePubkeyInner(uint32_t key_exp_index, const S extpubkey = extkey.Neuter(); out.keys.emplace(extpubkey.pubkey.GetID(), extkey.key); } - return std::make_unique<BIP32PubkeyProvider>(key_exp_index, extpubkey, std::move(path), type, apostrophe.value_or(false)); + return std::make_unique<BIP32PubkeyProvider>(key_exp_index, extpubkey, std::move(path), type, apostrophe); } /** Parse a public key including origin information (if enabled). */ @@ -1163,7 +1163,7 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(uint32_t key_exp_index, const Span<c return nullptr; } // This is set if either the origin or path suffix contains a hardened deriviation. - std::optional<bool> apostrophe; + bool apostrophe = false; if (origin_split.size() == 1) { return ParsePubkeyInner(key_exp_index, origin_split[0], ctx, out, apostrophe, error); } @@ -1190,7 +1190,7 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(uint32_t key_exp_index, const Span<c if (!ParseKeyPath(slash_split, info.path, apostrophe, error)) return nullptr; auto provider = ParsePubkeyInner(key_exp_index, origin_split[1], ctx, out, apostrophe, error); if (!provider) return nullptr; - return std::make_unique<OriginPubkeyProvider>(key_exp_index, std::move(info), std::move(provider), apostrophe.value_or(false)); + return std::make_unique<OriginPubkeyProvider>(key_exp_index, std::move(info), std::move(provider), apostrophe); } std::unique_ptr<PubkeyProvider> InferPubkey(const CPubKey& pubkey, ParseScriptContext, const SigningProvider& provider)

Ah yes, because ParseKeyPath leaves &apostrophe alone if it doesn't find hardened derivation. Fixed and added some documentation.

achow101 · 2022-11-03T17:46:59Z

src/test/descriptor_tests.cpp

It's not clear to me why this and the change in Check are necessary?

Could be a left-over from the squash. Will look into this once we decide whether or not to normalise private keys, because that might change things again: #26076 (comment)

Not a left-over; the test fails without it. The behaviour is quite different now that we echo the original descriptor rather than always put ' in it.

JeremyRubin · 2022-11-09T16:25:42Z

it might make sense to have new wallets only generate h by default if opted into via flag since there is still lots of software that might not be compatible with a new format.

Sjors · 2022-11-09T16:52:51Z

@JeremyRubin is there software that can't parse BIP32 paths with h? I'd rather stalk them a bit during the next six months (before v25.0) than work around the issue :-)

Rebased.

JeremyRubin · 2022-11-09T18:11:28Z

ah nope i misremembered that rust-bitcoin didn't support

darosior

light ACK bd13dc2. Code looks correct to me but i find the OP and release note misleading (stated purpose is already possible and not what this PR implements) and i'm a bit confused about the choice of where to make breaking changes:

Why not honour what was parsed for normalized serialization?
Why keep serializing hdkeypath's hardened paths with apostrophe for legacy wallet but not descriptor wallets or elsewhere (like in decodepsbt)?

Just to be clear: no strong opinion, and i don't mean to be blocking this. I think it's safe. Just i don't get the rationale for where to (slightly) break the API or not.

darosior · 2023-05-04T15:39:05Z

src/wallet/rpc/addresses.cpp

+                // In legacy wallets hdkeypath has always used an apostrophe for
+                // hardened derivation. Perhaps some external tool depends on that.
+                ret.pushKV("hdkeypath", WriteHDKeypath(meta->key_origin.path, /*apostrophe=*/!desc_spk_man));


But so has it in descriptor wallets? Why would it be more ok to make a breaking change for descriptor wallets than for legacy wallet?

Descriptors have always required parsers to handle both h and ', and I'm not aware of any that don't. For old BIP32 derivation parsing software I'm less sure about that.

darosior · 2023-05-04T16:20:09Z

doc/release-notes-26076.md

+  wallets use `h`. This change makes it easier to handle descriptor strings manually.
+  E.g. an RPC call that takes an array of descriptors can now use '["desc": ".../0h/..."]'.


This is already possible on master (and also at least on 24.0.1). This PR does not change that. For instance on a 24.0.1 regtest node:

importdescriptors '[{"desc":"pk([00aabbcc/0h/1h]cSWVo8YMehg6STxqw6xGgmMJE6Ac6RCJ7owcrBZRAT7QANtyAAEj)#uqdekwuc","timestamp":"now"}]'

works fine. So does:

scantxoutset start '["pk([00aabbcc/0h/1h]cSWVo8YMehg6STxqw6xGgmMJE6Ac6RCJ7owcrBZRAT7QANtyAAEj)#uqdekwuc"]'

I'll clarify what I meant here: you can copy-paste the result of listdescriptors when doing such an import, because this now uses h instead of '

darosior · 2023-05-05T10:14:49Z

doc/release-notes-26076.md

+RPC
+---
+
+- The `listdescriptors` RPC now shows `h` rather than apostrophe (`'`) to indicate


decodepsbt too.

Added (plus "and similar")

Sjors · 2023-05-08T14:08:59Z

Thanks for the review!

The reason for not breaking legacy wallet RPC calls is that I'm assuming any code that uses legacy wallets is really old and nobody wants to touch it. On our side eventually we want to get rid of the legacy wallet - or at least freeze it - so there's no need to modernise it.

I updated the title, description and release note. Pushed the latter as a new commit to not lose ACKs.

darosior

re-ACK fe49f06

achow101 · 2023-05-08T17:13:23Z

ACK fe49f06

murchandamus · 2023-05-15T16:01:11Z

doc/release-notes-26076.md

+  For legacy wallets the `hdkeypath` field in `getaddressinfo` is unchanged,
+  nor is the serialization format of wallet dumps. (#26076)


The “nor” here seems misplaced:

Suggested change

For legacy wallets the `hdkeypath` field in `getaddressinfo` is unchanged,

nor is the serialization format of wallet dumps. (#26076)

For legacy wallets the `hdkeypath` field in `getaddressinfo`

and the serialization format of wallet dumps remain unchanged. (#26076)

furszy reviewed Sep 13, 2022

View reviewed changes

glozow added Wallet RPC/REST/ZMQ labels Sep 13, 2022

DrahtBot mentioned this pull request Sep 14, 2022

descriptors: Be able to specify change and receiving in a single descriptor string #22838

Merged

luke-jr reviewed Sep 19, 2022

View reviewed changes

sipa closed this Oct 27, 2022

sipa reopened this Oct 27, 2022

Sjors force-pushed the 2022/09/descriptors_h branch 2 times, most recently from 008df84 to 9df0fd2 Compare October 31, 2022 13:56

Sjors changed the title ~~Switch hardened derivation marker to h in descriptors~~ Switch hardened derivation marker to h in normalized descriptors and new wallets Oct 31, 2022

Sjors changed the title ~~Switch hardened derivation marker to h in normalized descriptors and new wallets~~ Switch hardened derivation marker to h (in normalized descriptors and new wallets) Oct 31, 2022

Sjors force-pushed the 2022/09/descriptors_h branch from 9df0fd2 to 80f2031 Compare November 3, 2022 08:23

DrahtBot mentioned this pull request Nov 3, 2022

[Draft / POC] Silent Payments #24897

Closed

achow101 reviewed Nov 3, 2022

View reviewed changes

sipa mentioned this pull request Nov 8, 2022

Descriptor unit tests and simplifications #24361

Closed

DrahtBot mentioned this pull request Nov 9, 2022

test: add missing bech32m / BIP86 test-cases to wallet_descriptor.py #26472

Merged

DrahtBot added the Needs rebase label Nov 9, 2022

Sjors force-pushed the 2022/09/descriptors_h branch from 80f2031 to 5df4d99 Compare November 9, 2022 17:13

DrahtBot removed the Needs rebase label Nov 9, 2022

Sjors requested a review from darosior April 27, 2023 11:36

darosior reviewed May 5, 2023

View reviewed changes

DrahtBot requested review from w0xlt and removed request for w0xlt May 5, 2023 10:17

Sjors changed the title ~~Switch hardened derivation marker to h (in normalized descriptors and new wallets)~~ Switch hardened derivation marker to h May 8, 2023

DrahtBot requested review from w0xlt and removed request for w0xlt May 8, 2023 14:06

doc: clarify PR 26076 release note

fe49f06

DrahtBot requested review from w0xlt and removed request for w0xlt May 8, 2023 14:09

darosior reviewed May 8, 2023

View reviewed changes

DrahtBot requested review from achow101 and w0xlt and removed request for w0xlt May 8, 2023 14:12

DrahtBot added the CI failed label May 8, 2023

DrahtBot removed request for achow101 and w0xlt May 8, 2023 17:13

murchandamus reviewed May 15, 2023

View reviewed changes

pinheadmz mentioned this pull request May 17, 2023

Change handling of "h" versus "'" marker for hardened derivation in descriptors #15740

Closed

Sjors mentioned this pull request May 19, 2023

Improve display address handling for external signer #24313

Merged

pinheadmz mentioned this pull request Jun 19, 2023

Adds descriptor module with getdescriptorinfo rpc bcoin-org/bcoin#1152

Merged

mzumsande mentioned this pull request Jun 20, 2023

wallets created on master get corrupted when processed with v25 #27915

Closed

1 task

Sjors mentioned this pull request Jun 22, 2023

rpc: add path to gethdkey #22341

Closed

Sjors mentioned this pull request Jul 31, 2023

ParseHDKeypath: support h as hardened marker #28192

Closed

		wallets use `h`. This change makes it easier to handle descriptor strings manually.
		E.g. an RPC call that takes an array of descriptors can now use '["desc": ".../0h/..."]'.

		For legacy wallets the `hdkeypath` field in `getaddressinfo` is unchanged,
		nor is the serialization format of wallet dumps. (#26076)

Conversation

Sjors commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarolrod commented Sep 23, 2022

Uh oh!

achow101 commented Oct 27, 2022

Uh oh!

sipa commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Oct 27, 2022

Uh oh!

Sjors commented Oct 31, 2022

Uh oh!

achow101 commented Nov 1, 2022

Uh oh!

Sjors commented Nov 3, 2022 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Nov 9, 2022

Uh oh!

Sjors commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Nov 9, 2022

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Sjors commented Sep 13, 2022 •

edited

Loading

DrahtBot commented Sep 14, 2022 •

edited

Loading

Sjors Sep 22, 2022 •

edited

Loading

sipa commented Oct 27, 2022 •

edited

Loading

Sjors commented Nov 3, 2022 •

edited by maflcko

Loading

Sjors Nov 10, 2022 •

edited

Loading

Sjors commented Nov 9, 2022 •

edited

Loading

Sjors May 8, 2023 •

edited

Loading