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

w0xlt commented at 4:08 am on October 20, 2022: contributor

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.

fanquake added this to the milestone 24.0 on Oct 20, 2022

DrahtBot added the label RPC/REST/ZMQ on Oct 20, 2022

w0xlt force-pushed on Oct 20, 2022

maflcko added the label Needs backport (24.x) on Oct 20, 2022

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?

w0xlt commented at 2:56 pm on October 20, 2022:

Removed it in https://github.com/bitcoin/bitcoin/pull/26349/commits/670395cfa65988ab09afd59854d0d64c0c1438ba.

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.

maflcko approved

maflcko commented at 6:18 am on October 20, 2022: member

Thanks, Concept ACK

DrahtBot commented at 9:28 am on October 20, 2022: contributor

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

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.

hebasto commented at 12:05 pm on October 20, 2022: member

Concept ACK.

w0xlt force-pushed on Oct 20, 2022

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”

0src/wallet/rpc/transactions.cpp:450: ommited ==> omitted
1src/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.

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

0        assert 'address' not in op_ret_tx

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

0test/functional/wallet_listsinceblock.py:463:15: E275 missing whitespace after keyword
1test/functional/wallet_listtransactions.py:297:15: E275 missing whitespace after keyword

w0xlt commented at 1:55 pm on October 21, 2022:

Done in 078199c245cc475bb594930177fd505d84fede4b.

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

0        raw_tx = self.nodes[0].createrawtransaction([], [{'data': 'aa'}])

w0xlt commented at 1:55 pm on October 21, 2022:

Done in 078199c245cc475bb594930177fd505d84fede4b. Thanks.

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

Also in https://github.com/bitcoin/bitcoin/pull/26349/files#diff-33258976a327dfd9623df9379f0fd15e15c32a1488d658912e3973d2e0bf7a74R456 if you retouch (thanks!)

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

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

jonatack commented at 4:55 pm on October 20, 2022: contributor

Approach ACK

w0xlt force-pushed on Oct 21, 2022

jonatack commented at 4:39 pm on October 21, 2022: contributor

ACK modulo two comments

w0xlt force-pushed on Oct 21, 2022

rpc: make `address` field optional eb679a7896

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.

w0xlt commented at 4:20 am on October 26, 2022:

Done in https://github.com/bitcoin/bitcoin/pull/26349/commits/eb679a7896ce00e322972a011b023661766923b9.

w0xlt force-pushed on Oct 26, 2022

aureleoules approved

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:

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

Then on this PR:

 0$ ./src/bitcoin-cli loadwallet 26349
 1$ ./src/bitcoin-cli -rpcwallet=26349 listtransactions '*' 500
 2[
 3  {
 4    "parent_descs": [
 5      "raw(6a20a015693871d8bde65f57ec82f52f6b192e9a11fa26a37e63e17dc3092c6c7fab)#jgvt3p6e"
 6    ],
 7    "category": "receive",
 8    "amount": 0.00000000,
 9    "vout": 1,
10    "confirmations": 254851,
11    "blockhash": "00000000000000000025a92c80f5c259ca5d9d36d407906dbefbd8075c3ae77b",
12    "blockheight": 505672,
13    "blockindex": 857,
14    "blocktime": 1516693344,
15    "txid": "b6439e1c9eb3915b3cc89871d2c2479f3f1847f0c7bab252c3ebc503b8f6d344",
16    "wtxid": "393cfba86cb0d3274850aa461dab433b5d62c1dce68dc10d43de969b8dec2359",
17    "walletconflicts": [
18    ],
19    "time": 1516693344,
20    "timereceived": 1666874521,
21    "bip125-replaceable": "no"
22  }
23]

achow101 commented at 5:08 pm on October 27, 2022: member

ACK eb679a7896ce00e322972a011b023661766923b9

achow101 referenced this in commit 551c8e9526 on Oct 27, 2022

achow101 commented at 6:32 pm on October 27, 2022: member

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

achow101 closed this on Oct 27, 2022

w0xlt deleted the branch on Oct 27, 2022

jonatack commented at 11:19 pm on October 27, 2022: contributor

Post-merge ACK

fanquake referenced this in commit d5701900fc on Oct 28, 2022

fanquake removed the label Needs backport (24.x) on Oct 28, 2022

fanquake commented at 10:10 am on October 28, 2022: member

Backported to 24.x in #26410.

sidhujag referenced this in commit 705fbe4b99 on Oct 28, 2022

fanquake referenced this in commit 2e8880abc0 on Oct 31, 2022

bitcoin locked this on Oct 28, 2023

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

Conflicts

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