Switch hardened derivation marker to h #26076

pull Sjors wants to merge 2 commits into bitcoin:master from Sjors:2022/09/descriptors_h changing 20 files +162 βˆ’142
  1. Sjors commented at 1:19 pm on September 13, 2022: member

    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

  2. furszy commented at 1:31 pm on September 13, 2022: member
    Concept ACK
  3. glozow added the label Wallet on Sep 13, 2022
  4. glozow added the label RPC/REST/ZMQ on Sep 13, 2022
  5. DrahtBot commented at 9:30 am on September 14, 2022: contributor

    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.

  6. in doc/release-notes-26076.md:6 in fc5594d036 outdated
    0@@ -0,0 +1,9 @@
    1+RPC
    2+---
    3+
    4+- The `deriveaddresses`, `getdescriptorinfo` and `listdescriptors` RPC's now
    5+  use `h` rather than apostrophe (`'`) to indicate hardened derivation. This
    6+  makes it easier to handle descriptor strings manually. E.g. an RPC call that
    


    luke-jr commented at 11:36 pm on September 19, 2022:
    The enclosing single quotes are shell escaping, not part of the actual RPC call (which has no issue at present).

    Sjors commented at 9:41 am on September 22, 2022:
    By “manually” I mean using bitcoin-cli from the shell. Afaik the GUI console - probably used more often - has the same problem.
  7. jarolrod commented at 7:11 am on September 23, 2022: member
    concept ack
  8. achow101 commented at 6:33 pm on October 27, 2022: member
    ACK fc5594d036a47434ea301f5e6adff06c59649fd2
  9. sipa commented at 9:37 pm on October 27, 2022: member

    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.

  10. sipa closed this on Oct 27, 2022

  11. sipa reopened this on Oct 27, 2022

  12. sipa commented at 9:38 pm on October 27, 2022: member
    My apologies for the accidental close. That was unintentional.
  13. Sjors force-pushed on Oct 31, 2022
  14. Sjors force-pushed on Oct 31, 2022
  15. Sjors commented at 1:57 pm on October 31, 2022: member

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

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

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

  16. Sjors renamed this:
    Switch hardened derivation marker to h in descriptors
    Switch hardened derivation marker to h in normalized descriptors and new wallets
    on Oct 31, 2022
  17. Sjors renamed this:
    Switch hardened derivation marker to h in normalized descriptors and new wallets
    Switch hardened derivation marker to h (in normalized descriptors and new wallets)
    on Oct 31, 2022
  18. achow101 commented at 9:28 pm on November 1, 2022: member

    Concept ACK

    Please squash

  19. Sjors commented at 8:23 am on November 3, 2022: member
    @achow101 squashed
  20. Sjors force-pushed on Nov 3, 2022
  21. in src/script/descriptor.cpp:442 in 80f20318a5 outdated
    438     bool ToPrivateString(const SigningProvider& arg, std::string& out) const override
    439     {
    440         CExtKey key;
    441         if (!GetExtKey(arg, key)) return false;
    442-        out = EncodeExtKey(key) + FormatHDKeypath(m_path);
    443+        out = EncodeExtKey(key) + FormatHDKeypath(m_path, /*apostrophe=*/m_apostrophe);
    


    achow101 commented at 4:47 pm on November 3, 2022:
    Should this consider normalized as well?

    Sjors commented at 1:59 pm on November 8, 2022:
    I intentionally didn’t normalize the private-key versions. Though I could.

    darosior commented at 11:25 am on February 6, 2023:
    What was the rationale for only normalizing the public-key version with h?

    Sjors commented at 3:37 pm on April 4, 2023:
    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.
  22. in src/script/descriptor.cpp:1059 in 80f20318a5 outdated
    1055@@ -1047,14 +1056,18 @@ enum class ParseScriptContext {
    1056 };
    1057 
    1058 /** Parse a key path, being passed a split list of elements (the first element is ignored). */
    1059-[[nodiscard]] bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out, std::string& error)
    1060+[[nodiscard]] bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out, std::optional<bool>& apostrophe, std::string& error)
    


    achow101 commented at 4:51 pm on November 3, 2022:
    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.

    Sjors commented at 2:01 pm on November 8, 2022:
    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).

    achow101 commented at 7:00 pm on November 9, 2022:
    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.

    Sjors commented at 10:54 am on November 10, 2022:

    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)


    achow101 commented at 3:04 pm on November 10, 2022:

    This seems to work:

     0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
     1index e639e494582..877d2538b2c 100644
     2--- a/src/script/descriptor.cpp
     3+++ b/src/script/descriptor.cpp
     4@@ -1056,7 +1056,7 @@ enum class ParseScriptContext {
     5 };
     6 
     7 /** Parse a key path, being passed a split list of elements (the first element is ignored). */
     8-[[nodiscard]] bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out, std::optional<bool>& apostrophe, std::string& error)
     9+[[nodiscard]] bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out, bool& apostrophe, std::string& error)
    10 {
    11     for (size_t i = 1; i < split.size(); ++i) {
    12         Span<const char> elem = split[i];
    13@@ -1083,7 +1083,7 @@ enum class ParseScriptContext {
    14 }
    15 
    16 /** Parse a public key that excludes origin information. */
    17-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)
    18+std::unique_ptr<PubkeyProvider> ParsePubkeyInner(uint32_t key_exp_index, const Span<const char>& sp, ParseScriptContext ctx, FlatSigningProvider& out, bool& apostrophe, std::string& error)
    19 {
    20     using namespace spanparsing;
    21 
    22@@ -1149,7 +1149,7 @@ std::unique_ptr<PubkeyProvider> ParsePubkeyInner(uint32_t key_exp_index, const S
    23         extpubkey = extkey.Neuter();
    24         out.keys.emplace(extpubkey.pubkey.GetID(), extkey.key);
    25     }
    26-    return std::make_unique<BIP32PubkeyProvider>(key_exp_index, extpubkey, std::move(path), type, apostrophe.value_or(false));
    27+    return std::make_unique<BIP32PubkeyProvider>(key_exp_index, extpubkey, std::move(path), type, apostrophe);
    28 }
    29 
    30 /** Parse a public key including origin information (if enabled). */
    31@@ -1163,7 +1163,7 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(uint32_t key_exp_index, const Span<c
    32         return nullptr;
    33     }
    34     // This is set if either the origin or path suffix contains a hardened deriviation.
    35-    std::optional<bool> apostrophe;
    36+    bool apostrophe = false;
    37     if (origin_split.size() == 1) {
    38         return ParsePubkeyInner(key_exp_index, origin_split[0], ctx, out, apostrophe, error);
    39     }
    40@@ -1190,7 +1190,7 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(uint32_t key_exp_index, const Span<c
    41     if (!ParseKeyPath(slash_split, info.path, apostrophe, error)) return nullptr;
    42     auto provider = ParsePubkeyInner(key_exp_index, origin_split[1], ctx, out, apostrophe, error);
    43     if (!provider) return nullptr;
    44-    return std::make_unique<OriginPubkeyProvider>(key_exp_index, std::move(info), std::move(provider), apostrophe.value_or(false));
    45+    return std::make_unique<OriginPubkeyProvider>(key_exp_index, std::move(info), std::move(provider), apostrophe);
    46 }
    47 
    48 std::unique_ptr<PubkeyProvider> InferPubkey(const CPubKey& pubkey, ParseScriptContext, const SigningProvider& provider)
    

    Sjors commented at 10:14 am on November 11, 2022:
    Ah yes, because ParseKeyPath leaves &apostrophe alone if it doesn’t find hardened derivation. Fixed and added some documentation.
  23. in src/test/descriptor_tests.cpp:152 in 80f20318a5 outdated
    149-    } else {
    150-        parse_pub = Parse(pub, keys_pub, error);
    151+        pub = UseHInsteadOfApostrophe(pub);
    152     }
    153+    parse_pub = Parse(pub, keys_pub, error);
    154     BOOST_CHECK_MESSAGE(parse_pub, error);
    


    achow101 commented at 5:46 pm on November 3, 2022:
    It’s not clear to me why this and the change in Check are necessary?

    Sjors commented at 2:01 pm on November 8, 2022:
    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 (review)

    Sjors commented at 5:13 pm on November 9, 2022:
    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.
  24. DrahtBot added the label Needs rebase on Nov 9, 2022
  25. JeremyRubin commented at 4:25 pm on November 9, 2022: contributor
    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.
  26. Sjors commented at 4:52 pm on November 9, 2022: member

    @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.

  27. Sjors force-pushed on Nov 9, 2022
  28. JeremyRubin commented at 6:11 pm on November 9, 2022: contributor
    ah nope i misremembered that rust-bitcoin didn’t support
  29. DrahtBot removed the label Needs rebase on Nov 9, 2022
  30. Sjors force-pushed on Nov 11, 2022
  31. Sjors force-pushed on Nov 11, 2022
  32. achow101 commented at 8:58 pm on November 22, 2022: member
    ACK de83b8de1a3fee4134ceccadd0d09f5f8a5d3460
  33. fanquake requested review from sipa on Nov 22, 2022
  34. sipa commented at 9:51 pm on November 22, 2022: member
    utACK de83b8de1a3fee4134ceccadd0d09f5f8a5d3460
  35. fanquake requested review from luke-jr on Nov 23, 2022
  36. fanquake requested review from furszy on Nov 23, 2022
  37. in src/wallet/scriptpubkeyman.cpp:2296 in de83b8de1a outdated
    2290@@ -2291,7 +2291,7 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_
    2291 
    2292     // Mainnet derives at 0', testnet and regtest derive at 1'
    2293     if (Params().IsTestChain()) {
    2294-        desc_prefix += "/1'";
    2295+        desc_prefix += "/1h";
    2296     } else {
    2297         desc_prefix += "/0'";
    


    achow101 commented at 9:45 pm on December 5, 2022:
    Missed a ' here and one below at line 2300.

    Sjors commented at 10:49 am on December 6, 2022:
    Thanks, I also updated the combo() descriptor in MigrateToDescriptor.
  38. Sjors force-pushed on Dec 6, 2022
  39. achow101 commented at 10:14 pm on December 6, 2022: member
    There’s a FormatHDKeypath in ‘LegacyScriptPubKeyMan::MigrateToDescriptorthat should setapostrophe=false`
  40. w0xlt approved
  41. Sjors force-pushed on Dec 9, 2022
  42. Sjors commented at 3:17 pm on December 9, 2022: member

    There’s a FormatHDKeypath in ‘LegacyScriptPubKeyMan::MigrateToDescriptorthat should setapostrophe=false`

    Fixed.

  43. DrahtBot added the label Needs rebase on Dec 9, 2022
  44. Sjors force-pushed on Dec 12, 2022
  45. DrahtBot removed the label Needs rebase on Dec 12, 2022
  46. in src/script/descriptor.cpp:1173 in 6850bc550c outdated
    1169@@ -1148,7 +1170,11 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(uint32_t key_exp_index, const Span<c
    1170         error = "Multiple ']' characters found for a single pubkey";
    1171         return nullptr;
    1172     }
    1173-    if (origin_split.size() == 1) return ParsePubkeyInner(key_exp_index, origin_split[0], ctx, out, error);
    1174+    // This is set if either the origin or path suffix contains a hardened deriviation.
    


    furszy commented at 2:24 pm on December 19, 2022:
    s/deriviation/derivation
  47. in src/script/descriptor.cpp:1061 in 6850bc550c outdated
    1054@@ -1046,15 +1055,27 @@ enum class ParseScriptContext {
    1055     P2TR,    //!< Inside tr() (either internal key, or BIP342 script leaf)
    1056 };
    1057 
    1058-/** Parse a key path, being passed a split list of elements (the first element is ignored). */
    1059-[[nodiscard]] bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out, std::string& error)
    1060+/**
    1061+ * Parse a key path, being passed a split list of elements (the first element is ignored).
    1062+ *
    1063+ * @param[in] split BIP32 path string, using either ' or h for hardened deriviation
    


    furszy commented at 2:24 pm on December 19, 2022:
    s/deriviation/derivation
  48. furszy commented at 2:58 pm on December 19, 2022: member

    Light ACK 6850bc55

    not blocking point: To not mixup flows, would had liked to see two commits instead of one. The first one with the apostrophe read changes (the m_apostrophe field set flow) and a second one only containing the behavior change (the retrieval of descriptors using β€˜h’ instead of the apostrophe).

  49. furszy commented at 3:15 pm on December 19, 2022: member
    Also, have you thought about moving the getaddressinfo “hdkeypath” return value to use ‘h’ instead of the apostrophe? Atm, the value uses the legacy WriteHDKeypath function, which always sets apostrophe=true.
  50. achow101 commented at 5:55 pm on January 4, 2023: member
    ACK 6850bc550c67a55a939f0a47690badcc4d3b4b25
  51. achow101 requested review from sipa on Jan 31, 2023
  52. achow101 requested review from darosior on Feb 3, 2023
  53. achow101 requested review from instagibbs on Feb 3, 2023
  54. in src/wallet/scriptpubkeyman.cpp:1825 in 6850bc550c outdated
    1821@@ -1822,7 +1822,7 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
    1822 
    1823             // Make the combo descriptor
    1824             std::string xpub = EncodeExtPubKey(master_key.Neuter());
    1825-            std::string desc_str = "combo(" + xpub + "/0'/" + ToString(i) + "'/*')";
    1826+            std::string desc_str = "combo(" + xpub + "/0h/" + ToString(i) + "h/*')";
    


    darosior commented at 11:11 am on February 6, 2023:

    You are missing one occurrence in this expression:

    0            std::string desc_str = "combo(" + xpub + "/0h/" + ToString(i) + "h/*h)";
    
  55. in src/script/descriptor.cpp:427 in 6850bc550c outdated
    425-        std::string ret = EncodeExtPubKey(m_root_extkey) + FormatHDKeypath(m_path);
    426+        std::string ret = EncodeExtPubKey(m_root_extkey) + FormatHDKeypath(m_path, /*apostrophe=*/m_apostrophe);
    427         if (IsRange()) {
    428             ret += "/*";
    429-            if (m_derive == DeriveType::HARDENED) ret += '\'';
    430+            if (m_derive == DeriveType::HARDENED) ret += (!normalized && m_apostrophe) ? '\'' : 'h';
    


    darosior commented at 11:21 am on February 6, 2023:

    Looks like it should use h everywhere when normalized? (Not only in the hardened-wildcard step.)

     0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
     1index e3a20c17f4..a2372d7409 100644
     2--- a/src/script/descriptor.cpp
     3+++ b/src/script/descriptor.cpp
     4@@ -421,10 +421,11 @@ public:
     5     }
     6     std::string ToString(bool normalized) const
     7     {
     8-        std::string ret = EncodeExtPubKey(m_root_extkey) + FormatHDKeypath(m_path, /*apostrophe=*/m_apostrophe);
     9+        const bool use_apostrophe = !normalized && m_apostrophe;
    10+        std::string ret = EncodeExtPubKey(m_root_extkey) + FormatHDKeypath(m_path, /*apostrophe=*/use_apostrophe);
    11         if (IsRange()) {
    12             ret += "/*";
    13-            if (m_derive == DeriveType::HARDENED) ret += (!normalized && m_apostrophe) ? '\'' : 'h';
    14+            if (m_derive == DeriveType::HARDENED) ret += use_apostrophe ? '\'' : 'h';
    15         }
    16         return ret;
    17     }
    
  56. in src/script/descriptor.cpp:1080 in 6850bc550c outdated
    1077+        if (elem.size() > 0) {
    1078+            const char last = elem[elem.size() - 1];
    1079+            if (last == '\'' || last == 'h') {
    1080+                elem = elem.first(elem.size() - 1);
    1081+                hardened = true;
    1082+                apostrophe = last == '\'';
    


    darosior commented at 11:41 am on February 6, 2023:
    Should we keep the current behaviour of serializing with apostrophes if any derivation index contains an apostrophe? I guess it doesn’t really matter.

    Sjors commented at 11:36 am on February 17, 2023:
    Not sure. I think I’d rather have an error message for mixed use of h and '.
  57. in src/test/descriptor_tests.cpp:444 in 6850bc550c outdated
    445+    CheckUnparsable("sh(multi(2,[00000000/111'/222]xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0))#ggrsrxf", "sh(multi(2,[00000000/111'/222]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0))#hgmsckn", "Expected 8 character checksum, not 7 characters"); // Too short checksum
    446     CheckUnparsable("sh(multi(3,[00000000/111'/222]xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0))#ggrsrxfy", "sh(multi(3,[00000000/111'/222]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0))#tjg09x5t", "Provided checksum 'tjg09x5t' does not match computed checksum 'd4x0uxyv'"); // Error in payload
    447-    CheckUnparsable("sh(multi(2,[00000000/111'/222]xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0))#ggssrxfy", "sh(multi(2,[00000000/111'/222]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0))#tjq09x4t", "Provided checksum 'tjq09x4t' does not match computed checksum 'tjg09x5t'"); // Error in checksum
    448-    CheckUnparsable("sh(multi(2,[00000000/111'/222]xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0))##ggssrxfy", "sh(multi(2,[00000000/111'/222]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0))##tjq09x4t", "Multiple '#' symbols"); // Error in checksum
    449+    CheckUnparsable("sh(multi(2,[00000000/111'/222]xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0))#ggssrxfy", "sh(multi(2,[00000000/111'/222]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0))#tjg09x6t", "Provided checksum 'tjg09x6t' does not match computed checksum 'tjg09x5t'"); // Error in checksum
    450+    CheckUnparsable("sh(multi(2,[00000000/111'/222]xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0))##ggssrxfy", "sh(multi(2,[00000000/111'/222]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL,xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0))##tjg09x5t", "Multiple '#' symbols"); // Error in checksum
    


    darosior commented at 11:51 am on February 6, 2023:
    Why? These changes seem superfluous.

    Sjors commented at 11:25 am on February 17, 2023:
    Indeed, dropping these and a few others.
  58. darosior commented at 11:56 am on February 6, 2023: member

    I think you missed a couple places to switch from ' to h. Also a question regarding h in normalized private descriptors.

    Nit: if you retouch i think it would be nice to also update the fuzzing target parse_hd_keypathΒ to call FormatHDKeypath with apostrophe = false since the following WriteHDKeypath would always call it with true anyways:

     0diff --git a/src/test/fuzz/parse_hd_keypath.cpp b/src/test/fuzz/parse_hd_keypath.cpp
     1index 411b70230a..d52f08c28a 100644
     2--- a/src/test/fuzz/parse_hd_keypath.cpp
     3+++ b/src/test/fuzz/parse_hd_keypath.cpp
     4@@ -18,6 +18,6 @@ FUZZ_TARGET(parse_hd_keypath)
     5 
     6     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
     7     const std::vector<uint32_t> random_keypath = ConsumeRandomLengthIntegralVector<uint32_t>(fuzzed_data_provider);
     8-    (void)FormatHDKeypath(random_keypath);
     9+    (void)FormatHDKeypath(random_keypath, false);
    10     (void)WriteHDKeypath(random_keypath);
    11 }
    
  59. DrahtBot added the label Needs rebase on Feb 16, 2023
  60. Sjors commented at 12:49 pm on February 16, 2023: member
    Will rebase and address @darosior’s comments soon(tm).
  61. DrahtBot requested review from w0xlt on Feb 16, 2023
  62. Sjors force-pushed on Feb 17, 2023
  63. Sjors commented at 11:41 am on February 17, 2023: member

    Rebased, added the suggested fuzzer coverage and fixed some ' that I missed.

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

    I was thinking along the lines that normalization (moving the xpub to a logical position, converting ' -> h) was an opt-in thing that we only do with the public-key version. However it’s pretty much the default in practice, e.g. listdescriptors normalizes. We might as well do it for the private key version. The private key version isn’t used for checksums, nor as the wallet identifier, so this change should be harmless. However this requires even more changes to the tests, so I’d rather not do this now.

  64. Sjors commented at 11:42 am on February 17, 2023: member
    Oops, didn’t rebase far enough to get past the merge conflict…
  65. Sjors force-pushed on Feb 17, 2023
  66. DrahtBot removed the label Needs rebase on Feb 17, 2023
  67. in src/util/bip32.h:17 in ee65436400 outdated
    13@@ -14,6 +14,6 @@
    14 
    15 /** Write HD keypaths as strings */
    16 std::string WriteHDKeypath(const std::vector<uint32_t>& keypath);
    17-std::string FormatHDKeypath(const std::vector<uint32_t>& path);
    18+std::string FormatHDKeypath(const std::vector<uint32_t>& path, bool apostrophe = true);
    


    achow101 commented at 4:45 pm on March 1, 2023:
    Could this default to false so that we prefer h’s everywhere? In my own testing, this only caused a few functional tests to fail, the unit tests seem fine.

    Sjors commented at 12:28 pm on March 8, 2023:
    Done

    Sjors commented at 1:52 pm on March 8, 2023:
    Missed a few spots for legacy, pushed again.
  68. Sjors force-pushed on Mar 8, 2023
  69. Sjors force-pushed on Mar 8, 2023
  70. achow101 commented at 10:26 pm on March 8, 2023: member
    reACK 4677f1923c9509025de0e77892638d5013f044bf
  71. DrahtBot removed review request from w0xlt on Mar 8, 2023
  72. DrahtBot requested review from furszy on Mar 8, 2023
  73. DrahtBot requested review from w0xlt on Mar 8, 2023
  74. in doc/release-notes-26076.md:9 in 4677f1923c outdated
    0@@ -0,0 +1,11 @@
    1+RPC
    2+---
    3+
    4+- The `listdescriptors` RPC now uses `h` rather than apostrophe (`'`) to indicate
    5+  hardened derivation. This does not apply when using the `private` parameter, which
    6+  matches the marker used when descriptor was generated or imported. Newly created
    7+  wallets use `h`, but previously created descriptor wallets still use `'`.
    8+  This change makes it easier to handle descriptor strings manually. E.g. an RPC call that
    9+  takes an array of descriptors can now use '["desc": ".../0h/..."]'. Either `h`
    


    darosior commented at 10:10 am on March 20, 2023:
    nit: this reads like this change made it possible to use h as a marker when passing a descriptor as an RPC command argument. But it was already possible before. This PR only changes the RPC response. (Although making it indirectly simpler for when you copy paste from an RPC response directly to an RPC command’s argument).

    Sjors commented at 3:34 pm on April 4, 2023:
    I modified the text a bit, and also updated the PR and commit description.
  75. in src/test/fuzz/parse_hd_keypath.cpp:21 in 4677f1923c outdated
    17@@ -18,6 +18,6 @@ FUZZ_TARGET(parse_hd_keypath)
    18 
    19     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
    20     const std::vector<uint32_t> random_keypath = ConsumeRandomLengthIntegralVector<uint32_t>(fuzzed_data_provider);
    21-    (void)FormatHDKeypath(random_keypath);
    22+    (void)FormatHDKeypath(random_keypath, false);
    


    darosior commented at 11:18 am on March 20, 2023:
    Since you changed the default for apostrophe in FormatHDKeypath to false this should be set to true here (as the below WriteHDKeypaht will already call it with false).
  76. in src/wallet/scriptpubkeyman.cpp:1777 in 4677f1923c outdated
    1773@@ -1774,7 +1774,7 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
    1774         // Maybe this doesn't matter because floating keys here shouldn't have origins
    1775         KeyOriginInfo info;
    1776         bool has_info = GetKeyOrigin(keyid, info);
    1777-        std::string origin_str = has_info ? "[" + HexStr(info.fingerprint) + FormatHDKeypath(info.path) + "]" : "";
    1778+        std::string origin_str = has_info ? "[" + HexStr(info.fingerprint) + FormatHDKeypath(info.path, false) + "]" : "";
    


    darosior commented at 11:23 am on March 20, 2023:
    nit: false is already the default for FormatHDKeypath.

    Sjors commented at 4:18 pm on April 4, 2023:
    Dropped.
  77. in test/functional/wallet_descriptor.py:63 in 4677f1923c outdated
    59@@ -60,43 +60,43 @@ def run_test(self):
    60         addr = self.nodes[0].getnewaddress("", "legacy")
    61         addr_info = self.nodes[0].getaddressinfo(addr)
    62         assert addr_info['desc'].startswith('pkh(')
    63-        assert_equal(addr_info['hdkeypath'], 'm/44\'/1\'/0\'/0/0')
    64+        assert_equal(addr_info['hdkeypath'], 'm/44h/1h/0h/0/0')
    


    darosior commented at 11:26 am on March 20, 2023:
    This is an API break? (Albeit a small one)

    Sjors commented at 11:44 am on March 20, 2023:
    I suppose it would be better if legacy wallet RPC calls don’t change behavior. I’ll look into this (and the other comments above).

    Sjors commented at 4:16 pm on April 4, 2023:
    The bigger break was actually in the wallet dump format. I’ve added handling to keep hdkeypath the same for legacy wallets.
  78. darosior commented at 11:28 am on March 20, 2023: member

    Both the OP and the commit message state:

    The new apostrophe argument in FormatHDKeypath (bip32.h) ensures that the hdkeypath field in getaddressinfo is not impacted by this change.

    But the commit then goes on to change it (because this code uses WriteHDKeypath which calls FormatHDKeypath with the default argument for apostrophe which you changed to false in the last push).

  79. DrahtBot removed review request from w0xlt on Mar 20, 2023
  80. DrahtBot requested review from w0xlt on Mar 20, 2023
  81. DrahtBot removed review request from w0xlt on Mar 20, 2023
  82. DrahtBot requested review from w0xlt on Mar 20, 2023
  83. DrahtBot removed review request from w0xlt on Mar 20, 2023
  84. DrahtBot requested review from w0xlt on Mar 20, 2023
  85. DrahtBot removed review request from w0xlt on Apr 4, 2023
  86. DrahtBot requested review from w0xlt on Apr 4, 2023
  87. DrahtBot removed review request from w0xlt on Apr 4, 2023
  88. DrahtBot requested review from w0xlt on Apr 4, 2023
  89. DrahtBot removed review request from w0xlt on Apr 4, 2023
  90. DrahtBot requested review from w0xlt on Apr 4, 2023
  91. DrahtBot removed review request from w0xlt on Apr 4, 2023
  92. DrahtBot requested review from w0xlt on Apr 4, 2023
  93. Switch hardened derivation marker to h in descriptors
    This makes it easier to handle descriptor strings manually. E.g. an RPC call that takes an array of descriptors can now use '["desc": ".../0h/..."]'.
    
    Both markers can still be parsed. The default for new descriptors is changed to h. In normalized form h is also used. For private keys the chosen marker is preserved in a round trip.
    
    The hdkeypath field in getaddressinfo is also impacted by this change.
    bd13dc2f46
  94. Sjors force-pushed on Apr 4, 2023
  95. Sjors commented at 4:35 pm on April 4, 2023: member
    Biggest change is that the hdkeypath field in getaddressinfo keep using ' for legacy wallets. The legacy backup (import) dump format also uses '.
  96. achow101 commented at 7:22 pm on April 20, 2023: member
    re-ACK bd13dc2f46ea10302a928fcf0f53b7aed77ad260
  97. DrahtBot removed review request from w0xlt on Apr 20, 2023
  98. DrahtBot requested review from w0xlt on Apr 20, 2023
  99. Sjors requested review from darosior on Apr 27, 2023
  100. in src/wallet/rpc/addresses.cpp:612 in bd13dc2f46 outdated
    606@@ -607,7 +607,9 @@ RPCHelpMan getaddressinfo()
    607         if (const std::unique_ptr<CKeyMetadata> meta = spk_man->GetMetadata(dest)) {
    608             ret.pushKV("timestamp", meta->nCreateTime);
    609             if (meta->has_key_origin) {
    610-                ret.pushKV("hdkeypath", WriteHDKeypath(meta->key_origin.path));
    611+                // In legacy wallets hdkeypath has always used an apostrophe for
    612+                // hardened derivation. Perhaps some external tool depends on that.
    613+                ret.pushKV("hdkeypath", WriteHDKeypath(meta->key_origin.path, /*apostrophe=*/!desc_spk_man));
    


    darosior commented at 3:39 pm on May 4, 2023:
    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?

    Sjors commented at 2:06 pm on May 8, 2023:
    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.
  101. in doc/release-notes-26076.md:8 in bd13dc2f46 outdated
    0@@ -0,0 +1,12 @@
    1+RPC
    2+---
    3+
    4+- The `listdescriptors` RPC now shows `h` rather than apostrophe (`'`) to indicate
    5+  hardened derivation. This does not apply when using the `private` parameter, which
    6+  matches the marker used when descriptor was generated or imported. Newly created
    7+  wallets use `h`. This change makes it easier to handle descriptor strings manually.
    8+  E.g. an RPC call that takes an array of descriptors can now use '["desc": ".../0h/..."]'.
    


    darosior commented at 4:20 pm on May 4, 2023:

    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:

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

    works fine. So does:

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

    Sjors commented at 1:55 pm on May 8, 2023:
    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 '
  102. in doc/release-notes-26076.md:4 in bd13dc2f46 outdated
    0@@ -0,0 +1,12 @@
    1+RPC
    2+---
    3+
    4+- The `listdescriptors` RPC now shows `h` rather than apostrophe (`'`) to indicate
    


    darosior commented at 10:14 am on May 5, 2023:
    decodepsbt too.

    Sjors commented at 2:10 pm on May 8, 2023:
    Added (plus “and similar”)
  103. darosior commented at 10:17 am on May 5, 2023: member

    light ACK bd13dc2f46ea10302a928fcf0f53b7aed77ad260. 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.

  104. DrahtBot removed review request from w0xlt on May 5, 2023
  105. DrahtBot requested review from w0xlt on May 5, 2023
  106. DrahtBot removed review request from w0xlt on May 5, 2023
  107. DrahtBot requested review from w0xlt on May 5, 2023
  108. DrahtBot removed review request from w0xlt on May 8, 2023
  109. DrahtBot requested review from w0xlt on May 8, 2023
  110. Sjors renamed this:
    Switch hardened derivation marker to h (in normalized descriptors and new wallets)
    Switch hardened derivation marker to h
    on May 8, 2023
  111. DrahtBot removed review request from w0xlt on May 8, 2023
  112. DrahtBot requested review from w0xlt on May 8, 2023
  113. doc: clarify PR 26076 release note fe49f06c0e
  114. Sjors commented at 2:08 pm on May 8, 2023: member

    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.

  115. DrahtBot removed review request from w0xlt on May 8, 2023
  116. DrahtBot requested review from w0xlt on May 8, 2023
  117. darosior commented at 2:11 pm on May 8, 2023: member
    re-ACK fe49f06c0e91b96feb8d8f1bd478c3173f14782c
  118. DrahtBot removed review request from w0xlt on May 8, 2023
  119. DrahtBot requested review from achow101 on May 8, 2023
  120. DrahtBot requested review from w0xlt on May 8, 2023
  121. DrahtBot added the label CI failed on May 8, 2023
  122. achow101 commented at 5:13 pm on May 8, 2023: member
    ACK fe49f06c0e91b96feb8d8f1bd478c3173f14782c
  123. DrahtBot removed review request from achow101 on May 8, 2023
  124. DrahtBot removed review request from w0xlt on May 8, 2023
  125. DrahtBot requested review from w0xlt on May 8, 2023
  126. achow101 merged this on May 8, 2023
  127. achow101 closed this on May 8, 2023

  128. sidhujag referenced this in commit ea609bd598 on May 8, 2023
  129. Sjors deleted the branch on May 8, 2023
  130. in doc/release-notes-26076.md:13 in fe49f06c0e
     8+  E.g. the `importdescriptors` RPC call is easiest to use `h` as the marker: `'["desc": ".../0h/..."]'`.
     9+  With this change `listdescriptors` will use `h`, so you can copy-paste the result,
    10+  without having to add escape characters or switch `'` to 'h' manually.
    11+  Note that this changes the descriptor checksum.
    12+  For legacy wallets the `hdkeypath` field in `getaddressinfo` is unchanged,
    13+  nor is the serialization format of wallet dumps. (#26076)
    


    murchandamus commented at 4:01 pm on May 15, 2023:

    The β€œnor” here seems misplaced:

    0  For legacy wallets the `hdkeypath` field in `getaddressinfo` 
    1  and the serialization format of wallet dumps remain unchanged. (#26076)
    
  131. Sjors referenced this in commit fd3885756b on May 19, 2023
  132. Sjors referenced this in commit ee44942a41 on Aug 1, 2023
  133. hebasto referenced this in commit e7c96f75a9 on Dec 14, 2023
  134. Sjors referenced this in commit 3a1bbe6e1f on Feb 13, 2024
  135. Sjors referenced this in commit 6c1a2cc09a on Apr 16, 2024
  136. bitcoin locked this on May 14, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me