gettransaction: add an argument to decode the transaction #16185

pull darosior wants to merge 3 commits into bitcoin:master from darosior:decode_tx_gettransaction changing 4 files +21 −1
  1. darosior commented at 10:27 PM on June 10, 2019: member

    This PR adds a new parameter to the gettransaction call : decode. If set to true, it will add a new decoded field to the response. This mimics the behavior of getrawtransaction's verbose argument to avoid using 2 calls if we want to decode a wallet transaction (gettransaction then decoderawtransaction).

    Fix #16181 .

  2. fanquake added the label RPC/REST/ZMQ on Jun 10, 2019
  3. fanquake added the label Wallet on Jun 10, 2019
  4. in src/wallet/rpcwallet.cpp:1731 in 7894189c8c outdated 
    1727 | @@ -1725,10 +1728,15 @@ static UniValue gettransaction(const JSONRPCRequest& request)
1728 |      uint256 hash(ParseHashV(request.params[0], "txid"));
1729 |  
1730 |      isminefilter filter = ISMINE_SPENDABLE;
1731 | -    if(!request.params[1].isNull())
1732 | +    if (!request.params[1].isNull())
    promag commented at 10:35 PM on June 10, 2019:

    Unrelated change, remove?

    darosior commented at 10:41 PM on June 10, 2019:

    Thought it would be great to fix this nit while passing through, will remove this change

  5. in src/wallet/rpcwallet.cpp:1737 in 7894189c8c outdated 
    1733 |          if(request.params[1].get_bool())
1734 |              filter = filter | ISMINE_WATCH_ONLY;
1735 |  
1736 | +    bool decode_tx = false;
1737 | +    if (!request.params[2].isNull())
1738 | +        if (request.params[2].get_bool())
    promag commented at 10:36 PM on June 10, 2019:

    decode_tx = request.params[2].get_bool()

  6. promag commented at 10:37 PM on June 10, 2019: member

    Concept ACK, please include tests and a release note.

  7. in src/wallet/rpcwallet.cpp:1674 in 7894189c8c outdated 
    1670 |              RPCHelpMan{"gettransaction",
1671 |                  "\nGet detailed information about in-wallet transaction <txid>\n",
1672 |                  {
1673 |                      {"txid", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction id"},
1674 |                      {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Whether to include watch-only addresses in balance calculation and details[]"},
1675 | +                    {"decode", RPCArg::Type::BOOL, /* default */ "true", "Whether to add a field with the decoded transaction"},
    promag commented at 10:38 PM on June 10, 2019:

    Default to false?

    darosior commented at 10:43 PM on June 10, 2019:

    Yes, corrected.

  8. darosior force-pushed on Jun 10, 2019
  9. darosior commented at 10:45 PM on June 10, 2019: member

    @promag thank you for the review. I added the changes and will include the tests and release note soon.

  10. DrahtBot commented at 10:52 PM on June 10, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
    • #16199 (rpc: fix showing wrong amount when not all inputs are from me in gettransaction by shannon1916)
    • #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

    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.

  11. darosior commented at 10:33 PM on June 12, 2019: member

    Concept ACK, please include tests and a release note.

    Since there was no functional test for the gettransaction I created a new one. However it only tests the decoded field since I don't know if testing other would not be overkill.

  12. in test/functional/rpc_gettransaction.py:1 in a21f811038 outdated 
       0 | @@ -0,0 +1,31 @@
   1 | +#!/usr/bin/env python3
    promag commented at 10:34 PM on June 12, 2019:

    Maybe add the test in test/functional/wallet_basic.py instead?

    darosior commented at 10:41 PM on June 12, 2019:

    Ok.

  13. in test/functional/rpc_gettransaction.py:26 in a21f811038 outdated 
      21 | +    def test_gettransaction_decoding(self):
  22 | +        self.log.info("Testing gettransaction decoding...")
  23 | +        self.nodes[0].generate(101)
  24 | +        txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
  25 | +        # False for 'include_watchonly', True for 'decode'
  26 | +        tx = self.nodes[0].gettransaction(txid, False, True)
    promag commented at 10:36 PM on June 12, 2019:

    You can use named arguments and drop the comment above (untested):

    tx = self.nodes[0].gettransaction(txid=txid, decode=True)
    darosior commented at 10:38 PM on June 12, 2019:

    Yes, tested it it works thanks :D

  14. darosior force-pushed on Jun 12, 2019
  15. darosior force-pushed on Jun 12, 2019
  16. darosior commented at 10:46 PM on June 12, 2019: member

    Moved the test to test/functional/wallet_basic.py and used named arguments.

  17. in test/functional/test_runner.py:201 in 9759da2504 outdated 
     197 | @@ -198,6 +198,7 @@
 198 |      'feature_blocksdir.py',
 199 |      'feature_config_args.py',
 200 |      'rpc_help.py',
 201 | +    'rpc_gettransaction.py',
    fanquake commented at 7:53 AM on June 13, 2019:

    Please remove this entry now that the test is in wallet_basic.py.

  18. darosior force-pushed on Jun 13, 2019
  19. DrahtBot added the label Needs rebase on Jul 9, 2019
  20. darosior force-pushed on Jul 10, 2019
  21. darosior force-pushed on Jul 10, 2019
  22. DrahtBot removed the label Needs rebase on Jul 10, 2019
  23. darosior force-pushed on Jul 10, 2019
  24. darosior force-pushed on Jul 10, 2019
  25. darosior commented at 9:27 PM on July 10, 2019: member

    Rebased.

  26. in src/wallet/rpcwallet.cpp:1674 in 91f15c8740 outdated 
    1670 | @@ -1670,11 +1671,13 @@ static UniValue gettransaction(const JSONRPCRequest& request)
1671 |              "    ,...\n"
1672 |              "  ],\n"
1673 |              "  \"hex\" : \"data\"         (string) Raw data for transaction\n"
1674 | +            "  \"decoded\" : \"transaction\"         (json object) Optional, the decoded transaction\n"
    promag commented at 9:33 PM on July 10, 2019:

    Remove \"transaction\" as the value is not a string. Maybe mention that it's the same result as decoderawtransaction?

    darosior commented at 10:52 AM on July 18, 2019:

    done

  27. in src/wallet/rpcwallet.cpp:1699 in 91f15c8740 outdated 
    1695 | @@ -1693,6 +1696,10 @@ static UniValue gettransaction(const JSONRPCRequest& request)
1696 |          if(request.params[1].get_bool())
1697 |              filter = filter | ISMINE_WATCH_ONLY;
1698 |  
1699 | +    bool decode_tx = false;
    promag commented at 9:35 PM on July 10, 2019:

    nit, const bool decode = request.params[2].isNull() ? false : request.params[2].get_bool();

    darosior commented at 10:53 AM on July 18, 2019:

    done

  28. in src/wallet/rpcwallet.cpp:1700 in 91f15c8740 outdated 
    1695 | @@ -1693,6 +1696,10 @@ static UniValue gettransaction(const JSONRPCRequest& request)
1696 |          if(request.params[1].get_bool())
1697 |              filter = filter | ISMINE_WATCH_ONLY;
1698 |  
1699 | +    bool decode_tx = false;
1700 | +    if (!request.params[2].isNull())
    promag commented at 9:35 PM on July 10, 2019:

    nit, otherwise add { }.

  29. in test/functional/wallet_basic.py:503 in 91f15c8740 outdated 
     495 | @@ -496,6 +496,13 @@ def run_test(self):
 496 |          self.nodes[0].setlabel(change, 'foobar')
 497 |          assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], False)
 498 |  
 499 | +        # Test "decoded" field value in gettransaction response
 500 | +        self.log.info("Testing gettransaction decoding...")
    promag commented at 9:36 PM on July 10, 2019:

    nit, remove log.

    darosior commented at 5:14 PM on July 12, 2019:

    Is there a rationale for removing the log ? I added it to mimic the other tests' behavior

  30. in test/functional/wallet_basic.py:502 in 91f15c8740 outdated 
     495 | @@ -496,6 +496,13 @@ def run_test(self):
 496 |          self.nodes[0].setlabel(change, 'foobar')
 497 |          assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], False)
 498 |  
 499 | +        # Test "decoded" field value in gettransaction response
 500 | +        self.log.info("Testing gettransaction decoding...")
 501 | +        self.nodes[0].generate(1)
 502 | +        txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
    promag commented at 9:37 PM on July 10, 2019:

    nit, remove and use the last transaction below?

    darosior commented at 10:56 AM on July 18, 2019:

    done

  31. in test/functional/wallet_basic.py:505 in 91f15c8740 outdated 
     495 | @@ -496,6 +496,13 @@ def run_test(self):
 496 |          self.nodes[0].setlabel(change, 'foobar')
 497 |          assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], False)
 498 |  
 499 | +        # Test "decoded" field value in gettransaction response
 500 | +        self.log.info("Testing gettransaction decoding...")
 501 | +        self.nodes[0].generate(1)
 502 | +        txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
 503 | +        tx = self.nodes[0].gettransaction(txid=txid, decode=True)
 504 | +        assert_equal(tx["decoded"], self.nodes[0].decoderawtransaction(tx["hex"]))
    promag commented at 9:39 PM on July 10, 2019:

    Or you can simply change L488:

    -tx = self.nodes[0].decoderawtransaction(self.nodes[0].gettransaction(txid)['hex'])
+tx = self.nodes[0].gettransaction(txid, decode=True)['decoded']
    darosior commented at 10:58 AM on July 18, 2019:

    Probably better to leave it as a separated test

  32. darosior force-pushed on Jul 18, 2019
  33. darosior force-pushed on Jul 18, 2019
  34. darosior commented at 7:50 PM on July 18, 2019: member

    Some minor corrections according to last review.

  35. promag commented at 9:19 PM on July 18, 2019: member

    You could use https://github.com/bitcoin/bitcoin/commit/0b6ba66238c377116bc6c21e19cffbf1b6dfc788#diff-0f596a69ccfb4d07c007cd6fc993898dR50 (not yet in master) to do:

    assert_no_key('decoded', self.nodes[0].gettransaction(txid, decode=False))
  36. in test/functional/wallet_basic.py:499 in e4a46c74ba outdated 
     495 | @@ -496,6 +496,11 @@ def run_test(self):
 496 |          self.nodes[0].setlabel(change, 'foobar')
 497 |          assert_equal(self.nodes[0].getaddressinfo(change)['ischange'], False)
 498 |  
 499 | +        # Test "decoded" field value in gettransaction response
    jb55 commented at 11:34 AM on July 25, 2019:

    this is a weird nit, but just curious why there's a "Non-breaking space" character between the # and Test (utf-8 0xC2A0)

    git cat-file -p d5ab374c48a7f2708acc5e122044a3a9cfc1ac6a | grep '#.Test "decoded"' | xxd | head -n1
    darosior commented at 3:02 PM on July 31, 2019:

    I think it's caused by a vim module (deoplete-jedi on this one I think)

  37. in doc/release-notes.md:103 in 5f96ab3d4c outdated 
      98 | @@ -99,6 +99,10 @@ Low-level Changes section below.
  99 |    `-limitancestorcount`, `-limitdescendantcount` and `-walletrejectlongchains`
 100 |    command line arguments.
 101 |  
 102 | +* The `gettransaction` RPC now accepts a third (boolean) argument `decode`. If
 103 | +  set to `true`, a new `decoded` field will be added to the response containing
    jb55 commented at 11:41 AM on July 25, 2019:

    There's another one of those chars here after decoded and before field. Not a big deal, just was wondering what editor you are using that would be adding these randomly?

    git cat-file -p a2b23686c65d7e8c7e69b7547322d2eea1cf3b44 | xxd | grep c2a0
00000f90: 6465 6460 c2a0 6669 656c 6420 7769 6c6c  ded`..field will
    darosior commented at 3:00 PM on July 31, 2019:

    Vim, you right it's weird

  38. jb55 commented at 11:41 AM on July 25, 2019: member

    Concept ACK

  39. DrahtBot added the label Needs rebase on Jul 31, 2019
  40. darosior force-pushed on Aug 4, 2019
  41. darosior commented at 6:39 PM on August 4, 2019: member

    Rebased. (And addressed the spacing nits pointed by @jb55 :-) )

  42. DrahtBot removed the label Needs rebase on Aug 4, 2019
  43. in doc/release-notes-16185.md:3 in 5c28a1f92e outdated 
       0 | @@ -0,0 +1,3 @@
   1 | +RPC changes
   2 | +-----------
   3 | +The `gettransaction` RPC now accepts a third (boolean) argument `decode`. If set to `true`, a new `decoded` field will be added to the response containing
    achow101 commented at 3:00 PM on August 7, 2019:

    I think the sentence got cut off here.

  44. in src/wallet/rpcwallet.cpp:1734 in 5c28a1f92e outdated 
    1728 | @@ -1724,6 +1729,12 @@ static UniValue gettransaction(const JSONRPCRequest& request)
1729 |      std::string strHex = EncodeHexTx(*wtx.tx, pwallet->chain().rpcSerializationFlags());
1730 |      entry.pushKV("hex", strHex);
1731 |  
1732 | +    if (decode_tx) {
1733 | +        UniValue decoded(UniValue::VOBJ);
1734 | +        TxToUniv(*wtx.tx, uint256(), decoded, false, pwallet->chain().rpcSerializationFlags());
    achow101 commented at 3:03 PM on August 7, 2019:

    nit: It's not necessary to have pwallet->chain().rpcSerializationFlags().

  45. achow101 commented at 3:03 PM on August 7, 2019: member

    Concept ACK

  46. darosior force-pushed on Aug 7, 2019
  47. darosior commented at 4:31 PM on August 7, 2019: member

    @achow101 thank you for the review, rebased to rectify.

  48. achow101 commented at 4:02 PM on August 15, 2019: member

    ACK a636daba4f61c32ec83d6677b8cff66dbde3fda0 Reviewed code and tested locally

  49. DrahtBot added the label Needs rebase on Aug 16, 2019
  50. in src/wallet/rpcwallet.cpp:1688 in a636daba4f outdated 
    1676 | @@ -1676,11 +1677,13 @@ static UniValue gettransaction(const JSONRPCRequest& request)
1677 |              "    ,...\n"
1678 |              "  ],\n"
1679 |              "  \"hex\" : \"data\"         (string) Raw data for transaction\n"
1680 | +            "  \"decoded\" : transaction         (json object) Optional, the decoded transaction\n"
    luke-jr commented at 11:10 PM on August 19, 2019:

    This seems wanting... but expanding here would be ugly.

  51. luke-jr approved
  52. luke-jr commented at 11:11 PM on August 19, 2019: member

    utACK

  53. meshcollider commented at 1:46 AM on August 24, 2019: contributor

    utACK a636daba4f61c32ec83d6677b8cff66dbde3fda0

    Once rebased, I will merge this

  54. darosior force-pushed on Aug 29, 2019
  55. darosior force-pushed on Aug 29, 2019
  56. darosior commented at 2:47 PM on August 29, 2019: member

    Rebased.

  57. DrahtBot removed the label Needs rebase on Aug 29, 2019
  58. promag commented at 3:28 PM on August 29, 2019: member

    You must make linters happy.

  59. darosior commented at 8:46 AM on August 30, 2019: member

    You must make linters happy.

    No that's a Travis error : curl: (7) Failed to connect to storage.googleapis.com port 443: Connection timed out

  60. gettransaction: add an argument to decode the transaction
    This adds a new boolean parameter 'decode' to the gettransaction call, which, if set to true, add a 'decoded' field to the result containing the decoded transaction
    7f3bb247a8
  61. tests: Add a new functional test for gettransaction b8b3f0435a
  62. doc: Add release note for the new gettransaction argument 9965940e35
  63. darosior force-pushed on Aug 30, 2019
  64. meshcollider commented at 11:28 AM on September 2, 2019: contributor

    re-utACK 9965940e35c445ccded55510348af228ff22f0e9

  65. meshcollider referenced this in commit 33f9750b1b on Sep 2, 2019
  66. meshcollider merged this on Sep 2, 2019
  67. meshcollider closed this on Sep 2, 2019
  68. darosior deleted the branch on Sep 2, 2019
  69. sidhujag referenced this in commit 7fb1dcfa08 on Sep 3, 2019
  70. luke-jr referenced this in commit 953eb8bb15 on Sep 3, 2019
  71. luke-jr referenced this in commit 587a030644 on Sep 3, 2019
  72. in src/wallet/rpcwallet.cpp:1688 in 9965940e35 
    1684 | @@ -1684,11 +1685,13 @@ static UniValue gettransaction(const JSONRPCRequest& request)
1685 |              "    ,...\n"
1686 |              "  ],\n"
1687 |              "  \"hex\" : \"data\"         (string) Raw data for transaction\n"
1688 | +            "  \"decoded\" : transaction         (json object) Optional, the decoded transaction\n"
    MarcoFalke commented at 12:51 PM on September 4, 2019:

    Should mention that the field is identical to decoderawtransaction?

  73. jnewbery commented at 10:35 AM on September 10, 2019: member

    Sorry for the post-merge review comment, but would it make sense to call the new parameter verbose, to mirror the verbose argument in getrawtransaction?

    If so, we should make that change before v0.19 to avoid an API change.

  74. ryanofsky commented at 7:53 PM on September 11, 2019: member

    Sorry for the post-merge review comment, but would it make sense to call the new parameter verbose, to mirror the verbose argument in getrawtransaction?

    If so, we should make that change before v0.19 to avoid an API change.

    This seems like a good idea. You could open a one-line PR and add the milestone to get more feedback.

  75. promag commented at 6:51 AM on September 12, 2019: member

    +1 for API consistency.

  76. jonatack referenced this in commit 0f34f54888 on Sep 14, 2019
  77. meshcollider referenced this in commit b0a7a76c9d on Sep 15, 2019
  78. sidhujag referenced this in commit e87378392c on Sep 16, 2019
  79. luke-jr referenced this in commit a7eebbb21e on Sep 19, 2019
  80. luke-jr referenced this in commit a7ada583fa on Sep 19, 2019
  81. luke-jr referenced this in commit da9c4aca90 on Sep 21, 2019
  82. PierreRochard referenced this in commit 484d10248b on Oct 12, 2019
  83. barrystyle referenced this in commit 6657d592e8 on Nov 11, 2019
  84. HashUnlimited referenced this in commit 52fcbfe962 on Nov 17, 2019
  85. vansergen referenced this in commit 21ab707301 on Mar 26, 2020
  86. jasonbcox referenced this in commit 28501c5d74 on Sep 5, 2020
  87. jasonbcox referenced this in commit 627e1c4ee9 on Sep 5, 2020
  88. jasonbcox referenced this in commit 5c09cc3bad on Sep 5, 2020
  89. DrahtBot locked this on Dec 16, 2021
Contributors
darosiorpromagDrahtBotfanquakejb55achow101luke-jrmeshcolliderMarcoFalkejnewberyryanofsky
Labels
WalletRPC/REST/ZMQ
Linked (view graph)
#16181 wallet: decode raw transaction in gettransaction#16873 rpc: fix regression in gettransaction
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me