#36648 descriptor, rpc, wallet: do not echo key material in error messages
| Reviewability | Reviews | Agreement | Size | |||||||
|---|---|---|---|---|---|---|---|---|---|---|
| #36648 descriptor, rpc, wallet: do not echo key material in error messages | CI failing | 0 (+1) | Positive | 32 + 131 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
Stops echoing input key material and unparsed descriptor strings in error messages produced during descriptor parsing, wallet imports, and transaction funding. Replaces raw key echoes with generic descriptions, caps echoed tokens at lengths too short to contain keys, and prints public representations via desc->ToString() when available. Updates existing functional and unit tests to reflect the modified error strings and adds test coverage verifying private keys are omitted from errors.
Problem: Descriptor inputs can include secret material such as WIF private keys or extended private keys (xprvs). When descriptor parsing or derivation fails, current error messages quote the raw input strings directly, exposing private keys to application logs, exception reporters, or support screenshots.
Evidence of importance:
- PR description: 'Some descriptor error messages quote the string the caller sent, and a descriptor can hold a private key (a WIF, an xprv). The reply then carries the key into places the request may not go: an application's error log, an error reporter, a screenshot shared for help. Even the first characters of a key give away part of it.'
- Commit a3f26e8343: 'Error text is often logged or shared where the request is not. Even the first characters of a key give away part of it.'
Risk of a mistake: medium (rpc_api, wallet). Altering RPC error messages can break external scripts or wallet clients that rely on matching specific error string patterns.
Changed since the previous assessment: No changes to the PR's contents; the patch is now provided where it was previously missing.
Reviewability
Ready · CI failing
CI is red; check whether the failure is the PR's before a deep review.
Waiting on (from the thread read):
- reviewer: uqlidi to review the author's responses and changes 2026-11-01
Agreement
Positive
support without stated reasons, no open objection (jpk68)
Thread notes: The author responded to all four items raised by uqlidi, updating the error messages and explaining that MAX_QUOTED_CHARS was dropped.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| uqlidi (none) | objection, interface | For expressions like 'wpkh(KEY)x', the error message incorrectly claims 'wpkh' is not a valid descriptor function instead of indicating that the problem is trailing characters | answered · author changed the error to note unexpected text after the closing parenthesis | answered | yes | 2026-10-28: 'wpkh is a valid function; the real problem is the trailing x' Status from: 2026-11-01: fametrano: 'Done: `wpkh(KEY)x` now gives `Unexpected text after the closing ')' of wpkh()`' |
| uqlidi (none) | suggestion, interface | Print the key's position or index in the error message when a key fails parsing in a multi-key descriptor so users do not have to check each key by hand | answered · author updated the error to state the key's 1-indexed position | answered | yes | 2026-10-28: 'You can print the key's position.' Status from: 2026-11-01: fametrano: 'Done: the error gives the key's position in its list, counted from 1' |
| uqlidi (none) | suggestion, interface | Include the index of the failing descriptor in the error message when FundTransaction fails to parse a descriptor from solving_data | answered · author included descriptor index in the error message | answered | yes | 2026-10-28: 'You can print its index in the list.' Status from: 2026-11-01: fametrano: 'Done with your loop: `Unable to parse descriptor at index 1: Missing checksum`.' |
| uqlidi (none) | suggestion, maintenance | Define MAX_QUOTED_CHARS once and reuse it across bip32.cpp and descriptor.cpp | answered · author replied that MAX_QUOTED_CHARS was removed entirely | answered | yes | 2026-10-28: 'you can define it here and reuse it inside descriptor.cpp' Status from: 2026-11-01: fametrano: '`MAX_QUOTED_CHARS` is gone: no error quotes text by length any more' |
Support:
- jpk68 (contributor) (ACK, 2026-10-27): no reason given [not substantive]
Participants:
- jpk68 (contributor, support, 1 statements 2026-10-27 to 2026-10-27): ACK with commit hash
- uqlidi (none, objection, 6 statements 2026-10-28 to 2026-10-28): objected to inaccurate error messages for trailing characters after descriptor functions; suggested printing key and descriptor indices and deduplicating constants
Files
155 lines under test/bench/ci.
- src/test/descriptor_tests.cpp +59/-10
- test/functional/rpc_getdescriptorinfo.py +32/-6
- src/script/descriptor.cpp +27/-6
- test/functional/rpc_scantxoutset.py +18/-0
- test/functional/wallet_importdescriptors.py +9/-6
- test/functional/wallet_fundrawtransaction.py +12/-1
- src/rpc/util.cpp +1/-1
- src/util/bip32.cpp +2/-0
- src/wallet/imports.cpp +1/-1
- src/wallet/rpc/spend.cpp +1/-1
- test/functional/wallet_miniscript.py +1/-1
Card
The short text the ranking pass compares across PRs.
Prevents descriptor parsing and wallet RPC errors from echoing raw key material back to callers. Descriptors can contain private keys (WIF, xprv), and when parsing or derivation fails, current errors echo the input text, risking exposure in application logs, error aggregators, or screenshots. The change suppresses echoing raw key strings, restricts quoting of function names and path tokens to lengths that cannot fit key material, and prints sanitized public descriptors (desc->ToString()) where available. Tests are updated to assert the sanitized error messages and ensure no key prefixes leak.
Data
thread read through 2026-11-01 (13 statements), head a0217c4781
labels: CI failed
Categories
RPC / REST / ZMQ
P2 · bug fix
Prevents exposure of sensitive key material via RPC error replies that can be captured in application logs or shared bug reports. The PR description notes that error messages currently 'quote the raw input strings directly, exposing private keys to application logs, exception reporters, or support screenshots', directly addressing RPC interface data exposure.
Membership: Changes RPC utility error reporting in src/rpc/util.cpp and modifies RPC error responses for descriptor methods to avoid echoing secret keys.
Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 0
Wallet
P2 · bug fix
Stops leaking private keys (WIF, xprvs) in error messages when importing descriptors or funding transactions. The author notes that 'a descriptor can hold a private key' and 'the reply then carries the key into places the request may not go', safeguarding key confidentiality during wallet failures.
Membership: Modifies error handling during descriptor imports and transaction funding in src/wallet/imports.cpp and src/wallet/rpc/spend.cpp.
Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 0