Don't blacklist recipient's peer

[akumaigorodski]

Recipient may have only one peer in which case it will become unreachable after blacklisting. Chances are recipient's peer is not the one sending garbage since recipient have an incentive to use reliable peers and will sooner or later disconnect from an unreliable one.

[akumaigorodski]

Changing ignoreNodes = Set(c) to ignoreNodes = Set.empty will make "payment failed (unparseable failure)" test pass but will also render it irrelevant, don't know what to do here (trying to change d to e, f says underlying actor is stopped).

[pm47]

I'm not sure what you are trying to achieve here Anton? What if our direct peer is the one sending garbage? Why would we make an exception for it?

Note: this should happen very rarely, it happens quite often on testnet currently because of a bug in our encoding of a bug we fixed with #366.

[akumaigorodski]

If our direct peer is the one sending garbage then it's a problem of course. I've actually moved on since this PR and currently blacklist all of the route channels (i.e. shortChannelIds drop 1 dropRight 1) instead of all the route nodes in a case of garbage.

What concerns me is node blacklisting can be used as an attack vector and Eclair's handling encourages that: a single malicious node would be knocking off all the normal nodes along the route.

Also, this seems overly restrictive if getting a payment fulfilled is of highest priority. After some testing I've found out that blacklisting nodes (with or without a direct peer) often fails a payment quickly while blacklisting all the channels for the same payment request often succeeds, although the payment does sometimes take substantially more time to get fulfilled.

[pm47]

I've actually moved on since this PR and currently blacklist all of the route channels (i.e. shortChannelIds drop 1 dropRight 1) instead of all the route nodes in a case of garbage.

So you are ok with how it is currently handled?

eclair/eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentLifecycle.scala

Line 84 in 1db673a

val blacklist = hops.map(_.nextNodeId).drop(1).dropRight(1)

What concerns me is node blacklisting can be used as an attack vector and Eclair's handling encourages that: a single malicious node would be knocking off all the normal nodes along the route.

Same here, but this is a spec-level issue and has been pushed to 1.1 (see lightning/bolts#332).

I guess we can close this PR then?

[akumaigorodski]

Yes, this PR is no longer relveant. Would like you to consider blacklisting channels instead of nodes for this reason:

Also, this seems overly restrictive if getting a payment fulfilled is of highest priority. After some testing I've found out that blacklisting nodes (with or without a direct peer) often fails a payment quickly while blacklisting all the channels for the same payment request often succeeds, although the payment does sometimes take substantially more time to get fulfilled.

[pm47]

I suspect the reason why you have better luck blacklisting channels is because these failures are due to a channel-level bug we had in eclair. In real life, I don't think a malicious/buggy node would behave the same.