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.
In the fuzzer also exercise the apostrophe=false version of FormatHDKeypath.
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.

test: Refactor in parse_hd_keypath: 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.

67ae7a2df4

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.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	l0rinc
Stale ACK	haishmg

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:

#35069 (Refactor keypath parser by pythcoiner)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

Parse an HD keypaths like "m/7/0'/2000". -> Parse an HD keypath like "m/7/0'/2000". [“keypaths” is the wrong form here; singular matches the rest of the sentence]

optout21 force-pushed on Apr 28, 2026

DrahtBot added the label CI failed on Apr 28, 2026

optout21 force-pushed on Apr 28, 2026

test: parse_hd_keypath: Add expected result checks

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

16e25e8f98

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

test: parse_hd_keypath: 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).
Additionally, add negative tests for the unsupported 'h' hardened delimiter.

3e7115cd0c

fuzz: In parse_hd_keypath exercise the non-apostrophe version

FormatHDKeypath and WriteHDKeypath support both apostrophe and 'h' hardened
delimiter format, for completeness include both in the fuzz test.

3d18945074

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-

a548fac787

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