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

Handle RBF signaling publication failures in wallet #3510

Merged
merged 4 commits into from Sep 25, 2019

Conversation

halseth
Copy link
Collaborator

@halseth halseth commented Sep 16, 2019

Earlier we would not return ErrDoubleSpend if a transaction failed to publish because there was an existing, RBF-signaling, transaction in the mempool. This could happen in cases where we attempted to force close the channel, but a different closing transaction was already broadcasted.

Now we'll properly now that this error is not critical, such that we can continue resolving the closing channel.

Depends on btcsuite/btcwallet#647

Needed for #3016

@halseth halseth added this to the 0.8.0 milestone Sep 16, 2019
@wpaulino wpaulino added backend Related to the node backend software/interface (e.g. btcd, bitcoin-core) v0.8.0 wallet The wallet (lnwallet) which LND uses labels Sep 16, 2019
@wpaulino wpaulino self-requested a review September 16, 2019 19:15
lnwallet/interface_test.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
go.mod Outdated
@@ -58,6 +58,8 @@ replace github.com/lightningnetwork/lnd/ticker => ./ticker

replace github.com/lightningnetwork/lnd/queue => ./queue

replace github.com/btcsuite/btcwallet => github.com/halseth/btcwallet v0.0.0-20190919123305-ca06abc72b9c
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to self: remove when btcwallet PR is merged.

lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
@halseth
Copy link
Collaborator Author

halseth commented Sep 20, 2019

Reveiw addressed. PTAL @Roasbeef @wpaulino

Copy link
Collaborator

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Nice cleanup! Changes LGTM pending approval of the dependent btcwallet PR.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 💿

Merged pending inclusion of the dependent btcwallet PR.

In preparation for extending the testPublishTransaction test, shorten it
by moving utility methods out of the local scope.
We update to a new version of btcwallet where specific errors
(ErrDoubleSpend and ErrReplacement) will be returned from
PublishTransaction.
error

Since btcwallet will return typed errors now, we can simplify the
matching logic in order to return ErrDoubleSpend.

In case a transaction cannot be published since it did not satisfy the
requirements for a valid replacement, return ErrDoubleSpend to indicate
it was not propagated.
Checks that we get ErrDoubleSpend as expected when publishing a
conflicting mempool transaction with the same fee as the existing one,
and that we can publish a replacement with a higher fee successfully.
@halseth
Copy link
Collaborator Author

halseth commented Sep 25, 2019

btcwallet PR merged, and dependency updated.

@halseth halseth merged commit b7e1bb0 into lightningnetwork:master Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to the node backend software/interface (e.g. btcd, bitcoin-core) wallet The wallet (lnwallet) which LND uses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants