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.

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

[PoC/Bitcoin/Utxo] Correctly Consider Fees for UTXO Selection #3667

Merged
merged 29 commits into from
Jan 22, 2024

Conversation

lamafab
Copy link
Contributor

@lamafab lamafab commented Jan 17, 2024

See #3666 for more context.

Update

So, I know made the following changes:

  • The input selector in tw_utxo now considers fees in UTXO selection.
  • Implement a bunch of extra sanity checks internally.
  • Correctly update selected UTXOs returned by tw_utxo when calling it indirectly via BitcoinEntry.sign.
  • The InputSelector::SelectAscending is used by default.
  • Simplify the transaction builder; Brc20Plan now returns the signed outputs directly (@JaimeToca ).
  • Add new test module input_selection.rs which checks the input selection and fee calculation accordingly.
    • Remove weight/fee checks in tw_utxo, I need to adjust/inspect those later (covered in input_selection.rs).
    • Remove fee_estimate.rs, covered in input_selection.rs
    • Remove the BitcoinPlanBuilder (for now).

Overall this is good to go, but I'd like to take some extra time for the polishing. @JaimeToca could already make use of this, so I can push the polishing in another PR.

@lamafab lamafab changed the title [PoC/Bitcoin/Utxo] Correclty Consider Fees for UTXO selectionelection [PoC/Bitcoin/Utxo] Correclty Consider Fees for UTXO Selection Jan 17, 2024
@lamafab lamafab changed the title [PoC/Bitcoin/Utxo] Correclty Consider Fees for UTXO Selection [PoC/Bitcoin/Utxo] Correctly Consider Fees for UTXO Selection Jan 17, 2024
@lamafab lamafab marked this pull request as ready for review January 18, 2024 22:47
Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

The estimation algorithm looks promising and quite clear to understand!
I have only one major change request, please consider it t be changed or discussed

src/proto/BitcoinV2.proto Outdated Show resolved Hide resolved
rust/tw_bitcoin/tests/free_estimate.rs Outdated Show resolved Hide resolved
@satoshiotomakan satoshiotomakan merged commit 43bf58c into master Jan 22, 2024
11 checks passed
@satoshiotomakan satoshiotomakan deleted the poc-consider-fees-for-selection branch January 22, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants