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_> Here is something I think about, correct me if I'm wrong: Imagine a different client (not bitcoin core) -- would that client need this change?
<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
<larryruane_> glozow: yes, that's correct, I stated it wrong I think, there's one `CWallet` instance (or now can there be multiple?), and it contains a `mapWallet`
<svav> The label for an external bitcoin address is a name that you can use to keep track of the different BTC addresses for your own use. It is not a mandatory field.
<larryruane_> so the `get` versions return just a simple amount, not JSON (see jnewbery's comment above about newer RPCs preferring to return JSON -- these `get` are older RPCs)
<_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> Including coinbase outputs in the `receivedby` RPCs. I think of it as somewhat of a bug fix, since there's no reason to filter _all_ coinbase outputs
<larryruane_> marqusat: yes exactly! the local variable when iterating this container is typically called `wtx` which stands for "wallet tx" (not "witness")
<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
<svav> _andrewtoth_ : Under what circumstances would a coinbase output be received? Is this something just a successful miner receives? How does this affect the average Bitcoin user who has a wallet?
<larryruane_> marqusat: absolutely right, this is a change, as by default, the coinbase will be included, whereas it wasn't before. What did the PR author do to address this concern?
<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.
<marqusat> The coinbase maturity concept exists so that impact of forks that may naturally briefly occur in the network doesnât have an effect on spendability.
<glozow> a coinbase output looks identical to any other output though, so you'd need to see the transaction to identify that it's a coinbase (null input). that's why we store `fCoinbase` in coins?
<_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.
<sishir> Coinbase outputs are very identical to normal tx, the only difference is that they have exactly 1 input which must be of a special form which is This txin's 0000...0000.
<lightlike> but whats the different treatment there? if normal transaction became invalid, their descendants would become invalid just the same - so why a special rule for coinbase transactions?
<larryruane_> I think we're good on this point... so this is why the PR author (_andrewtoth_ ) has added the `include_immature`argument ... the default value is false, does that seem right?
<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!