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

funding: notify on pending channel timeout #7228

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

morehouse
Copy link
Collaborator

For the non-initiator funding flow, the initiator is responsible for broadcasting the funding transaction. If the initiator fails to broadcast, we currently time out the pending channel after 2016 blocks.

However, we aren't notifying anyone else about the timeout, so things like channel backups and the channel arbitrator are continuing to operate as if the channel was still pending.

This PR notifies other components on timeout, allowing them to clean up their state properly.

Tested:

$ make unit pkg=funding
$ make itest icase=funding_timeout temptest=1

# Check the itest log and make sure log spam described in #7208 is gone after timeout
$ vim lntest/itest/.logs-tranche0/1-funding_timeout-Bob-* 

Fixes #7208.

After a long timeout, the non-initiating node should abandon the pending
channel to save resources. The initiating node, however, needs to
remember the channel since their funds are at risk.
If we previously notified on pending open, we also need to notify if we
close the pending channel. Otherwise, things like channel backup won't
be aware of the change until LND is restarted.
When we abandon and close a pending channel, we should also tell the
ChainNotifier so it doesn't keep watching for onchain events related to
the channel.
@morehouse
Copy link
Collaborator Author

@saubyk Do we want this for 0.16.0 or 0.16.1?

@saubyk
Copy link
Collaborator

saubyk commented Dec 2, 2022

@saubyk Do we want this for 0.16.0 or 0.16.1?

Tagged the issue for 0.16.1. Scope for 0.16.0 is now frozen

@morehouse morehouse marked this pull request as draft December 7, 2022 16:20
@morehouse
Copy link
Collaborator Author

Some new itests are failing on this PR, and I can't repro locally. Marking this as a draft while I debug. I may push some debug patches or speculative fixes.

Ensure active nodes are synced to the latest block mined, since the
nodes may actually be "fully synced" to an old block when SyncedToChain
is true.
// After more than 2016 blocks without the funding transaction appearing
// onchain, Bob should forget the pending channel as non-initiator while
// Alice remembers it as initiator.
ht.MineBlocksAndAssertNumTxes(2100, 0)
Copy link
Member

Choose a reason for hiding this comment

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

hmmm that's a lot of blocks to be mined in a single test...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can probably disable the test by default, since we have unit tests for the same thing.

The reason for the itest was to reproduce the log spam from #7208 and verify the fix.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's nice to have an itest covering it. We could probably make this value smaller for itest so it's easier to test. Or, maybe recreate miners after a certain block height?

@Roasbeef Roasbeef added funding Related to the opening of new channels with funding transactions on the blockchain refactoring labels Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
funding Related to the opening of new channels with funding transactions on the blockchain refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: timeout waiting for funding confirmation: channel still appears in logs
4 participants