test: Descriptor roundtrip and raw()/ addr() coverage #36013

pablomartin4btc commented at 7:29 PM on August 18, 2026: member

Part 1 of 2, reviving #24361.

<details> <summary>Ports the two test-only commits from the original PR above, re-applied against the current <code>descriptor_tests.cpp</code> — the original no longer applies cleanly, since <code>DoCheck()</code>'s signature has changed substantially since the original PR was created back in 2022.</summary> <br>

norm_prv was dropped, and op_desc_id, spender_nlocktime, spender_nsequence, preimages, expected_prv/expected_pub, and desc_index were all added - so the original diff no longer applies...

diff -u <(git show 15220ec903:src/test/descriptor_tests.cpp | sed -n '/^void DoCheck/,/^{/p') \
        <(git show HEAD:src/test/descriptor_tests.cpp | sed -n '/^void DoCheck/,/^{/p')

@@ -1,3 +1,7 @@
-void DoCheck(const std::string& prv, const std::string& pub, const std::string& norm_prv, const std::string& norm_pub, int flags, const std::vector<std::vector<std::string>>& scripts, const std::optional<OutputType>& type, const std::set<std::vector<uint32_t>>& paths = ONLY_EMPTY,
-    bool replace_apostrophe_with_h_in_prv=false, bool replace_apostrophe_with_h_in_pub=false)
+void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int flags,
+             const std::vector<std::vector<std::string>>& scripts, const std::optional<OutputType>& type, std::optional<uint256> op_desc_id = std::nullopt,
+             const std::set<std::vector<uint32_t>>& paths = ONLY_EMPTY, bool replace_apostrophe_with_h_in_prv=false,
+             bool replace_apostrophe_with_h_in_pub=false, uint32_t spender_nlocktime=0, uint32_t spender_nsequence=CTxIn::SEQUENCE_FINAL,
+             std::map<std::vector<uint8_t>, std::vector<uint8_t>> preimages={},
+             std::optional<std::string> expected_prv = std::nullopt, std::optional<std::string> expected_pub = std::nullopt, int desc_index = 0)
 {

</details>

Roundtrip testing that inferring a descriptor from a generated scriptPubKey reproduces the original descriptor's structure (keys stripped from both sides for comparison). Added extension for MuSig key expressions (#31244) and BIP389 multipath (#22838), both added since 2022, and skipping for multi-branch taproot trees, where sibling order isn't meaningful. (For more details check 1st commit body)
Check() coverage for valid raw() and addr() descriptors, previously exercised only for invalid inputs and only in the script-to-descriptor direction. Added a new NO_KEYS flag, since these descriptors have zero keys and several existing checks assumed at least one. (For more details check 2nd commit body)

No behavior change, test-only.

Part 2 (not yet opened): the ToString()/ToPrivateString() merge from #24361, independent of this PR — needs more design work before it's ready.

test: Roundtripping of descriptors ignoring keys

Add DropKeys(), which strips a descriptor string down to structure
only (key material replaced with "<KEY>", sortedmulti() normalized to
multi(), checksum dropped), and use it to check that inferring a
descriptor back from a script it generated reproduces the original
descriptor's structure.

A few descriptor features need special handling for this comparison
to hold:

- MuSig key expressions (musig(...)) aren't recognized by the plain
  key-matching regex. Added a separate pass collapsing musig()
  expressions (and any trailing derivation path) to <KEY> before the
  plain-key regex runs, rather than teaching that regex to understand
  musig()'s internals recursively.
- BIP389 multipath key expressions (<1;2;3>) in derivation paths.
  Extended the path-segment pattern to recognize them, which added
  two capture groups to the key-matching regex and shifted the
  position of the group the replacement substitutes back in.
  Computed that position from the regex's own capture count instead
  of hardcoding it, so it can't silently drift out of sync again.
- Sibling branches within a taproot script tree have no meaningful
  order (it's a Merkle tree), so InferDescriptor() reconstructing a
  multi-leaf tree in a different but equally valid branch order isn't
  something a plain string comparison can account for. Skipped the
  check for any taproot descriptor with more than one script leaf,
  detected via the presence of '{' in the descriptor string (the
  grammar only emits '{'/'}' for grouping 2+ branches).

Co-authored-by: Pieter Wuille <pieter@wuille.net>

d840df45ea

DrahtBot added the label Tests on Aug 18, 2026

DrahtBot commented at 7:30 PM on August 18, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36013.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	w0xlt, jeanpablojp

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:

#36154 (wallet: fill PSBT_GLOBAL_XPUB for descriptors with more than one key by jeanpablojp)
#36133 (wallet: store multipath descriptor by Sjors)
#35713 (Remove boost as a unit test runner by rustaceanrob)

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.

w0xlt commented at 9:17 PM on August 18, 2026: contributor

Concept ACK

in src/test/descriptor_tests.cpp:356 in 5b4c861a60

 350 | @@ -285,13 +351,23 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int
 351 |      BOOST_CHECK_EQUAL(parse_pub->IsRange(), (flags & RANGE) != 0);
 352 |      BOOST_CHECK_EQUAL(parse_priv->IsRange(), (flags & RANGE) != 0);
 353 |  
 354 | -    // Check that the highest key expression index matches the number of keys in the descriptor
 355 | +    // Check that the highest key expression index matches the number of keys in the descriptor.
 356 | +    // Descriptors with no keys at all (flags & NO_KEYS) have neither.
 357 |      BOOST_TEST_INFO("Pub desc: " + pub);

rustaceanrob commented at 10:10 AM on August 24, 2026:

Now that we have a few checks in the true part of the if branch below, 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.

pablomartin4btc commented at 2:01 AM on August 27, 2026:

Done in 2d230d8366d2149c71e69a2e2ec65561ca09cd0e, thanks!

in src/test/descriptor_tests.cpp:364 in 5b4c861a60

 361 | +        BOOST_CHECK_EQUAL(key_exprs, 0U);
 362 | +        BOOST_CHECK_EQUAL(parse_pub->GetKeyCount(), 0U);
 363 | +    } else {
 364 | +        BOOST_CHECK_EQUAL(key_exprs + 1, parse_pub->GetKeyCount());
 365 | +    }
 366 |      BOOST_TEST_INFO("Priv desc: " + prv);

rustaceanrob commented at 10:11 AM on August 24, 2026:

Same feedback as the comment above, perhaps could be BOOST_TEST_INFO_SCOPE

pablomartin4btc commented at 2:01 AM on August 27, 2026:

Done in 2d230d8366d2149c71e69a2e2ec65561ca09cd0e, thanks!

rustaceanrob commented at 10:12 AM on August 24, 2026: member

Left a comment regarding BOOST_TEST_INFO, which only prints the value the next failing check, not any check in scope

test: raw() and addr() descriptors in unit tests

Add Check() coverage for valid raw() and addr() descriptors: a raw
script, legacy and P2SH addresses, two bech32 v0 addresses, and two
future-witness-version addresses (v1 and v2, both expected
non-standard/unsolvable) - verifying checksum handling, solvability,
script derivation, and structural roundtripping. Address/script pairs
verified against a live node's own validateaddress before use.

These descriptors have no key expressions at all, which DoCheck()
didn't previously account for:

- keys_priv.keys is expected to be empty rather than non-empty, gated
  on a new NO_KEYS flag.
- GetMaxKeyExpr()/GetKeyCount() are expected to both report zero
  rather than the usual "highest index + 1 == count" relationship.
  Added the NO_KEYS-aware branch for it.
- HavePrivateKeys() returns false for a keyless descriptor, not
  vacuously true - it didn't exist when the original patch was
  written, so this wasn't previously exercised. Gated
  have_all_private_keys on NO_KEYS in addition to the existing
  MISSING_PRIVKEYS.
- raw()/addr() hardcode ToPrivateString() to always fail, so the
  existing "can serialize back to the private version" check needs
  skipping for these descriptors too. Extended its MISSING_PRIVKEYS
  gate to also cover NO_KEYS.

Co-authored-by: Pieter Wuille <pieter@wuille.net>

2d230d8366

pablomartin4btc force-pushed on Aug 26, 2026

DrahtBot added the label CI failed on Aug 26, 2026

DrahtBot removed the label CI failed on Aug 27, 2026

pablomartin4btc commented at 2:06 AM on August 27, 2026: member

-<ins>Updates</ins>:

Addressed @rustaceanrob's feedback: replaced BOOST_TEST_INFO with BOOST_TEST_INFO_SCOPE in https://github.com/bitcoin/bitcoin/commit/2d230d8366d2149c71e69a2e2ec65561ca09cd0e.

jeanpablojp commented at 4:56 PM on September 3, 2026: contributor

Concept ACK

Built at the head, ran the unit suite, and checked the six new addresses by decoding them outside of Core.

in src/test/descriptor_tests.cpp:186 in 2d230d8366

 181 | +const std::string STR_PUBKEYHASH = "([a-f0-9]{40})";
 182 | +/** ERE string matching an xpub or xprv. */
 183 | +const std::string STR_XPUBPRV = "((xpub|xprv)" + STR_BASE58CHAR + "{74,108})";
 184 | +/** ERE string matching a single derivation path step: a plain index, or a BIP389 multipath
 185 | + *  specifier like <1;2;3>. */
 186 | +const std::string STR_KEYSTEP = "([0-9]+['h]?|[<][0-9]+['h]?([;][0-9]+['h]?)*[>])";

jeanpablojp commented at 4:56 PM on September 3, 2026:

The multipath alternative here seems avoidable. It only exists because the roundtrip comparison uses pub, the literal test string. In these checks, pub1 is expanded, and DoCheck() already compares it against pub, or the expanded expected_pub literal for multipath. With the old STR_KEYSTEP and the comparison on pub1, the suite passes. Both forms run the same 666 roundtrip checks and produce the same 60 failures under the tested InferScript() mutation. mark_count() is worth keeping either way. Is the literal intended to catch something pub1 would not?

in src/test/descriptor_tests.cpp:195 in 2d230d8366

 190 | +const std::string STR_KEYEXPR = "(" + STR_MAYBE_ORIGIN + "(" + STR_PUBKEY + "|" + STR_PUBKEYHASH + "|" + STR_BIP32KEY + "))";
 191 | +
 192 | +/** Regular expression matching a descriptor key expression. */
 193 | +const std::regex KEY_RE("([(,])" + STR_KEYEXPR + "([),])", std::regex::extended);
 194 | +/** Regular expression matching the word "sortedmulti". */
 195 | +const std::regex SORTEDMULTI_RE("(^|[(,])sortedmulti[(]", std::regex::extended);

jeanpablojp commented at 4:56 PM on September 3, 2026:

nit: this matches sortedmulti( but not sortedmulti_a(, which InferMultiA() gives back as multi_a. No vector uses it today, so nothing breaks, but I flipped the existing 1-of-1 multi_a vector to sortedmulti_a, same script by construction, and only the roundtrip comparison failed, six times, multi_a versus sortedmulti_a. Allowing an optional _a here and keeping it in the replacement fixes it, ran that and the suite went back to green.

in src/test/descriptor_tests.cpp:309 in 2d230d8366

 304 | @@ -240,7 +305,8 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int
 305 |      }
 306 |  
 307 |      // Check that both can be serialized with private key back to the private version, but not without private key.
 308 | -    if (!(flags & MISSING_PRIVKEYS)) {
 309 | +    // (Descriptors with no keys at all have nothing to check here.)
 310 | +    if (!(flags & (MISSING_PRIVKEYS | NO_KEYS))) {

jeanpablojp commented at 4:56 PM on September 3, 2026:

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, which is exactly the raw() and addr() case. I changed both implementations to return true and the whole unit suite stayed green. An else if (flags & NO_KEYS) with the two negative checks closes it. I ran that, and the same mutation now fails 14 times, twice per new vector.