-
Notifications
You must be signed in to change notification settings - Fork 265
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
Splice with pending committed htlcs #2720
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #2720 +/- ##
==========================================
+ Coverage 85.90% 85.96% +0.05%
==========================================
Files 215 215
Lines 17813 17825 +12
Branches 794 739 -55
==========================================
+ Hits 15302 15323 +21
+ Misses 2511 2502 -9
|
57b77c8
to
62b9221
Compare
62b9221
to
c128696
Compare
I've added modified duplicate versions for some of the tests that already exist in
I'm leaning towards option 1 since we'd like to replace the poor-man's quiescence for all splices. Any preference @t-bast ? |
Yes, I think option 1 is best. Each existing tests would become a method, where at the beginning we would have an Then each test can be duplicated into two one-liners, something like: def testSpliceInCmd(f: FixtureParam, quiescence: Boolean): Unit = {
...
}
test("recv CMD_SPLICE (splice-in, no quiescence)") { f =>
testSpliceInCmd(f, quiescence = false)
}
test("recv CMD_SPLICE (splice-in, quiescence)", Tag(Quiescence)) { f =>
testSpliceInCmd(f, quiescence = true)
} When we get rid of the poor-man's quiescence, it will make it easy to update. |
eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala
Outdated
Show resolved
Hide resolved
504317d
to
bd6e76e
Compare
05ef07d
to
008eabc
Compare
We decided to stick with including the local/remote/shared values in the shared inputs/outputs to help with future logging and to make it easier to reason about when looking at the code. |
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.
The code looks good to me, and it's a really simple change, easy to reason about 👍
I'll need to spend a bit more time reviewing the tests: once you're happy with them, can you ping me and move this PR out of draft?
8f8b39d unifies the tests with and without quiescence. The force close tests should also check that claim-htlc txs are published. In the case of |
3979155
to
81a4814
Compare
I cleaned up @t-bast I wasn't entirely happy with my solution to add a new |
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalSplicesStateSpec.scala
Outdated
Show resolved
Hide resolved
LGTM, let's just wait for #2613 to get in first, and then we can rebase and merge 👍 |
If a channel has negotiated quiescence with pending htlcs then `InteractiveTxBuilder` will create/verify commit txs which include htlc outputs and create/verify htlc signatures when exchanging `CommitSig`. The new attribute `htlcsAmount` of `Output.Shared` and `Input.Shared` accounts for the value in htlcs when computing fees.
15bdebf
to
ee8d325
Compare
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalSplicesStateSpec.scala
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, good job!
This PR updates splices to account for pending htlcs that may exist if the channel has first negotiated quiescence.
The helper function
makeCommitTxsWithoutHtlcs
is renamed tomakeCommitTxs
and uses the set of committed htlcs that exist whenInteractiveTxBuilder
is instantiated for a splice.A new attribute
htlcsAmount
is added toOutput.Shared
to account for the value committed to htlcs from the shared funding input but not assigned tolocalAmount
orremoteAmount
.