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

w0xlt · 2022-10-20T04:08:36Z

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.

maflcko

Thanks, Concept ACK

maflcko · 2022-10-20T06:16:46Z

test/functional/wallet_listtransactions.py

Does it need to be random?

Removed it in 670395c.

maflcko · 2022-10-20T06:17:57Z

test/functional/wallet_listtransactions.py

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

Done in Done in 670395c. Thanks.

DrahtBot · 2022-10-20T09:28:09Z

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 · 2022-10-20T12:05:20Z

Concept ACK.

jonatack

Approach ACK

jonatack · 2022-10-20T15:38:37Z

src/wallet/rpc/transactions.cpp

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

Done in 078199c245cc475bb594930177fd505d84fede4b.

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)."

Done in b725cbe. Thanks for the suggestion.

jonatack · 2022-10-20T15:39:06Z

test/functional/wallet_listsinceblock.py

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

Suggested change

assert('address' not in op_ret_tx)

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

Done in 078199c245cc475bb594930177fd505d84fede4b.

jonatack · 2022-10-20T16:29:45Z

test/functional/wallet_listtransactions.py

In your diff in both test files, if you retouch

Suggested change

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

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

Done in 078199c245cc475bb594930177fd505d84fede4b. Thanks.

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

Done in b725cbe. Thanks.

jonatack · 2022-10-21T16:39:37Z

ACK modulo two comments

maflcko · 2022-10-22T07:09:58Z

src/wallet/rpc/transactions.cpp

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

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

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

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.

Done in eb679a7.

aureleoules

ACK eb679a7

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"
  }
]

achow101 · 2022-10-27T17:08:01Z

ACK eb679a7

…{transactions, sinceblock}` response eb679a7 rpc: make `address` field optional (w0xlt) Pull request description: Close bitcoin/bitcoin#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. ACKs for top commit: achow101: ACK eb679a7 aureleoules: ACK eb679a7 Tree-SHA512: b267439626e2ec3134ae790c849949a4c40ef0cebd20092e8187be3db0a61941b2da10bbbba92ca880b8369f46c1aaa806d057eaa5159325f65cbec7cb33c52f

achow101 · 2022-10-27T18:32:16Z

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

jonatack · 2022-10-27T23:19:02Z

Post-merge ACK

Github-Pull: bitcoin#26349 Rebased-From: eb679a7

fanquake · 2022-10-28T10:10:41Z

Backported to 24.x in #26410.

…ons, …

d570190 rpc: make `address` field optional (w0xlt) e4b8c9b rpc: add non-regression test about deriveaddresses crash when index is 2147483647 (muxator) bf2bf73 rpc: fix crash in deriveaddresses when derivation index is 2147483647 (muxator) b04f5f9 test: Test for out of bounds vout in sendall (Andrew Chow) dedee6a wallet: Check utxo prevout index out of bounds in sendall (Andrew Chow) 931db78 test: Test that sendall works with watchonly spending specific utxos (Andrew Chow) bbe864a wallet: Correctly check ismine for sendall (Andrew Chow) 4b7d30d Adjust `.tx/config` for new Transifex CLI (Hennadii Stepanov) Pull request description: Backports: * #26321 * #26344 * #26275 * #26349 ACKs for top commit: instagibbs: ACK d570190 hebasto: ACK d570190, I've cherry-picked commits manually and got zero diff with this PR branch. Tree-SHA512: dad64f4074b4f06d666c0f2d804eda92df241bcce0a49c28486311a151f2e9d46b75e1bce02de570dcc85957c9ce936debb2a4faa045800c9757c6c495115d7c

fanquake added this to the 24.0 milestone Oct 20, 2022

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

w0xlt force-pushed the issue_26338 branch from 13b938a to 1a00465 Compare October 20, 2022 04:35

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

maflcko approved these changes Oct 20, 2022

View reviewed changes

DrahtBot mentioned this pull request Oct 20, 2022

wallet, rpc: add label to listsinceblock #25934

Merged

w0xlt force-pushed the issue_26338 branch 2 times, most recently from 670395c to 4b48838 Compare October 20, 2022 14:59

jonatack reviewed Oct 20, 2022

View reviewed changes

w0xlt force-pushed the issue_26338 branch from 4b48838 to 078199c Compare October 21, 2022 13:54

w0xlt force-pushed the issue_26338 branch from 078199c to b725cbe Compare October 21, 2022 21:31

maflcko reviewed Oct 22, 2022

View reviewed changes

rpc: make address field optional

eb679a7

w0xlt force-pushed the issue_26338 branch from b725cbe to eb679a7 Compare October 26, 2022 04:18

aureleoules approved these changes Oct 27, 2022

View reviewed changes

achow101 closed this Oct 27, 2022

w0xlt deleted the issue_26338 branch October 27, 2022 21:04

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 28, 2022

rpc: make address field optional

d570190

Github-Pull: bitcoin#26349 Rebased-From: eb679a7

fanquake mentioned this pull request Oct 28, 2022

[24.x] rc3 backports #26410

Merged

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

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 28, 2022

Merge bitcoin#26349: rpc: make address field optional `list{transacti…

705fbe4

…ons, …

bitcoin locked and limited conversation to collaborators Oct 28, 2023

	assert('address' not in op_ret_tx)
	assert 'address' not in op_ret_tx

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

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

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

Uh oh!

Conversation

w0xlt commented Oct 20, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Oct 20, 2022

Conflicts

Uh oh!

hebasto commented Oct 20, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Oct 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aureleoules left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 27, 2022

Uh oh!

achow101 commented Oct 27, 2022

Uh oh!

jonatack commented Oct 27, 2022

Uh oh!

fanquake commented Oct 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

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

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

w0xlt Oct 21, 2022 •

edited

Loading

aureleoules left a comment •

edited

Loading