Add failed node ID field to FailureSummary

[rorp]

It's not always easy to determine which hop failed from the getsentinfo output.
This PR adds failedNode field to it.

[codecov-commenter]

Codecov Report

Merging #2042 (113d106) into master (6cc37cb) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2042      +/-   ##
==========================================
- Coverage   87.55%   87.51%   -0.04%     
==========================================
  Files         165      165              
  Lines       12694    12688       -6     
  Branches      553      536      -17     
==========================================
- Hits        11114    11104      -10     
- Misses       1580     1584       +4     

Impacted Files	Coverage Δ
...src/main/scala/fr/acinq/eclair/db/PaymentsDb.scala	84.37% <100.00%> (-8.49%)	⬇️
...ain/scala/fr/acinq/eclair/db/pg/PgPaymentsDb.scala	86.82% <100.00%> (+0.19%)	⬆️
...a/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala	99.04% <100.00%> (+0.84%)	⬆️
...clair/blockchain/bitcoind/rpc/BatchingClient.scala	86.36% <0.00%> (-13.64%)	⬇️
...main/scala/fr/acinq/eclair/io/PeerConnection.scala	84.16% <0.00%> (-0.42%)	⬇️

[t-bast]

You're not handling DB migration, your codecs aren't backwards-compatible so this will fail if there is previous data in the DB, won't it?

I see two ways to move forward:

• go down the rabbit hole with DB migration (frankly painful, if we decide to move forward with a DB migration we would likely change the payment failure summary structure more heavily)
• simply add the nodeId to the FailureSummary.failureMessage field since it's a string, so it won't have any backwards-compat problems, you could prepend them with s"$nodeId sent failure: "

[rorp]

You're not handling DB migration, your codecs aren't backwards-compatible so this will fail if there is previous data in the DB, won't it?

The codecs are backwards compatible, they are working on my regtest and mainnet nodes. paymentFailuresCodec covers both versions: https://github.com/ACINQ/eclair/pull/2042/files#diff-f371128aa65c57f5bdbcf5c7798ce7ddc1f6959e176ecf40b66e1b9dfecc8dc8R285

[t-bast]

Right, I thought the .typecase(0x01, listOfN(uint8, hopSummaryCodec)) was something that you added in this PR, since it moved file I misreviewed it, but we had actually thought ahead and introduced it a while ago to make backwards-compatibility simple!

Do you really need a dedicated field though or can you live with having it in the failure message (which would be a tiny change)? The case class feels hackish with that Option added in all cases when we know it only makes sense for one type of error (Remote). If we go down that road, I'd need some time thinking about it more, maybe it would make more sense to just store it as a json blob in the DB that we could then simply forward back to RPC callers, this way we wouldn't even need these custom codecs.

[rorp]

Do you really need a dedicated field though or can you live with having it in the failure message (which would be a tiny change)? The case class feels hackish with that Option added in all cases when we know it only makes sense for one type of error (Remote).

I don't find the use of Option hackish, since failed node ID is not always available. Alternatively, we can store failed hop index.

If we go down that road, I'd need some time thinking about it more, maybe it would make more sense to just store it as a json blob in the DB that we could then simply forward back to RPC callers, this way we wouldn't even need these custom codecs.

Personally, I'd prefer to store everything in the database in dedicated fields instead of blobs of any kind.

[t-bast]

Personally, I'd prefer to store everything in the database in dedicated fields instead of blobs of any kind.

That's not practical at all, that would mean a DB migration would be needed every time we change this data class internally.
And it means being able to serialize/deserialize custom exceptions in the DB, which is a mess as well.

You didn't answer the following question: do you really need a dedicated field though or can you live with having it in the failure message (which would be a tiny change)? Do you plan on processing this data (in an external app) or is it just for user information? I'm not sure how an external app would process this automatically, it feels better to let eclair internals handle penalizing faulty nodes (when necessary).

I'll wait for your answer on this before reviewing the PR.

[rorp]

Do you plan on processing this data (in an external app) or is it just for user information? I'm not sure how an external app would process this automatically, it feels better to let eclair internals handle penalizing faulty nodes (when necessary).

Here's an example:

https://github.com/rorp/rebalance-eclair/blob/main/eclair.py#L50
https://github.com/rorp/rebalance-eclair/blob/main/logic.py#L227

1. Eclair's (and other implementations' as well) internals are not flexible enough to be relied on and used in all cases. That's why such apps as rebalance-lnd or charge-lnd exist.
2. LND has such a field in its API output. I can't see any reason why not have this field in Eclair. It's unfortunate that in order to add just one output field I need to make so many changes.

[t-bast]

The way we used codecs here was a bit subtle, but actually quite clever.
It's easy to update these data classes without any DB update and without much code, which is a very nice property, my first pass was a bit too fast.