test: Better test coverage for ParseHDKeypath() #35170

optout21 commented at 9:21 AM on April 28, 2026: contributor

Background: The ParseHDKeypath method (src/util/bip32.cpp) is used directly only in a limited way: when loading legacy wallets. However, it shares internals with parsing within descriptors (ParseKeyPath, ParseKeyPathNum in src/script/descriptor.cpp, c.f. #35069).

Deficiencies: The method has a unit test, but it checks only the bool result, and not the parsed keypath. This can be demonstrated by breaking the implementation by adding a keypath.clear(); statement just before the final return true;. With this obviously broken implementation the tests pass.

Solution proposed: Extend the tests with checks (without modifying the behavior).

Changes:

Extend the unit tests with checks for the returned parsed keypath. Add the expected parsed numerical keypath as test data. Note: breaking the production code as described above now results in test failure.
In the unit test also perform round-trip conversion: convert to string, then parse again, and convert to string again. In the last two steps the results should match the input before the round-trip (note: this is not true for the first roundtrip). Additionally, add negative tests for the unsupported 'h' hardened delimiter.

DrahtBot added the label Tests on Apr 28, 2026

DrahtBot commented at 9:21 AM on April 28, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/35170
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/35170.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/35170#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	l0rinc
Stale ACK	haishmg, brunoerg, w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36443 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36443.svg"></sub> (descriptor: Reconstruct a multipath descriptor from its expansion by achow101)
#36442 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36442.svg"></sub> (descriptor: Represent multipath descriptors in a single Descriptor object 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.

optout21 force-pushed on Apr 28, 2026

DrahtBot added the label CI failed on Apr 28, 2026

optout21 force-pushed on Apr 28, 2026

in src/wallet/test/psbt_wallet_tests.cpp:91 in 67ae7a2df4

 152 | +    };
 153 |  
 154 | -    BOOST_CHECK(ParseHDKeypath("m/0/4294967295", keypath)); // 4294967295 == 0xFFFFFFFF (uint32_t max)
 155 | -    BOOST_CHECK(!ParseHDKeypath("m/0/4294967296", keypath)); // 4294967296 == 0xFFFFFFFF (uint32_t max) + 1
 156 | +    const std::vector<TestCase> tests{
 157 | +        {true,  "1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1/1"},

l0rinc commented at 10:15 AM on April 28, 2026:

67ae7a2 test: Refactor in parse_hd_keypath: extract test data in an array:

could we swap the order to store the value first and the validity next to follow the order in the usage, i.e. BOOST_CHECK_EQUAL(ParseHDKeypath(keypath_str, keypath), is_valid);

optout21 commented at 4:18 AM on April 29, 2026:

That's how it was done first, but to keep the formatting so that items align to the same columns, it produced a lot of extra white space. The length of the string input varies a lot, while the bool only a little (4 or 5 chars), so it was put first for that reason.

in src/test/fuzz/parse_hd_keypath.cpp:21 in 1f80ddb330

  17 | @@ -18,6 +18,7 @@ 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, /*apostrophe=*/true); // WriteHDKeypath calls this with false
  22 | -    (void)WriteHDKeypath(random_keypath);
  23 | +    const bool apostrophe = fuzzed_data_provider.ConsumeBool();

l0rinc commented at 10:18 AM on April 28, 2026:

1f80ddb fuzz: In parse_hd_keypath exercise the non-apostrophe version:

we don't want to correlate the two calls for the fuzzer:

    (void)FormatHDKeypath(random_keypath, /*apostrophe=*/fuzzed_data_provider.ConsumeBool());
    (void)WriteHDKeypath(random_keypath, /*apostrophe=*/fuzzed_data_provider.ConsumeBool());

optout21 commented at 4:20 AM on April 29, 2026:

Done.

in src/test/fuzz/parse_hd_keypath.cpp:17 in 2bd25fc3a4 outdated

  13 | @@ -14,7 +14,7 @@ FUZZ_TARGET(parse_hd_keypath)
  14 |  {
  15 |      const std::string keypath_str(buffer.begin(), buffer.end());
  16 |      std::vector<uint32_t> keypath;
  17 | -    (void)ParseHDKeypath(keypath_str, keypath);
  18 | +    (void)ParseHDKeypathLegacy(keypath_str, keypath);

l0rinc commented at 10:19 AM on April 28, 2026:

2bd25fc refactor: Rename ParseHDKeypath to ParseHDKeypathLegacy:

Can you make this a scripted diff for simplicity?

optout21 commented at 4:20 AM on April 29, 2026:

Thanks for insisting, done.

l0rinc commented at 10:19 AM on April 28, 2026: contributor

Concept ACK, only reviewed lightly to understand the extent of the change

DrahtBot removed the label CI failed on Apr 28, 2026

haishmg commented at 4:24 PM on April 28, 2026: none

ACK 2bd25fc3a4ae663b0da77a9a4d9fecac50260183

Reviewed the test refactor and added coverage, the ParseHDKeypathLegacy rename, and the fuzz target change. The new assertions cover returned path elements and round-trip behavior without changing the legacy wallet load behavior.

Ran locally on macOS arm64:

build/bin/test_bitcoin --run_test=psbt_wallet_tests/parse_hd_keypath
ctest --test-dir build -R 'psbt_wallet|ipc_tests|walletdb_tests' --output-on-failure -j 9\n- ctest --test-dir build -j 9 --output-on-failure (0 failed out of 348; script_assets_tests skipped)\n- git diff --check upstream/master...HEAD

DrahtBot requested review from l0rinc on Apr 28, 2026

in src/wallet/test/psbt_wallet_tests.cpp:180 in bab6209ed4

 175 | +            // str2 -> num2
 176 | +            std::vector<uint32_t> keypath_num2;
 177 | +            BOOST_CHECK(ParseHDKeypath(keypath_str2_adjusted, keypath_num2));
 178 | +            BOOST_REQUIRE_EQUAL(keypath_num2.size(), keypath_num.size());
 179 | +            for (size_t i{0}; i < keypath_num2.size(); ++i) {
 180 | +                BOOST_CHECK_EQUAL(static_cast<int32_t>(keypath_num2[i]), keypath_num[i]);

optout21 commented at 5:03 PM on April 28, 2026:

bab6209 test: parse_hd_keypath: Add round-trip tests:

This type conversion is unnecessary (and wrong).

optout21 commented at 4:20 AM on April 29, 2026:

Done

optout21 marked this as ready for review on Apr 28, 2026

optout21 commented at 4:21 AM on April 29, 2026: contributor

Thanks for the reviews! Pushed changes for the suggestions from the first review. Also extended the PR description with 'Potential future steps'.

The duplicated implementation -- ParseHDKeypath and ParseKeyPath -- could be merged, to reduce code duplication. Combining the two implementations could result in slightly better code; the former implementation looks better, while the latter has better unit test coverage.

git diff 2bd25fc3a4ae663b0da77a9a4d9fecac50260183..a548fac787a89e2e69ec89a6f9450935b3a9e08f

optout21 force-pushed on Apr 29, 2026

optout21 force-pushed on Jun 17, 2026

optout21 commented at 7:49 PM on June 17, 2026: contributor

Rebased to current master (to refresh)

sedited requested review from brunoerg on Jul 23, 2026

brunoerg commented at 8:48 PM on July 24, 2026: contributor

Concept ACK

in src/wallet/test/psbt_wallet_tests.cpp:167 in 2d6efe8414

 161 | @@ -159,6 +162,25 @@ BOOST_AUTO_TEST_CASE(parse_hd_keypath)
 162 |              for (size_t i{0}; i < keypath_num.size(); ++i) {
 163 |                  BOOST_CHECK_EQUAL(keypath_num[i], expected[i]);
 164 |              }
 165 | +
 166 | +            // Round-trip test:
 167 | +            // num -> str2

brunoerg commented at 8:49 PM on July 24, 2026:

2d6efe8414aeb75675769ca3ca71b0113ebf7169: nit: These comments "num -> str2" and "str2 -> num2" seem unnecessary, the code is already clear.

optout21 commented at 6:08 PM on August 18, 2026:

Applied (as I had to touch due to merge conflict). Note that this code has been moved to src/test/bip32_tests.cpp now.

in src/test/fuzz/parse_hd_keypath.cpp:22 in 94536aba8f

  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, /*apostrophe=*/true); // WriteHDKeypath calls this with false
  22 | -    (void)WriteHDKeypath(random_keypath);
  23 | +    (void)FormatHDKeypath(random_keypath, /*apostrophe=*/fuzzed_data_provider.ConsumeBool());
  24 | +    (void)WriteHDKeypath(random_keypath, /*apostrophe=*/fuzzed_data_provider.ConsumeBool());

brunoerg commented at 8:54 PM on July 24, 2026:

94536aba8f0f374953e765b9b76772765a18d2aa: Nice catch! The apostrophe param was added in WriteHDKeypath in bd13dc2f46ea10302a928fcf0f53b7aed77ad260 (3 years ago). I'm just wondering how effective is fuzzing FormatHDKeypath if WriteHDKeypath can exactly call FormatHDKeypath again with same values.

optout21 commented at 6:09 PM on August 18, 2026:

Note: this is irrelevant now, as #32784 had solved this (in a different way).

brunoerg commented at 9:24 PM on July 24, 2026: contributor

code review ACK cf76d345c85283e64a37e850b92b748c5ee29d2b

Nothing blocker from my comments but happy to re-ack if any of them (or any other thing) is addressed.

DrahtBot added the label Needs rebase on Aug 15, 2026

optout21 force-pushed on Aug 18, 2026

optout21 commented at 6:06 PM on August 18, 2026: contributor

PR #32784 has been merged recently, and it overlaps with the changes of this PR (resulted in a conflict). Rebasing had to be done manually, including changes:

The test cases had to be updated, as they were changed & extended in #32784
The change to the fuzz test was dropped, as the same change has been independently done in #32768 as well.

git range-diff cf76d345c85283e64a37e850b92b748c5ee29d2b...b3e4cd2eef85a55b2a734f5f9817993ff943e852

optout21 commented at 6:10 PM on August 18, 2026: contributor

@brunoerg, thanks for the review. I've applied the nit, as the PR had to be changed (due to merge conflict).

DrahtBot removed the label Needs rebase on Aug 18, 2026

w0xlt commented at 9:49 PM on August 18, 2026: contributor

Concept ACK

DrahtBot added the label Needs rebase on Aug 19, 2026

test: Refactor in bip32_tests: extract test data in an array

For easier expansion of the test calls, extract test data in an array,
and have a single instance of calling the method under test.

41bba5e1b0

test: bip32_tests: Add expected result checks

Extend the unit tests with checks for the returned parsed keypath.
Add the expected parsed numerical keypath as test data.

4c7969f6e2

test: bip32_tests: Add round-trip tests

In the unit test perform round-trip conversion: convert to string, then parse again,
and convert to string again. In the last two steps the results should match
the input before the round-trip (note: this is not true for the first roundtrip).

bb057076dc

test: bip32_tests: Add some more tests cases dd665be15b

optout21 force-pushed on Aug 21, 2026

optout21 commented at 11:32 AM on August 21, 2026: contributor

Reworked, due to new conflict. #35069 has been merged, whereby the implementation of ParseHDKeypath and ParsePubkey has been mostly deduplicated, and test data changed. Updated this PR accordingly, it still keeps the test improvements (test data as data, check expected results, add round-trip tests), and the renaming. Ready for review.

DrahtBot removed the label Needs rebase on Aug 21, 2026

optout21 requested review from brunoerg on Sep 4, 2026

brunoerg commented at 7:06 PM on September 30, 2026: contributor

reACK 56b1a59b47bf7eb80bad5cbaf115e524a4192d57

w0xlt commented at 9:12 PM on September 30, 2026: contributor

ACK 56b1a59b47bf7eb80bad5cbaf115e524a4192d57

in src/util/bip32.h:33 in 56b1a59b47

  29 | @@ -30,7 +30,7 @@ struct KeyPathElement {
  30 |  util::Expected<KeyPathElement, std::string> ParseKeyPathElement(std::span<const char> elem);
  31 |  
  32 |  /** Parse an HD keypaths like "m/7/0'/2000". */
  33 | -[[nodiscard]] bool ParseHDKeypath(const std::string& keypath_str, std::vector<uint32_t>& keypath);
  34 | +[[nodiscard]] bool ParseHDKeypathLegacy(const std::string& keypath_str, std::vector<uint32_t>& keypath);

sedited commented at 11:09 AM on October 5, 2026:

In commit 56b1a59b47bf7eb80bad5cbaf115e524a4192d57:

I'm not convinced this rename makes sense after the addition of derivehdkey. I'd prefer keeping it as is.

optout21 commented at 4:03 PM on October 5, 2026:

I don't fully get the "after the addition of derivehdkey" part. Do you meant that after #35069 the two implementations share the same internals? So although ParseHDKeypath is used only in the legacy wallet case, it's internals are also used by the descriptor parsing (ParseKeyPath from descriptor.cpp)? If so, I can agree and will consider it.

achow101 commented at 8:07 PM on October 6, 2026:

Agree with keeping the original naming. There is nothing legacy about this function. That the legacy wallet is currently the only caller should not make a difference for its naming, there may be other unrelated callers in the future.

optout21 force-pushed on Oct 6, 2026

optout21 renamed this:
~~test: Better test coverage for legacy ParseHDKeypath()~~
test: Better test coverage for ParseHDKeypath()
on Oct 6, 2026

optout21 commented at 9:02 PM on October 6, 2026: contributor

With #35069 the two implementations has been brought together (share the same internal implementation), so the rename to 'Legacy' makes not much sense. Dropped the rename, and updated the OD accordingly. This change is a pure test-only change now. Thanks for the comments!

Re-reviews are welcome -- the rename was the very last commit, it was dropped, but the other 4 commits are unmodified (current changeset is a subset of the previous version).

optout21 requested review from brunoerg on Oct 6, 2026

optout21 requested review from sedited on Oct 6, 2026

optout21 requested review from w0xlt on Oct 6, 2026