From 0f34f54888f680bfbe7a29ac278636d7178a99bb Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sat, 14 Sep 2019 19:48:38 +0200 Subject: [PATCH 1/2] rpc: fix regression in gettransaction PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction. However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change. This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency. It also addresses [this comment](https://github.com/bitcoin/bitcoin/pull/16185#discussion_r320740413) to mention that the 'decoded' field is identical to decoderawtransaction. Update the RPC help, functional test, and release note. --- doc/release-notes-16185.md | 5 ++++- src/wallet/rpcwallet.cpp | 15 +++++++++------ test/functional/wallet_basic.py | 4 ++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/doc/release-notes-16185.md b/doc/release-notes-16185.md index 14e4004f7..2567ebea4 100644 --- a/doc/release-notes-16185.md +++ b/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. diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 9952e868f..216205ed6 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1648,8 +1648,10 @@ static UniValue gettransaction(const JSONRPCRequest& request) "\nGet detailed information about in-wallet transaction \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; diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 4c3fe3078..40ad9081a 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -499,10 +499,10 @@ class WalletTest(BitcoinTestFramework): self.nodes[0].setlabel(change, 'foobar') assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], False) - # Test "verbose" field value in gettransaction response + # Test "decoded" field value in gettransaction `verbose` response. self.log.info("Testing verbose gettransaction...") tx = self.nodes[0].gettransaction(txid=txid, verbose=True) - assert_equal(tx["details"], self.nodes[0].decoderawtransaction(tx["hex"])) + assert_equal(tx["decoded"], self.nodes[0].decoderawtransaction(tx["hex"])) if __name__ == '__main__': From 1b41c2c8a126ef4be183e1d800a17d85cab8837b Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sun, 15 Sep 2019 13:27:15 +0200 Subject: [PATCH 2/2] test: improve gettransaction test coverage - Test gettransaction response without verbose, with verbose=False, and with verbose=True. - In each case, test presence of expected fields in the output, including absence of the "decoded" field when `verbose` is not passed or false. - Test that the "details" field contains the expected receive vout in each case. --- test/functional/wallet_basic.py | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 40ad9081a..ce0411024 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -499,10 +499,35 @@ class WalletTest(BitcoinTestFramework): self.nodes[0].setlabel(change, 'foobar') assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], False) - # Test "decoded" field value in gettransaction `verbose` 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["decoded"], 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__':