-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add quiescence #558
Conversation
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. |
c5140e6
to
02adb84
Compare
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. |
There was a problem hiding this 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/wire/LightningMessages.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/fr/acinq/lightning/channel/states/Normal.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/fr/acinq/lightning/channel/states/Normal.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/fr/acinq/lightning/channel/states/Normal.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/fr/acinq/lightning/channel/states/Normal.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/fr/acinq/lightning/channel/states/Normal.kt
Outdated
Show resolved
Hide resolved
I noticed while working on the test |
Fixed in ef52264. I'll also make this change in the Eclair PR with feedback from this PR. |
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. |
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:
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 |
There was a problem hiding this 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
?
src/commonTest/kotlin/fr/acinq/lightning/channel/TestsHelper.kt
Outdated
Show resolved
Hide resolved
src/commonTest/kotlin/fr/acinq/lightning/channel/states/QuiescenceTestsCommon.kt
Outdated
Show resolved
Hide resolved
src/commonTest/kotlin/fr/acinq/lightning/channel/states/QuiescenceTestsCommon.kt
Outdated
Show resolved
Hide resolved
src/commonTest/kotlin/fr/acinq/lightning/channel/states/QuiescenceTestsCommon.kt
Outdated
Show resolved
Hide resolved
src/commonTest/kotlin/fr/acinq/lightning/channel/states/QuiescenceTestsCommon.kt
Outdated
Show resolved
Hide resolved
src/commonTest/kotlin/fr/acinq/lightning/channel/states/QuiescenceTestsCommon.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 32d4f29
Included as part of #568 |
Porting quiescence PR from Eclair.