Merge bitcoin/bitcoin#34799: rpc: Run type check on decodepsbt result

fadf901fd4 rpc: Run type check on decodepsbt result (MarcoFalke) fa4d5891b9 refactor: Introduce TxDocOptions (MarcoFalke) fa8250e961 refactor: Add and use RPCResultOptions (MarcoFalke) Pull request description: 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 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 ACKs for top commit: nervana21: tACK fadf901fd4 achow101: ACK fadf901fd4 willcl-ark: ACK fadf901fd4 satsfy: re-ACK fadf901fd4 seduless: re-ACK fadf901fd4 Tree-SHA512: 4fb000dba9fe39bcd2bac72e2d88553f54134a250c985b4ca7150b483d7185009047d8fe4ba75c522bfc26706de20c913b8905e7552ab0c41802ae744cb92038
2026-04-16 10:37:21 +02:00 · 2026-03-18 11:59:18 -07:00
parent 33eaf910bd fadf901fd4
commit d81b562fca
8 changed files with 71 additions and 34 deletions
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -2739,7 +2739,7 @@ static RPCHelpMan getdescriptoractivity()
                        {RPCResult::Type::OBJ, "output_spk", "", ScriptPubKeyDoc()},
                    }},
                    // TODO is the skip_type_check avoidable with a heterogeneous ARR?
-                }, /*skip_type_check=*/true},
+                }, {.skip_type_check=true}, },
            },
        },
        RPCExamples{
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -248,7 +248,7 @@ static RPCHelpMan getrawtransaction()
                             {RPCResult::Type::NUM, "time", /*optional=*/true, "Same as \"blocktime\""},
                             {RPCResult::Type::STR_HEX, "hex", "The serialized, hex-encoded data for 'txid'"},
                         },
-                         DecodeTxDoc(/*txid_field_doc=*/"The transaction id (same as provided)", /*wallet=*/false)),
+                         TxDoc({.txid_field_doc="The transaction id (same as provided)"})),
                    },
                    RPCResult{"for verbosity = 2",
                        RPCResult::Type::OBJ, "", "",
@@ -422,7 +422,7 @@ static RPCHelpMan decoderawtransaction()
                },
                RPCResult{
                    RPCResult::Type::OBJ, "", "",
-                    DecodeTxDoc(/*txid_field_doc=*/"The transaction id", /*wallet=*/false),
+                    TxDoc(),
                },
                RPCExamples{
                    HelpExampleCli("decoderawtransaction", "\"hexstring\"")
@@ -781,9 +781,8 @@ const RPCResult decodepsbt_inputs{
        {RPCResult::Type::OBJ, "", "",
        {
            {RPCResult::Type::OBJ, "non_witness_utxo", /*optional=*/true, "Decoded network transaction for non-witness UTXOs",
-            {
-                {RPCResult::Type::ELISION, "",""},
-            }},
+                TxDoc({.elision_description="The layout is the same as the output of decoderawtransaction."})
+            },
            {RPCResult::Type::OBJ, "witness_utxo", /*optional=*/true, "Transaction output for witness UTXOs",
            {
                {RPCResult::Type::NUM, "amount", "The value in " + CURRENCY_UNIT},
@@ -1023,9 +1022,8 @@ static RPCHelpMan decodepsbt()
                    RPCResult::Type::OBJ, "", "",
                    {
                        {RPCResult::Type::OBJ, "tx", "The decoded network-serialized unsigned transaction.",
-                        {
-                            {RPCResult::Type::ELISION, "", "The layout is the same as the output of decoderawtransaction."},
-                        }},
+                            TxDoc({.elision_description="The layout is the same as the output of decoderawtransaction."})
+                        },
                        {RPCResult::Type::ARR, "global_xpubs", "",
                        {
                            {RPCResult::Type::OBJ, "", "",
--- a/src/rpc/rawtransaction_util.cpp
+++ b/src/rpc/rawtransaction_util.cpp
@@ -344,16 +344,18 @@ void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const
    }
 }

-std::vector<RPCResult> DecodeTxDoc(const std::string& txid_field_doc, bool wallet)
+std::vector<RPCResult> TxDoc(const TxDocOptions& opts)
 {
+    std::optional<std::string> maybe_skip{};
+    if (opts.elision_description) maybe_skip.emplace();
    return {
-        {RPCResult::Type::STR_HEX, "txid", txid_field_doc},
-        {RPCResult::Type::STR_HEX, "hash", "The transaction hash (differs from txid for witness transactions)"},
-        {RPCResult::Type::NUM, "size", "The serialized transaction size"},
-        {RPCResult::Type::NUM, "vsize", "The virtual transaction size (differs from size for witness transactions)"},
-        {RPCResult::Type::NUM, "weight", "The transaction's weight (between vsize*4-3 and vsize*4)"},
-        {RPCResult::Type::NUM, "version", "The version"},
-        {RPCResult::Type::NUM_TIME, "locktime", "The lock time"},
+        {RPCResult::Type::STR_HEX, "txid", opts.txid_field_doc, {}, {.print_elision=opts.elision_description}},
+        {RPCResult::Type::STR_HEX, "hash", "The transaction hash (differs from txid for witness transactions)", {}, {.print_elision=maybe_skip}},
+        {RPCResult::Type::NUM, "size", "The serialized transaction size", {}, {.print_elision=maybe_skip}},
+        {RPCResult::Type::NUM, "vsize", "The virtual transaction size (differs from size for witness transactions)", {}, {.print_elision=maybe_skip}},
+        {RPCResult::Type::NUM, "weight", "The transaction's weight (between vsize*4-3 and vsize*4)", {}, {.print_elision=maybe_skip}},
+        {RPCResult::Type::NUM, "version", "The version", {}, {.print_elision=maybe_skip}},
+        {RPCResult::Type::NUM_TIME, "locktime", "The lock time", {}, {.print_elision=maybe_skip}},
        {RPCResult::Type::ARR, "vin", "",
        {
            {RPCResult::Type::OBJ, "", "",
@@ -372,7 +374,7 @@ std::vector<RPCResult> DecodeTxDoc(const std::string& txid_field_doc, bool walle
                }},
                {RPCResult::Type::NUM, "sequence", "The script sequence number"},
            }},
-        }},
+        }, {.print_elision=maybe_skip}},
        {RPCResult::Type::ARR, "vout", "",
        {
            {RPCResult::Type::OBJ, "", "", Cat(
@@ -381,11 +383,11 @@ std::vector<RPCResult> DecodeTxDoc(const std::string& txid_field_doc, bool walle
                    {RPCResult::Type::NUM, "n", "index"},
                    {RPCResult::Type::OBJ, "scriptPubKey", "", ScriptPubKeyDoc()},
                },
-                    wallet ?
+                    opts.wallet ?
                    std::vector<RPCResult>{{RPCResult::Type::BOOL, "ischange", /*optional=*/true, "Output script is change (only present if true)"}} :
                    std::vector<RPCResult>{}
                )
            },
-        }},
+        }, {.print_elision=maybe_skip}},
    };
 }
--- a/src/rpc/rawtransaction_util.h
+++ b/src/rpc/rawtransaction_util.h
@@ -56,7 +56,15 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in);
 /** Create a transaction from univalue parameters */
 CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional<bool> rbf, uint32_t version);

+struct TxDocOptions {
+    /// The description of the txid field
+    std::string txid_field_doc{"The transaction id"};
+    /// Include wallet-related fields (e.g. ischange on outputs)
+    bool wallet{false};
+    /// Treat this as an elided Result in the help
+    std::optional<std::string> elision_description{};
+};
 /** Explain the UniValue "decoded" transaction object, may include extra fields if processed by wallet **/
-std::vector<RPCResult> DecodeTxDoc(const std::string& txid_field_doc, bool wallet);
+std::vector<RPCResult> TxDoc(const TxDocOptions& opts = {});

 #endif // BITCOIN_RPC_RAWTRANSACTION_UTIL_H
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -1015,9 +1015,22 @@ void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const
               (this->m_description.empty() ? "" : " " + this->m_description);
    };

+    // Ensure at least one elision description exists, if there is any elision
+    const auto elision_has_description{[](const std::vector<RPCResult>& inner) {
+        return std::ranges::none_of(inner, [](const auto& res) { return res.m_opts.print_elision.has_value(); }) ||
+               std::ranges::any_of(inner, [](const auto& res) { return res.m_opts.print_elision.has_value() && !res.m_opts.print_elision->empty(); });
+    }};
+
+    if (m_opts.print_elision) {
+        if (!m_opts.print_elision->empty()) {
+            sections.PushSection({indent + "..." + maybe_separator, *m_opts.print_elision});
+        }
+        return;
+    }
+
    switch (m_type) {
    case Type::ELISION: {
-        // If the inner result is empty, use three dots for elision
+        // Deprecated alias of m_opts.print_elision
        sections.PushSection({indent + "..." + maybe_separator, m_description});
        return;
    }
@@ -1059,6 +1072,7 @@ void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const
            i.ToSections(sections, OuterType::ARR, current_indent + 2);
        }
        CHECK_NONFATAL(!m_inner.empty());
+        CHECK_NONFATAL(elision_has_description(m_inner));
        if (m_type == Type::ARR && m_inner.back().m_type != Type::ELISION) {
            sections.PushSection({indent_next + "...", ""});
        } else {
@@ -1074,6 +1088,7 @@ void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const
            sections.PushSection({indent + maybe_key + "{}", Description("empty JSON object")});
            return;
        }
+        CHECK_NONFATAL(elision_has_description(m_inner));
        sections.PushSection({indent + maybe_key + "{", Description("json object")});
        for (const auto& i : m_inner) {
            i.ToSections(sections, OuterType::OBJ, current_indent + 2);
@@ -1130,7 +1145,7 @@ static std::optional<UniValue::VType> ExpectedType(RPCResult::Type type)
 // NOLINTNEXTLINE(misc-no-recursion)
 UniValue RPCResult::MatchesType(const UniValue& result) const
 {
-    if (m_skip_type_check) {
+    if (m_opts.skip_type_check) {
        return true;
    }

--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -292,6 +292,18 @@ struct RPCArg {
    std::string ToDescriptionString(bool is_named_arg) const;
 };

+struct RPCResultOptions {
+    bool skip_type_check{false};
+    /// 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.
+    ///
+    /// - If nullopt: normal display.
+    /// - If empty string: suppress from help.
+    /// - If non-empty: show "..." with this description.
+    std::optional<std::string> print_elision{std::nullopt};
+};
 // NOLINTNEXTLINE(misc-no-recursion)
 struct RPCResult {
    enum class Type {
@@ -314,7 +326,7 @@ struct RPCResult {
    const std::string m_key_name;         //!< Only used for dicts
    const std::vector<RPCResult> m_inner; //!< Only used for arrays or dicts
    const bool m_optional;
-    const bool m_skip_type_check;
+    const RPCResultOptions m_opts;
    const std::string m_description;
    const std::string m_cond;

@@ -324,12 +336,13 @@ struct RPCResult {
        std::string m_key_name,
        bool optional,
        std::string description,
-        std::vector<RPCResult> inner = {})
+        std::vector<RPCResult> inner = {},
+        RPCResultOptions opts = {})
        : m_type{std::move(type)},
          m_key_name{std::move(m_key_name)},
          m_inner{std::move(inner)},
          m_optional{optional},
-          m_skip_type_check{false},
+          m_opts{std::move(opts)},
          m_description{std::move(description)},
          m_cond{std::move(cond)}
    {
@@ -342,8 +355,9 @@ struct RPCResult {
        Type type,
        std::string m_key_name,
        std::string description,
-        std::vector<RPCResult> inner = {})
-        : RPCResult{std::move(cond), type, std::move(m_key_name), /*optional=*/false, std::move(description), std::move(inner)} {}
+        std::vector<RPCResult> inner = {},
+        RPCResultOptions opts = {})
+        : RPCResult{std::move(cond), type, std::move(m_key_name), /*optional=*/false, std::move(description), std::move(inner), std::move(opts)} {}

    RPCResult(
        Type type,
@@ -351,12 +365,12 @@ struct RPCResult {
        bool optional,
        std::string description,
        std::vector<RPCResult> inner = {},
-        bool skip_type_check = false)
+        RPCResultOptions opts = {})
        : m_type{std::move(type)},
          m_key_name{std::move(m_key_name)},
          m_inner{std::move(inner)},
          m_optional{optional},
-          m_skip_type_check{skip_type_check},
+          m_opts{std::move(opts)},
          m_description{std::move(description)},
          m_cond{}
    {
@@ -368,8 +382,8 @@ struct RPCResult {
        std::string m_key_name,
        std::string description,
        std::vector<RPCResult> inner = {},
-        bool skip_type_check = false)
-        : RPCResult{type, std::move(m_key_name), /*optional=*/false, std::move(description), std::move(inner), skip_type_check} {}
+        RPCResultOptions opts = {})
+        : RPCResult{type, std::move(m_key_name), /*optional=*/false, std::move(description), std::move(inner), std::move(opts)} {}

    /** Append the sections of the result. */
    void ToSections(Sections& sections, OuterType outer_type = OuterType::NONE, int current_indent = 0) const;
--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -705,7 +705,7 @@ RPCHelpMan gettransaction()
                        {RPCResult::Type::STR_HEX, "hex", "Raw data for transaction"},
                        {RPCResult::Type::OBJ, "decoded", /*optional=*/true, "The decoded transaction (only present when `verbose` is passed)",
                        {
-                            DecodeTxDoc(/*txid_field_doc=*/"The transaction id", /*wallet=*/true),
+                            TxDoc({.wallet = true}),
                        }},
                        RESULT_LAST_PROCESSED_BLOCK,
                    })
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -53,7 +53,7 @@ static RPCHelpMan getwalletinfo()
                        {
                            {RPCResult::Type::NUM, "duration", "elapsed seconds since scan start"},
                            {RPCResult::Type::NUM, "progress", "scanning progress percentage [0.0, 1.0]"},
-                        }, /*skip_type_check=*/true},
+                        }, {.skip_type_check=true}, },
                        {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for output script management"},
                        {RPCResult::Type::BOOL, "external_signer", "whether this wallet is configured to use an external signer such as a hardware wallet"},
                        {RPCResult::Type::BOOL, "blank", "Whether this wallet intentionally does not contain any keys, scripts, or descriptors"},