rpc: Run type check on decodepsbt result

maflcko commented at 11:13 am on March 11, 2026: member

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:

0diff --git a/src/core_io.cpp b/src/core_io.cpp
1index 7492e9ca50..4927b70c8e 100644
2--- a/src/core_io.cpp
3+++ b/src/core_io.cpp
4@@ -436,2 +436,3 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
5     entry.pushKV("version", tx.version);
6+    entry.pushKV("bug", "error!");
7     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

refactor: Add and use RPCResultOptions

Initially only move skip_type_check there.

In the future, more options can be added, without having to touch the
constructors.

fa8250e961

DrahtBot renamed this:
~~rpc: Run type check on decodepsbt result~~
rpc: Run type check on decodepsbt result
on Mar 11, 2026

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

DrahtBot commented at 11:14 am on March 11, 2026: contributor

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 <!–meta-tag:bot-skip–> 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

refactor: Introduce TxDocOptions

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

fa4d5891b9

maflcko force-pushed on Mar 11, 2026

DrahtBot added the label CI failed on Mar 11, 2026

DrahtBot removed the label CI failed on Mar 11, 2026

satsfy commented at 4:14 pm on March 11, 2026: none

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:

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

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

maflcko force-pushed on Mar 11, 2026

willcl-ark commented at 8:39 pm on March 11, 2026: member

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 commented at 11:46 pm on March 11, 2026: none

Concept ACK fa1d7604a8e2.

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.

in src/rpc/util.cpp:1018 in fa1d7604a8 outdated

1014@@ -1015,9 +1015,17 @@ void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const
1015                (this->m_description.empty() ? "" : " " + this->m_description);
1016     };
1017 
1018+    if (m_opts.print_elision) {

seduless commented at 4:22 am on March 12, 2026:

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?

maflcko commented at 10:11 am on March 12, 2026:

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

seduless commented at 4:26 am on March 12, 2026: contributor

Tested ACK fa1d7604a8e20557f04be629a5b36342a4d724e4

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

DrahtBot requested review from willcl-ark on Mar 12, 2026

DrahtBot requested review from Bortlesboat on Mar 12, 2026

DrahtBot requested review from satsfy on Mar 12, 2026

rpc: Run type check on decodepsbt result

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
```

fafa5b6ad0

maflcko force-pushed on Mar 12, 2026

maflcko commented at 10:55 am on March 12, 2026: member

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

It should work, see

 0diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
 1index a04e0903b9..5c0866e3dc 100644
 2--- a/src/wallet/rpc/addresses.cpp
 3+++ b/src/wallet/rpc/addresses.cpp
 4@@ -365,6 +365,50 @@ static UniValue DescribeWalletAddress(const CWallet& wallet, const CTxDestinatio
 5     return ret;
 6 }
 7 
 8+static std::vector<RPCResult> AddrInfoDoc(const std::optional<std::string>& elision_description = {})
 9+{
10+    std::optional<std::string> maybe_skip{};
11+    if (elision_description) maybe_skip.emplace();
12+    return {
13+        {RPCResult::Type::STR, "address", "The bitcoin address validated.", {}, {.print_elision = elision_description}},
14+        {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded output script generated by the address.", {}, {.print_elision = maybe_skip}},
15+        {RPCResult::Type::BOOL, "isscript", /*optional=*/true, "If the key is a script.", {}, {.print_elision = maybe_skip}},
16+        {RPCResult::Type::BOOL, "iswitness", "If the address is a witness address.", {}, {.print_elision = maybe_skip}},
17+        {RPCResult::Type::NUM, "witness_version", /*optional=*/true, "The version number of the witness program.", {}, {.print_elision = maybe_skip}},
18+        {RPCResult::Type::STR_HEX, "witness_program", /*optional=*/true, "The hex value of the witness program.", {}, {.print_elision = maybe_skip}},
19+        {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}},
20+        {RPCResult::Type::STR, "script", /*optional=*/true, "The output script type. Only if isscript is true and the redeemscript is known. Possible\n"
21+                                                            "types: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_keyhash,\n"
22+                                                            "witness_v0_scripthash, witness_unknown.",
23+         {},
24+         {.print_elision = maybe_skip}},
25+        {RPCResult::Type::STR_HEX, "hex", /*optional=*/true, "The redeemscript for the p2sh address.", {}, {.print_elision = maybe_skip}},
26+        {RPCResult::Type::BOOL, "iscompressed", /*optional=*/true, "If the pubkey is compressed.", {}, {.print_elision = maybe_skip}},
27+        {
28+            RPCResult::Type::OBJ,
29+            "embedded",
30+            /*optional=*/true,
31+            "Information about the address embedded in P2SH or P2WSH, if relevant and known.",
32+            {
33+                {RPCResult::Type::STR, "address", "The bitcoin address validated.", {}, {.print_elision = elision_description}},
34+                {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded output script generated by the address.", {}, {.print_elision = maybe_skip}},
35+                {RPCResult::Type::BOOL, "isscript", /*optional=*/true, "If the key is a script.", {}, {.print_elision = maybe_skip}},
36+                {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}},
37+                {RPCResult::Type::STR, "script", /*optional=*/true, "The output script type. Only if isscript is true and the redeemscript is known. Possible\n"
38+                                                                    "types: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_keyhash,\n"
39+                                                                    "witness_v0_scripthash, witness_unknown.",
40+                 {},
41+                 {.print_elision = maybe_skip}},
42+                {RPCResult::Type::STR_HEX, "hex", /*optional=*/true, "The redeemscript for the p2sh address.", {}, {.print_elision = maybe_skip}},
43+                {RPCResult::Type::BOOL, "iswitness", "If the address is a witness address.", {}, {.print_elision = maybe_skip}},
44+                {RPCResult::Type::BOOL, "iscompressed", /*optional=*/true, "If the pubkey is compressed.", {}, {.print_elision = maybe_skip}},
45+            },
46+            {.print_elision = maybe_skip},
47+        },
48+    };
49+}
50+
51+
52 RPCHelpMan getaddressinfo()
53 {
54     return RPCHelpMan{
55@@ -401,8 +445,8 @@ RPCHelpMan getaddressinfo()
56                         {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)."},
57                         {RPCResult::Type::OBJ, "embedded", /*optional=*/true, "Information about the address embedded in P2SH or P2WSH, if relevant and known.",
58                         {
59-                            {RPCResult::Type::ELISION, "", "Includes all getaddressinfo output fields for the embedded address, excluding metadata (timestamp, hdkeypath, hdseedid)\n"
60-                            "and relation to the wallet (ismine)."},
61+                            AddrInfoDoc("Includes all getaddressinfo output fields for the embedded address, excluding metadata (timestamp, hdkeypath, hdseedid)\n"
62+                            "and relation to the wallet (ismine)."),
63                         }},
64                         {RPCResult::Type::BOOL, "iscompressed", /*optional=*/true, "If the pubkey is compressed."},
65                         {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 commented at 3:44 pm on March 12, 2026: none

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 commented at 3:56 pm on March 12, 2026: member

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 commented at 4:08 pm on March 12, 2026: none

Re-ACK fafa5b6ad00e (source-level).

I reviewed the changes since my prior ACK on fa1d7604a8e2, 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.

DrahtBot requested review from seduless on Mar 12, 2026

satsfy commented at 4:23 pm on March 12, 2026: none

ACK https://github.com/bitcoin/bitcoin/commit/fafa5b6ad00e16ef08fc141af2aa58b84dafde93

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.

in src/rpc/util.h:301 in fafa5b6ad0

293@@ -294,6 +294,11 @@ struct RPCArg {
294 
295 struct RPCResultOptions {
296     bool skip_type_check{false};
297+    /// Whether to treat this as elided in the human-readable description, and
298+    /// possibly supply a description for the elision. Normally, there will be
299+    /// one string on any of the elided results, for example `Same output as
300+    /// verbosity = 1`, and all other elided strings will be empty.
301+    std::optional<std::string> print_elision{std::nullopt};

willcl-ark commented at 8:29 pm on March 13, 2026:

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

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

DrahtBot requested review from willcl-ark on Mar 13, 2026

nervana21 commented at 9:08 pm on March 13, 2026: contributor

Concept ACK

seduless commented at 9:48 pm on March 13, 2026: contributor

re-ACK fafa5b6ad00e16ef08fc141af2aa58b84dafde93

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

rpc: Run type check on decodepsbt result #34799

Reviews

Conflicts

LLM Linter (✨ experimental)