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

Merged
merged 4 commits into from
Nov 21, 2024

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 11952886458

Details

  • 3 of 104 (2.88%) changed or added relevant lines in 4 files are covered.
  • 15 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.06%) to 41.238%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapgarden/planter.go 3 18 16.67%
rpcserver.go 0 26 0.0%
tapfreighter/chain_porter.go 0 27 0.0%
itest/utils.go 0 33 0.0%
Files with Coverage Reduction New Missed Lines %
rpcserver.go 1 0.0%
tapgarden/planter.go 2 74.12%
asset/mock.go 3 92.2%
asset/asset.go 4 80.96%
universe/interface.go 5 50.65%
Totals Coverage Status
Change from base Build 11949726721: -0.06%
Covered Lines: 25537
Relevant Lines: 61926

💛 - 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
@gijswijs gijswijs requested a review from jharveyb November 15, 2024 16:07
Copy link
Contributor

@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
@gijswijs gijswijs force-pushed the min-relay-fee-bump branch 2 times, most recently from 882d026 to 8325301 Compare November 20, 2024 14:01
@gijswijs gijswijs changed the base branch from main to enforce-unique-script-key November 20, 2024 14:19
@gijswijs gijswijs changed the base branch from enforce-unique-script-key to main November 20, 2024 14:43
@gijswijs gijswijs requested review from jharveyb and guggero November 20, 2024 15:44
@gijswijs
Copy link
Contributor Author

The itest can be expanded a bit

Famous last words. 😄

Anyways, I've changed both sendAssetsToAddr and MintAssetsConfirmBatch to be able to test with specific feerates and to be able to test for specific errors. I hope we can benefit from this more granular control in the future for other itests as well.

Please also have a look at

taproot-assets/rpcserver.go

Lines 685 to 686 in 1159a3f

// TODO (gijs): Can minRelayFee ever be lower than feeFloor? If not, we
// should remove the feeFloor check.

If I'm right we could remove a check there.

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.

Nice, LGTM 🎉

rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
tapgarden/planter.go Outdated Show resolved Hide resolved
itest/addrs_test.go Show resolved Hide resolved
itest/addrs_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Great changes + nice to see itest improvements 🎉

tapfreighter/chain_porter.go Outdated Show resolved Hide resolved
tapgarden/planter.go Outdated Show resolved Hide resolved
This commit includes a minrelayfee check in `checkFeeRateSanity` so that
we can err fast if a manually provided feerate doesn't meet minrelayfee.
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
@guggero guggero merged commit 965edcd into main Nov 21, 2024
17 of 18 checks passed
@guggero guggero deleted the min-relay-fee-bump branch November 21, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
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