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 sure to provide an upper bound for max inputs when adjusting fees #2117

Merged
merged 1 commit into from
Sep 9, 2020

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Sep 7, 2020

Issue Number

TODO (discovered as part of the slow fee estimation investigation, this is slightly related but actually a bug).

Overview

In order to prevent transactions from becoming too big, we compute an estimated maximum number of inputs that can be selected. This is well respected by coin selection algorithms but was disregarded by the fee adjustment function which would basically pick UTxO until it has either covered fees or depleted the entire UTxO. Although in practice it should be rarely necessary to pick more than 1 extra input, there are edge-cases where we may need many (especially in wallets with a lot of dust). Picking inputs without any upper bound could cause a transaction to go beyond an acceptable size, and be later rejected.

Comments

@KtorZ KtorZ added the RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG label Sep 7, 2020
@KtorZ KtorZ requested review from Anviking and rvl September 7, 2020 17:43
@KtorZ KtorZ self-assigned this Sep 7, 2020
@KtorZ KtorZ force-pushed the KtorZ/2032/fee-estimation-slow branch 2 times, most recently from d4b0fd9 to 89f8a06 Compare September 8, 2020 07:20
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

The diff shown in github looks a little bit mixed up with #2116.
I just looked at senderPaysFee and coverRemainingFee and these changes look good.
And the unit test looks fine.

@KtorZ
Copy link
Member Author

KtorZ commented Sep 8, 2020

@rvl Yeah, I need to rebase the branch here, will do once #2116 is merged.

Base automatically changed from KtorZ/2032/fee-estimation-slow to master September 8, 2020 09:46
  In order to prevent transactions from becoming too big, we compute an
  estimated maximum number of inputs that can be selected. This is well
  respected by coin selection algorithms but was disregarded by the fee
  adjustment function which would basically pick UTxO until it has
  either covered fees or depleted the entire UTxO. Although in practice
  it should be rarely necessary to pick more than 1 extra input, there
  are edge-cases where we may need many (especially in wallets with a
  lot of dust). Picking inputs without any upper bound could cause a
  transaction to go beyond an acceptable size, and be later rejected.
@KtorZ KtorZ force-pushed the KtorZ/TODO/fix-fee-adjustment-no-upper-bound branch from 28a9f52 to 84d5378 Compare September 9, 2020 08:16
@KtorZ
Copy link
Member Author

KtorZ commented Sep 9, 2020

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 9, 2020

Build succeeded:

@iohk-bors iohk-bors bot merged commit 3e09f2a into master Sep 9, 2020
@iohk-bors iohk-bors bot deleted the KtorZ/TODO/fix-fee-adjustment-no-upper-bound branch September 9, 2020 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants