Use CLTV as tie-breaker for offered htlc output sorting

[araspitzu]

Fixes lightning/bolts#448 , by using the CLTV value from the htlc-timeout-tx when sorting the htlc_sigs in the commit_signature message.

[codecov-io]

Codecov Report

Merging #790 into master will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #790      +/-   ##
==========================================
+ Coverage   86.20%   86.44%   +0.23%     
==========================================
  Files         119      119              
  Lines        9202     9219      +17     
  Branches      390      400      +10     
==========================================
+ Hits         7933     7969      +36     
+ Misses       1269     1250      -19     

Impacted Files	Coverage Δ
...eclair/blockchain/bitcoind/BitcoinCoreWallet.scala	89.28% <100.00%> (ø)
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	85.46% <100.00%> (+0.45%)	⬆️
...in/scala/fr/acinq/eclair/channel/Commitments.scala	90.75% <100.00%> (+0.06%)	⬆️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala	96.50% <100.00%> (+0.51%)	⬆️
.../fr/acinq/eclair/transactions/CommitmentSpec.scala	84.84% <100.00%> (+1.51%)	⬆️
...la/fr/acinq/eclair/transactions/Transactions.scala	96.89% <100.00%> (+0.12%)	⬆️
...ain/scala/fr/acinq/eclair/wire/ChannelCodecs.scala	100.00% <100.00%> (ø)
...src/main/scala/fr/acinq/eclair/router/Router.scala	92.65% <0.00%> (+0.66%)	⬆️
... and 3 more

[pm47]

Let's bring back this PR to life! 👻

[sstone]

I still finds this too complex, and it does not fix the hack we introduced with the "outputs already used" set. I think instead we should address this in 2 steps:

• first, create a proper link between commitment specs and commit tx outputs, instead of matching pubkey scripts to find which commit tx output we should be spending
• then, update how outputs are sorted to check CLTV expiries when amounts and payment hashes are the same

To implement the first step, when we build the commit tx and HTLC txs:

• filter spec items and prune dust outputs etc...
• from each remaining spec item create a pair (item, tx output)
• sort the list: the tx outputs become our commit tx outputs, built from the corresponding spec item in the list.

WDYT ?

[araspitzu]

I agree it's too complex, i initially tried to link outputs with HTLCs with the type alias but it's clearly not readable enough. I like the suggestion of creating a stronger link between the CommitmentSpec and the transaction outputs probably with a Map or a list or tuples, this will be easier to carry around and to reason about (rather than matching with pubkey script).

[sstone]

I have an idea for that, I'll open a PR so we can discuss it

[araspitzu]

I'm curious to see your proposal :), in any case i just pushed mine in f646576. It removes both the type alias for HtlcTimeoutTxInputInfo and the infamous map for "outputsAlreadyUsed", instead the makeCommitTx method returns an additional list of tuples (SpecItem, Int) where spec item can be a placeholder for toLocal/toRemote OR a DirectedHtlc, the second element of the tuple is simply the output index of the item in the sorted commit transaction. I think we can do even better by placing this extra list in the CommitTx class but this triggers a codec update and db migration..

[sstone]

I've updated my code to use some of your ideas and published it there https://github.com/ACINQ/eclair/tree/link-committx-outputs
They're similar but how spec items are referenced when you build htlc txs is handled differently in makeHtlcTxs

[t-bast]

This touches some very important parts of the protocol, before merging this we must make sure we've done extensive testing with lnd and c-lightning for identical HTLCs. Once we're close to merging it, can you please list the E2E tests done?

[araspitzu]

I think we should re-do the E2E test once we agree on the changes to the code (IIRC i did some test after opening the PR but it has passed a year now), also there is lightning/bolts#539 that can help with interop testing.

[araspitzu]

Commit 453c7d0 applies the new naming, if there isn't other feedback we can go ahead with testing.

[t-bast]

I went more in-depth, and now it makes much more sense to me.
I think it's a great clean-up, I like it!
Let's discuss my current comments and then I think we can start compat' testing.

[t-bast]

LGTM!
Let's do E2E tests and it's all good on my side 🚀
I'll do some tests on my side right now and will report back.

[t-bast]

I've finished my compatibility testing.
Our behavior is correct (I also tested local / remote force-close).
We're fully compatible with c-lightning master.
However lnd doesn't correctly handle this cltv ordering. I'm opening an issue for them to fix it, it doesn't prevent us from merging this PR IMO.

[araspitzu]

Following yesterday's conversation and hoping to get everyone on board i'm proposing to type DirectedHtlc in 748acd4. The changes introduce a small hierarchy of types where DirectedHtlc is extended by 2 subclasses, each for IN/OUT direction. There is still the duality in names Outgoing/Offered HTLC and Incoming/Received HTLC, I started using Outgoing/Incoming but we should make the naming more consistent across the codebase (and now it's a good time to make a choice).

[pm47]

Following yesterday's conversation and hoping to get everyone on board i'm proposing to type DirectedHtlc in 748acd4. The changes introduce a small hierarchy of types where DirectedHtlc is extended by 2 subclasses, each for IN/OUT direction.

We are going in the right direction (pun intended), but the Direction trait should not be needed anymore.

[araspitzu]

How do we store a DirectedHtlc without the direction? When we encode with scodecs we must encode the direction too (or the type) because we need to remember it when decoding.

[t-bast]

How do we store a DirectedHtlc without the direction? When we encode with scodecs we must encode the direction too (or the type) because we need to remember it when decoding.

If it's only needed in the codecs, that's an enum we can put closer to the codec code? Or we keep that enum here and use it only for the codec.

[araspitzu]

Last update only addresses the PR feedback, i will push another commit for the removal of Direction

[t-bast]

Can't you get the result of the directionCodec here? I think we have some codecs that do that, I'll look around.

[pm47]

Told you it had the potential to be rabbit hole-y 🤣

[t-bast]

Actually your proposal works fine, I like it. I just find the indentation a bit confusing, what about:

val htlcCodec: Codec[DirectedHtlc] = (("direction" | directionCodec) :: ("add" | updateAddHtlcCodec)).xmap(
    {
      case IN :: add :: HNil => IncomingHtlc(add)
      case OUT :: add :: HNil => OutgoingHtlc(add)
    },
    {
      htlc => htlc.direction :: htlc.add :: HNil
    }
  )

And we should put the import shapeless.:: at the top of the file IMHO, not a big fan of scattering imports in the file.

[araspitzu]

Will do, i also don't like the scattered imports but i'd make an exception for shapeless.:: if imported at the top of the file it's very hard to understand what :: does if it's from scala's collection or shapeless.

[araspitzu]

Can we postpone the removal of Direction to another PR? It's a pretty big refactoring and in many places is not trivial to replace the explicit use of the direction with pattern matching (and we end up using manifest/classtag). I will address the latest feedback and we could merge this PR as-is and without going too much out of scope.

[t-bast]

LGTM, thanks for this long-standing work!
If you don't have time in the next few days, I can work on the leftover refactoring we mentioned.

[araspitzu]

LGTM, thanks for this long-standing work!
If you don't have time in the next few days, I can work on the leftover refactoring we mentioned.

I think i can work on the refactoring, by the way before merging this PR i'd like to do a final E2E test.

[pm47]

This is missing non-reg tests for the codec change, a failure here could be catastrophic.

[araspitzu]

At 60ba2e4 i applied the latest feedback by @pm47 (including the non-reg test). I also tested the compatibility with CL and it went okay (i compared the commitment transactions after adding HTLCs with same amount and preimage but different CLTV), i didn't test with LND master yet as there is a known issue that is being fixed.

dismissing stale review