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.
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
blip-0004.md
Outdated
| * otherwise: | ||
| * SHOULD set `endorsed` to `0`. |
There was a problem hiding this comment.
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.
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.
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
0c62229 to
db9e618
Compare
|
Updated to include Antione's suggestion of including a |
db9e618 to
6fbd427
Compare
|
Seems like everyone is happy to go ahead with 3 bits of information. While we certainly could bikeshed a more complex scheme, I think that this is good enough for our purposes and strikes a good balance of:
@ProofOfKeags @thomash-acinq @vincenzopalazzo anything outstanding on your end? Last on my end is picking dates, WDYT about:
|
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
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.
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.
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.
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.
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.
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.
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.
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.
| * 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.
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.
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.
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
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.
|
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_htlcfor experimental endorsement signaling