#35170 test: Better test coverage for legacy ParseHDKeypath()
https://github.com/bitcoin/bitcoin/pull/35170 · · +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:
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.