Enable 0conf and 0reserve on channels with trusted peers#853
Enable 0conf and 0reserve on channels with trusted peers#853tankyleo wants to merge 8 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
Cargo.toml
Outdated
| prost = { version = "0.11.6", default-features = false} | ||
| #bitcoin-payment-instructions = { version = "0.6" } | ||
| bitcoin-payment-instructions = { git = "https://github.com/joostjager/bitcoin-payment-instructions", branch = "ldk-dcf0c203e166da2348bef12b2e5eff4a250cdec7" } | ||
| bitcoin-payment-instructions = { git = "https://github.com/tankyleo/bitcoin-payment-instructions", branch = "ldk-688544da72cb348e4405d39a75e4d81102c1278a" } |
There was a problem hiding this comment.
Should we use rev here? Seems otherwise the revision could move if the branch is updated.
src/lib.rs
Outdated
| }, | ||
| let is_trusted_peer = self.config.trusted_peers_0conf_0reserve.contains(&node_id); | ||
| if is_trusted_peer { | ||
| match self.channel_manager.create_channel_to_trusted_peer_0reserve( |
There was a problem hiding this comment.
Hmm, I'm a bit dubious if we indeed should always opt for 0reserve if the peer is marked trusted?
There was a problem hiding this comment.
For the outbound case, I now add Node::{open_0reserve_channel, open_0reserve_channel_with_all}. For now, we do not allow 0-reserve channels to be announced.
There was a problem hiding this comment.
So if we mix it in trusted_peers_0conf_0reserve, this is somewhat strange, as I'd expect 0reserve to be set by default also for outbound channels if I add the peer to the list. But maybe making it more fine-grained would help, see below?
src/config.rs
Outdated
| /// allow them to spend their entire balance (zero-reserve). If we open a channel to a peer | ||
| /// on this list, we will allow them to spend their entire channel balance (note that for | ||
| /// channels *we* open, the decision of whether to accept HTLC forwards with no | ||
| /// confirmations of the funding transaction is *the peer's* decision). |
There was a problem hiding this comment.
Hmm, to be honest this is what makes it a bit weird to mix the concepts: yes, both require trust in the counterparty, but the AFAIU the use case is entirely opposite:
- We usually opt into 0conf if we are the client and want to allow the LSP to open 0conf channels
- If we are the client, there is little incentive to allow the LSP to not keep any reserve? So setting 0 reserve only makes sense if we are the LSP and want to allow the client to spend their full channel funds, no?
So, rather than also setting 0reserve if we are a LSPS client (see above), shouldn't we only set that if we are the LSP? Or maybe I'm missing something?
There was a problem hiding this comment.
If we are the client, there is little incentive to allow the LSP to not keep any reserve? So setting 0 reserve only makes sense if we are the LSP and want to allow the client to spend their full channel funds, no?
Thanks for the review will mull on this more, a quick reaction to this point: the LSP might want 0-reserve on their side of the channel for greater capital efficiency, ie dedicate as much of their capital as possible towards allowing clients to receive payments. If they get caught making even a single cheating attempt, their entire reputation is burned, and their business goes to 0 (hopefully !) so the incentives are aligned. 1000sats (the min reserve) * x users quickly becomes a lot of dead capital.
So, rather than also setting 0reserve if we are a LSPS client (see above), shouldn't we only set that if we are the LSP? Or maybe I'm missing something?
I see your point yes. Do you have in mind to not offer any public API for setting 0-reserve on outbound channels (assuming the client is usually the opener of the channel, and not the LSP) ?
If we were to offer a completely separate API we have these options:
- Standalone function calls; this would at least require two more
open_channelfunctions, one with max funding, and one with a parameterized amount. - A separate list, similar to what we currently have for inbound channels.
- Some kind of boolean / enum flag on the existing
open_channelfunctions.
There was a problem hiding this comment.
Discussed offline see the revised API below:
trusted_peers_0conf_0reservenow only applies to inbound channels.- Add
allow_client_0reservetoLSPS2ServiceConfig - For outbound 0-reserve channels, offer
Node::{open_0reserve_channel, open_0reserve_channel_with_all}
Question:
1. in case LSP opens the channel, the client might want 0-conf, but not 0-reserve (ie LSP keeps some funds at stake).
2. In case the client opens the channel, the LSP wants both 0-conf, and 0-reserve (ie client keeps no funds at stake).
At the moment the list trusted_peers_0conf_0reserve accommodates point number 2. For point number 1, it seems to me if you trust the LSP with a 0-conf channel, you would also trust them enough to set 0-reserve.
There was a problem hiding this comment.
trusted_peers_0conf_0reservenow only applies to inbound channels.
This is a bit odd to me, we should def. clarify that in the docs.
How about #853 (comment) ? Then the user has more fine-grained control, and maybe we can even avoid the dedicated new open_channel variants? Or, if we want to keep them, it's more clear that they'd override whatever is in the trusted_peers map?
There was a problem hiding this comment.
This is a bit odd to me, we should def. clarify that in the docs.
Done below
This setting allows LSPS2 services to open 0-reserve channels to their clients.
28276e6 to
9125870
Compare
src/config.rs
Outdated
| /// transaction never gets confirmed on-chain. Zero-reserve channels allow the peer to try | ||
| /// to steal your funds with no financial penalty. Zero-confirmation, and zero-reserve | ||
| /// channels should therefore only be accepted from trusted peers. | ||
| pub trusted_peers_0conf_0reserve: Vec<PublicKey>, |
There was a problem hiding this comment.
Hmm, given the potentially very different usecases, I'm still not sure whether it makes sense to mix the two concepts like this. Should this maybe be a HashMap<PublicKey, TrustedChannelFeatures> to allow finer-grained control?
There was a problem hiding this comment.
- As long as this only applies to inbound channels, I think these are similar use cases no ? If you trust your inbound peer with a 0-conf, you very likely also trust them with a 0-reserve channel, and vice versa.
- On the
mainbranch this list already applies only to inbound channels. - For a value set to
TrustedChannelFeatures::ZeroConfZeroReservein the hashmap, we would actually discard theZeroConfbit for outbound channels, would this be a source of confusion ? "Hey I set my peer to trusted, and opened a channel to that peer, why is it not 0conf ?" - I think this would be quite easy to grok: "Hey this list only applies to inbound channels, if you want to open a zero-reserve yourself, see
open_0reserve_channel."
There was a problem hiding this comment.
I think this would be quite easy to grok: "Hey this list only applies to inbound channels, if you want to open a zero-reserve yourself, see open_0reserve_channel."
If me and you want to open a 0 reserve channel to eachother, but don't want to accept 0 conf from eachother. Would open_0reserve_channel work? Or when I do open_0reserve_channel will your node just reject it because its not in the list?
There was a problem hiding this comment.
when alice calls open_0reserve_channel, she allows bob to spend his entire balance. Bob will accept regardless of whether Alice is on his "trusted peers" list.
If Bob has placed alice on his "trusted peers" list, bob in turn allows alice to spend her entire balance, and at the moment, allows alice to start immediately using the channel with 0-conf.
There was a problem hiding this comment.
I think this would be quite easy to grok: "Hey this list only applies to inbound channels, if you want to open a zero-reserve yourself, see open_0reserve_channel."
Added this in the commit below
src/lib.rs
Outdated
| }, | ||
| let is_trusted_peer = self.config.trusted_peers_0conf_0reserve.contains(&node_id); | ||
| if is_trusted_peer { | ||
| match self.channel_manager.create_channel_to_trusted_peer_0reserve( |
There was a problem hiding this comment.
So if we mix it in trusted_peers_0conf_0reserve, this is somewhat strange, as I'd expect 0reserve to be set by default also for outbound channels if I add the peer to the list. But maybe making it more fine-grained would help, see below?
| /// | ||
| /// [`Config::trusted_peers_0conf_0reserve`]: crate::config::Config::trusted_peers_0conf_0reserve | ||
| /// [`AnchorChannelsConfig::per_channel_reserve_sats`]: crate::config::AnchorChannelsConfig::per_channel_reserve_sats | ||
| pub fn open_0reserve_channel( |
There was a problem hiding this comment.
If we go ahead with these new APIs, they need to be exposed in uniffi bindings.
| /// | ||
| /// [`Config::trusted_peers_0conf_0reserve`]: crate::config::Config::trusted_peers_0conf_0reserve | ||
| /// [`AnchorChannelsConfig::per_channel_reserve_sats`]: crate::config::AnchorChannelsConfig::per_channel_reserve_sats | ||
| pub fn open_0reserve_channel( |
There was a problem hiding this comment.
we are getting a combinatorial explosion with these
There was a problem hiding this comment.
Discussed offline, we retain separate calls for now, and will ship your ChannelBuilder::new().with_no_reserve().announced().fund_withall() suggestion in case there is further expansion of these calls.
src/config.rs
Outdated
| /// transaction never gets confirmed on-chain. Zero-reserve channels allow the peer to try | ||
| /// to steal your funds with no financial penalty. Zero-confirmation, and zero-reserve | ||
| /// channels should therefore only be accepted from trusted peers. | ||
| pub trusted_peers_0conf_0reserve: Vec<PublicKey>, |
There was a problem hiding this comment.
I think this would be quite easy to grok: "Hey this list only applies to inbound channels, if you want to open a zero-reserve yourself, see open_0reserve_channel."
If me and you want to open a 0 reserve channel to eachother, but don't want to accept 0 conf from eachother. Would open_0reserve_channel work? Or when I do open_0reserve_channel will your node just reject it because its not in the list?
|
My takeaways after some discussion offline with Ben:
|
No description provided.