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

Make LowestFee metric better #6

Closed
evanlinjin opened this issue Nov 30, 2023 · 1 comment
Closed

Make LowestFee metric better #6

evanlinjin opened this issue Nov 30, 2023 · 1 comment
Labels
bug Something isn't working enhancement New feature or request

Comments

@evanlinjin
Copy link
Member

@evanlinjin evanlinjin added bug Something isn't working enhancement New feature or request labels Nov 30, 2023
jp1ac4 added a commit to jp1ac4/liana that referenced this issue Dec 11, 2023
This is a partial fix for bitcoindevkit/coin-select#6.

When calculating the score, the excess should be added to changeless solutions
instead of those with change.

Given a solution has been found, this fix adds or removes the excess to its
incorrectly calculated score as required so that two changeless solutions can
be differentiated if one has higher excess (and therefore pays a higher fee).

Note that the `bound` function is also affected by this bug, which could mean
some branches are not considered when running BnB, but at least this fix will
mean the score for those solutions that are found is correct.
jp1ac4 added a commit to jp1ac4/liana that referenced this issue Dec 11, 2023
This is a temporary partial fix for
bitcoindevkit/coin-select#6 that should be
reverted once the upstream fix has been made.

When calculating the score, the excess should be added to changeless solutions
instead of those with change.

Given a solution has been found, this fix adds or removes the excess to its
incorrectly calculated score as required so that two changeless solutions can
be differentiated if one has higher excess (and therefore pays a higher fee).

Note that the `bound` function is also affected by this bug, which could mean
some branches are not considered when running BnB, but at least this fix will
mean the score for those solutions that are found is correct.
jp1ac4 added a commit to jp1ac4/liana that referenced this issue Dec 12, 2023
This is a temporary partial fix for
bitcoindevkit/coin-select#6 that should be
reverted once the upstream fix has been made.

When calculating the score, the excess should be added to changeless solutions
instead of those with change.

Given a solution has been found, this fix adds or removes the excess to its
incorrectly calculated score as required so that two changeless solutions can
be differentiated if one has higher excess (and therefore pays a higher fee).

Note that the `bound` function is also affected by this bug, which could mean
some branches are not considered when running BnB, but at least this fix will
mean the score for those solutions that are found is correct.
darosior added a commit to wizardsardine/liana that referenced this issue Dec 12, 2023
5f534eb spend: a temporary partial fix for `LowestFee` (jp1ac4)

Pull request description:

  This is a temporary partial fix for bitcoindevkit/coin-select#6 that should be reverted once the upstream fix has been made.

  When calculating the score, the excess should be added to changeless solutions instead of those with change.

  Given a solution has been found, this fix adds or removes the excess to its incorrectly calculated score as required so that two changeless solutions can be differentiated if one has higher excess (and therefore pays a higher fee).

  Note that the `bound` function is also affected by this bug, which could mean some branches are not considered when running BnB, but at least this fix will mean the score for those solutions that are found is correct.

  The new functional test included here fails without this fix.

ACKs for top commit:
  darosior:
    ACK 5f534eb

Tree-SHA512: f3a0e67434cad005389a938c0833394ff2c242d262f1ed1e6da9d1ce403af221bbce265a6fe67ad72f8fa8b461530faac1387c980f7f539cbb187603371c58d7
@LLFourn
Copy link
Collaborator

LLFourn commented Jan 25, 2024

I did this in #13. It is certainly "better" (as in not completely broken) now :)

@LLFourn LLFourn closed this as completed Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants