Focused on the following changes:
ListCoins by including them directly on the AvailableCoins result (where we were skipping them before).FindNonChangeParentOutput functions (only called from ListCoins)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | aureleoules, theStack, achow101 |
| Concept ACK | josibake |
| Stale ACK | w0xlt |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
235 | - const CTransaction* ptx = &tx; 236 | - int n = output; 237 | + const CWalletTx* wtx = wallet.GetWalletTx(outpoint.hash); 238 | + assert(wtx); 239 | + 240 | + const CTransaction* ptx = wtx->tx.get();
nit
const CWalletTx& wtx{*Assert(wallet.GetWalletTx(outpoint.hash));
const CTransaction* ptx = wtx.tx.get();
thx, done
ACK 6451afadd468bbff5b88a3fe318d3af1329770ab.
AvailableCoinsParams struct.AvailableCoins function instead of being post-processed to avoid iterating over all coins again.ListCoins is now much more readable.
Thanks for the feedback @aureleoules!
nit pushed inside adfdd02. Inlined the assert + declaration using the Assert() helper function.
I understand that skipped coins are skipped in the AvailableCoins function instead of being post-processed to avoid iterating over all coins again.
On master, by default, we skip the locked coins inside AvailableCoins, which forces ListCoins to do a secondary lookup only to fetch them.
As AvailableCoins walks-through the entire wallet txs map (which includes the locked coins), this PR adds a filter option to include the locked coins in the result, instead of discard them, so ListCoins doesn't need to do the secondary lookup after calling AvailableCoins.
tACK https://github.com/bitcoin/bitcoin/pull/25659/commits/adfdd027d1edc635a2af59ce94757b2ef87e4f1a
The removed code seems redundant and not discarding the locked coins in AvailableCoins is a better solution.
Rebased on master due conflicts with recently merged 24584.
Ready to go again.
rebased
Concept ACK
Concept ACK on simplifying ListCoins but a little concerned with the general trend of making AvailableCoins a general purpose wallet traversal tool. I'm more thinking out loud here, so feel free to ignore me.
It seems we've added a few flags recently to AvailableCoins to remove additional lookups in other wallet functions and simplify code, which is great, but I'm worried that instead of simplifying we are instead obfuscating complexity and expanding the responsibility of AvailableCoins to do too many things.
This is more of a meta concern and not really a criticism of this PR, so don't want to block if other reviewers feel strongly this is the right way to go. Personally, I'm more in favor of leaving it as is for now and instead starting a discussion on a more holistic solution. If we do decide to keep increasing the responsibility of AvailableCoins, I'd recommend we leave the function signature as is and not wrap params in a struct so as to keep complexity explicit rather than hidden.
Are you still working on this?
Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
rebased, status updated.
367 | AssertLockHeld(wallet.cs_wallet); 368 | - const CTransaction* ptx = &tx; 369 | - int n = output; 370 | + const CWalletTx* wtx{Assert(wallet.GetWalletTx(outpoint.hash))}; 371 | + 372 | + const CTransaction* ptx = wtx->tx.get();
nit: maybe assert wtx->tx is not null first?
cannot be null, it's set on the CWalletTx constructor.
Can remove the locked coins lookup if we include them directly
inside the AvailableCoins result
The function is only used in ListCoins.
ACK a2ac6f9582c4c996fa36e4801fa0aac756235754, LGTM
I verified the call to AvailableCoins takes the same coin controls params as AvailableCoinsListUnspent except for skip_locked which is now false and fAllowWatchOnly to allow the removal of the large and redundant chunk of code below.
Code-review ACK a2ac6f9582c4c996fa36e4801fa0aac756235754
As discussed in IRC, note that the first commit is more than just a refactor, as it introduces some changes in behaviour regarding locked coins, since the filtering in AvailableCoins is much more detailled than the manual loop in ListCoins that is replaced:
CCoinControl's m_include_unsafe_inputs is false by default)CoinFilterParams' include_immature_coinbase is false by default)However, all of those behaviour changes seem to make sense, the commit could be seen as a bugfix. It doesn't make much sense to display unsafe or immature coins for selection in the GUI (that's the only place where ListCoins is currently used, via the listCoins interface). For a follow-up candidate, would be nice to add a proper test for ListCoins which would have detected the behaviour changes.
ACK a2ac6f9582c4c996fa36e4801fa0aac756235754
However, all of those behaviour changes seem to make sense, the commit could be seen as a bugfix. It doesn't make much sense to display unsafe or immature coins for selection in the GUI
Additionally all locked coins are not able to be selected in the GUI, so this does not materially affect what users are able to choose. However it does affect what is displayed to users.