#36013 test: Descriptor roundtrip and raw()/ addr() coverage
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36013 test: Descriptor roundtrip and raw()/ addr() coverage | Waiting on author | 1 | Strong | 0 + 112 tests | |||
|
|
|
|
| |||
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
Adds structural roundtrip testing and valid raw() and addr() test coverage to descriptor unit tests. InferDescriptor() output from generated scripts is compared against the serialized descriptor with key material stripped using a regex helper. A NO_KEYS test flag is added to exercise invariants for keyless descriptors in DoCheck().
Problem: Descriptor unit tests did not verify that inferring a descriptor from a generated scriptPubKey preserved the original descriptor structure. Additionally, valid raw() and addr() descriptors lacked test coverage in DoCheck(), leaving keyless descriptor behaviors (such as key counts, solvability, and ToPrivateString failure) unexercised.
Evidence of importance:
- Commit b970993985 and description: 'Roundtrip testing that inferring a descriptor from a generated scriptPubKey reproduces the original descriptor's structure (keys stripped from both sides for comparison).'
- Commit cf9821aaa5 and description: 'Check() coverage for valid raw() and addr() descriptors, previously exercised only for invalid inputs and only in the script-to-descriptor direction.'
Risk of a mistake: low (tests). Touches only unit tests in descriptor_tests.cpp; a bug would only cause unit test failures.
Changed since the previous assessment: Switched roundtrip comparison to check against the re-serialized descriptor rather than the input string to avoid multipath regex matching, added support for sortedmulti_a, and added an explicit check that ToPrivateString() returns false for descriptors with NO_KEYS.
Reviewability
Paused · Waiting on author
Open objections from rustaceanrob with no author reply for 15 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: review rustaceanrob's suggestions on DropKeys simplification and flag check readability 2026-10-17
Agreement
Strong
substantive support, no open objection (jeanpablojp, fametrano, fametrano)
Thread notes: Added two new open suggestions from rustaceanrob on DropKeys simplification (rc:4304909678) and flag check readability (rc:4304968151). The PR is currently waiting on the author to respond to these suggestions.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| rustaceanrob (member) | suggestion, maintenance | use BOOST_TEST_INFO_SCOPE so failure diagnostics cover all checks in the branch | fixed · fix pushed · addressed in 2d230d8366d2149c71e69a2e2ec65561ca09cd0e | answered | yes | 2026-08-24: 'perhaps we can change this to be BOOST_TEST_INFO_SCOPE so the descriptor is also printed if BOOST_CHECK_EQUAL(parse_pub->GetKeyCount(), 0U); fails.' Status from: 2026-08-27: pablomartin4btc: 'Done in 2d230d8366d2149c71e69a2e2ec65561ca09cd0e, thanks!' |
| rustaceanrob (member) | suggestion, maintenance | use BOOST_TEST_INFO_SCOPE for the second check block | fixed · fix pushed · replaced BOOST_TEST_INFO with BOOST_TEST_INFO_SCOPE in 2d230d8 | answered | yes | 2026-08-24: 'Same feedback as the comment above, perhaps could be BOOST_TEST_INFO_SCOPE' Status from: 2026-08-27: pablomartin4btc: 'Done in 2d230d8366d2149c71e69a2e2ec65561ca09cd0e, thanks!' |
| jeanpablojp (contributor) | suggestion, approach | avoid the multipath alternative in DropKeys by comparing on pub1 instead of literal pub, or explain if the literal catches something pub1 would not | fixed · fix pushed · author switched to pub1 in cf9821a | answered | yes | 2026-09-03: 'The multipath alternative here seems avoidable. It only exists because the roundtrip comparison uses pub, the literal test string.' Status from: 2026-10-08: pablomartin4btc: 'Done.' |
| jeanpablojp (contributor) | suggestion, correctness | allow an optional _a in the sortedmulti regex so sortedmulti_a roundtrips properly | fixed · fix pushed · addressed in cf9821a | answered | yes | 2026-09-03: 'nit: this matches sortedmulti( but not sortedmulti_a(, which InferMultiA() gives back as multi_a.' Status from: 2026-10-08: pablomartin4btc: 'Done, thanks!' |
| jeanpablojp (contributor) | suggestion, correctness | assert the ToPrivateString() contract for NO_KEYS when set instead of skipping the check | fixed · fix pushed · addressed in cf9821a | answered | yes | 2026-09-03: 'Could we assert the ToPrivateString() contract for NO_KEYS here instead of only skipping the block? The header promises a false return when no private keys are available at all' Status from: 2026-10-08: pablomartin4btc: 'Done.' |
| fametrano (contributor) | suggestion, maintenance | add a comment explaining why the v1 addr() vector uses a 40-byte program instead of a 32-byte taproot program | accepted · fix pushed · comment added in cf9821a and confirmed by fametrano in r:5452859423 | answered | yes | 2026-10-03: '// 40-byte v1 program: a 32-byte one would infer as a solvable rawtr() and fail the checks' Status from: 2026-10-08: fametrano: 'Thanks for the comment on the 40-byte v1 program.' |
| rustaceanrob (member) | suggestion, approach | simplify DropKeys using std::regex_replace and std::regex with lookahead | open | answered | no | 2026-10-17: 'Not sure if I am missing something, but it seems like this function can be simplified using `regex_replace` and `std::regex`:' |
| rustaceanrob (member) | suggestion, style | use `(flags & NO_KEYS) != 0` instead of `!!` for legibility | open | answered | no | 2026-10-17: 'nit: Could use `(flags & NO_KEYS) != 0` for legibility. Had to look up the `!!` case' |
Support:
- w0xlt (contributor) (Concept ACK, 2026-08-18): (no reason given) [not substantive]
- jeanpablojp (contributor) (Concept ACK, 2026-09-03) [both]: Built at the head, ran the unit suite, and checked the six new addresses by decoding them outside of Core.
- fametrano (contributor) (ACK, 2026-10-03) [both]: Built test_bitcoin at the head and ran descriptor_tests, no errors. Recomputed the six new addr() vectors with btclib: the scriptPubKeys, the addresses back from them, the witness versions and program
- fametrano (contributor) (ACK, 2026-10-08) [both]: descriptor_tests passes at the new head, and the raw()/addr() vectors are unchanged since 2d230d8, so my earlier check of them still holds. Thanks for the comment on the 40-byte v1 program.
Participants:
- w0xlt (contributor, support, 1 statements 2026-08-18 to 2026-08-18): Concept ACK
- rustaceanrob (member, neutral, 5 statements 2026-08-24 to 2026-10-17): suggested BOOST_TEST_INFO_SCOPE diagnostics (addressed), DropKeys simplification, and flag check legibility
- jeanpablojp (contributor, support, 4 statements 2026-09-03 to 2026-09-03): Concept ACK; suggested DropKeys multipath simplification, handling sortedmulti_a, and asserting ToPrivateString contract for NO_KEYS
- fametrano (contributor, support, 3 statements 2026-10-03 to 2026-10-08): ACK; verified addr() vectors against btclib and confirmed comment explaining 40-byte v1 program
Files
120 lines under test/bench/ci.
- src/test/descriptor_tests.cpp +112/-8
Card
The short text the ranking pass compares across PRs.
Adds unit test coverage to verify that inferring a descriptor from a generated scriptPubKey preserves the descriptor's structure, and adds test vectors for valid raw() and addr() descriptors. It ports and updates test-only commits originally authored by Pieter Wuille in PR 24361 to account for newer features like MuSig and multipath descriptors. Keyless descriptors now explicitly test invariants around key count, solvability, and serialization failure. This change has no dependencies and touches only unit tests.
Data
thread read through 2026-10-17 (23 statements), head cf9821aaa5
labels: Tests