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

lnwallet/btcwallet: use relay fee not tx fee rate for dust check #3218

Merged
merged 3 commits into from Jun 19, 2019

Conversation

Projects
None yet
2 participants
@Roasbeef
Copy link
Member

commented Jun 19, 2019

In this commit we fix a hidden bug in the transaction creating logic
that was only manifested recently due to higher fees on Bitcoin's
mainnet. Before this commit, we would use the target fee rate to
determine if an output was dust or not. However, this is incorrect, as
instead the relay fee should be used as this matches the policy checks
widely deployed in Bitcoin full node today.

To fix this issue we now properly use the relay fee when computing dust.
This fixes the issue for the EstimateFee call, but the SendOutputs
call also has a similar issue. However, this must be fixed within
btcwallet itself, so it has been left out of this commit

Fixes #3217.

@wpaulino
Copy link
Collaborator

left a comment

LGTM 🔥

Roasbeef added some commits Jun 19, 2019

lnwallet/btcwallet: use relay fee not tx fee rate for dust check
In this commit we fix a hidden bug in the transaction creating logic
that was only manifested recently due to higher fees on Bitcoin's
mainnet. Before this commit, we would use the target fee rate to
determine if an output was dust or not. However, this is incorrect, as
instead the relay fee should be used as this matches the policy checks
widely deployed in Bitcoin full node today.

To fix this issue we now properly use the relay fee when computing dust.
This fixes the issue for the `EstimateFee` call, but the `SendOutputs`
call also has a similar issue. However, this must be fixed within
`btcwallet` itself, so it has been left out of this commit

Fixes #3217.
lnwallet: fix logic in testCreateSimpleTx test case
In this commit, we fix a logic flaw in the testCreateSimpleTx test case
which emerged once we the bug fix for dust outputs landed. Before this
commit, we would erroneously fail during valid test execution.

@Roasbeef Roasbeef force-pushed the Roasbeef:fee-estimate-fee-dust branch from 51a0fba to 4203542 Jun 19, 2019

@Roasbeef Roasbeef merged commit 507ebea into lightningnetwork:master Jun 19, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.02%) to 60.739%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.