test: Better test coverage for legacy ParseHDKeypath() #35170

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

Background: The ParseHDKeypath method (src/util/bip32.cpp) is used only in a limited way: it is used when loading legacy wallets. Note that similar functionality is implemented independently in src/script/descriptor.cpp (ParseKeyPath, ParseKeyPathNum).

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), and also rename the method.

Notes:

Behavior is not changed. This is a legacy method expected to be removed in the future. Production code is touched only by the rename.
Here there is no attempt to merge ParseHDKeypath with ParseKeyPath.

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.

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.
Rename the method ParseHDKeypath to ParseHDKeypathLegacy, to make it clearer that it is used only in the legacy wallet use case, and reduce the chance of confusion with similar parsing functionality in descriptor.cpp.

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.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35170.

Reviews

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

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

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

Conflicts

No conflicts as of last run.

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

scripted-diff: Rename ParseHDKeypath to ParseHDKeypathLegacy

Rename the method `ParseHDKeypath` to `ParseHDKeypathLegacy`,
to make it clearer that it is used only in the legacy wallet use case,
and reduce the chance of confusion with similar parsing functionality
in `descriptor.cpp`.

-BEGIN VERIFY SCRIPT-
git grep -l ParseHDKeypath -- '*.h' '*.cpp' | xargs sed -i 's/ParseHDKeypath/ParseHDKeypathLegacy/g'
-END VERIFY SCRIPT-

56b1a59b47

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