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

Fee bumping when fee estimation doesn't meet min relay fee #1191

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gijswijs
Copy link
Contributor

Both the minting transaction and normal on chain transactions suffer from edge cases where the fee estimation comes up with a fee that doesn't meet the current min relay fee. This PR changes that behavior by checking the estimated fee against the min relay fee, and bumps the fee if it doesn't clear the minimum fee height implied by min relay fee.

fixes #1171

@coveralls
Copy link

coveralls commented Nov 14, 2024

Pull Request Test Coverage Report for Build 11896420534

Details

  • 3 of 34 (8.82%) changed or added relevant lines in 2 files are covered.
  • 21 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.005%) to 41.011%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapgarden/planter.go 3 16 18.75%
tapfreighter/chain_porter.go 0 18 0.0%
Files with Coverage Reduction New Missed Lines %
commitment/tap.go 1 84.17%
tapgarden/planter.go 2 74.22%
tapgarden/caretaker.go 4 68.87%
tapchannel/aux_leaf_signer.go 5 35.92%
universe/interface.go 9 47.09%
Totals Coverage Status
Change from base Build 11889092912: -0.005%
Covered Lines: 25172
Relevant Lines: 61379

💛 - Coveralls

@dstadulis
Copy link
Collaborator

itest failing due to feeEstimation changes
some values aren't reset -- will investigate more

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice, LGTM 🎉

tapgarden/planter.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

Looks close! Just one note: re handling manual feerates.

The itest can be expanded a bit to check for failure on an insufficient manual feerate for both mint and transfer as well.

tapgarden/planter.go Outdated Show resolved Hide resolved
tapfreighter/chain_porter.go Outdated Show resolved Hide resolved
itest/send_test.go Show resolved Hide resolved
itest/send_test.go Outdated Show resolved Hide resolved
If the fee estimation returns a fee rate lower than the min relay fee,
we should use the min relay fee instead. This commit implements this
behavior for the minting transaction.
If the fee estimation returns a fee rate lower than the min relay fee,
we should use the min relay fee instead. This commit implements this
behavior for the tapfreighter.
The `testMinRelayFeeBump` itest checks that the minting transaction and
a basic send obtain a fee bump when the min relay fee is increased to a
value that is higher than the fee estimation.

fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

[bug]: On signet, min-relay-fee is not met if fee is not specified. Fee will be 1 sat short
5 participants