-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
blip-0004: experimental endorsement signaling in update_add_htlc #27
Conversation
ACK, @thomash-acinq does that work for you? |
A bit weird we have a BLIP reserving a value for a BOLT PR - do we want to include like a timeout on this reservation - "just for testing, after date X this is returned to the available pool"? |
Yeah .. there just isn't really a better place for reserving the value.
Can't we just come back and deprecate it once we're done? Because hard setting a date required that folks are upgraded to stop using it by then? Not like we're short of TLV types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, now that I think about it, what's stopping us from placing the HTLC Endorsement inside the BLIP and then deprecating the BLIP itself?
I believe that, in their current state, BLIPs are more like self-contained user stories. However, I don't think it would be problematic to describe an additional TLV type in a BLIP for a BOLTG message, right?
As I understand the line between BLIPs/BOLTS, this is the wrong place for the TLV reservation (because this is something that we intend to deploy to the whole network, so it's a BOLT), so I'd be hesitant to continue further down the BLIP path. I opened this just to flag that the TLV field is being used somewhere public, I'm not sure it even needs merging tbh.
For example - I could copy the text in, but then would we do a review round on the BLIP and the BOLT? Seems messy. |
Well before all we should decide for what we want to use the BLIPs repository and for what we want to use the BOLT one. as I see, the jamming research can be a BLIP without touch the BOLT, but this is just my feeling, and I my understand wrong the scope of the BLIPs |
b89b4c8
to
571d52d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This experiment is opened as a bLIP because it is not intended to be a permanent part of the lightning specification.
Concept ACK for this. I like that we are giving a meaning to the usage of BLIPs
blip-0004.md
Outdated
* otherwise: | ||
* SHOULD set `endorsed` to `0`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am missing something or this otherwise can be removed? if there is any endorsed
inside the update_add_htlc
there is no reason to specify one?
Or you are using it to signal the endorsed
support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you are using it to signal the endorsed support?
Indeed. I think for the purposes of an experiment, it's useful to know the difference between a zero and null (not set) value?
For the real spec, we could just not have the field at all (and likely drop it to just be a regular bool
). Just giving us some leeway for the experimental one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I was just in the code that I was rewriting today, and reading the definition it seems to me that a node should expect 0
also if the endorsed
is not supported (e.g: turning off the experimental feature in cln terms)
In the code current if the feature is turned off the value will be set to NULL
571d52d
to
0c62229
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if there is an implementation of this.
0c62229
to
db9e618
Compare
Updated to include Antione's suggestion of including a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
SGTM. |
db9e618
to
6fbd427
Compare
f2c56a6
to
3fce513
Compare
Thanks again for the reviews! Addressed last few nits/clarifications - I think this is ready to go 🚀 |
34e5540
to
9772bb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 9772bb1
Just reviewed the diff:
endorsed
type from106822
to106823
- added a new "MUST NOT use the experimental
endorsed
field for resource allocation decision upgrade
torelease
read-only
mention- "upgraded portion of the route" mention
About the latest point on "upgraded portion of the route", I think it's good enough as is it is on the suggestion to not use the experimental endorsed
field in resource allocation decision, including routing decisions.
Once deployed, this might lead to information-useful results such as "motley" trails of endorsed
signals. E.g topology A <-> B <-> C <-> D <-> E, where only B-C link signals endorsing, and what is observable on segment C-D:D-E can be probably pure noise, from an analytical viewpoint.
Such results can be sanitized at analysis and if so it's more likely beyond the scope of this blip. So I think too this is ready to go.
9772bb1
to
a651945
Compare
Rebased + updated feature bit to not clash with |
blip-0004.md
Outdated
The 3 least significant bits of the endorsement TLV are used to represent an | ||
endorsement value. A HTLC is considered to be endorsed if it is received | ||
with `endorsed`=7 and unendorsed otherwise. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want a binary endorsement, less than 4 should be unendorsed and 4 or more should be endorsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about only defining the extremes in the blip to allow more flexibility:
- Endorsed if
endorsed =7
- Unendorsed if
endorsed =0
- Otherwise up to the reputation algorithm's interpretation
Because in the binary scheme Clara and I are working on, we wouldn't want to count something that only got 50% (ie, a 4 in the proposed scheme) as endorsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that a node that uses the full range 0 - 7 will very rarely set endorsed
to 7. I agree that we should not specify what counts or not as endorsed, everyone should use the threshold they're comfortable with, but 7 is extreme.
My idea was that "endorsement" should correspond to the confidence that the HTLC will succeed. I think that's how it should be understood by downstream nodes. And even in a perfect world I would expect a sizeable portion (more than 30%) of HTLCs to fail because of liquidity being on the other side of a channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is some inference that does not follow the premises in saying "let's assume a perfect world" then introduce a percentage of HTLC failure. Either it's a perfect world and all HTLCs always succeed and all the channel are perfectly liquidity balanced on time, or we assume it's a deviation of such perfect world.
While it is plausible for someone to propose a mathematical theory of lightning liquidity and what such "perfect world" looks like, one should not forget that pure mathematic is an axiomatic science. As such there will be probably a lot of assumptions disqualifying the theory for the current goal of this blip to have network-wide measurement in the real-world of lightning. Recommending the field value to 7
can be more understood as an
arbitrary choice like done for key size parameters of cryptographic algorithms, one can start with a test value and once there is more practical experiences in its usage and those experiences have been dissected, a new value could be recommended.
Otherwise, I think there are not that much ways we can progress in the design of jamming mitigations in a lively dynamic system like lightning in a scientifically thorough way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "perfect world" I meant everyone being nice, no jamming and no holding HTLCs, because that's what this proposal is about. Having channels balanced is not a goal of this proposal and was not included in my "perfect world".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current lightning world where lightning HTLC can be freely settled or not by the payee under some timelock bounds, in coordination with the payer or not, I don't think there is a way to prevent "honest" failure to be dissociated from a jamming traffic. This without full visibility on the payment path from the viewpoint of an intermediary forwarding node, that I think cannot be achieved without removing onion encryption on the whole previous link.
The proposal is about putting in place protocol fields like endorsed
to measure the efficiency of reputation algorithm on jamming and other phenomenas like holding HTLCs. This somehow starts from the observation that jamming is a plausible vector of attack on the lightning network, and if everyone was always nice we wouldn't to have this conversation.
So under current lightning protocol has fundamentally deployed by all lightning implementations since the early days on mainet, I don't think it's possible to strictly dissociate a "nice" peer from a jamming attacker dissimulating jamming traffic in a mathematically well-defined fashion with your out of a hat 30% rate of failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to just specify the interpretation of endorsed=7
/endorsed=0
and left the remainder up to implementation's choice.
The problem is that a node that uses the full range 0 - 7 will very rarely set endorsed to 7. I agree that we should not specify what counts or not as endorsed, everyone should use the threshold they're comfortable with, but 7 is extreme.
It's inevitable that we run into this type of interpretation issue with multiple algorithms when we allow a range of endorsement values. This is one of the reasons that I think a binary signal is a better choice. That said, this is a read-only experiment with no impact on traffic, so I think that we should move forward with these permissive interpretation rules and see how the numbers shake out.
| 54/55 | `keysend` | A form of spontaneous payment | N | `var_onion_optin` | [bLIP 3](./blip-0003.md) | | ||
| 256/257 | `hosted_channels` | This node accepts requests for hosted channels | IN | | [bLIP 17](./blip-0017.md) | | ||
| 258/259 | `dns_resolver` | This node accepts DNSSEC proof requests | N | | [bLIP 32](./blip-0032.md) | | ||
| 260/261 | `htlc_endorsement` | This node forwards experimental htlc endorsement signals | N | | [bLIP 4](./blip-004.md) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced that it's useful to advertise this feature. Nodes that don't support it will just ignore the endorsement value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Advertising the feature allows us to get an idea of when we should start setting non-zero values as a sender. If we don't have this feature and senders start setting a positive endorsed signal and nobody is relaying yet, we trivially expose them as the sender.
* if `endorsed`=7 in the incoming `update_add_htlc`: | ||
* SHOULD set `endorsed`=7 on its outgoing `update_add_htlc` | ||
* otherwise: | ||
* SHOULD set `endorsed` to `0`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* if `endorsed`=7 in the incoming `update_add_htlc`: | |
* SHOULD set `endorsed`=7 on its outgoing `update_add_htlc` | |
* otherwise: | |
* SHOULD set `endorsed` to `0`. | |
* SHOULD copy `endorsed` from the incoming `update_add_htlc` to the outgoing one. |
But who is going to support the endorsed
TLV and not run a reputation algorithm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LND won't ship a reputation algorithm, just the basic mechanics to relay the signal. The reputation algorithm will be implemented externally using its interceptor APIs, and people can opt-in to running it.
I imagine a similar things will be true for CLN - ship basic mechanic by default and then add reputation as a plugin? cc: @vincenzopalazzo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both Eclair and LDK have an architecture modular enough to add a reputation algorithm as an opt-in external module, Eclair has already a plugin interface (Plugin.scala
) and LDK could have a runtime crate catching endorsed
-supporting HTLCIntercepted
event.
Usually, spec doesn't say how any feature should be supported at the node architecture-level, though here there would be a point to recommend the reputation algorithm to be an external process. If there is a DoS bug in the algorithm, it won't bring down the more safety primordial mechanisms of a LN node, e.g transactions broadcast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, thinking more about it I think it's a bad idea to copy the endorsement value if you don't run a reputation algorithm. If you copy blindly the incoming endorsement value, attackers will run their attacks through your node to use your reputation instead of theirs.
If you don't assign reputation to your peers, just always set endorsed
to the same constant value regardless of the incomming endorsed
, or don't set it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not as simple as this. This is correct that an attacker can use another lightning node through which a jamming HTLC flow can be circulated, predicting that the endorsed
value will be blindly copied to the outgoing HTLC.
On the other hand, a lightning node can have paid out-of-band fees to get inbound liquidity from said lightning
node forwarding a HTLC, and as such there is already a cost for whatever of the jamming on downstream channels.
Dropping out the incoming endorsed
field or setting to a constant value will lead to an information loss on the outgoing HTLC flow, and in function of hypothetical reputation algorithms run by downstream channels not have the full benefit of the hypothetical reputation reward of routing "economically honest" HTLCs.
I think following this recommendation of dropping on the floor the endorsed
field of incoming HTLC will introduce a bias at the detriment of "economically honest" lightning node, and this independently of channel topology considerations, when I think this blip goals aims solely to introduce minimal experimental protocol fields to allow measurements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you copy blindly the incoming endorsement value, attackers will run their attacks through your node to use your reputation instead of theirs.
This BLIP is for a read-only experimental field that allows us to see how reputation algorithms do in the wild. Nobody should be making routing decisions based on this information (MUST NOT use the experimental endorsed field in resource allocation decisions.
), so this isn't an issue.
If you don't assign reputation to your peers, just always set endorsed to the same constant value regardless of the incomming endorsed, or don't set it at all.
Certainly agree for when we add a real endorsement signal to the bolts.
I'm assuming that only a small number of nodes will actually run data collection/reputation algorithms for us. If the default behavior is to drop this signal, it'll be a pretty useless experiment.
a651945
to
81e0afd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-ACK 81e0afd no spec changes apart of the blips bit.
81e0afd
to
5cc6837
Compare
5cc6837
to
e4c8241
Compare
Summary of discussion from today's call:
|
Implements lightning/blips#27, a subsequent PR will implement a confidence estimator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-ACK e4c8241.
Only change is on the most-significant bits of the TLV that shall be interpreted as unendorsed if lower than 7
as a value.
will run the numbers with their algo to give a reasonable threshold for interpretation based on existing data (eg: consider >6 endorsed) which I'll use in my impl
After looking more on Eclair's PR and the call on the Kamong.histogram
, I don't see how it is viable to have a "reasonable threshold" where it is a defined as common identical value for the whole network of node participating in the HTLC forward. Pragmatically, the indice of payment.relay.confidence
can only be computed from a finite sample space of processed HTLCs. Any lightning node even if a topological hub has only limited computational resources to process HTLC (i.e to store and reconciliate the histogram).
I think this confidence threshold about when to endorse this outgoing update_add_htlc
should be left as an internal policy of the implementations reputation algorithmss. I believe it's good if you have jamming traffic which is circling around the confidence threshold, the traffic falling above the threshold limit can be instantly fan-out towards the topologically adjacent forwarding nodes, assuming the next path hop is the same and there are redundant paths to carry the jamming traffic. It might be even good for the network decentralization, though here a bit more of math analysis could be welcome.
Practically, I think this blip scope should stay on defining the update_add_htlc
signalling method and that what is about a confidence indice and its computation by reputation algorithm for altering outgoing HTLC endorsement devolved to another blip.
Can we go ahead and merge this @t-bast / @thomash-acinq? It's been sitting for a while and is starting to run into feature bit conflicts. |
Sounds good to me, we've added support for this to |
bLIP 0004 for experimental endorsement signalling:
update_add_htlc
for experimental endorsement signaling