Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move change logic to coin_select #630

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Move change logic to coin_select #630

merged 1 commit into from
Jul 26, 2022

Conversation

csralvall
Copy link
Contributor

@csralvall csralvall commented Jun 13, 2022 •
edited
Loading

Description

The former way to compute and create change was inside create_tx, just after
performing coin selection.
It blocked the opportunity to have an "ensemble" algorithm to decide between
multiple coin selection algorithms based on a metric, like Waste.
Now, change is not created inside coin_select but the change amount and the
possibility to create change is decided inside the coin_select method. In
this way, change is associated with the coin selection algorithm that generated
it, and a method to decide between them can be implemented.

Fixes #147.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@csralvall csralvall marked this pull request as draft June 13, 2022 13:57
@csralvall csralvall changed the title The bug is fixed Move change logic to coin_select Jun 13, 2022
@csralvall csralvall changed the title Move change logic to coin_select Move change logic to coin_select Jun 13, 2022
@csralvall csralvall mentioned this pull request Jun 14, 2022
Closed
6 tasks
danielabrozzoni
danielabrozzoni reviewed Jun 15, 2022
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, it looks good to me! Just a couple of comments.

src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
afilini
afilini reviewed Jun 21, 2022
View reviewed changes
Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, this looks very similar to what I had in mind in terms of architecture

src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
@notmandatory notmandatory mentioned this pull request Jun 21, 2022
Closed
14 tasks
@csralvall csralvall force-pushed the 147_get_change_from_coin_select branch from 9cbf925 to eb53376 Compare June 26, 2022 18:23
@danielabrozzoni
Copy link
Member

Can you rebase on master to pick up the CI fixes?

@csralvall csralvall force-pushed the 147_get_change_from_coin_select branch 2 times, most recently from 514af7d to 504133d Compare June 30, 2022 17:52
@csralvall csralvall marked this pull request as ready for review June 30, 2022 17:55
danielabrozzoni
danielabrozzoni reviewed Jul 6, 2022
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good! I have a small comment about decide_change, otherwise it's good to go

Edit: also it needs a rebase for conflicts

src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
@csralvall csralvall force-pushed the 147_get_change_from_coin_select branch 2 times, most recently from ce3c86f to 515cc7c Compare July 11, 2022 00:07
danielabrozzoni
danielabrozzoni reviewed Jul 11, 2022
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you squash the two commits together plz? Then I'll ACK :)

@csralvall csralvall force-pushed the 147_get_change_from_coin_select branch from 515cc7c to 816f6f8 Compare July 11, 2022 15:38
@danielabrozzoni danielabrozzoni mentioned this pull request Jul 14, 2022
Merged
6 tasks
danielabrozzoni
danielabrozzoni reviewed Jul 14, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@csralvall csralvall force-pushed the 147_get_change_from_coin_select branch 2 times, most recently from 08c1f34 to b0f6a20 Compare July 16, 2022 14:28
@notmandatory notmandatory added the new feature New feature or request label Jul 16, 2022
@notmandatory notmandatory mentioned this pull request Jul 16, 2022
Closed
22 tasks
danielabrozzoni
danielabrozzoni approved these changes Jul 18, 2022
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK b0f6a20

vladimirfomene
vladimirfomene reviewed Jul 20, 2022
View reviewed changes
Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK. Just Nits

src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
src/wallet/coin_selection.rs Show resolved Hide resolved
afilini
afilini reviewed Jul 21, 2022
View reviewed changes
Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small comments.

Also I was thinking: do you think we should add some tests for the coin selection? I noticed that most of the tests were just updated to assume an empty drain_script and I'm actually a bit suprised none of the tests started failing (I was assuming at least some tests were asserting conditions on BDK not adding a change output, and reducing the size of the drain_script should cause them to potentially start adding one).

Can you double check if we are missing something there maybe?

src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
@csralvall
Copy link
Contributor Author

csralvall commented Jul 22, 2022 •
edited
Loading

@afilini

Also I was thinking: do you think we should add some tests for the coin selection? I noticed that most of the tests were just updated to assume an empty drain_script and I'm actually a bit suprised none of the tests started failing (I was assuming at least some tests were asserting conditions on BDK not adding a change output, and reducing the size of the drain_script should cause them to potentially start adding one).

They are not failing because the former code didn't make any change computations inside coin_selection, so the excess field who stores the change isn't touched by any of the tests inside coin_selection.rs. However, the correctness of change results is tested in the tests inside "mod", as the decide_change output is directly used there. Not the best option in terms of architecture, but at the beginning I thought they were enough.
That said, while reviewing your comments, I discovered the selected_amount I was using to compute the remaining_amount and then the change, was not considering the required_utxos amount in the BranchAndBoundAlgorithm and the tests didn't throw any error.
So, I'm in favor of adding or improving the tests. Like @rajarshimaitra said here, we may not be adequately testing the fee situations and edge cases.

Can you double check if we are missing something there maybe?

So yes, I'm going to edit those empty scripts and see how I can improve the tests.

@csralvall
Move change calculus to coin_select
32ae95f 
The former way to compute and create change was inside `create_tx`, just after
performing coin selection.
It blocked the opportunity to have an "ensemble" algorithm to decide between
multiple coin selection algorithms based on a metric, like Waste.

Now, change isn't created inside `coin_select` but the change amount and the
possibility to create change is decided inside the `coin_select` method. In
this way, change is associated with the coin selection algorithm that generated
it, and a method to decide between them can be implemented.
@csralvall csralvall force-pushed the 147_get_change_from_coin_select branch from b0f6a20 to 32ae95f Compare July 25, 2022 01:33
@csralvall
Copy link
Contributor Author

I've pushed some changes in this branch underpinning the view that my proposal for the coin_selection module would have in the future.
The code is to display my ideas, so it won't compile yet.
I think that the return value of coin_select in that branch might be the final version. So, I'm not sure if it's worthy, at the end, to include the tests that I mentioned, since the functionality will be moved outside coin_select to a more general function.
I made some attempts to include tests, but all my ideas fell in circular logic, resembling highly to the decide_change function, already inside tested code.
The current checks and the fact that it's a refactoring and not an enhancement, are telling us that the PR didn't broke any current functionality. If something is wrong (and isn't reported by checks), it must be due to something previous, so a new PR addressing exclusively those errors and edge cases, or the tests for wallet in general, may be more appropriate.
With all this I mean that I've reviewed the current state of tests and any new change to the the modified code won't improve them any further.

afilini
afilini approved these changes Jul 26, 2022
View reviewed changes
Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 32ae95f

@afilini afilini merged commit 6bae52e into bitcoindevkit:master Jul 26, 2022
@danielabrozzoni danielabrozzoni mentioned this pull request Jul 27, 2022
Closed
afilini added a commit that referenced this pull request Aug 3, 2022
@afilini
Merge #666: Various fixes to the fee_amount calculation in create_tx
1730e01 
419dc24 test: Document `test_bump_fee_add_input_change_dust` (Daniela Brozzoni)
632daba test: Check tx feerate with longer signatures (Daniela Brozzoni)
2756411 test: Reproduce #660 conditions (Daniela Brozzoni)
50af51d test: Fix P2WPKH_FAKE_WITNESS_SIZE (Daniela Brozzoni)
ae91906 Take into account the segwit tx header when... ...selecting coins (Daniela Brozzoni)
7ac87b8 TXIN_BASE_WEIGHT shouldn't include the script len (Daniela Brozzoni)
ac051d7 Calculate fee amount after output addition (Daniela Brozzoni)
00d426b test: Check that the feerate is never below... ...the requested one in assert_fee_rate (Daniela Brozzoni)
42fde6d test: Check fee_amount in assert_fee_rate (Daniela Brozzoni)

Pull request description:

  ### Description

  This PR mainly fixes two bugs:
  1. TXIN_BASE_WEIGHT wrongly included the `script_len` (Fixes #160)
  2. We wouldn't take into account the segwit header in the fee calculation, which could have resulted in a transaction with a lower feerate than the requested one
  3. In tests we used to push 108 bytes on the witness as a fake signature, but we should have pushed 106 instead

  I also add a test to reproduce the conditions of #660, to check if it's solved. Turns out it's been solved already in #630, but if you're curious about what the bug was, here it is: #660 (comment)
  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  afilini:
    ACK 419dc24

Tree-SHA512: c7b55342eac440a3607a16b94560cb9c08c4805c853432adfda8e21c5177f85d5a8afe0e7e61140e92c8f10934332459c6234fc5f1509ea699d97b1d04f030c6
@csralvall csralvall deleted the 147_get_change_from_coin_select branch August 18, 2022 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladimirfomene vladimirfomene vladimirfomene left review comments

@danielabrozzoni danielabrozzoni danielabrozzoni approved these changes

@afilini afilini afilini approved these changes

Assignees

@csralvall csralvall

Labels
new feature New feature or request
Projects
BDK Maintenance
Archived in project
Milestone
Release 0.21.0 Feature Freeze
Development

Successfully merging this pull request may close these issues.

Move change creation/addition inside CoinSelectionAlgorithm
5 participants
@csralvall @danielabrozzoni @afilini @vladimirfomene @notmandatory