Merge #16873: rpc: fix regression in gettransaction

1b41c2c test: improve gettransaction test coverage (Jon Atack)
0f34f54 rpc: fix regression in gettransaction (Jon Atack)

Pull request description:

  Closes #16872.

  PR #16866 renamed the `decode` argument in gettransaction to `verbose` to make it more consistent with other RPC calls like getrawtransaction. However, it inadvertently overloaded the "details" field when `verbose` is passed. The result is that the original "details" field is no longer returned correctly, which seems to be a breaking API change.

  This PR:

  - takes the simplest path to restoring the "details" field by renaming the decoded one back to "decoded" while leaving the `verbose` argument for API consistency, which was the main intent of #16866,

  - addresses [this comment](bitcoin/bitcoin#16185 (comment)) by mentioning in the RPC help that the new decoded field is equivalent to decoderawtransaction, and

  - updates the help, functional test, and release note.

  Reviewers, to test this manually, build and run `bitcoin-cli help gettransaction` and `bitcoin-cli gettransaction <wallet txid> false true`, and verify that the command returns both `details` and `decoded` fields.

ACKs for top commit:
  jnewbery:
    tACK 1b41c2c

Tree-SHA512: 287edd5db7ed58fe8b548975aba58628bd45ed708b28f40174f10a35a455d89f796fbf27430aa881fc376f47aabda8803f74d4d100683bd86577a02279091cf3

Author: meshcollider

doc/release-notes-16185.md

@@ -1,3 +1,6 @@
 RPC changes
 -----------
-The `gettransaction` RPC now accepts a third (boolean) argument `verbose`. If set to `true`, a new `details` field will be added to the response containing additional transaction details.
+The `gettransaction` RPC now accepts a third (boolean) argument `verbose`. If
+set to `true`, a new `decoded` field will be added to the response containing
+the decoded transaction. This field is equivalent to RPC `decoderawtransaction`,
+or RPC `getrawtransaction` when `verbose` is passed.

src/wallet/rpcwallet.cpp

@@ -1648,8 +1648,10 @@ static UniValue gettransaction(const JSONRPCRequest& request)
                 "\nGet detailed information about in-wallet transaction <txid>\n",
                 {
                     {"txid", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction id"},
-                    {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Whether to include watch-only addresses in balance calculation and details[]"},
-                    {"verbose", RPCArg::Type::BOOL, /* default */ "false", "Whether to add a field with additional transaction details"},
+                    {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false",
+                            "Whether to include watch-only addresses in balance calculation and details[]"},
+                    {"verbose", RPCArg::Type::BOOL, /* default */ "false",
+                            "Whether to include a `decoded` field containing the decoded transaction (equivalent to RPC decoderawtransaction)"},
                 },
                 RPCResult{
             "{\n"
@@ -1685,7 +1687,8 @@ static UniValue gettransaction(const JSONRPCRequest& request)
             "    ,...\n"
             "  ],\n"
             "  \"hex\" : \"data\"         (string) Raw data for transaction\n"
-            "  \"details\" : transaction         (json object) Optional, additional transaction details. This object contains the same transaction details as the `getrawtransaction` RPC method\n"
+            "  \"decoded\" : transaction         (json object) Optional, the decoded transaction (only present when `verbose` is passed), equivalent to the\n"
+            "                                                RPC decoderawtransaction method, or the RPC getrawtransaction method when `verbose` is passed.\n"
             "}\n"
                 },
                 RPCExamples{
@@ -1739,9 +1742,9 @@ static UniValue gettransaction(const JSONRPCRequest& request)
     entry.pushKV("hex", strHex);
 
     if (verbose) {
-        UniValue details(UniValue::VOBJ);
-        TxToUniv(*wtx.tx, uint256(), details, false);
-        entry.pushKV("details", details);
+        UniValue decoded(UniValue::VOBJ);
+        TxToUniv(*wtx.tx, uint256(), decoded, false);
+        entry.pushKV("decoded", decoded);
     }
 
     return entry;

test/functional/wallet_basic.py

@@ -499,10 +499,35 @@ def run_test(self):
         self.nodes[0].setlabel(change, 'foobar')
         assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], False)
 
-        # Test "verbose" field value in gettransaction response
-        self.log.info("Testing verbose gettransaction...")
+        # Test gettransaction response with different arguments.
+        self.log.info("Testing gettransaction response with different arguments...")
+        self.nodes[0].setlabel(change, 'baz')
+        baz = self.nodes[0].listtransactions(label="baz", count=1)[0]
+        expected_receive_vout = {"label":    "baz",
+                                 "address":  baz["address"],
+                                 "amount":   baz["amount"],
+                                 "category": baz["category"],
+                                 "vout":     baz["vout"]}
+        expected_fields = frozenset({'amount', 'bip125-replaceable', 'confirmations', 'details', 'fee',
+                                     'hex', 'time', 'timereceived', 'trusted', 'txid', 'walletconflicts'})
+        verbose_field = "decoded"
+        expected_verbose_fields = expected_fields | {verbose_field}
+
+        self.log.debug("Testing gettransaction response without verbose")
+        tx = self.nodes[0].gettransaction(txid=txid)
+        assert_equal(set([*tx]), expected_fields)
+        assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
+
+        self.log.debug("Testing gettransaction response with verbose set to False")
+        tx = self.nodes[0].gettransaction(txid=txid, verbose=False)
+        assert_equal(set([*tx]), expected_fields)
+        assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
+
+        self.log.debug("Testing gettransaction response with verbose set to True")
         tx = self.nodes[0].gettransaction(txid=txid, verbose=True)
-        assert_equal(tx["details"], self.nodes[0].decoderawtransaction(tx["hex"]))
+        assert_equal(set([*tx]), expected_verbose_fields)
+        assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
+        assert_equal(tx[verbose_field], self.nodes[0].decoderawtransaction(tx["hex"]))
 
 
 if __name__ == '__main__':