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

2077 Followups #2360

Closed
5 of 6 tasks
dunxen opened this issue Jun 16, 2023 · 3 comments · Fixed by #2382
Closed
5 of 6 tasks

2077 Followups #2360

dunxen opened this issue Jun 16, 2023 · 3 comments · Fixed by #2382
Milestone

Comments

@dunxen
Copy link
Contributor

dunxen commented Jun 16, 2023

There are a bunch of followups to #2077 that need to be included before 116.

See: #2077 (review)

Reproduced below:

  • close_channel_internal should fallback to force-closure if the channel is in the inbound/outbound sets, though I'm not sure its super critical,
  • update_partial_channel_config should probably be updated to work on any channel
  • internal_shutdown should FC any pending channels
  • do_chain_event needs to call best_block_updated even on unfunded channels so we time out channels if they aren't funded in 2 weeks
  • peer_connected tries to remove outbound unfunded channels on reconnect but doesn't look at the new outbound map (which, now that I look at it, should be generating a ChannelClosed event, but isn't...ugh we need to consolidate the channel close codepaths)
  • Rename get_funding_outbound_created to get_funding_created
@dunxen dunxen added this to the 0.0.116 milestone Jun 16, 2023
@dunxen
Copy link
Contributor Author

dunxen commented Jun 26, 2023

@TheBlueMatt So for this feedback:

do_chain_event needs to call best_block_updated even on unfunded channels so we time out channels if they aren't funded in 2 weeks

It seems and I feel that the timeout is distinct in the sense that it's for established, but not yet confirmed inbound channels which seems right. (i.e. specifically waiting for funding to confirm) These are necessarily already Channels.

Question: If we've always just relied on this as a timeout for receiving a funding_created as well? I feel like that should be handled differently and probably have a shorter timeout honestly. As we're not waiting on a funding tx to confirm, we're actually waiting on our peer to just send a message.

@wpaulino
Copy link
Contributor

Question: If we've always just relied on this as a timeout for receiving a funding_created as well? I feel like that should be handled differently and probably have a shorter timeout honestly. As we're not waiting on a funding tx to confirm, we're actually waiting on our peer to just send a message.

Agreed, I don't see any reason to not impose a shorter deadline then. Mostly we just want to revert back to the behavior we had previously, which is giving up after some time of still being unconfirmed, regardless of receiving funding_created or not.

@TheBlueMatt
Copy link
Collaborator

I'm happy with either one - it does seem like that could totally be a shorter deadline (taking two weeks to build the funding tx at all is absurd), but also fine with going back to what it was and just doing it based on block height.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants