Refactor keypath parser #35069

03f2d97 bip32: accept 'H' as hardened derivation indicator:

this doesn't seem like a "refactor" as the title claims

I dropped the commit after noticing H has been disalowed as hardened indicator in BIP-0380.

Thanks, this gave me dejavu :-)

In c98498fd507f2f3761d7d69f933986ec9c880f13: It would be less distracting if the term child is not used here. Also, a flag suffix can make it easier to read.

diff --git a/src/util/bip32.h b/src/util/bip32.h
index 9ecc5c052b..f1443aff03 100644
--- a/src/util/bip32.h
+++ b/src/util/bip32.h
@@ -11,13 +11,13 @@
 #include <string>
 #include <vector>
 
-/** BIP32 unhardened child index (no high bit set) */
-static constexpr uint32_t BIP32_UNHARDENED = 0x0;
+/** BIP32 unhardened derivation index (no high bit set) */
+static constexpr uint32_t BIP32_UNHARDENED_FLAG = 0x0;
 /** BIP32 hardened derivation flag (2^31) */
-static constexpr uint32_t BIP32_HARDENED = 0x80000000;
+static constexpr uint32_t BIP32_HARDENED_FLAG = 0x80000000;

There are more places where the hardened constant can be used.

git grep 0x80000000 src/wallet

Nit In 4ba2d10f5b75f24d3758d56aa5cc4a6c22f8c6e3:

 /** Parse a single key path element like "0", "0'", or "0h".
- *  Returns the child index and sets is_hardened, or nullopt on failure
+ *  Returns the derivation index and sets is_hardened, or nullopt on failure
  *  (in which case `error` is populated with a human-readable message). */
 std::optional<uint32_t> ParseKeyPathElement(std::span<const char> elem, bool& is_hardened, std::string& error);

In 4ba2d10f5b75f24d3758d56aa5cc4a6c22f8c6e3: This can return the binary or'ed portion here, otherwise it's duplicated across the callers.

-    return *number;
+    return *number | (is_hardened ? BIP32_HARDENED : BIP32_UNHARDENED);
 }

In 4ba2d10:

A new error flow is added here that was not present earlier. Though it looks fine but the error message at this stage is not entirely correct because a hardened marker can also be present. So maybe the following diff:

     if (elem.empty()) {
-        error = strprintf("Key path value '%s' is not a valid uint32", raw);
+        error = strprintf("Key path value '%s' is not valid", raw);
         return std::nullopt;
     }

There would be 3 unit test failures but they can be updated.

In 4ba2d10: This function is only used within ParseKeyPath and after this change, it seems more like a wrapper for ParseKeyPathElem. Can consider making this a lambda and moving it inside ParseKeyPath.

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 6a5aafb0bd..b2013d7340 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1751,18 +1751,6 @@ enum class ParseScriptContext {
     MUSIG,   //!< Inside musig() (implies P2TR, cannot have nested musig())
 };
 
-std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostrophe, std::string& error, bool& has_hardened)
-{
-    bool hardened = false;
-    const auto index{ParseKeyPathElement(elem, hardened, error)};
-    if (!index.has_value()) return std::nullopt;
-    if (hardened) {
-        has_hardened = true;
-        apostrophe = elem.back() == '\'';
-    }
-    return *index | (hardened ? BIP32_HARDENED : BIP32_UNHARDENED);
-}
-
 /**
  * Parse a key path, being passed a split list of elements (the first element is ignored because it is always the key).
  *
@@ -1776,6 +1764,17 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr
  **/
 [[nodiscard]] bool ParseKeyPath(const std::vector<std::span<const char>>& split, std::vector<KeyPath>& out, bool& apostrophe, std::string& error, bool allow_multipath, bool& has_hardened)
 {
+    auto parserWrapper = [&](std::span<const char> elem) -> std::optional<uint32_t> {
+        bool hardened = false;
+        const auto index{ParseKeyPathElement(elem, hardened, error)};
+        if (!index.has_value()) return std::nullopt;
+        if (hardened) {
+            has_hardened = true;
+            apostrophe = elem.back() == '\'';
+        }
+        return *index | (hardened ? BIP32_HARDENED : BIP32_UNHARDENED);
+    };
+
     KeyPath path;
     struct MultipathSubstitutes {
         size_t placeholder_index;
@@ -1808,7 +1807,7 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr
             substitutes.emplace();
             std::unordered_set<uint32_t> seen_substitutes;
             for (const auto& num : nums) {
-                const auto& op_num = ParseKeyPathNum(num, apostrophe, error, has_hardened);
+                const auto& op_num = parserWrapper(num);
                 if (!op_num) return false;
                 auto [_, inserted] = seen_substitutes.insert(*op_num);
                 if (!inserted) {
@@ -1821,7 +1820,7 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr
             path.emplace_back(); // Placeholder for multipath segment
             substitutes->placeholder_index = path.size() - 1;
         } else {
-            const auto& op_num = ParseKeyPathNum(elem, apostrophe, error, has_hardened);
+            const auto& op_num = parserWrapper(elem);
             if (!op_num) return false;
             path.emplace_back(*op_num);
         }