What major data structures do these RPCs access? What locking is needed?
How do these RPCs scale or not scale (“big-O” behavior)?
What’s the purpose of the minconf argument?
Why were coinbase payments excluded from the results in the past?
Why does this PR add the include_immature argument? What does it do?
Why is there a concept of immature coins to begin with?
Do you agree with this argument’s default being false?
This PR makes the logic that decides whether to include a transaction in
the results more complex. Can you explain the various conditions?
The functional test seems to exercise all of the various conditions.
But how can you be sure? How can you “test the test” as part of your review?
Are there any downsides to merging this PR?
This PR changes the default behavior of these RPCs in ways that may
break existing user’s tools. Is it a good idea to include a mechanism
to revert the behavior of these PRs? Does this PR include such a
mechanism? Is it tested? When can it be removed?
<larryruane_> yes, all good answers... imagine a script on some user's system that no one knows how to maintain ... it would be bad to break that script, it may be very inconvenient for the user to fix it
<larryruane_> BTW, For the newer people here, I just want to mention that this PR is different than other recently-reviewed PRs -- it's quite simple and you don't need a lot of background knowledge. So please speak up! You'll be glad you did :)
<svav> will__ : A coinbase transaction is the first transaction in a block. It is a unique type of bitcoin transaction that can be created by a miner. The miners use it to collect the block reward for their work and any other transaction fees collected by the miner are also sent in this transaction.
<jnewbery> larryruane_: Something that we try to do to help with backwards compatibility is make the return a JSON object (rather than a array, string, or number). That means that additional key-values can be added without breaking clients that are receiving the return object.
<glozow> larryruane_: honestly not sure, I haven't looked at the wallet code that much. It's a map from txid to `CWalletTx`, I assume it's all the transactions that the wallet is interested in, e.g. sent and received
<_andrewtoth_> svav: consider I received a coinbase output to one of my addresses from mining a block 100 blocks ago, and I wanted to see the balance received by that address. The current behaviour of getreceivedbyaddress would not show the value of the coinbase output, causing me to wonder where my money is.
<glozow> I believe there's some considerations wrt user expectations about the spendability of their outputs. the number of confirmations is usually only relevant for confidence that this coin is indeed going to stay in our wallet, but for coinbases, it also affects whether or not we can spend it
<larryruane_> marqusat: right, I believe the thinking is that although this is a change, it's one that most people won't mind ... but just in case someone does, there's an escape hatch (without having to recompile the client from source)
<jnewbery> svav: A coinbase output is an output from the coinbase transaction. The coinbase transaction is the first transaction in the block, which contains the miner's reward. It's the only type of transaction that doesn't have inputs
<sishir> For a coinbase tx to be spendable they have to be mature i.e. wait till 100 confirmations, I believe this is because lets say a block is orphaned before it gets 100 blocks deep into the chain, then only the miner is affected.
<_andrewtoth_> svav: In most cases yes, coinbase outputs are received by successful miners. Average Bitcoin users will likely not be affected by this bug, hence why I believe it has received little review. There are also cases like regtest where the wallet can receive coinbase outputs with generate RPCs, and for watchonly wallets that want to watch miner addresses.
<_andrewtoth_> a normal reorg can cause some txs to be unconfirmed again, but they will all still be valid and re-enter the mempool. If coinbase txs were spendable and there were many ancestors, then all those ancestor txs would become invalid in the reorg.
<marqusat> It makes sense to have include_immature with default as false as that matches that by default minconf=1 so does not include funds from normal transactions which are not yet confirmed (spendable).
<svav> coinbase outputs are rewards for miners, and they might disappear if another chain surpasses the one the miner did. Hence, the coinbase output should not be spendable until 100 more blocks have passed, to ensure the miner's reward won't disappear, and to prevent the minder spending money that subsequently gets taken from them. Correct?
<larryruane_> Also I recall reading somewhere that immediately spendable coinbase would make coins _nonfungible_ ... receivers would have a strong incentive to check whether they were being paid using (immature) coinbase or not
<jonatack> larryruane_: depends on if the code could use test coverage and complexity vs lines saved and the value of having a single point of truth, but personally i like smart, lean code but don't mind dumb verbose tests
<larryruane_> Great points, TIL! I love your comment about verbose tests ... I once worked at a company that had a test framework that almost required a PhD to understand! Just to minimize test code. There's no reason to minimize test code, it must be understandable above all!