Move funding tx broadcasting to Fundingmanager #1635
Conversation
Roasbeef
added
funding
There was a problem hiding this comment.
Nice! This is a great step towards being able to properly clean up the database state for a channel whose funding transaction fails to broadcast. I commented below about also ensuring we remove the transaction from the database depending on the error we get back for the failed broadcast. However, I think this can safely be done as a follow up since it requires us to properly obtain granular errors so we can know if it's a case where we'll never be able to get the funding transaction into the mempool.
| if err != nil { | ||
| fndgLog.Errorf("unable to broadcast funding "+ | ||
| "txn: %v", err) | ||
| // We failed to broadcast the funding transaction, but watch |
There was a problem hiding this comment.
If we fail to broadcast the transaction, depending on what the failure is, we should remove it from the database immediately. The internal implementation of PublishTransaction for btcwallet will remove the transaction from the txstore if it fails to broadcast.
There was a problem hiding this comment.
This is part of the issue we have atm, where if we can't ever get the node to accept our transaction, we account for the pending balance of the channel in channelbalance incorrectly as the utxo has already been marked as unspent again in the wallet internals.
There was a problem hiding this comment.
Would it be possible to still store the tx in the db, but in a failed open state? That way we can not count them towards the max pending channels, but still resume if somehow they do confirm (and prune if something else uses the utxo). Seems dangerous to delete them entirely, as we wouldn't have the information to recover
There was a problem hiding this comment.
I'm worried that just deleting the channel from the DB completely at this point could mean loss of funds if the transaction actually do confirm. If we did, we basically would trust the chain backend to guarantee that a failure here means that the tx cannot ever "leak" to the network.
A better approach is, as we talked about, at this point increase the fee of the funding tx and rebroadcast. We can then go on, with all the state needed intact.
There was a problem hiding this comment.
Yeah ideally (assuming these other bugs are fixed), the only reason it's rejected is due to insufficient fees. With the fixes in btcwallet to allow replacement, etc we should no longer see cases where we accidentally double spend an input, or something similar.
fundingmanager_test.go
Outdated
| alice, bob := setupFundingManagers(t, maxPending) | ||
| defer tearDownFundingManagers(t, alice, bob) | ||
|
|
||
| // Create openChaneReqs for maxPending+1 channels. |
fundingmanager_test.go
Outdated
|
|
||
| // TestFundingManagerMaxPendingChannels checks that trying to open another | ||
| // channel with the same peer when MaxPending channels are pending fails. | ||
| func TestFundingManagerMaxPendingChannels(t *testing.T) { |
| if err != nil { | ||
| fndgLog.Errorf("unable to broadcast funding "+ | ||
| "txn: %v", err) | ||
| // We failed to broadcast the funding transaction, but watch |
There was a problem hiding this comment.
Would it be possible to still store the tx in the db, but in a failed open state? That way we can not count them towards the max pending channels, but still resume if somehow they do confirm (and prune if something else uses the utxo). Seems dangerous to delete them entirely, as we wouldn't have the information to recover
This commit makes sure we delete a pending channel from the set of activeReservations within the fundingmanager immediately after the channel is moved to the openChannelBucket in the DB. Previously we wouldn't do this before the funding tx was confirmed, making it possible that failing the funding flow at a later point would try to cancel a non-existent reservation context.
This commit moves the responsibility for publishing the funding tx to the network from the wallet to the funding manager. This is done to distinguish the failure of completing the reservation within the wallet and failure of publishing the transaction. Earlier we could fail to broadcast the transaction, which would cause us to fail the funding flow. This is not something we can do directly, since the CompeteReservation call will mark the channel IsPending in the databas.e
halseth
force-pushed
the
funding-broadcast-fail
branch
from
bcdf0ee
to
b437d03
Compare
|
@wpaulino that's not a bad idea, we could have a state such as |
|
Noted down to make issues for the set of follow up changes. |
|
Note that this change still will keep the channel marked as pending if the funding tx fails to publish; the difference is that we now can distinguish the publishing failure from other errors in |
Two commits to improve the handling of funding tx broadcasting errors.
fundingmanager: delete active reservation after channel is in DB
This commit makes sure we delete a pending channel from the set of
activeReservations within the fundingmanager immediately after the
channel is moved to the openChannelBucket in the DB. Previously we
wouldn't do this before the funding tx was confirmed, making it possible
that failing the funding flow at a later point would try to cancel a
non-existent reservation context.
lnwallet + funding: move funding tx publish to fundingmgr
This commit moves the responsibility for publishing the funding tx to
the network from the wallet to the funding manager. This is done to
distinguish the failure of completing the reservation within the wallet
and failure of publishing the transaction.
Earlier we could fail to broadcast the transaction, which would cause us
to fail the funding flow. This is not something we can do directly,
since the CompeteReservation call will mark the channel IsPending in the
databas.e