#35170 test: Better test coverage for ParseHDKeypath()

ReviewabilityReviewsAgreementSize
#35170 optout21 test: Better test coverage for ParseHDKeypath()Ready1 (+3)Strong0 + 120 tests
  • Ensure BIP32 keypath parser tests verify parsed keypath outputs rather than only checking boolean success
  • Catch bugs where a broken parser signals success but corrupts or clears the parsed output vector
  • Ready to review
  • Substantive support reviewing test coverage and round-trip assertions (haishmg)
  • Approvals across multiple revisions and rebases (brunoerg, w0xlt)
  • Dropped a contested function rename after reviewer feedback (sedited, achow101)
  • Approved after ensuring test coverage is not temporarily reduced in commit history (Sjors)
  • 0 lines added or modified outside tests
  • 120 lines added or modified in tests
  • 96 lines removed in total
  • 1 files, 4 commits
Utilities (logging, arguments, libraries)#52 of 59
P3 · test coverage
  • P3 because it strengthens regression tests for utility functions without fixing an active bug
  • Closes a testing blind spot where an invalid parser producing empty paths still passed unit tests
  • Protects BIP32 keypath handling correctness for developers and maintainers while remaining deferrable
  • Ranking pass: Moved down: test-only hardening with no active bug, below the behaviour-affecting fixes in this band.
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Restructures the parse_hd_keypath unit test in bip32_tests into a table-driven test and verifies the parsed numeric keypath vector against expected values rather than only asserting boolean success. It also adds round-trip formatting checks with FormatHDKeypath and tests additional edge cases.

Problem: The unit test for ParseHDKeypath previously asserted only the boolean return status for most test inputs. A broken parser implementation returning true while clearing or corrupting the output vector would still pass unit tests.

Evidence of importance:

  • PR description: '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.'

Risk of a mistake: low (tests). Touches only unit test assertions in bip32_tests; a mistake would cause false-positive test failures.

Changed since the previous assessment: Dropped the codebase-wide scripted rename of ParseHDKeypath to ParseHDKeypathLegacy, restricting the PR strictly to unit test coverage in bip32_tests.cpp.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • nothing: no pending actions

Agreement

Strong

substantive support, no open objection (haishmg)

Thread notes: Added Sjors' suggestion regarding temporary coverage reduction in the first commit; author addressed it by keeping the test case separate initially and updating the commit message, and Sjors re-ACKed.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
l0rinc (member)suggestion, approachSwap the order of arguments to store the value first and the validity next to match usageanswered · author explained keeping boolean first avoids excessive whitespace from string length varianceansweredyes2026-04-28: 'could we swap the order to store the value first and the validity next to follow the order in the usage'
Status from: 2026-04-29: optout21: '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 e'
l0rinc (member)suggestion, approachDo not correlate the two calls for the fuzzer by consuming separate booleansfixed · fix pushed · author implemented separate fuzzed boolean consumption in pushansweredyes2026-04-28: 'we don't want to correlate the two calls for the fuzzer'
Status from: 2026-04-29: optout21: 'Done.'
l0rinc (member)suggestion, approachMake the rename a scripted diff for simplicityfixed · fix pushed · author implemented as scripted diff in pushansweredyes2026-04-28: 'Can you make this a scripted diff for simplicity?'
Status from: 2026-04-29: optout21: 'Thanks for insisting, done.'
brunoerg (member)suggestion, styleRemove comments like 'num -> str2' and 'str2 -> num2' as unnecessaryfixed · fix pushed · author removed the comments during conflict resolutionansweredyes2026-07-24: 'nit: These comments "num -> str2" and "str2 -> num2" seem unnecessary, the code is already clear.'
Status from: 2026-08-18: optout21: 'Applied (as I had to touch due to merge conflict).'
brunoerg (member)question, approachHow effective is fuzzing FormatHDKeypath if WriteHDKeypath can call it again with the same values?answered · author noted irrelevant after upstream merge #32784 resolved fuzzing independentlyansweredyes2026-07-24: 'I'm just wondering how effective is fuzzing FormatHDKeypath if WriteHDKeypath can exactly call FormatHDKeypath again with same values.'
Status from: 2026-08-18: optout21: 'Note: this is irrelevant now, as #32784 had solved this (in a different way).'
sedited (member)suggestion, scopeKeep ParseHDKeypath without renaming it to ParseHDKeypathLegacyfixed · fix pushed · author dropped the rename commit in pushansweredyes2026-10-05: 'I'm not convinced this rename makes sense after the addition of `derivehdkey`. I'd prefer keeping it as is.'
Status from: 2026-10-06: optout21: 'Dropped the rename, and updated the OD accordingly.'
achow101 (member)suggestion, scopeKeep the original naming as there is nothing legacy about ParseHDKeypathfixed · fix pushed · author dropped the rename commit in pushansweredyes2026-10-06: 'Agree with keeping the original naming. There is nothing legacy about this function.'
Status from: 2026-10-06: optout21: 'Dropped the rename, and updated the OD accordingly.'
Sjors (member)suggestion, correctnessAvoid temporarily reducing test coverage in the refactor commit or document it in the commit messageaccepted · fix pushed · author kept the test case separate in the first commit and updated the commit message; Sjors re-ACKedansweredyes2026-10-28: 'IIUC this temporarily reduces test coverage. The commit message could mention it, or leave it out of the array initially:'
Status from: 2026-10-28: Sjors: 're-ACK 3ce3641'

Support:

  • haishmg (none) (ACK, 2026-04-28) [both: src/test/bip32_tests.cpp, src/test/fuzz/key_io.cpp]: 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
  • l0rinc (member) (Concept ACK, 2026-04-28): only reviewed lightly to understand the extent of the change [not substantive]
  • brunoerg (member) (Concept ACK, 2026-07-24): Concept ACK [not substantive]
  • brunoerg (member) (ACK, 2026-07-24): code review ACK cf76d345c85283e64a37e850b92b748c5ee29d2b [not substantive]
  • w0xlt (contributor) (Concept ACK, 2026-08-18): Concept ACK [not substantive]
  • brunoerg (member) (ACK, 2026-09-30): reACK 56b1a59b47bf7eb80bad5cbaf115e524a4192d57 [not substantive]
  • w0xlt (contributor) (ACK, 2026-09-30): ACK 56b1a59b47bf7eb80bad5cbaf115e524a4192d57 [not substantive]
  • w0xlt (contributor) (ACK, 2026-10-22): reACK dd665be15b553ef4aa5913c03f4b5e3a53751ce3 [not substantive]
  • Sjors (member) (ACK, 2026-10-28): re-ACK 3ce3641 [not substantive]
  • Sjors (member) (ACK, 2026-10-28): Concept ACK and ACK dd665be15b553ef4aa5913c03f4b5e3a53751ce3 [not substantive]

Participants:

  • haishmg (none, support, 1 statements 2026-04-28 to 2026-04-28): ACK with local testing and review of coverage improvements
  • l0rinc (member, support, 4 statements 2026-04-28 to 2026-04-28): Concept ACK; reviewed fuzz and unit test structure, made suggestions
  • brunoerg (member, support, 5 statements 2026-07-24 to 2026-09-30): ACK and reACK after rebases; noted non-blocking nit and question
  • w0xlt (contributor, support, 3 statements 2026-08-18 to 2026-10-22): Concept ACK, ACK, and reACK after rebase
  • sedited (member, neutral, 1 statements 2026-10-05 to 2026-10-05): suggested keeping ParseHDKeypath name as is rather than renaming to ParseHDKeypathLegacy
  • achow101 (member, neutral, 1 statements 2026-10-06 to 2026-10-06): agreed with keeping original naming rather than renaming to ParseHDKeypathLegacy
  • Sjors (member, support, 3 statements 2026-10-28 to 2026-10-28): ACK, noted temporary reduction of coverage in first commit, and re-ACKed after author's fix

Files

216 lines under test/bench/ci.

  • src/test/bip32_tests.cpp +120/-96

Card

The short text the ranking pass compares across PRs.

Restructures unit tests for ParseHDKeypath in bip32_tests into a table-driven test that verifies exact parsed numeric derivation paths and round-trip formatting. Existing tests only checked the boolean return value, allowing an implementation that cleared or corrupted the output vector to pass tests unnoticed. This change strictly modifies test code to ensure keypath parsing accuracy without altering runtime behavior.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-29T01:22, confidence high, input hash b176bfe801e21104
thread read through 2026-10-28 (34 statements), head 3ce364124c
labels: Tests

Categories

Utilities (logging, arguments, libraries)

P3 · test coverage (ranking pass: Moved down: test-only hardening with no active bug, below the behaviour-affecting fixes in this band.)

Improves unit test coverage for ParseHDKeypath by validating parsed vector values and round-trip formatting rather than merely checking boolean return status. The author showed the test suite previously had a blind spot where an 'obviously broken implementation' such as inserting keypath.clear() before return true would still pass. While worthwhile to protect parser correctness, it addresses no active production bug and is reasonably deferrable.

Membership: Modifies src/test/bip32_tests.cpp to test the ParseHDKeypath and FormatHDKeypath utilities in src/util/bip32.cpp.

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