fixup! Use CLTV as tie-breaker for offered htlc output sorting (#790) by pm47 · Pull Request #1360 · ACINQ/eclair

pm47 · 2020-03-31T09:49:12Z

This is a follow-up to #790 with more refactoring.

@araspitzu

- check actual content instead of only success and roundtrip - use randomized data for all fields instead of all-zero - check the remaining data, not only the decoded value (codecs are chained so a regression here will cause the next codec to fail) cc @araspitzu

eclair-core/src/main/scala/fr/acinq/eclair/transactions/CommitmentSpec.scala

eclair-core/src/test/scala/fr/acinq/eclair/payment/PostRestartHtlcCleanerSpec.scala

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala

araspitzu · 2020-03-31T11:04:06Z

I wonder if we should make a type alias or case class for Set[DirectedHtlc]. Since we often run filter/collect functions on it we could simplify the code by exposing our own filterIncoming and filterOutgoing but maybe it's an overkill.

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

t-bast · 2020-03-31T11:03:33Z

eclair-core/src/main/scala/fr/acinq/eclair/transactions/CommitmentSpec.scala

-    spec.findHtlcById(htlcId, direction.opposite) match {
-      case Some(htlc) if direction == OUT => spec.copy(toLocal = spec.toLocal + htlc.add.amountMsat, htlcs = spec.htlcs - htlc)
-      case Some(htlc) if direction == IN => spec.copy(toRemote = spec.toRemote + htlc.add.amountMsat, htlcs = spec.htlcs - htlc)
+  def fulfillOutgoingHtlc(spec: CommitmentSpec, htlcId: Long): CommitmentSpec = {


I think the naming becomes counter-intuitive with this refactoring.
It made sense to say fulfillHtlc(spec, OUT, id) to mean that you were the one sending the fulfill (and fulfillHtlc(spec, IN, id) when you were the one receiving the fulfill).

Now it's really weird because we're saying fulfillOutgoingHtlc for an incoming HTLC, and fulfillIncomingHtlc for an outgoing HTLC...I would reverse that: fulfillOutgoingHtlc should fulfill an HTLC you sent, and fulfillIncomingHtlc should fulfill one you received.

It's the same for the failXXX functions below.

Agree, this file is a mess, surely we can do better.

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala

eclair-core/src/test/scala/fr/acinq/eclair/wire/ChannelCodecsSpec.scala

t-bast

Nice, it's much better with these partial functions

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

eclair-core/src/test/scala/fr/acinq/eclair/wire/ChannelCodecsSpec.scala

…pec.scala Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>

araspitzu

Just a final nit on my side but the rest is ACK

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

t-bast

Do you want to address https://github.com/ACINQ/eclair/pull/1360/files#r400825220 right now or do you want to do it in a follow-up PR? I think the current naming of the functions is really backwards

pm47 · 2020-03-31T15:16:24Z

I'm not sure, tbh I started this just with 617e2e6 in mind and pulled the thread... I think if we don't have a better idea right now, let's merge this now, we can always improve later.

t-bast · 2020-03-31T15:21:51Z

My proposal is to simply switch the names between the two functions, what do you think?

fulfillOutgoingHtlc is what you call when the remote sent you an UpdateFulfillHtlc
fulfillIncomingHtlc is what you call when you are about to send an UpdateFulfillHtlc

That sounds simple enough and coherent, is there an issue with that?
It always uses our node as the "point of view".

codecov-io · 2020-03-31T15:23:00Z

Codecov Report

Merging #1360 into master will increase coverage by 0.10%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master    #1360      +/-   ##
==========================================
+ Coverage   86.35%   86.45%   +0.10%     
==========================================
  Files         119      119              
  Lines        9253     9259       +6     
  Branches      398      390       -8     
==========================================
+ Hits         7990     8005      +15     
+ Misses       1263     1254       -9

Impacted Files	Coverage Δ
.../fr/acinq/eclair/transactions/CommitmentSpec.scala	`82.50% <92.59%> (-2.35%)`	⬇️
...in/scala/fr/acinq/eclair/channel/Commitments.scala	`90.90% <100.00%> (+0.15%)`	⬆️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala	`96.49% <100.00%> (-0.01%)`	⬇️
.../eclair/payment/relay/PostRestartHtlcCleaner.scala	`88.00% <100.00%> (-0.19%)`	⬇️
...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%)`	⬆️
...q/eclair/blockchain/electrum/ElectrumWatcher.scala	`56.00% <0.00%> (+2.39%)`	⬆️
...clair/blockchain/electrum/ElectrumClientPool.scala	`82.79% <0.00%> (+4.30%)`	⬆️

pm47 · 2020-03-31T15:39:44Z

My proposal is to simply switch the names between the two functions, what do you think?

That sounds entirely reasonable, done with 33977e5.

eclair-core/src/main/scala/fr/acinq/eclair/transactions/CommitmentSpec.scala

pm47 added 2 commits March 30, 2020 19:13

fixup! Use CLTV as tie-breaker for offered htlc output sorting (#790)

617e2e6

removed the Direction class

68b3cbf

pm47 requested review from araspitzu and t-bast March 31, 2020 09:49

araspitzu reviewed Mar 31, 2020

View reviewed changes

t-bast reviewed Mar 31, 2020

View reviewed changes

pm47 added 2 commits March 31, 2020 16:01

addressed review comments

988d2ac

added comment on remainder

7809e58

t-bast reviewed Mar 31, 2020

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala Outdated Show resolved Hide resolved

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala Outdated Show resolved Hide resolved

pm47 added 2 commits March 31, 2020 16:25

refactored helper methods

4b1a70b

nits in Helpers

fa2a134

t-bast reviewed Mar 31, 2020

View reviewed changes

eclair-core/src/test/scala/fr/acinq/eclair/wire/ChannelCodecsSpec.scala Outdated Show resolved Hide resolved

Update eclair-core/src/test/scala/fr/acinq/eclair/wire/ChannelCodecsS…

9139e12

…pec.scala Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>

araspitzu reviewed Mar 31, 2020

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala Outdated Show resolved Hide resolved

more nit in Helpers

a4484e6

araspitzu previously approved these changes Mar 31, 2020

View reviewed changes

t-bast reviewed Mar 31, 2020

View reviewed changes

switched fulfillIncomingHtlc/fulfillOutgointHtlc

33977e5

pm47 dismissed araspitzu’s stale review via 33977e5 March 31, 2020 15:39

t-bast reviewed Mar 31, 2020

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/transactions/CommitmentSpec.scala Show resolved Hide resolved

fixup! switched fulfillIncomingHtlc/fulfillOutgointHtlc

b5f3a1d

t-bast approved these changes Mar 31, 2020

View reviewed changes

araspitzu approved these changes Mar 31, 2020

View reviewed changes

pm47 merged commit 4875d4c into master Mar 31, 2020

pm47 deleted the fix-directed-htlc branch March 31, 2020 16:31

Conversation

pm47 commented Mar 31, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

araspitzu commented Mar 31, 2020

Uh oh!

Uh oh!

t-bast Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

pm47 Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

araspitzu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

pm47 commented Mar 31, 2020

Uh oh!

t-bast commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 31, 2020

Codecov Report

Uh oh!

pm47 commented Mar 31, 2020

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

t-bast commented Mar 31, 2020 •

edited

Loading