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

Add quiescence #558

Closed
wants to merge 1 commit into from
Closed

Add quiescence #558

wants to merge 1 commit into from

Conversation

remyers
Copy link
Contributor

@remyers remyers commented Nov 3, 2023

Porting quiescence PR from Eclair.

@remyers
Copy link
Contributor Author

remyers commented Nov 3, 2023

This is still in progress while I work out some issues with the last two tests related to what happens when an incoming htlc is about to timeout while quiescent.

@remyers remyers force-pushed the quiescence branch 2 times, most recently from c5140e6 to 02adb84 Compare November 8, 2023 09:48
@remyers
Copy link
Contributor Author

remyers commented Nov 8, 2023

This is still in progress while I work out some issues with the last two tests related to what happens when an incoming htlc is about to timeout while quiescent.

I believe the correct solution for htlcs that timeout is that on disconnect/reconnect or when quiescence ends, incoming htlcs are processed by the peer. If they are close to timing out, they will be failed at that point.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

I think the HTLC reprocessing is actually simpler than what you thought, see one of my comments.

src/commonMain/kotlin/fr/acinq/lightning/Features.kt Outdated Show resolved Hide resolved
src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt Outdated Show resolved Hide resolved
src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt Outdated Show resolved Hide resolved
@remyers
Copy link
Contributor Author

remyers commented Nov 13, 2023

I noticed while working on the test receive SpliceInit when channel is not quiescent that currently our peer can send SpliceInit immediately without negotiating quiescence first and if the peers are idle, it will not reject it. I can't think of a reason to reject SpliceInit in this situation except that it means something is wrong with your peer.

@remyers
Copy link
Contributor Author

remyers commented Nov 13, 2023

I noticed while working on the test receive SpliceInit when channel is not quiescent that currently our peer can send SpliceInit immediately without negotiating quiescence first and if the peers are idle, it will not reject it. I can't think of a reason to reject SpliceInit in this situation except that it means something is wrong with your peer.

Fixed in ef52264. I'll also make this change in the Eclair PR with feedback from this PR.

@remyers
Copy link
Contributor Author

remyers commented Nov 13, 2023

Is it correct that if our pending outoing htlc times out, we should force close during a splice? This is new situation we didn't have to consider before quiescence. It seems like the risk is on the htlc receiver's side so we shouldn't need to do a preemptive force close. We might have an unprocessed htlc success or fail that came in while we were quiescent. Since our node must be the source of the htlc, we don't risk our upstream force closing on us.

@t-bast
Copy link
Member

t-bast commented Nov 14, 2023

It seems like the risk is on the htlc receiver's side so we shouldn't need to do a preemptive force close.

That's a very good point, in theory we wouldn't need to force-close, even if the HTLC has expired, because we have no funds at risk.

There are two different cases here:

  • Our peer has the preimage for that HTLC
  • Our peer doesn't have the preimage for that HTLC (it was failed on their side or they forced close the downstream channel)

In the first case, our peer will force-close before the HTLC times out anyway because they have funds at risk.

But in the second case, our peer won't preemptively force-close (at least eclair won't). So it could be beneficial if we don't force-close either, because we can wait any amount of time before receiving update_fail_htlc. But that opens the door to funds being stuck: if you never force-close, your peer can keep timed out HTLCs forever in the commitment, which locks some of your liquidity. Not force-closing would increase the trust required in your peer's behavior. This shouldn't happen anyway, since splice negotiation should always be quick, so I think we should keep the default behavior of force-closing when HTLCs time out.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

I've spent some time on the quiescence tests, reworked them and made some refactoring in 3ca8b06, could you include this commit?

It made me discover that we aren't correctly handling concurrency between stfu and shutdown. This commit contains fixes for all my comments below.

Can you also rebase to fix the conflict with Peer.kt?

@remyers remyers marked this pull request as ready for review November 14, 2023 22:09
@remyers remyers changed the title Quiescence Add quiescence Nov 14, 2023
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM! Can you squash that into a single commit? It will make it easier for future rebase while you work on the follow-up PR that handles splicing with pending HTLCs.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM after rebase, still waiting to integrate along with the splice-htlc follow-up PR.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK 32d4f29

@t-bast
Copy link
Member

t-bast commented Feb 6, 2024

Included as part of #568

@t-bast t-bast closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants