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

full analysis

https://github.com/bitcoin/bitcoin/pull/35170 · optout21 · +127/-103 in 6 files, 5 commits · labels: Tests

Goal

  • Verify actual parsed keypaths in unit tests instead of only checking success booleans
  • Clarify in the codebase that this keypath parsing function is legacy wallet code

Refactors unit tests for ParseHDKeypath in bip32_tests to verify actual parsed numeric keypaths and round-trip string formatting instead of only checking return booleans. Renames the function to ParseHDKeypathLegacy across the codebase via a scripted diff to highlight its limited scope in legacy wallet loading.

Problem: The unit test for ParseHDKeypath previously checked only whether parsing succeeded, meaning an implementation that cleared the returned path would still pass tests. In addition, the function name did not make clear that it is legacy code superseded by descriptor keypath parsing.

Category: Utilities (logging, arguments, libraries) (#64 of 66)

P4 · test coverage

  • P4 because it adds incremental test assertions and a cosmetic rename to a legacy utility
  • Modifies no production behavior and carries no user-facing impact or downstream leverage

P4 because the change provides incremental unit test assertions and a cosmetic rename for a legacy utility method expected to be phased out. As the author notes, production behavior is not modified and no user-facing changes or critical dependencies are involved.

Membership: Modifies src/util/bip32.cpp and src/util/bip32.h by renaming ParseHDKeypath and expanding its unit test suite.

Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0

Reviewability: Ready

  • Ready to review: clean diff against master with passing CI and resolved reviewer feedback

Clean diff against current master with CI passing and all reviewer nits addressed.

Author status: Active; rebased and resolved conflicts against master on 2026-08-21.

Resolved concerns:

  • Inline feedback on test parameter ordering and removing redundant comments was addressed across force-pushes.
  • Merge conflicts with PR #32784 and PR #35069 were resolved by adapting the test suite to recent bip32 and fuzz changes.

Agreement: Strong

  • Strong support for testing parsed output values and clarifying legacy status
  • Verified by local testing of test coverage and rename (haishmg)
  • Code review approval after reviewing test cases (brunoerg)
  • Concept approval without stated reasons (l0rinc, w0xlt)

Strong consensus with prior code ACKs (haishmg, brunoerg) and concept support (l0rinc, w0xlt).

Reviewers agreed on the value of testing output values and clarifying the legacy status of the function. No objections were raised.

  • haishmg ACKed with local testing verification of the test coverage and rename.
  • brunoerg gave a code review ACK after reviewing the test cases.
  • l0rinc and w0xlt provided Concept ACKs.

Review verdicts (DrahtBot): 0 (+2)

Files

222 lines under test/bench/ci.

  • src/test/bip32_tests.cpp +120/-96
  • src/test/fuzz/parse_hd_keypath.cpp +3/-3
  • src/rpc/util.cpp +1/-1
  • src/util/bip32.cpp +1/-1
  • src/util/bip32.h +1/-1
  • src/wallet/walletdb.cpp +1/-1

Card

This PR refactors bip32_tests for ParseHDKeypath to check parsed vector values and round-trip string conversions, and renames the method to ParseHDKeypathLegacy. It solves a test deficiency where the parser's return value could be emptied without failing the existing unit tests. The change is marginal since it touches only test assertions and renames a legacy method without changing runtime behavior. The PR has strong support from several reviewers with prior code ACKs, has addressed all feedback, and is ready for review.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T16:18, confidence high, input hash e14b47fb382da5be