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

Splice with pending committed htlcs #2720

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Conversation

remyers
Copy link
Contributor

@remyers remyers commented Jul 28, 2023

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 to makeCommitTxs and uses the set of committed htlcs that exist when InteractiveTxBuilder is instantiated for a splice.

A new attribute htlcsAmount is added to Output.Shared to account for the value committed to htlcs from the shared funding input but not assigned to localAmount or remoteAmount.

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2023

Codecov Report

Merging #2720 (bd6e76e) into master (47e0b83) will increase coverage by 0.05%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

❗ 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     
Files Changed Coverage Δ
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 94.89% <100.00%> (-0.15%) ⬇️
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 86.05% <100.00%> (+0.38%) ⬆️
...inq/eclair/channel/fund/InteractiveTxBuilder.scala 91.21% <100.00%> (+0.16%) ⬆️
...cinq/eclair/channel/fund/InteractiveTxFunder.scala 92.18% <100.00%> (ø)

... and 4 files with indirect coverage changes

@remyers remyers force-pushed the splice-with-htlcs branch 2 times, most recently from 57b77c8 to 62b9221 Compare August 7, 2023 09:20
@remyers remyers force-pushed the splice-with-htlcs branch from 62b9221 to c128696 Compare August 11, 2023 10:09
@remyers
Copy link
Contributor Author

remyers commented Aug 11, 2023

I've added modified duplicate versions for some of the tests that already exist in NormalSplicesStateSpec to add htlcs updates before the splice testing, and to check the commitment state after the splice. Ultimately I think I should to do one of the following:

  1. integrate these changes into the original tests (eg. add Tag(Quiescence), etc)
  2. move these tests to NormalQuiescentStateSpec and focus more on that the commitment state matches what is expected and not include the redundant splice testing logic
  3. move these tests to a new NormalQuiescentSplicesSpec and keep the redundant splice testing logic

I'm leaning towards option 1 since we'd like to replace the poor-man's quiescence for all splices. Any preference @t-bast ?

@t-bast
Copy link
Member

t-bast commented Aug 11, 2023

Yes, I think option 1 is best. Each existing tests would become a method, where at the beginning we would have an if (test.tags.contains(Quiescence)) where you add HTLCs before the splice, and at the end of the test you would have the same if to check the end result.

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.

@remyers
Copy link
Contributor Author

remyers commented Aug 30, 2023

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.

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.

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?

@remyers
Copy link
Contributor Author

remyers commented Sep 7, 2023

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 "force-close with multiple splices (simple, quiescence)" I need to do more research to determine why no 3rd stage htlc claim txs are published. Any ideas @t-bast ?

@remyers remyers force-pushed the splice-with-htlcs branch 2 times, most recently from 3979155 to 81a4814 Compare September 12, 2023 12:19
@remyers remyers marked this pull request as ready for review September 12, 2023 12:25
@remyers
Copy link
Contributor Author

remyers commented Sep 12, 2023

I cleaned up signCommitTx and WaitingForSigs.receiveCommitSig from InteractiveTxBuilder in 81a4814 to reduce the amount of copy/paste logic. This is especially important to ensure receiveCommitSig validates received signatures in a well tested way.

@t-bast I wasn't entirely happy with my solution to add a new isSplice: Boolean = false parameter to the existing receiveCommit. Is there a cleaner way to handle this? Is it correct to not increment the index of the local commit when signing for a splice? This is an area where I might need to add more tests to better understand it.

@t-bast
Copy link
Member

t-bast commented Sep 19, 2023

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.
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, good job!

@remyers remyers merged commit 3e1cc9d into ACINQ:master Sep 26, 2023
1 check passed
@remyers remyers deleted the splice-with-htlcs branch September 26, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants