wallet: Refactor and document CoinControl #26066

aureleoules commented at 10:38 AM on September 12, 2022: member

Moves CoinControl function definitions from coincontrol.h to coincontrol.cpp
Adds more documentation
Renames class member for an improved comprehension
Use std::optional for GetExternalOutput

fanquake added the label Wallet on Sep 12, 2022

yancyribbens commented at 1:32 PM on September 12, 2022: contributor

These changes improve the code quality and implement the recommended practices described in the developer notes - source code organization.

Concept ACK

w0xlt approved

w0xlt commented at 1:55 PM on September 12, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26066/commits/ae67293025392e8fb5054b07a5173209c793ed6c

DrahtBot commented at 4:44 PM on September 12, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Xekyo, achow101
Concept ACK	yancyribbens
Stale ACK	w0xlt, satsie, furszy, theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26567 (Wallet: estimate the size of signed inputs using descriptors by darosior)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction 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.

furszy commented at 12:22 PM on September 13, 2022: member

In dba7f9b5: Why only change IsSelected and HasSelected and not the setters? Should be consistent.

Saying that, even if we are consistent on the renaming, I'm not so sure about it. Do you think that PreSelect(outpoint)/IsPreSelected(outpoint) is more understandable than Select(outpoint)/IsSelected(output)?

At least in my view, it doesn't look more readable than before. Would instead aim for a good docstring and done.

aureleoules force-pushed on Sep 13, 2022

aureleoules commented at 5:08 PM on September 13, 2022: member

@furszy I've slightly improved the docstring of Select and SelectExternal and rolled back to IsSelected and HasSelected. I kept the old setSelected as m_preset_inputs though.

Is it better?

in src/wallet/spend.cpp:75 in 8aecc02eca outdated

  72 | +            const auto& txout{coin_control->GetExternalOutput(input.prevout)};
  73 | +            if (!txout) {
  74 |                  return TxSize{-1, -1};
  75 |              }
  76 | -            txouts.emplace_back(txout);
  77 | +            txouts.emplace_back(*txout);

furszy commented at 9:30 PM on September 15, 2022:

In 8aecc02e: nit: could inline the return statement as well.

if (!txout) return TxSize{-1, -1};

(same for the other one).

in src/wallet/coincontrol.cpp:22 in 69b67d5f3f outdated

  17 | +    return !m_preset_inputs.empty();
  18 | +}
  19 | +
  20 | +bool CCoinControl::IsSelected(const COutPoint& output) const
  21 | +{
  22 | +    return (m_preset_inputs.count(output) > 0);

furszy commented at 9:32 PM on September 15, 2022:

nit: now that we are here, could remove the extra ()

(same for the other methods)

furszy commented at 10:00 PM on September 15, 2022: member

Yeah @aureleoules better 👍🏼 .

aureleoules force-pushed on Sep 15, 2022

aureleoules commented at 10:22 PM on September 15, 2022: member

All cleaned-up @furszy!

aureleoules force-pushed on Sep 15, 2022

in src/wallet/coincontrol.h:84 in 9d4cfd6c5a outdated

 146 | +     * Returns the external output for the given outpoint if it exists.
 147 | +     */
 148 | +    std::optional<CTxOut> GetExternalOutput(const COutPoint& outpoint) const;
 149 | +    /**
 150 | +     * Pre-selects the given output.
 151 | +     * The output will be included in the transaction even if it's not the most optimal choice.

furszy commented at 2:48 PM on September 16, 2022:

nit: would remove the "pre-selects" term and go directly with something like "store the given outpoint."

I don't think that the "pre" term here provides information to readers. (the second paragraph is great to express what the process will do with the selected outpoint)

(same for the others methods as well).

aureleoules commented at 3:12 PM on September 16, 2022:

How about this?

     * Lock-in the selected output for spending.
     * The output will be included in the transaction even if it's not the most optimal choice.

furszy commented at 5:02 PM on September 16, 2022:

yeah, sounds good 👌🏼 .

furszy approved

furszy commented at 2:52 PM on September 16, 2022: member

ACK 9d4cfd6c

in src/wallet/coincontrol.h:122 in 9d4cfd6c5a outdated

 182 | +    int64_t GetInputWeight(const COutPoint& outpoint) const;
 183 |  
 184 |  private:
 185 | -    std::set<COutPoint> setSelected;
 186 | +    std::set<COutPoint> m_preset_inputs;
 187 |      std::map<COutPoint, CTxOut> m_external_txouts;

satsie commented at 4:40 PM on September 16, 2022:

nit: you've done such a fabulous job documenting the header methods (no good deed goes unpunished!) that I think I understand an external input is one that does not belong to the wallet.

If you find yourself in the code again, is there any chance to get a comment on this data structure describing what it is? Something like //! Map of inputs that do not belong to the wallet (if that is indeed the correct definition). Actually... just for my own learning, is that the correct definition?

aureleoules commented at 5:07 PM on September 16, 2022:

Done! And yes you are correct :) The wallet would not know how to sign these inputs but they are in the utxo set.

satsie commented at 5:02 PM on September 16, 2022: contributor

ACK 9d4cfd6c5a40cafaeed31d0102b45a7e44dc0076, reviewed diff and ran unit + functional tests

I'm very new to the project but this was a great PR to review, especially after the changes that came in from earlier code review. Thanks for the comments you added on all the function definitions. Can confirm that they are helpful to new eyes and as far as I can tell, the changes are a nice cleanup that make things easier to read, understand, and follow the recommended practices (as mentioned in an earlier comment).

aureleoules force-pushed on Sep 16, 2022

aureleoules commented at 5:09 PM on September 16, 2022: member

Thanks for the reviews, I addressed your comments @satsie @furszy.

furszy approved

furszy commented at 5:10 PM on September 16, 2022: member

ACK 287719a

aureleoules force-pushed on Sep 16, 2022

aureleoules commented at 5:11 PM on September 16, 2022: member

sorry I failed my rebase @furszy

furszy commented at 5:12 PM on September 16, 2022: member

or well, not ACK :p. You forgot to change the m_preset_inputs inside CCoinControl

aureleoules commented at 5:14 PM on September 16, 2022: member

Should be all good now, I've actually renamed setSelected to m_selected_inputs instead of m_preset_inputs. Edit: also fixed typo Lock-in the selected coins for spending. to Lock-in the given output for spending.

aureleoules force-pushed on Sep 16, 2022

satsie commented at 8:27 PM on September 16, 2022: contributor

ACK 5db911a369df3977c79e6d8f06ccc38551db9efb

furszy approved

furszy commented at 1:00 PM on September 17, 2022: member

ACK 5db911a3

aureleoules commented at 10:39 AM on October 19, 2022: member

I think this is ready for merging? @w0xlt if you could reACK that would be great :smile:.

DrahtBot added the label Needs rebase on Oct 27, 2022

aureleoules force-pushed on Oct 28, 2022

DrahtBot removed the label Needs rebase on Oct 28, 2022

aureleoules commented at 1:42 PM on October 28, 2022: member

Rebased.

theStack approved

theStack commented at 11:28 AM on January 23, 2023: contributor

Code-review ACK 22801dff82d5d5329a75cb7fc14bd6e05dc67a5b

Nice cleanup and documentation improvement! nit: I have one more suggestion for an improvement, the interface of CCoinControl::ListSelected would be more straight-forward if the result is returned directly instead of using an out-parameter, simplifying the caller-side:

diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp
index bd9a90a89..4b9ed9028 100644
--- a/src/qt/coincontroldialog.cpp
+++ b/src/qt/coincontroldialog.cpp
@@ -412,8 +412,7 @@ void CoinControlDialog::updateLabels(CCoinControl& m_coin_control, WalletModel *
     unsigned int nQuantity      = 0;
     bool fWitness               = false;

-    std::vector<COutPoint> vCoinControl;
-    m_coin_control.ListSelected(vCoinControl);
+    auto vCoinControl{m_coin_control.ListSelected()};

     size_t i = 0;
     for (const auto& out : model->wallet().getCoins(vCoinControl)) {
diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp
index 9837bb442..009e95c2c 100644
--- a/src/wallet/coincontrol.cpp
+++ b/src/wallet/coincontrol.cpp
@@ -58,9 +58,9 @@ void CCoinControl::UnSelectAll()
     m_selected_inputs.clear();
 }

-void CCoinControl::ListSelected(std::vector<COutPoint>& vOutpoints) const
+std::vector<COutPoint> CCoinControl::ListSelected() const
 {
-    vOutpoints.assign(m_selected_inputs.begin(), m_selected_inputs.end());
+    return {m_selected_inputs.begin(), m_selected_inputs.end()};
 }

 void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight)
diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h
index 0ff2ced72..953e20ab4 100644
--- a/src/wallet/coincontrol.h
+++ b/src/wallet/coincontrol.h
@@ -100,7 +100,7 @@ public:
     /**
      * List the selected inputs.
      */
-    void ListSelected(std::vector<COutPoint>& vOutpoints) const;
+    std::vector<COutPoint> ListSelected() const;
     /**
      * Set an input's weight.
      */
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 2da95f037..c5deb8d33 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -145,9 +145,7 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
                                             const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {
     PreSelectedInputs result;
-    std::vector<COutPoint> vPresetInputs;
-    coin_control.ListSelected(vPresetInputs);
-    for (const COutPoint& outpoint : vPresetInputs) {
+    for (const COutPoint& outpoint : coin_control.ListSelected()) {
         int input_bytes = -1;
         CTxOut txout;
         if (auto ptr_wtx = wallet.GetWalletTx(outpoint.hash)) {

</details>

aureleoules commented at 4:03 PM on January 23, 2023: member

Thanks for the suggestion @theStack. I pushed b76fbbe8bf9504d8c08cc2f1281782db5393c02d and gave you authorship.

theStack approved

theStack commented at 4:19 PM on January 23, 2023: contributor

ACK b76fbbe8bf9504d8c08cc2f1281782db5393c02d

furszy commented at 1:27 PM on February 16, 2023: member

While I would like to merge this one (it's good as is), I think that would be better to check #25273 first. Mainly because this PR is about improving the CoinControl class documentation and #25273 modifies a big part of it (which, if we merge this one first, will make us re-do this doc improvements in another follow-up later).

DrahtBot requested review from furszy on Feb 16, 2023

DrahtBot requested review from satsie on Feb 16, 2023

DrahtBot requested review from w0xlt on Feb 16, 2023

furszy commented at 1:30 PM on February 16, 2023: member

@MarcoFalke, guess that Drahtbot shouldn't have requested review to me.

maflcko commented at 1:55 PM on February 16, 2023: member

Yeah, if it was live 3 weeks ago, it would have requested a review from you before your comment. Which, I guess, would have made sense.

Also, it will only request reviews when there is an email notification sent out anyway, so the harm/risk should be minimal. But let me know if it should be tamed down or turned off.

DrahtBot added the label Needs rebase on Feb 27, 2023

achow101 commented at 4:18 PM on April 25, 2023: member

Are you still working on this?

DrahtBot removed review request from satsie on Apr 25, 2023

DrahtBot removed review request from w0xlt on Apr 25, 2023

DrahtBot requested review from satsie on Apr 25, 2023

DrahtBot requested review from w0xlt on Apr 25, 2023

achow101 removed review request from satsie on Apr 25, 2023

achow101 removed review request from w0xlt on Apr 25, 2023

achow101 requested review from murchandamus on Apr 25, 2023

scripted-diff: Rename setSelected->m_selected_inputs

-BEGIN VERIFY SCRIPT-
sed -i 's/setSelected/m_selected_inputs/g' src/wallet/coincontrol.h src/wallet/coincontrol.cpp
-END VERIFY SCRIPT-

becc45b589

wallet: Use std::optional for GetExternalOutput and fixups 1db23da6e1

wallet: Move CoinCointrol definitions to .cpp

Move definitions to coincontrol.cpp and add documentation.

94776621ba

aureleoules force-pushed on Apr 26, 2023

refactor: Make ListSelected return vector daba95700b

aureleoules force-pushed on Apr 26, 2023

DrahtBot removed the label Needs rebase on Apr 26, 2023

aureleoules commented at 10:36 AM on April 26, 2023: member

Rebased

in src/wallet/coincontrol.cpp:51 in 94776621ba outdated

  46 | +{
  47 | +    m_selected_inputs.insert(outpoint);
  48 | +    m_external_txouts.emplace(outpoint, txout);
  49 | +}
  50 | +
  51 | +void CCoinControl::UnSelect(const COutPoint& output)

murchandamus commented at 1:47 PM on May 3, 2023:

Nit: If it’s not too much of a bother, and if you have to touch the code again, I would consider it an improvement to change UnSelect to Unselect. The first time I scanned that passage, the capitalized “S” in the middle of the word made me think that it was actually just “Select”. Even more nitty, I’d use the adjective “unselected” to refer to things that were not picked in the first place, I would prefer “to deselect” to refer to removing things from a selection.

void CCoinControl::Deselect(const COutPoint& output)

in src/wallet/coincontrol.cpp:56 in 94776621ba outdated

  51 | +void CCoinControl::UnSelect(const COutPoint& output)
  52 | +{
  53 | +    m_selected_inputs.erase(output);
  54 | +}
  55 | +
  56 | +void CCoinControl::UnSelectAll()

murchandamus commented at 1:48 PM on May 3, 2023:

Nit with the same reasoning as above:

void CCoinControl::DeselectAll()

murchandamus approved

murchandamus commented at 2:03 PM on May 3, 2023: contributor

ACK daba95700b0b77a2e898299f218c47a69ed2c7d0

DrahtBot requested review from satsie on May 3, 2023

DrahtBot requested review from theStack on May 3, 2023

DrahtBot requested review from w0xlt on May 3, 2023

DrahtBot removed review request from satsie on May 3, 2023

DrahtBot removed review request from w0xlt on May 3, 2023

DrahtBot requested review from satsie on May 3, 2023

DrahtBot requested review from w0xlt on May 3, 2023

achow101 commented at 3:10 PM on May 3, 2023: member

ACK daba95700b0b77a2e898299f218c47a69ed2c7d0

DrahtBot removed review request from satsie on May 3, 2023

DrahtBot removed review request from w0xlt on May 3, 2023

DrahtBot requested review from satsie on May 3, 2023

DrahtBot requested review from w0xlt on May 3, 2023

achow101 merged this on May 3, 2023

achow101 closed this on May 3, 2023

sidhujag referenced this in commit 2c93f3edbc on May 4, 2023

bitcoin locked this on May 2, 2024