[RPC] Include coinbase transactions in receivedby RPCs by andrewtoth · Pull Request #14707 · bitcoin/bitcoin

andrewtoth · 2018-11-12T02:20:32Z

The current *receivedby* RPCs filter out coinbase transactions. This doesn't seem correct since an output to your address in a coinbase transaction is receiving those coins.

This PR corrects this behaviour. Also, a new option include_immature_coinbase is added (default=false) that includes immature coinbase transactions when set to true.

However, since this is potentially a breaking change this PR introduces a hidden configuration option -deprecatedrpc=exclude_coinbase. This can be set to revert to previous behaviour. If no reports of broken workflow are received, then this option can be removed in a future release.

Fixes #14654.

sipa · 2018-11-12T02:55:38Z

Concept ACK, but you can't break compatibility by reordering another argument to listreceivedbyaddress. It's also unnecessary I think; you can specify "null" to get the default.

andrewtoth · 2018-11-12T04:11:50Z

@sipa Not sure how to pass "null" as address_filter parameter without breaking #14417.

src/wallet/rpcwallet.cpp

promag

Concept ACK

@sipa Not sure how to pass "null" as address_filter parameter without breaking #14417.

You can use named arguments instead.

Could have release note of the changed RPC's and the new argument.

src/wallet/rpcwallet.cpp

src/rpc/client.cpp

src/wallet/rpcwallet.cpp

gmaxwell · 2018-11-12T20:21:45Z

Do we know a usecase where anyone would want this behaviour? It seems transparently broken to me.

If not, I think I would prefer we fix it and have a LOUD release note about the change, and if we're feeling especially conservative have a hidden config option to restore the old behaviour which we can drop after a couple releases if no one reports needing it.

Having an option presents a perpetual usage complexity increase, and defaulting the option to the old behaviour risks users being messed up by continuing to miss these payments.

One thing to consider, however, is that coinbase payments have different deconfirmation risks then general transactions... and cannot be spent for 100 blocks. E.g. you don't want service that accept a 1 conf deposit and withdraw being exploited as a orphan risk eliminator. Nor do we want to undermine parties cashflow handling by causing them to think they can spend more now than they can. There is justification for treating coinbase payments differently (e.g. only showing them once they're spendable), just not for hiding them completely. Having a "include coinbase payments yes/no" doesn't solve these concerns. Having a "delay them by 100 blocks, yes/no" probably would.

So maybe I would suggest having an argument to hide them until maturity, defaulting on, a hidden option to get the old behaviour, defaulting off, to be removed once it's confirmed no one needs it, and a loud release note explaining the change.

promag · 2018-11-12T20:34:05Z

@gmaxwell it is however useful to know if a payment was made regardless if it takes 100 blocks to be spendable?

gmaxwell · 2018-11-12T23:18:51Z

It can be, but listreceivedbyaddress doesn't normally show unconfirmed payments and the output IIRC doesn't even tell you if its an immature coinbase payment (and if it did, many people who didn't even realize coinbase payments were possible would not know to check for it)

promag · 2018-11-12T23:39:29Z

@gmaxwell what if we consider minconf the depth added to the minimum spendable depth? Then minconf=1 for coinbase transactions would be 101?

andrewtoth · 2018-11-13T00:07:31Z

@gmaxwell I agree. I think it is an error that should be corrected. I'm not sure why anyone would want coinbase txs excluded. I haven't seen a good explanation for why they are filtered out, or a usecase.

DrahtBot · 2018-11-13T15:20:44Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

andrewtoth · 2018-11-15T04:56:42Z

Commits and PR description have been updated.

andrewtoth · 2018-11-16T18:52:10Z

I refactored the getreceivedby calls to use a common function GetReceived. This mirrors the listreceivedby calls and doesn't repeat as much code.

andrewtoth · 2018-11-20T02:43:02Z

@gmaxwell Updated this PR with your suggestions.

src/wallet/rpcwallet.cpp

jnewbery · 2021-04-14T10:31:30Z

src/wallet/rpcwallet.cpp

+            || (wtx.IsCoinBase() && (depth < 1 || filter_coinbase))
+            || (!include_immature && wtx.IsImmatureCoinBase())


Consistency helps readability. I think you can make this more consistent and readable as:

Suggested change

|| (wtx.IsCoinBase() && (depth < 1 || filter_coinbase))

|| (!include_immature && wtx.IsImmatureCoinBase())

|| (wtx.IsCoinBase() && (depth < 1 || !include_coinbase))

|| (wtx.IsImmatureCoinBase() && !include_immature_coinbase)

(where include_coinbase = !(wallet.chain().rpcEnableDeprecated("filter_coinbase")))

Another idea, this logic is close to that added to ListReceived() further down in this file; have you considered refactoring this logic into a separate small function? This may make the logic unit-testable, but may not be worth it.

@jnewbery Great suggestion, I agree this makes it much more readable.

@LarryRuane Since there are only two instances of this duplication, should we defer to the rule of three?

src/wallet/rpcwallet.cpp

doc/release-notes-14707.md

src/wallet/rpcwallet.cpp

test/functional/wallet_listreceivedby.py

glozow

Concept ACK to not filtering out coinbases (especially if they're mature, I can't imagine why someone would want to filter those out). I have a few questions.

test/functional/wallet_listreceivedby.py

glozow · 2021-04-14T14:22:29Z

src/wallet/rpcwallet.cpp

+            // Coinbase with less than 1 confirmation is an orphan
+            || (wtx.IsCoinBase() && (depth < 1 || filter_coinbase))
+            || (!include_immature && wtx.IsImmatureCoinBase())
+            || !wallet.chain().checkFinalTx(*wtx.tx)) {


I expect this refers to outputs that we can't spend yet because a timelock hasn't been met. If a user wants to include immature coinbases, might they want to include nonfinal ones too? I understand that include_immature is supposed to be only for immature coinbases but I think a user might find it more helpful to be able to configure spendability in general rather than just for coinbases

I thought this was an interesting idea. However, while implementing and trying to write a test, I was unable to broadcast a non-final tx (rejected with non-final). Do you know any way that a wallet would be able to have a non-final tx? If not, then it's possible this check is unnecessary and can be removed.

Oh hm, I guess the finality wouldn't really be reflected in the received transaction's header, but in the redeem script for it. I'm not too sure how the wallet keeps track of timelocked outputs 🤔 I don't think this check should be removed though

glozow · 2021-04-14T14:28:08Z

test/functional/wallet_listreceivedby.py

+        self.nodes[0].invalidateblock(hash)
+
+        self.log.info("getreceivedbyaddress does not include orphan when minconf is 0 when including immature")
+        balance = self.nodes[0].getreceivedbyaddress(address=address, minconf=0, include_immature=True)


Question: What happens if a user puts -spendzeroconfchange=0 (it's True by default, right?) and then calls getreceivedbyaddress(minconf=0)?

Hmm I'm not sure there is any effect in this case. The getreceivedby* RPCs only return a balance, and setting minconf=0 would return the unconfirmed balance using the tally code in GetReceived. I don't believe it would be affected by spendzeroconfchange, which from what I can see is only used in CWallet::SelectCoins.

andrewtoth · 2021-04-19T19:15:57Z

@jnewbery @glozow @LarryRuane thank you all for your review! I've updated with all of your suggestions.

src/rpc/client.cpp

jnewbery

Code review ACK 5e13dac43f257cc0869150c8f8b400b353885997 modulo release note comment.

doc/release-notes-14707.md

jnewbery · 2021-06-14T09:13:27Z

test/functional/wallet_listreceivedby.py

nit: remove extra linebreak if you retouch this branch.

Feel free to remove this extra newline if you need to touch this branch again.

andrewtoth · 2021-06-16T20:26:37Z

@jnewbery not sure what commit your ACK is referencing. Looks like github does not recognize it either.

jnewbery · 2021-06-17T10:12:30Z

@jnewbery not sure what commit your ACK is referencing. Looks like github does not recognize it either.

Oops, sorry @andrewtoth - I rebased your branch on master before building/reviewing (building a branch that's based on a very old master commit results in a very slow build since lots of header files have changed). I then accidentally left my ACK for my new head commit hash instead of the one in this PR.

ACK 164e26d42e

jnewbery · 2021-11-04T12:39:25Z

reACK 1f084ec0ef

@andrewtoth: github doesn't generate a notification for reviewers if you just force-push a rebase. If you want people to re-review after a rebase you should leave a comment. That'll notify everyone who has previously reviewed the PR.

LarryRuane

This PR has hidden conflicts in the functional test; the generate() and generatetoaddress() signatures have changed.

src/wallet/rpcwallet.cpp

maflcko · 2021-12-06T09:20:30Z

Yeah, needs rebase again.

                                   TypeError: generatetoaddress() missing 1 required keyword-only argument: 'invalid_call'

andrewtoth · 2021-12-06T17:33:36Z

Rebased.

maflcko · 2021-12-07T08:00:50Z

Looks like there are test failures with descriptor wallets?

andrewtoth · 2021-12-07T15:49:26Z

@MarcoFalke fixed.

jnewbery · 2021-12-07T17:04:52Z

reACK 1dcba99

maflcko

Sorry for the incremental review

maflcko · 2021-12-07T19:30:54Z

doc/release-notes-14707.md

+This release changes this behaviour and returns results accounting for received coins from coinbase outputs.
+
+A new option, `include_immature_coinbase` (default=`false`), determines whether to account for immature coinbase transactions.
+Immature coinbase transactions are coinbase transactions that have 100 or fewer confirmations, and are not spendable.


Is this still up-to-date? The default seems to be true. Also, instead of using the release notes to explain what immature txs are, what about moving this to the rpc help? Either this is relevant for future releases as well, or not at all.

The option is defined here and here, made clear to be false when no option is specified by your suggestion here.

I can move the immature txs explanation to the rpc help if I have to rebase again. Do you think it's a blocker? The release notes will have to be manually updated anyways, right?

No, not a blocker.

Sorry for the brain fart. I keep messing up true and false for boolean values lately.

Maybe for clarity (in the future) the second section can be switched with the third section, so that the behavior changing stuff is bundled together and the new rpc arg option is in a separate bundle?

If someone decides to address the nits, the notes snippet can also be moved to the main release notes file in the same pull request, to avoid having to touch it again.

meshcollider added the RPC/REST/ZMQ label Nov 12, 2018

meshcollider reviewed Nov 12, 2018

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

promag reviewed Nov 12, 2018

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

src/rpc/client.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

andrewtoth force-pushed the receivedby-coinbase branch from f3d8ca1 to 8d03722 Compare November 15, 2018 04:51

andrewtoth force-pushed the receivedby-coinbase branch from 8d03722 to 1a622e7 Compare November 16, 2018 18:46

andrewtoth force-pushed the receivedby-coinbase branch from 1a622e7 to ca9da49 Compare November 16, 2018 19:43

andrewtoth changed the title ~~[RPC] Add include_coinbase option to receiveby RPCs~~ [RPC] Include coinbase transactions in receiveby RPCs Nov 17, 2018

andrewtoth changed the title ~~[RPC] Include coinbase transactions in receiveby RPCs~~ [RPC] Include coinbase transactions in receivedby RPCs Nov 17, 2018

andrewtoth force-pushed the receivedby-coinbase branch 3 times, most recently from 3a1cc7c to 97602b5 Compare November 20, 2018 02:40

andrewtoth force-pushed the receivedby-coinbase branch from 97602b5 to edb44f7 Compare November 23, 2018 03:06

DrahtBot added the Needs rebase label Nov 23, 2018

andrewtoth force-pushed the receivedby-coinbase branch from edb44f7 to a9d182d Compare November 23, 2018 14:48

DrahtBot removed the Needs rebase label Nov 23, 2018

andrewtoth force-pushed the receivedby-coinbase branch 2 times, most recently from b49685a to 0fa9b60 Compare November 24, 2018 15:01

andrewtoth force-pushed the receivedby-coinbase branch from 13e344a to 08c1c6e Compare December 1, 2018 17:53

DrahtBot added the Needs rebase label Dec 5, 2018

andrewtoth force-pushed the receivedby-coinbase branch from c7c662b to 77da829 Compare April 7, 2021 00:25

LarryRuane reviewed Apr 10, 2021

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Apr 14, 2021

View reviewed changes

glozow reviewed Apr 14, 2021

View reviewed changes

This was referenced Apr 14, 2021

rpc: Keep default argument value in correct type #21679

Merged

rpc: Add RPCContext #20017

Closed

maflcko reviewed Apr 19, 2021

View reviewed changes

src/rpc/client.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request May 30, 2021

refactor: Clean up new wallet spend, receive files added #21207 #22100

Merged

jnewbery reviewed Jun 14, 2021

View reviewed changes

This was referenced Sep 15, 2021

scripted-diff: update license URLs to https #22975

Closed

rpc, wallet: Add listaddresses RPC #23019

Closed

This was referenced Dec 2, 2021

MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet #23647

Closed

Split up rpcwallet #23667

Merged

LarryRuane reviewed Dec 6, 2021

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

Include coinbase transactions in receivedby wallet rpcs

bce20c3

andrewtoth added 2 commits December 7, 2021 10:48

Test including coinbase transactions in receivedby wallet rpcs

b569675

Coinbase receivedby rpcs release notes

1dcba99

maflcko reviewed Dec 7, 2021

View reviewed changes

DrahtBot mentioned this pull request Dec 8, 2021

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

theStack mentioned this pull request May 19, 2022

rpc: wallet: remove -deprecatedrpc=exclude_coinbase logic #25171

Merged

kouloumos mentioned this pull request Aug 3, 2022

rpc: listunspent doesn't return immature coinbase outputs #25728

Closed

		\|\| (wtx.IsCoinBase() && (depth < 1 \|\| filter_coinbase))
		\|\| (!include_immature && wtx.IsImmatureCoinBase())

Conversation

andrewtoth commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewtoth commented Nov 12, 2018

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmaxwell commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 12, 2018

Uh oh!

gmaxwell commented Nov 12, 2018

Uh oh!

promag commented Nov 12, 2018

Uh oh!

andrewtoth commented Nov 13, 2018

Uh oh!

DrahtBot commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

andrewtoth commented Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewtoth commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewtoth commented Nov 20, 2018

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

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!

andrewtoth commented Apr 19, 2021

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewtoth commented Nov 12, 2018 •

edited

Loading

sipa commented Nov 12, 2018 •

edited

Loading

gmaxwell commented Nov 12, 2018 •

edited

Loading

DrahtBot commented Nov 13, 2018 •

edited

Loading

andrewtoth commented Nov 15, 2018 •

edited

Loading

andrewtoth commented Nov 16, 2018 •

edited

Loading