rpc: make `address` field optional `list{transactions, sinceblock}` response #26349

pull w0xlt wants to merge 1 commits into bitcoin:master from w0xlt:issue_26338 changing 3 files +28 −2
  1. w0xlt commented at 4:08 AM on October 20, 2022: contributor

    Close #26338.

    This PR makes optional the address field in the response of listtransactions and listsinceblock RPC. And adds two tests that fail on master, but not on this branch.

  2. fanquake added this to the milestone 24.0 on Oct 20, 2022
  3. DrahtBot added the label RPC/REST/ZMQ on Oct 20, 2022
  4. w0xlt force-pushed on Oct 20, 2022
  5. maflcko added the label Needs backport (24.x) on Oct 20, 2022
  6. in test/functional/wallet_listtransactions.py:297 in 1a00465726 outdated 
     291 | @@ -284,6 +292,22 @@ def run_invalid_parameters_test(self):
 292 |          assert_raises_rpc_error(-8, "Negative count", self.nodes[0].listtransactions, count=-1)
 293 |          assert_raises_rpc_error(-8, "Negative from", self.nodes[0].listtransactions, skip=-1)
 294 |  
 295 | +    def test_op_return(self):
 296 | +
 297 | +        data = random_bytes(8)
    maflcko commented at 6:16 AM on October 20, 2022:

    Does it need to be random?

  7. in test/functional/wallet_listtransactions.py:300 in 1a00465726 outdated 
     291 | @@ -284,6 +292,22 @@ def run_invalid_parameters_test(self):
 292 |          assert_raises_rpc_error(-8, "Negative count", self.nodes[0].listtransactions, count=-1)
 293 |          assert_raises_rpc_error(-8, "Negative from", self.nodes[0].listtransactions, skip=-1)
 294 |  
 295 | +    def test_op_return(self):
 296 | +
 297 | +        data = random_bytes(8)
 298 | +
 299 | +        tx = CTransaction()
 300 | +        tx.vout = [CTxOut(nValue=2, scriptPubKey=CScript([OP_RETURN, data]))]
    maflcko commented at 6:17 AM on October 20, 2022:

    would be shorter to just call createrawtransaction '[]' '[{"data":"aa"}]'?

    w0xlt commented at 2:57 PM on October 20, 2022:

    Done in Done in https://github.com/bitcoin/bitcoin/pull/26349/commits/670395cfa65988ab09afd59854d0d64c0c1438ba. Thanks.

  8. maflcko approved
  9. maflcko commented at 6:18 AM on October 20, 2022: member

    Thanks, Concept ACK

  10. DrahtBot commented at 9:28 AM on October 20, 2022: contributor

    <!--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:

    • #25934 (wallet, rpc: add label to listsinceblock by brunoerg)

    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. hebasto commented at 12:05 PM on October 20, 2022: member

    Concept ACK.

  12. w0xlt force-pushed on Oct 20, 2022
  13. w0xlt force-pushed on Oct 20, 2022
  14. in src/wallet/rpc/transactions.cpp:564 in 4b48838133 outdated 
     560 | @@ -561,7 +561,7 @@ RPCHelpMan listsinceblock()
 561 |                              {RPCResult::Type::OBJ, "", "", Cat(Cat<std::vector<RPCResult>>(
 562 |                              {
 563 |                                  {RPCResult::Type::BOOL, "involvesWatchonly", /*optional=*/true, "Only returns true if imported addresses were involved in transaction."},
 564 | -                                {RPCResult::Type::STR, "address", "The bitcoin address of the transaction."},
 565 | +                                {RPCResult::Type::STR, "address",  /*optional=*/true, "The bitcoin address of the transaction."},
    jonatack commented at 3:38 PM on October 20, 2022:

    It might be helpful to describe in these two changed helps why (in what case) the address wouldn't be returned.

    w0xlt commented at 1:54 PM on October 21, 2022:

    Done in 078199c245cc475bb594930177fd505d84fede4b.

    jonatack commented at 4:38 PM on October 21, 2022:

    Thanks for updating! The spelling linter doesn't like "ommited"

    src/wallet/rpc/transactions.cpp:450: ommited ==> omitted
src/wallet/rpc/transactions.cpp:564: ommited ==> omitted

    Maybe something like:

    "The bitcoin address of the transaction (not returned if the transaction contains OP_RETURN null data)."

    w0xlt commented at 9:36 PM on October 21, 2022:

    Done in https://github.com/bitcoin/bitcoin/pull/26349/commits/b725cbea13b0f4774abbdf939b3ffe224fc63696. Thanks for the suggestion.

  15. in test/functional/wallet_listsinceblock.py:463 in 4b48838133 outdated 
     458 | +        signed_tx = self.nodes[2].signrawtransactionwithwallet(funded_tx['hex'])
 459 | +        tx_id = self.nodes[2].sendrawtransaction(signed_tx['hex'])
 460 | +
 461 | +        op_ret_tx = [tx for tx in self.nodes[2].listsinceblock(blockhash=block_hash)["transactions"] if tx['txid'] == tx_id][0]
 462 | +
 463 | +        assert('address' not in op_ret_tx)
    jonatack commented at 3:39 PM on October 20, 2022:

    In your diff in both test files (see #26257):

            assert 'address' not in op_ret_tx

    This avoids the python linter raising locally and needing to update #26257

    test/functional/wallet_listsinceblock.py:463:15: E275 missing whitespace after keyword
test/functional/wallet_listtransactions.py:297:15: E275 missing whitespace after keyword
    w0xlt commented at 1:55 PM on October 21, 2022:

    Done in 078199c245cc475bb594930177fd505d84fede4b.

  16. in test/functional/wallet_listtransactions.py:290 in 4b48838133 outdated 
     284 | @@ -284,6 +285,17 @@ def run_invalid_parameters_test(self):
 285 |          assert_raises_rpc_error(-8, "Negative count", self.nodes[0].listtransactions, count=-1)
 286 |          assert_raises_rpc_error(-8, "Negative from", self.nodes[0].listtransactions, skip=-1)
 287 |  
 288 | +    def test_op_return(self):
 289 | +        """Test if OP_RETURN outputs will be displayed correctly."""
 290 | +        raw_tx = self.nodes[0].createrawtransaction([],[{'data':'aa'}])
    jonatack commented at 4:29 PM on October 20, 2022:

    In your diff in both test files, if you retouch

            raw_tx = self.nodes[0].createrawtransaction([], [{'data': 'aa'}])
    w0xlt commented at 1:55 PM on October 21, 2022:

    Done in 078199c245cc475bb594930177fd505d84fede4b. Thanks.

  17. jonatack commented at 4:55 PM on October 20, 2022: contributor

    Approach ACK

  18. w0xlt force-pushed on Oct 21, 2022
  19. jonatack commented at 4:39 PM on October 21, 2022: contributor

    ACK modulo two comments

  20. w0xlt force-pushed on Oct 21, 2022
  21. rpc: make `address` field optional eb679a7896
  22. in src/wallet/rpc/transactions.cpp:450 in b725cbea13 outdated 
     446 | @@ -447,7 +447,7 @@ RPCHelpMan listtransactions()
 447 |                          {RPCResult::Type::OBJ, "", "", Cat(Cat<std::vector<RPCResult>>(
 448 |                          {
 449 |                              {RPCResult::Type::BOOL, "involvesWatchonly", /*optional=*/true, "Only returns true if imported addresses were involved in transaction."},
 450 | -                            {RPCResult::Type::STR, "address", "The bitcoin address of the transaction."},
 451 | +                            {RPCResult::Type::STR, "address",  /*optional=*/true, "The bitcoin address of the transaction (not returned if the transaction contains OP_RETURN null data)."},
    maflcko commented at 7:09 AM on October 22, 2022:

    I don't think this is true. A scriptpubkey can be nonstandard, but that doesn't imply it has an OP_RETURN

    maflcko commented at 7:10 AM on October 22, 2022:

    Also (unrelated): A transaction does not have an address. At most it is the address of the output

    w0xlt commented at 6:51 AM on October 23, 2022:

    Is the following message better ? not returned if the output address cannot be decoded (such as OP_RETURN null data).

    achow101 commented at 8:07 PM on October 24, 2022:

    I think "does not have an address" is more correct. "cannot be decoded" implies that there is a correct way to encode addresses in a transaction, but that's not correct as addresses are not encoded into transactions, they are turned into scripts.

  23. w0xlt force-pushed on Oct 26, 2022
  24. aureleoules approved
  25. aureleoules commented at 1:06 PM on October 27, 2022: member

    ACK eb679a7896ce00e322972a011b023661766923b9

    I was able to verify that this PR fixes the original issue by reproducing the crash on master+mainnet (with --enable-debug) using the following steps:

    $ ./src/bitcoind -blockfilterindex
$ ./src/bitcoin-cli -named createwallet wallet_name=26349 disable_private_keys=true
$ ./src/bitcoin-cli -rpcwallet=26349 importdescriptors '[{"desc": "raw(6a20a015693871d8bde65f57ec82f52f6b192e9a11fa26a37e63e17dc3092c6c7fab)#jgvt3p6e", "timestamp": 1516693300}]'
$ ./src/bitcoin-cli -rpcwallet=26349 listtransactions '*' 500
error code: -1
error message:
Internal bug detected: "std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [&ret](const RPCResult& res) { return res.MatchesType(ret); })"
rpc/util.cpp:587 (HandleRequest)
Please report this issue here: https://github.com/bitcoin/bitcoin/issues

    Then on this PR:

    $ ./src/bitcoin-cli loadwallet 26349
$ ./src/bitcoin-cli -rpcwallet=26349 listtransactions '*' 500
[
  {
    "parent_descs": [
      "raw(6a20a015693871d8bde65f57ec82f52f6b192e9a11fa26a37e63e17dc3092c6c7fab)#jgvt3p6e"
    ],
    "category": "receive",
    "amount": 0.00000000,
    "vout": 1,
    "confirmations": 254851,
    "blockhash": "00000000000000000025a92c80f5c259ca5d9d36d407906dbefbd8075c3ae77b",
    "blockheight": 505672,
    "blockindex": 857,
    "blocktime": 1516693344,
    "txid": "b6439e1c9eb3915b3cc89871d2c2479f3f1847f0c7bab252c3ebc503b8f6d344",
    "wtxid": "393cfba86cb0d3274850aa461dab433b5d62c1dce68dc10d43de969b8dec2359",
    "walletconflicts": [
    ],
    "time": 1516693344,
    "timereceived": 1666874521,
    "bip125-replaceable": "no"
  }
]
  26. achow101 commented at 5:08 PM on October 27, 2022: member

    ACK eb679a7896ce00e322972a011b023661766923b9

  27. achow101 referenced this in commit 551c8e9526 on Oct 27, 2022
  28. achow101 commented at 6:32 PM on October 27, 2022: member

    For some reason github is not detecting that this was merged.

  29. achow101 closed this on Oct 27, 2022
  30. w0xlt deleted the branch on Oct 27, 2022
  31. jonatack commented at 11:19 PM on October 27, 2022: contributor

    Post-merge ACK

  32. fanquake referenced this in commit d5701900fc on Oct 28, 2022
  33. fanquake removed the label Needs backport (24.x) on Oct 28, 2022
  34. fanquake commented at 10:10 AM on October 28, 2022: member

    Backported to 24.x in #26410.

  35. sidhujag referenced this in commit 705fbe4b99 on Oct 28, 2022
  36. fanquake referenced this in commit 2e8880abc0 on Oct 31, 2022
  37. bitcoin locked this on Oct 28, 2023
Contributors
w0xltmaflckoDrahtBothebastojonatackachow101aureleoulesfanquake
Labels
RPC/REST/ZMQ

Milestone
24.0

Linked (view graph)
#26338 doc: Internal bug detected running listtransactions on OP_RETURN output#26410 [24.x] rc3 backports
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:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me