Skip to content

Close pending channel when unexpected error is returned from PublishTransaction #5158

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Close pending channel when unexpected error is returned from PublishTransaction #5158

wants to merge 3 commits into from

Conversation

@yyforyongyu
Copy link
Member

Fix #4760

We will abort the funding flow and close the pending channel if the returned error from PublishTransaction is unexpected.

@yyforyongyu
funding: remove pending open channel when tx error is unknown
cdb61f7 
In this commit, we will abort the funding flow when the error returned
from PublishTransaction is unknown, which happens in two calls, in
start() and in handleFundingSigned(). The pending open channel will be
closed in the first case. In addition to that, the error message is sent
to peer in the second case.
@yyforyongyu yyforyongyu changed the title 4760 fix tx error close pending channel when unexpected error is returned from PublishTransaction Mar 30, 2021
@yyforyongyu yyforyongyu changed the title close pending channel when unexpected error is returned from PublishTransaction Close pending channel when unexpected error is returned from PublishTransaction Mar 30, 2021
alexbosworth
alexbosworth reviewed Mar 31, 2021
View reviewed changes
funding/manager.go
"ChannelPoint(%v): %v", fundingTxBuf.Bytes(),
ch.FundingOutpoint, err)

// Note that the ErrDoubleSpend actually wraps two erros returned from
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Note that the ErrDoubleSpend actually wraps two erros returned from
// Note that the ErrDoubleSpend actually wraps two errors returned from

@alexbosworth
Copy link
Contributor

With external funding I've had a case where the publish had an error but then the channel did confirm anyway and everything worked out. This was due to a funding tx that was valid but had an unconfirmed parent that was pushed out of the local mempool due to fee conditions, making the child invalid

@yyforyongyu
Copy link
Member Author

With external funding I've had a case where the publish had an error but then the channel did confirm anyway and everything worked out. This was due to a funding tx that was valid but had an unconfirmed parent that was pushed out of the local mempool due to fee conditions, making the child invalid

Sounds like it's due to the check here that causes the btcwallet to return an ErrDoubleSpend here. Let me try to patch an itest to this PR to address this case.

@Crypt-iQ Crypt-iQ added funding Related to the opening of new channels with funding transactions on the blockchain testing Improvements/modifications to the test suite labels Mar 31, 2021
@Roasbeef Roasbeef requested review from Crypt-iQ and halseth March 31, 2021 20:06
@Roasbeef Roasbeef modified the milestones: 0.12.0, 0.13.0 Mar 31, 2021
@Roasbeef Roasbeef added bug fix P2 should be fixed if one has time labels Mar 31, 2021
@halseth
Copy link
Contributor

halseth commented Apr 6, 2021

It could be dangerous to assume a publication error means the channel will never confirm.

An alternative solution that could be more "safe" is one I started way back: #1755

Essentially the idea here was to make it possible to "release" the inputs used to the funding tx, and then wait for them to be spent before abandoning the channel.

@Crypt-iQ
Copy link
Collaborator

Crypt-iQ commented Apr 6, 2021

It could be dangerous to assume a publication error means the channel will never confirm.

Not disagreeing, but under what circumstances can this occur?

@yyforyongyu
Copy link
Member Author

It could be dangerous to assume a publication error means the channel will never confirm.

Yeah agreed. I think one direction is to look deeper into what is causing the publication error, probably here in btcwallet and here in bitcoind. This means we need to decide explicitly to abort the funding flow or not based on each of the error.

The other solution you mentioned, if I got it right, which is to double spend the input then fails the channel. This is nice as we don't need to examine each of the errors. However what if the input was never spent again and the pending channel would stuck there forever?

@halseth
Copy link
Contributor

halseth commented Apr 7, 2021

It could be dangerous to assume a publication error means the channel will never confirm.

Not disagreeing, but under what circumstances can this occur?

Alex mentioned one above. Also in case of Neutrino we currently just look at any errors the peers sends us (tbh we should probably change this) to determine if publication succeeded. This means that the peer could easily trick us by sending a fake reject.

However what if the input was never spent again and the pending channel would stuck there forever?

I think this is just a cosmetic edge case. If the input is never spent again then the use most likely isn't actively using the wallet.

@Crypt-iQ
Copy link
Collaborator

Crypt-iQ commented Apr 7, 2021

With external funding I've had a case where the publish had an error but then the channel did confirm anyway and everything worked out. This was due to a funding tx that was valid but had an unconfirmed parent that was pushed out of the local mempool due to fee conditions, making the child invalid

how did the channel eventually confirm?

@alexbosworth
Copy link
Contributor

With external funding I've had a case where the publish had an error but then the channel did confirm anyway and everything worked out. This was due to a funding tx that was valid but had an unconfirmed parent that was pushed out of the local mempool due to fee conditions, making the child invalid

how did the channel eventually confirm?

I'd imagine a case where you have an unconfirrmed parent, that isn't present on the local machine but then it does confirm, you restart lnd, this rebroadcasts, now your channel that failed broadcast locally initially is confirmed

@Crypt-iQ
Copy link
Collaborator

Crypt-iQ commented Apr 8, 2021

In Alex's scenario with this patch, the funding tx would not be broadcast and then the pending channel is failed. Which would be fine as rebroadcast would not be attempted on startup (and thus won't ever get into the mempool).

For neutrino, as Johan said, the peer could lie in MsgReject and relay the funding tx, where we would fail the pending channel completely.

For the PR that Johan linked, it would forget the channel if the funding tx's inputs are used. But, if lnd has locked these inputs, it may not be possible to spend these inputs and forget the channel.

The linked issue is mostly concerned with policy-level checks. I think we could do some sanity checks before the transaction is ever broadcast in handleFundingSigned.

@Roasbeef Roasbeef modified the milestones: 0.13.0, v0.14.0 May 11, 2021
@Roasbeef Roasbeef modified the milestones: v0.14.0, v0.15.0 Aug 30, 2021
@Roasbeef Roasbeef mentioned this pull request Feb 7, 2022
Closed
@yyforyongyu
Copy link
Member Author

!lightninglabs-deploy mute 2022-Feb-21

@Roasbeef Roasbeef added P1 MUST be fixed or reviewed and removed P2 should be fixed if one has time labels Mar 7, 2022
@Roasbeef Roasbeef requested review from bhandras and removed request for halseth March 7, 2022 18:31
@bhandras
Copy link
Collaborator

@yyforyongyu please ping when ready for review.

@yyforyongyu yyforyongyu marked this pull request as draft March 18, 2022 08:48
@yyforyongyu yyforyongyu mentioned this pull request Mar 24, 2022
Closed
@lightninglabs-deploy
Copy link

@bhandras: review reminder
@Crypt-iQ: review reminder
@yyforyongyu, remember to re-request review from reviewers when ready

@Roasbeef
Copy link
Member

Superseded by #6400?

@Roasbeef Roasbeef modified the milestones: v0.15.0, v0.16.0 Apr 12, 2022
@yyforyongyu
Copy link
Member Author

Replaced by #6400.

@yyforyongyu yyforyongyu deleted the 4760-fix-tx-error branch July 14, 2022 13:38
@saubyk
Copy link
Collaborator

saubyk commented Aug 22, 2022

Since this PR is replaced by #6400, removed the release milestone

@saubyk saubyk removed this from the v0.16.0 milestone Aug 22, 2022
@yyforyongyu yyforyongyu mentioned this pull request Aug 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Crypt-iQ Crypt-iQ Awaiting requested review from Crypt-iQ

@bhandras bhandras Awaiting requested review from bhandras

1 more reviewer

@alexbosworth alexbosworth alexbosworth left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug fix funding Related to the opening of new channels with funding transactions on the blockchain P1 MUST be fixed or reviewed testing Improvements/modifications to the test suite

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

wallet: respect policy-level transaction size limits when crafting regular and funding transactions

8 participants

@yyforyongyu @alexbosworth @halseth @Crypt-iQ @bhandras @lightninglabs-deploy @Roasbeef @saubyk