Conversation
thomash-acinq
force-pushed
the
accountability
branch
4 times, most recently
from
c44e38f to
7e21640
Compare
thomash-acinq
force-pushed
the
accountability
branch
6 times, most recently
from
f8160e5 to
d807c65
Compare
Add accountability signal for HTLCs, it replaces endorsement. See lightning/bolts#1280
thomash-acinq
force-pushed
the
accountability
branch
from
d807c65 to
1a162d1
Compare
We update codecs to use `case class` instead of `case object`, like we do for the similar `require_confirmed_inputs` TLV, which lets us use the helper functions on TLV streams and provide better consistency. We add documentation to the acountability TLV fields. We also simplify the trait hierarchy in `PaymentOnion.scala` by removing unnecessary additions.
We refactor the following parts of the payment flow: - when fetching confidence score, the downstream node is always known, we don't need to use an `Option` in `GetConfidence` - we remove the restrictions that require accountability for on-the-fly funding, we don't want to negatively impact wallets yet - we never fail HTLCs based on accountability / incoming confidence, we only log that we would have failed them to collect data, it's way too early to actually enforce any of those restrictions - we reorder the `accountability` parameter in various functions, to make it appear before optional, less important parameters (for example custom TLVs) - we revert a bunch of small, unnecessary changes to minimize the diff with `master` We also add documentation and comments to help readers.
t-bast
left a comment
t-bast
left a comment
There was a problem hiding this comment.
This is simpler and less invasive than the previous endorsement mechanism, I like it. Let's get that on master soon to avoid dealing with merge conflicts since a lot of tests are being updated (hopefully for the last time).
I think it's fine to set the accountability bit in the route blinding data even though it isn't in the spec yet, we can still change it on master in a follow-up PR if the final decision is slightly different than what we're doing.
eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/ChannelRelay.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/NodeRelay.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/reputation/ReputationRecorder.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/HtlcTlv.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/PaymentOnion.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/reputation/ReputationSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/ChannelRelay.scala
Outdated
Show resolved
Hide resolved
|
Another reason to get this change included is that we have exceptions in our logs for an array out of bounds access that crashes the reputation recorder. I haven't figured out where it comes from because the code is quite hairy, but it gets simpler after this change so hopefully it's fixed or easier to investigate! |
|
I've merged your changes. And added another commit to make sure that we don't set the accountable bit if the HTLC does not support it. |
2 participants
Add accountability signal for HTLCs, it replaces endorsement.
See lightning/bolts#1280