rpc: Run type check on decodepsbt result by maflcko · Pull Request #34799 · bitcoin/bitcoin

maflcko · 2026-03-11T11:13:25Z

For RPCResults, the type may be ELISION, which is confusing and brittle:

The elision should only affect the help output, not the type.
The type should be the real type, so that type checks can be run on it.

Fix this issue by introducing a new print_elision option and using it in decodepsbt.

This change will ensure that RPCResult::MatchesType is properly run.

Can be tested by introducing a bug:

diff --git a/src/core_io.cpp b/src/core_io.cpp
index 7492e9ca50..4927b70c8e 100644
--- a/src/core_io.cpp
+++ b/src/core_io.cpp
@@ -436,2 +436,3 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
     entry.pushKV("version", tx.version);
+    entry.pushKV("bug", "error!");
     entry.pushKV("size", tx.ComputeTotalSize());

And then running (in a debug build) decodepsbt cHNidP8BAAoCAAAAAAAAAAAAAA==

Before, on master: passes
Now, on this pull: Properly detects the bug

Initially only move skip_type_check there. In the future, more options can be added, without having to touch the constructors.

DrahtBot · 2026-03-11T11:14:02Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Bortlesboat, satsfy, seduless
Concept ACK	willcl-ark, nervana21

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34764 (rpc: replace ELISION references with explicit result fields by satsfy)
#34683 (RFC: Support a format description of our JSON-RPC interface by willcl-ark)
#34514 (refactor: remove unnecessary std::move for trivially copyable types by l0rinc)
#21283 (Implement BIP 370 PSBTv2 by achow101)

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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

RPCResult{RPCResult::Type::BOOL, "ischange", /optional=/true, "Output script is change (only present if true)"} in src/rpc/rawtransaction_util.cpp

^{2026-03-12 10:11:24}

This prepares the function to be more flexible, when more options are passed in the future.

satsfy

Concept ACK

Thanks for this, it enables much finer control of the TxDoc() in bitcoin-cli help. I'm rebasing it in #34764.

Separately, I'm planning to handle the more general elision case in my branch, taking inspiration from your print_elision mechanism. For example, getrawtransaction verbosity = 2 previously showed:

Result (for verbosity = 2):
{                                    (json object)
  ...,                               Same output as verbosity = 1
  "fee" : n,                         (numeric, optional) transaction fee in BTC, omitted if block undo data is not available,
  ...only shows what is different from verbosity = 1
}

But now expands everything. The solution you introduced here could potentially solve that too.

willcl-ark · 2026-03-11T20:39:22Z

Concept ACK.

This seems like the correct fix to the conflation of type-check skipping with (display) elision, and therefore fixes it in the right layer. I think this is the pattern we'd want to follow to convert the remaining elision sites to use print_elision (although not sure yet how that would work with getaddressinfo...)

Bortlesboat

Concept ACK fa1d760.

I traced the relevant path through RPCHelpMan::HandleRequest() -> RPCResult::MatchesType() and RPCResult::ToSections(), and the separation here looks right to me: print_elision is now a presentation concern, while type-check suppression stays under skip_type_check.

That seems to fix the decodepsbt issue in the correct layer. Before this change, using ELISION for human-readable help also caused the result metadata to skip validation entirely. After this change, decodepsbt can keep the concise help text via TxDoc({.elision_description=...}) while still participating in runtime doc checks.

I wasn't able to rebuild this revision from the current terminal because cmake is not on PATH here, so this is a source-level review rather than a tested ACK.

seduless

Tested ACK fa1d760

Verified the mutation test from the PR description catches the injected bug. Built debug, ran functional and unit tests.

seduless · 2026-03-12T04:22:08Z

src/rpc/util.cpp

               (this->m_description.empty() ? "" : " " + this->m_description);
    };

+    if (m_opts.print_elision) {


If print_elision is set to an empty string, all children silently return and the parent's OBJ handler corrupts the output. Do you think it's worth guarding against?

Yeah, I guess it can't hurt to fail than to silently produce a corrupt help test. Done, thx!

For RPCResults, the type may be ELISION, which is confusing and brittle: * The elision should only affect the help output, not the type. * The type should be the real type, so that type checks can be run on it. Fix this issue by introducing a new print_elision option and using it in decodepsbt. This change will ensure that RPCResult::MatchesType is properly run. Also, this clarifies the RPC output minimally: ```diff --- a/decodepsbt +++ b/decodepsbt @@ -35,7 +35,7 @@ Result: "inputs" : [ (json array) { (json object) "non_witness_utxo" : { (json object, optional) Decoded network transaction for non-witness UTXOs - ... + ... The layout is the same as the output of decoderawtransaction. }, "witness_utxo" : { (json object, optional) Transaction output for witness UTXOs "amount" : n, (numeric) The value in BTC ```

maflcko · 2026-03-12T10:55:36Z

although not sure yet how that would work with getaddressinfo...)

It should work, see

a diff

diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index a04e0903b9..5c0866e3dc 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -365,6 +365,50 @@ static UniValue DescribeWalletAddress(const CWallet& wallet, const CTxDestinatio
     return ret;
 }
 
+static std::vector<RPCResult> AddrInfoDoc(const std::optional<std::string>& elision_description = {})
+{
+    std::optional<std::string> maybe_skip{};
+    if (elision_description) maybe_skip.emplace();
+    return {
+        {RPCResult::Type::STR, "address", "The bitcoin address validated.", {}, {.print_elision = elision_description}},
+        {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded output script generated by the address.", {}, {.print_elision = maybe_skip}},
+        {RPCResult::Type::BOOL, "isscript", /*optional=*/true, "If the key is a script.", {}, {.print_elision = maybe_skip}},
+        {RPCResult::Type::BOOL, "iswitness", "If the address is a witness address.", {}, {.print_elision = maybe_skip}},
+        {RPCResult::Type::NUM, "witness_version", /*optional=*/true, "The version number of the witness program.", {}, {.print_elision = maybe_skip}},
+        {RPCResult::Type::STR_HEX, "witness_program", /*optional=*/true, "The hex value of the witness program.", {}, {.print_elision = maybe_skip}},
+        {RPCResult::Type::STR_HEX, "pubkey", /*optional=*/true, "The hex value of the raw public key for single-key addresses (possibly embedded in P2SH or P2WSH).", {}, {.print_elision = maybe_skip}},
+        {RPCResult::Type::STR, "script", /*optional=*/true, "The output script type. Only if isscript is true and the redeemscript is known. Possible\n"
+                                                            "types: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_keyhash,\n"
+                                                            "witness_v0_scripthash, witness_unknown.",
+         {},
+         {.print_elision = maybe_skip}},
+        {RPCResult::Type::STR_HEX, "hex", /*optional=*/true, "The redeemscript for the p2sh address.", {}, {.print_elision = maybe_skip}},
+        {RPCResult::Type::BOOL, "iscompressed", /*optional=*/true, "If the pubkey is compressed.", {}, {.print_elision = maybe_skip}},
+        {
+            RPCResult::Type::OBJ,
+            "embedded",
+            /*optional=*/true,
+            "Information about the address embedded in P2SH or P2WSH, if relevant and known.",
+            {
+                {RPCResult::Type::STR, "address", "The bitcoin address validated.", {}, {.print_elision = elision_description}},
+                {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded output script generated by the address.", {}, {.print_elision = maybe_skip}},
+                {RPCResult::Type::BOOL, "isscript", /*optional=*/true, "If the key is a script.", {}, {.print_elision = maybe_skip}},
+                {RPCResult::Type::STR_HEX, "pubkey", /*optional=*/true, "The hex value of the raw public key for single-key addresses (possibly embedded in P2SH or P2WSH).", {}, {.print_elision = maybe_skip}},
+                {RPCResult::Type::STR, "script", /*optional=*/true, "The output script type. Only if isscript is true and the redeemscript is known. Possible\n"
+                                                                    "types: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_keyhash,\n"
+                                                                    "witness_v0_scripthash, witness_unknown.",
+                 {},
+                 {.print_elision = maybe_skip}},
+                {RPCResult::Type::STR_HEX, "hex", /*optional=*/true, "The redeemscript for the p2sh address.", {}, {.print_elision = maybe_skip}},
+                {RPCResult::Type::BOOL, "iswitness", "If the address is a witness address.", {}, {.print_elision = maybe_skip}},
+                {RPCResult::Type::BOOL, "iscompressed", /*optional=*/true, "If the pubkey is compressed.", {}, {.print_elision = maybe_skip}},
+            },
+            {.print_elision = maybe_skip},
+        },
+    };
+}
+
+
 RPCHelpMan getaddressinfo()
 {
     return RPCHelpMan{
@@ -401,8 +445,8 @@ RPCHelpMan getaddressinfo()
                         {RPCResult::Type::STR_HEX, "pubkey", /*optional=*/true, "The hex value of the raw public key for single-key addresses (possibly embedded in P2SH or P2WSH)."},
                         {RPCResult::Type::OBJ, "embedded", /*optional=*/true, "Information about the address embedded in P2SH or P2WSH, if relevant and known.",
                         {
-                            {RPCResult::Type::ELISION, "", "Includes all getaddressinfo output fields for the embedded address, excluding metadata (timestamp, hdkeypath, hdseedid)\n"
-                            "and relation to the wallet (ismine)."},
+                            AddrInfoDoc("Includes all getaddressinfo output fields for the embedded address, excluding metadata (timestamp, hdkeypath, hdseedid)\n"
+                            "and relation to the wallet (ismine)."),
                         }},
                         {RPCResult::Type::BOOL, "iscompressed", /*optional=*/true, "If the pubkey is compressed."},
                         {RPCResult::Type::NUM_TIME, "timestamp", /*optional=*/true, "The creation time of the key, if available, expressed in " + UNIX_EPOCH_TIME + "."},

However, I think the docs are wrong, and it could make sense to fix them. However, this pull is mostly meant as a refactor and fixing the docs can be done in a follow-up.

satsfy · 2026-03-12T15:44:09Z

It should work, see

Nice! I had tried this before but ran into one ambiguity: the runtime code in ProcessSubScript is genuinely recursive (the NOLINTNEXTLINE(misc-no-recursion) annotations confirm this). It calls std::visit(*this, embedded), which for ScriptHash or WitnessV0ScriptHash destinations re-enters ProcessSubScript, producing nested embedded.embedded. What the diff seems to imply is that we only need to worry about two embedded levels for common real-world case P2SH-P2WSH(multisig). But AddrInfoDoc() hardcoding embedded levels won't fully match all potential runtime scenarios.

can be done in a follow-up.

I'm planning to try and do this in #34764, or do you think it should be follow-up?

maflcko · 2026-03-12T15:56:52Z

But AddrInfoDoc() hardcoding embedded levels won't fully match all potential runtime scenarios.

Why not? It is not possible to, let's say, nest witness twice. Also, if the docs didn't match, it would be caught, which is the goal of this pull. However, I am thinking if in-lining is better here, so I think it should be a separate pull request or a follow-up pull request.

The goal of this pull is to not fix any bugs or change the docs. It is mostly meant as a refactor.

Bortlesboat

Re-ACK fafa5b6 (source-level).

I reviewed the changes since my prior ACK on fa1d760, including the RPCResultOptions migration and the new print_elision path in RPCResult::ToSections(). The separation between display elision and type-check behavior now looks consistent, and decodepsbt now references TxDoc({.elision_description=...}) in a way that keeps MatchesType() active.

No blocking issues found in this delta. I still haven’t rebuilt this revision locally in this environment, so this is untested source review.

satsfy

ACK fafa5b6

Successfully provides elision description while specifying the full doc for given rpcs.

It is not possible to, let's say, nest witness twice

Thanks for clearing up my thinking.

willcl-ark · 2026-03-13T20:29:27Z

src/rpc/util.h

+    /// Whether to treat this as elided in the human-readable description, and
+    /// possibly supply a description for the elision. Normally, there will be
+    /// one string on any of the elided results, for example `Same output as
+    /// verbosity = 1`, and all other elided strings will be empty.
+    std::optional<std::string> print_elision{std::nullopt};


Would it be worth adding even more comment here to explain the three states:

/// If nullopt: normal display. If empty: suppress from help. If non-empty: show "..." with this description.

nervana21 · 2026-03-13T21:08:27Z

Concept ACK

seduless

re-ACK fafa5b6

git range-diff bitcoin-core/master fa1d760 fafa5b6 looks good.

Verified the empty string edge case is caught:

Debug build: node aborts as expected
Release build: returns RPC error without crashing

refactor: Add and use RPCResultOptions

fa8250e

Initially only move skip_type_check there. In the future, more options can be added, without having to touch the constructors.

DrahtBot changed the title ~~rpc: Run type check on decodepsbt result~~ rpc: Run type check on decodepsbt result Mar 11, 2026

DrahtBot added the RPC/REST/ZMQ label Mar 11, 2026

refactor: Introduce TxDocOptions

fa4d589

This prepares the function to be more flexible, when more options are passed in the future.

maflcko force-pushed the 2603-rpc-elision-type-check branch from fa23ecd to fab38de Compare March 11, 2026 11:27

DrahtBot added the CI failed label Mar 11, 2026

maflcko mentioned this pull request Mar 11, 2026

rpc: replace ELISION references with explicit result fields #34764

Draft

DrahtBot removed the CI failed label Mar 11, 2026

satsfy reviewed Mar 11, 2026

View reviewed changes

maflcko force-pushed the 2603-rpc-elision-type-check branch from fab38de to fa1d760 Compare March 11, 2026 16:26

Bortlesboat reviewed Mar 11, 2026

View reviewed changes

seduless reviewed Mar 12, 2026

View reviewed changes

DrahtBot requested review from Bortlesboat, satsfy and willcl-ark March 12, 2026 04:26

maflcko force-pushed the 2603-rpc-elision-type-check branch from fa1d760 to fafa5b6 Compare March 12, 2026 10:10

Bortlesboat reviewed Mar 12, 2026

View reviewed changes

DrahtBot requested a review from seduless March 12, 2026 16:08

satsfy reviewed Mar 12, 2026

View reviewed changes

willcl-ark reviewed Mar 13, 2026

View reviewed changes

DrahtBot requested a review from willcl-ark March 13, 2026 20:29

seduless reviewed Mar 13, 2026

View reviewed changes

DrahtBot mentioned this pull request Mar 14, 2026

RFC: Support a format description of our JSON-RPC interface #34683

Draft

DrahtBot mentioned this pull request Mar 14, 2026

refactor: remove unnecessary std::move for trivially copyable types #34514

Open

Conversation

maflcko commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

satsfy left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented Mar 11, 2026

Uh oh!

Bortlesboat left a comment

Choose a reason for hiding this comment

Uh oh!

seduless left a comment

Choose a reason for hiding this comment

Uh oh!

seduless Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 12, 2026

Uh oh!

satsfy commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Mar 12, 2026

Uh oh!

Bortlesboat left a comment

Choose a reason for hiding this comment

Uh oh!

satsfy left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

nervana21 commented Mar 13, 2026

Uh oh!

seduless left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maflcko commented Mar 11, 2026 •

edited

Loading

DrahtBot commented Mar 11, 2026 •

edited

Loading

satsfy commented Mar 12, 2026 •

edited

Loading