Splice with pending committed htlcs

[remyers]

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]

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]

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]

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]

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.

[t-bast]

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]

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]

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]

LGTM, let's just wait for #2613 to get in first, and then we can rebase and merge 👍

[t-bast]

LGTM, good job!