[RPC] Include coinbase transactions in receivedby RPCs #14707

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.

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.

…y RPCs

…eceivedby RPCs 9a6ee0c Coinbase receivedby rpcs release notes (Andrew Toth) c774578 Test including coinbase transactions in receivedby wallet rpcs (Andrew Toth) 4347045 Include coinbase transactions in receivedby wallet rpcs (Andrew Toth) Pull request description: 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 bitcoin/bitcoin#14654. ACKs for top commit: jnewbery: reACK 9a6ee0c Tree-SHA512: bfc43b81279fea5b6770a4620b196f6bc7c818d221b228623e9f535ec75a2406bc440e3df911608a3680f11ab64c5a4103917162114f5ff7c4ca8ab07bb9d3df

…ogic a4703ce doc: add release notes about removal of the `deprecatedrpc=exclude_coinbase` (Sebastian Falbesoner) ef0aa74 rpc: wallet: remove `-deprecatedrpc=exclude_coinbase` logic (Sebastian Falbesoner) Pull request description: Including coinbase transactions in `receivedby` RPCs and adding the `-deprecatedrpc=exclude_coinbase` was done in PR #14707 (released in v23.0). For the next release v24.0, this configuration option can be removed. ACKs for top commit: fanquake: ACK a4703ce Tree-SHA512: 97cd4e78501e64f678c78d2ebb5be5376688c023e34fced71dd24e432d27aa31a74b5483545f49ba0bdf48656d8b8b7bee74e3db26cf6daf112613f1caa4dfa4

…base` logic a4703ce doc: add release notes about removal of the `deprecatedrpc=exclude_coinbase` (Sebastian Falbesoner) ef0aa74 rpc: wallet: remove `-deprecatedrpc=exclude_coinbase` logic (Sebastian Falbesoner) Pull request description: Including coinbase transactions in `receivedby` RPCs and adding the `-deprecatedrpc=exclude_coinbase` was done in PR bitcoin#14707 (released in v23.0). For the next release v24.0, this configuration option can be removed. ACKs for top commit: fanquake: ACK a4703ce Tree-SHA512: 97cd4e78501e64f678c78d2ebb5be5376688c023e34fced71dd24e432d27aa31a74b5483545f49ba0bdf48656d8b8b7bee74e3db26cf6daf112613f1caa4dfa4

I'm not certain my merge conflict resolution in src/wallet/rpcwallet.cpp was correct, please double check that thoroughly.

…eived function a1d5b12 Merge getreceivedby tally into GetReceived function (Andrew Toth) Pull request description: This PR merges the tally code of `getreceivedbyaddress` and `getreceivedbylabel` into a single function `GetReceived`. This reduces repeated code and makes it similar to `listreceivedbyaddress` and `listreceivedbylabel`, which use the function `ListReceived`. It will also make the change in bitcoin#14707 simpler and easier to review. ACKs for top commit: theStack: re-ACK bitcoin@a1d5b12 meshcollider: utACK a1d5b12 Tree-SHA512: 43d9cd92f7c2c6a8b9c7509aa85a9b9233a6cfec1c43a9062e3bdfb83515413d1feafa8938c828351278ba22bd31c47e62ab5341e4bddc2493103b094d73b047

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

Include coinbase transactions in receivedby wallet rpcs

bce20c3

andrewtoth force-pushed the receivedby-coinbase branch from 1f084ec to 7c583e7 Compare December 6, 2021 17:33

andrewtoth added 2 commits December 7, 2021 10:48

Test including coinbase transactions in receivedby wallet rpcs

b569675

Coinbase receivedby rpcs release notes

1dcba99

andrewtoth force-pushed the receivedby-coinbase branch from 7c583e7 to 1dcba99 Compare December 7, 2021 15:49

maflcko reviewed Dec 7, 2021

View reviewed changes

maflcko merged commit 63c63b5 into bitcoin:master Dec 7, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 7, 2021

Merge bitcoin#14707: [RPC] Include coinbase transactions in receivedb…

224b859

…y RPCs

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

andrewtoth deleted the receivedby-coinbase branch November 16, 2022 01:05

delta1 added a commit to delta1/elements that referenced this pull request Jun 5, 2023

Merge 63c63b5 into merged_master (Bitcoin PR bitcoin/bitcoin#14707)

91f2b11

bitcoin locked and limited conversation to collaborators Nov 16, 2023

[RPC] Include coinbase transactions in receivedby RPCs #14707

[RPC] Include coinbase transactions in receivedby RPCs #14707

Uh oh!

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!

maflcko commented Dec 6, 2021

Uh oh!

andrewtoth commented Dec 6, 2021

Uh oh!

maflcko commented Dec 7, 2021

Uh oh!

andrewtoth commented Dec 7, 2021

Uh oh!

jnewbery commented Dec 7, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

andrewtoth Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

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