wallet: Refactor and document CoinControl #26066

aureleoules · 2022-09-12T10:38:30Z

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

yancyribbens · 2022-09-12T13:32:53Z

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

Concept ACK

w0xlt

ACK ae67293

DrahtBot · 2022-09-12T16:44:39Z

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

In dba7f9b:
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 · 2022-09-13T17:08:19Z

@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?

furszy

Yeah @aureleoules better 👍🏼 .

src/wallet/spend.cpp

src/wallet/coincontrol.cpp

aureleoules · 2022-09-15T22:22:17Z

All cleaned-up @furszy!

furszy

ACK 9d4cfd6

furszy · 2022-09-16T14:48:27Z

src/wallet/coincontrol.h

+     * Pre-selects the given output.
+     * The output will be included in the transaction even if it's not the most optimal choice.


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).

How about this?

Suggested change

* Pre-selects the given output.

* The output will be included in the transaction even if it's not the most optimal choice.

* Lock-in the selected output for spending.

* The output will be included in the transaction even if it's not the most optimal choice.

yeah, sounds good 👌🏼 .

satsie · 2022-09-16T16:40:57Z

src/wallet/coincontrol.h


 private:
-    std::set<COutPoint> setSelected;
+    std::set<COutPoint> m_preset_inputs;
    std::map<COutPoint, CTxOut> m_external_txouts;


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?

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

satsie · 2022-09-16T17:02:32Z

ACK 9d4cfd6, 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 · 2022-09-16T17:09:01Z

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

furszy

ACK 287719a

aureleoules · 2022-09-16T17:11:48Z

sorry I failed my rebase @furszy

furszy · 2022-09-16T17:12:09Z

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

aureleoules · 2022-09-16T17:14:40Z

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.

satsie · 2022-09-16T20:27:28Z

ACK 5db911a

furszy

ACK 5db911a

aureleoules · 2022-10-19T10:39:19Z

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

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

Move definitions to coincontrol.cpp and add documentation.

aureleoules · 2023-04-26T10:36:32Z

Rebased

murchandamus

ACK daba957

murchandamus · 2023-05-03T13:47:27Z

src/wallet/coincontrol.cpp

+    m_external_txouts.emplace(outpoint, txout);
+}
+
+void CCoinControl::UnSelect(const COutPoint& output)


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.

Suggested change

void CCoinControl::UnSelect(const COutPoint& output)

void CCoinControl::Deselect(const COutPoint& output)

murchandamus · 2023-05-03T13:48:06Z

src/wallet/coincontrol.cpp

+    m_selected_inputs.erase(output);
+}
+
+void CCoinControl::UnSelectAll()


Nit with the same reasoning as above:

Suggested change

void CCoinControl::UnSelectAll()

void CCoinControl::DeselectAll()

achow101 · 2023-05-03T15:10:12Z

ACK daba957

daba957 refactor: Make ListSelected return vector (Sebastian Falbesoner) 9477662 wallet: Move CoinCointrol definitions to .cpp (Aurèle Oulès) 1db23da wallet: Use std::optional for GetExternalOutput and fixups (Aurèle Oulès) becc45b scripted-diff: Rename setSelected->m_selected_inputs (Aurèle Oulès) Pull request description: - 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` ACKs for top commit: achow101: ACK daba957 Xekyo: ACK daba957 Tree-SHA512: 3bf2dc834a3246c2f53f8c55154258e605fcb169431d3f7b156931f33c7e3b1ae28e03e16b37f9140a827890eb7798be485b2c36bfc23ff29bb01763f289a07c

fanquake added the Wallet label Sep 12, 2022

w0xlt approved these changes Sep 12, 2022

View reviewed changes

furszy reviewed Sep 13, 2022

View reviewed changes

aureleoules force-pushed the 2022-09-refactor-coincontrol branch from ae67293 to 69b67d5 Compare September 13, 2022 17:04

This was referenced Sep 13, 2022

wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection #25685

Merged

wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction #25273

Merged

furszy reviewed Sep 15, 2022

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

src/wallet/coincontrol.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-09-refactor-coincontrol branch from 69b67d5 to 4d77ada Compare September 15, 2022 22:20

aureleoules force-pushed the 2022-09-refactor-coincontrol branch 2 times, most recently from d0156a0 to 9d4cfd6 Compare September 15, 2022 22:27

furszy approved these changes Sep 16, 2022

View reviewed changes

satsie reviewed Sep 16, 2022

View reviewed changes

aureleoules force-pushed the 2022-09-refactor-coincontrol branch from 9d4cfd6 to 287719a Compare September 16, 2022 17:05

furszy approved these changes Sep 16, 2022

View reviewed changes

aureleoules force-pushed the 2022-09-refactor-coincontrol branch from 287719a to 8031f47 Compare September 16, 2022 17:11

aureleoules force-pushed the 2022-09-refactor-coincontrol branch from 8031f47 to 5db911a Compare September 16, 2022 17:38

furszy approved these changes Sep 17, 2022

View reviewed changes

DrahtBot added the Needs rebase label Oct 27, 2022

aureleoules force-pushed the 2022-09-refactor-coincontrol branch from 5db911a to 3f1a6d9 Compare October 28, 2022 12:36

DrahtBot requested review from satsie and w0xlt and removed request for w0xlt April 25, 2023 16:18

achow101 requested review from murchandamus and removed request for satsie and w0xlt April 25, 2023 16:19

aureleoules added 3 commits April 26, 2023 10:16

scripted-diff: Rename setSelected->m_selected_inputs

becc45b

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

wallet: Use std::optional for GetExternalOutput and fixups

1db23da

wallet: Move CoinCointrol definitions to .cpp

9477662

Move definitions to coincontrol.cpp and add documentation.

aureleoules force-pushed the 2022-09-refactor-coincontrol branch from b76fbbe to c975c42 Compare April 26, 2023 08:37

refactor: Make ListSelected return vector

daba957

aureleoules force-pushed the 2022-09-refactor-coincontrol branch from c975c42 to daba957 Compare April 26, 2023 08:41

DrahtBot removed the Needs rebase label Apr 26, 2023

murchandamus approved these changes May 3, 2023

View reviewed changes

DrahtBot requested review from satsie, theStack and w0xlt and removed request for satsie and w0xlt May 3, 2023 14:03

DrahtBot requested review from satsie and w0xlt and removed request for satsie and w0xlt May 3, 2023 15:10

achow101 merged commit 0e70a1b into bitcoin:master May 3, 2023

bitcoin locked and limited conversation to collaborators May 2, 2024

		* Pre-selects the given output.
		* The output will be included in the transaction even if it's not the most optimal choice.

	void CCoinControl::UnSelect(const COutPoint& output)
	void CCoinControl::Deselect(const COutPoint& output)

	void CCoinControl::UnSelectAll()
	void CCoinControl::DeselectAll()

wallet: Refactor and document CoinControl #26066

wallet: Refactor and document CoinControl #26066

Uh oh!

Conversation

aureleoules commented Sep 12, 2022

Uh oh!

yancyribbens commented Sep 12, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules commented Sep 13, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aureleoules commented Sep 15, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

aureleoules Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

furszy Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

satsie Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

aureleoules Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

satsie commented Sep 16, 2022

Uh oh!

aureleoules commented Sep 16, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules commented Sep 16, 2022

Uh oh!

furszy commented Sep 16, 2022

Uh oh!

aureleoules commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

satsie commented Sep 16, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules commented Oct 19, 2022

Uh oh!

aureleoules commented Apr 26, 2023

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus May 3, 2023

Choose a reason for hiding this comment

Uh oh!

murchandamus May 3, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 3, 2023

Uh oh!

DrahtBot commented Sep 12, 2022 •

edited

Loading

aureleoules commented Sep 16, 2022 •

edited

Loading