-
Notifications
You must be signed in to change notification settings - Fork 385
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
Implement the SCIDAlias Channel Type and provide SCID Privacy #1351
Implement the SCIDAlias Channel Type and provide SCID Privacy #1351
Conversation
4c2a120
to
e5b5bdb
Compare
Codecov Report
@@ Coverage Diff @@
## main #1351 +/- ##
==========================================
+ Coverage 90.65% 91.47% +0.82%
==========================================
Files 73 73
Lines 40462 48497 +8035
Branches 0 48497 +48497
==========================================
+ Hits 36682 44365 +7683
- Misses 3780 4132 +352
Continue to review full report at Codecov.
|
e5b5bdb
to
7e95dae
Compare
Note first commit is #1331, which we still depend on here. |
3bf9cea
to
ff0dce9
Compare
lightning/src/util/config.rs
Outdated
/// | ||
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager | ||
/// [`DecodeError:InvalidValue`]: crate::ln::msgs::DecodeError::InvalidValue | ||
pub negotiate_scid_alias: bool, |
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.
First, I would say it's more a ChannelConfig
flag as it's related to channel metada and not HTLC flow or channel format itself ? Though reading the types of configs comments I'm not sure if we have that strong a reasoning to sort config flags.
Second, do you think we should introduce some config sanitization to avoid invalid flags to be set up and thus detect buggy config ? In that present case, we would disallow negotiate_scid_alias
if announced_channel
is true
, I think ?
edit: I see you do such sanitization in get_initial_channel_type
maybe you could log a warning message for the incompatible combination ?
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 believe we'd like to move towards ChannelConfig
being only update-able fields (ie fees and such), see-also #1270. As for sanitation, yea, we probably should, historically we've just ignored nonsense settings, which we should document/do here as well.
let scid_pref = if chan.should_announce() { | ||
chan.get_short_channel_id().or(chan.latest_inbound_scid_alias()) | ||
} else { | ||
chan.latest_inbound_scid_alias().or(chan.get_short_channel_id()) |
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.
Just to understand better, let's say the HTLC routing topology is Alice -> Bob -> Caroll, here we're returning a error date for the failure on Bob to Caroll as reported by Bob ? If so, it sounds counter-intuitive to use latest_inbound_scid_alias
as it's discovered by Bob from Caroll
's funding_locked
iiuc and never announced by Alice by a routing hint ?
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.
No, the channel here would be the channel from Alice to Bob, note the comment in the docs:
/// This is for failures on the channel on which the HTLC was *received*, not failures
/// forwarding
ff0dce9
to
df09831
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.
Overall looks pretty good. Need to take a more thorough look at the tests and when failing HTLCs.
lightning/src/util/events.rs
Outdated
@@ -429,6 +430,10 @@ pub enum Event { | |||
funding_satoshis: u64, | |||
/// Our starting balance in the channel if the request is accepted, in milli-satoshi. | |||
push_msat: u64, | |||
/// The features which this channel will operate with. If you reject the channel, 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.
s/which/that
Rule of thumb: Use "that" if what follows restricts the preceding phrase and hence removing the clause would change its meaning. Use "which" if the clause can be removed without changing the meaning.
/// requires that our counterparty only relay HTLCs to us which use the channel's SCID alias. | ||
/// | ||
/// If this option is set, channels may be created which will not be readable by LDK versions | ||
/// prior to 0.0.106, causing [`ChannelManager`]'s read method to return 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.
s/[ChannelManager
]'s read method/[ChannelManager::read
]
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.
Sadly the link is broken if I do that since its through a trait - I can do it without [], or I can leave it as is, which do you prefer?
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.
Fine to keep it as is.
/// not of our ability to open any channel at all. Thus, on error, we should first call this | ||
/// and see if we get a new `OpenChannel` message, otherwise the channel is failed. | ||
pub(crate) fn advance_channel_type_pref(&mut self, chain_hash: BlockHash) -> Result<msgs::OpenChannel, ()> { | ||
if !self.is_outbound() || self.channel_state != ChannelState::OurInitSent as u32 { return Err(()); } |
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.
Hmmm... this check may be more suitable at the call site. That way it is clear the error message is in response to an open_channel
message. Though, I suppose you wouldn't want this called when in a different state. Maybe it would simpler to inline the method in handle_error
?
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.
Hmmmm, I really prefer to keep as much channel state machine logic in channel.rs
as possible. In general we've failed at this increasingly, but if I ever find time I'm gonna try to push a chunk back down. channelmanager should just be for inter-channel stuff, never have any real knowledge of channel's state machine transitions, though it has to sometimes ask about the current state.
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.
Ah, I see - good point! I guess it seems that ChannelManager::handle_event
has state machine logic, though, because it assumes the error
message is a response to open_channel
. Would it make sense to add a handle_error
method to Channel
, which ChannelManager
could delegate to? That would in turn have this logic to determine whether advance_channel_type_pref
should be called or something else in the future depending on the channel state. I suppose it would need to return an event for ChannelManager
to enqueue instead of assuming it is SendOpenChannel
.
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.
because it assumes the error message is a response to open_channel.
Heh, that's why i put the state check in channel.rs
that way channelmanager
doesn't really know what the error is in response to, it just tries and lets channel figure it out. I'm gonna rename advance_channel_type_pref
to maybe_handle_error_without_close
to make it "feel" more generic.
Unless you feel strongly I'm not gonna bother changing its return type, though, currently all of the channel
/channelmanager
bounday has chanellmanger "know" what type of message is being sent, cause its enforced by the type-checker. I agree in the future we should move some of those to events, but that's also a larger refactor.
lightning/src/ln/features.rs
Outdated
@@ -388,6 +396,10 @@ mod sealed { | |||
define_feature!(45, ChannelType, [InitContext, NodeContext], | |||
"Feature flags for `option_channel_type`.", set_channel_type_optional, | |||
set_channel_type_required, supports_channel_type, requires_channel_type); | |||
define_feature!(47, SCIDAlias, [InitContext, NodeContext, ChannelTypeContext], |
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.
Is this subject to change? Seems you thought 50 was the actual desired feature bit: https://github.com/lightning/bolts/pull/910/files#r807405646
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.
Oh, yea, somewhat unclear, I guess we have to wait for this to get merged....
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.
To be clear, are we waiting on clarification on this?
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.
Oh, oops, sorry, no, so the 48/50 thing is the zero_conf feature, not the SCID feature, the SCID feature is consistently 46/47 everywhere in the current PR.
@@ -2514,6 +2524,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana | |||
Some(id) => id, | |||
}; | |||
|
|||
self.get_channel_update_for_onion(short_channel_id, chan) | |||
} | |||
fn get_channel_update_for_onion(&self, short_channel_id: u64, chan: &Channel<Signer>) -> Result<msgs::ChannelUpdate, LightningError> { |
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.
Seems this can't fail and can just return a ChannelUpdate
instead of Result
?
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.
Hmm, maybe we should check state before returning actually? I guess if the channel isn't live we shouldn't even be there, but in any cast it doesn't cost anything to return the Result, no? its just used in one place.
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.
My thinking is it's cleaner to not return a result if it's not necessary. Fine if the check is desired though
lightning/src/ln/channelmanager.rs
Outdated
let enc = if desired_err_code == 0x1000 | 20 { | ||
let mut res = Vec::new(); | ||
// TODO: underspecified, follow https://github.com/lightning/bolts/issues/791 | ||
res.extend_from_slice(&byte_utils::be16_to_array(0)); | ||
res.extend_from_slice(&upd.encode_with_len()); | ||
res | ||
} else { upd.encode_with_len() }; |
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.
Think this'd be slightly cleaner as:
let mut res = Vec::with_capacity(8 + 128);
// TODO: underspecified, follow https://github.com/lightningnetwork/lightning-rfc/issues/791
if error_code == 0x1000 | 20 {
res.extend_from_slice(&byte_utils::be16_to_array(0));
}
res.extend_from_slice(&upd.encode_with_len()[..]);
taken from 694ef1e
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 wanted to avoid the hard-coded length upper bound assumption, but I can use serialized_length
, too, I suppose.
@@ -4466,10 +4524,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana | |||
// channel_update here if the channel is not public, i.e. we're not sending an | |||
// announcement_signatures. | |||
log_trace!(self.logger, "Sending private initial channel_update for our counterparty on channel {}", log_bytes!(chan.get().channel_id())); |
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.
Maybe move the log to after Ok
, similar below
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.
Hmm, it should always succeed, and is kinda nice to always log - if we don't see the later generation logs in the get_channel_update* methods we know something is wrong.
df09831
to
0fec7e0
Compare
@@ -2514,6 +2524,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana | |||
Some(id) => id, | |||
}; | |||
|
|||
self.get_channel_update_for_onion(short_channel_id, chan) | |||
} | |||
fn get_channel_update_for_onion(&self, short_channel_id: u64, chan: &Channel<Signer>) -> Result<msgs::ChannelUpdate, LightningError> { |
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.
My thinking is it's cleaner to not return a result if it's not necessary. Fine if the check is desired though
lightning/src/ln/channel.rs
Outdated
/// If we receive an error message, it may only be a rejection of the channel type we tried, | ||
/// not of our ability to open any channel at all. Thus, on error, we should first call this | ||
/// and see if we get a new `OpenChannel` message, otherwise the channel is failed. | ||
pub(crate) fn advance_channel_type_pref(&mut self, chain_hash: BlockHash) -> Result<msgs::OpenChannel, ()> { |
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.
Feel like downgrade_channel_type
would be more accurate
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.
Hmm, is it strictly a "downgrade" always, though? I guess my mental model here was we have a list of channel types we are willing to use, and we're iterating through that list until we find one that works.
lightning/src/ln/channel.rs
Outdated
let mut allowed_type = ChannelTypeFeatures::only_static_remote_key(); | ||
if *channel_type != allowed_type { | ||
allowed_type.set_scid_alias_required(); |
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.
Given the deserialization issue for older versions of LDK, should we also refuse inbound channels with scid_alias_required
unless the config is set?
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.
Yea, I was trying to avoid adding an explicit config for it by letting users do a generic "check channel type flags" thing, though I admit its definitely forcing users to jump through quite a few hoops for something that they may want. It'll need calling out in the release notes either way, just don't know how much we want to complexify the config objects. We could also complexify them and remove the new option in the next release, but doing the manual accept hops isn't crazy either. I dunno.
c2e88e8
to
d0e8402
Compare
lightning/src/ln/features.rs
Outdated
@@ -388,6 +396,10 @@ mod sealed { | |||
define_feature!(45, ChannelType, [InitContext, NodeContext], | |||
"Feature flags for `option_channel_type`.", set_channel_type_optional, | |||
set_channel_type_required, supports_channel_type, requires_channel_type); | |||
define_feature!(47, SCIDAlias, [InitContext, NodeContext, ChannelTypeContext], |
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.
To be clear, are we waiting on clarification on this?
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.
Looks good, test coverage sounds correct.
lightning/src/ln/channelmanager.rs
Outdated
// Leave channel updates as None for private channels. | ||
let chan_update_opt = if chan.should_announce() { | ||
Some(self.get_channel_update_for_unicast(chan).unwrap()) } else { None }; | ||
let chan_update_opt = self.get_channel_update_for_broadcast(chan).ok(); |
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 you should write better the rational for why we're using get_channel_update_broadcast
instead of unicast. I didn't get the why at first read.
Something like "SCID alias aims to mask the real channel SCID from the payment sender. If we return the real channel SCID in a channel update error message, it would allow a payment sender to deanonymize the alias by triggering HTLC failures on the routing fees or HTLC minimum amount".
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.
Good catch, but this doesn't need a comment, this is just wrong!
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.
Also updated the test to hit this case.
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 thought if short_channel_id
is the real scid for a private channel, we still want to leave the update as None
?
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.
We do - the chan.get_channel_type().supports_scid_alias() && *short_channel_id != chan.outbound_scid_alias()
check below the chan_update_opt
get breaks with None
, dropping the chan_update_opt
. For clarity I moved the get_channel_update_for_onion
call down a few lines.
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.
Missing something here -- if we're attempting to forward over a private channel without an alias, and we break on e.g. if !chan.is_live()
10 lines down, won't we include the private channel update with the current code?
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.
Hmm, so the SCIDAlias
feature is supposed to be the "never, ever, ever tell anyone the real SCID" flag (maybe we should rename it scid_privacy in LDK?). I guess in principle we could treat any private channel as an SCIDPrivacy/SCIDAlias channel for the purposes of channel_update
generation, but I think eventually after people upgrade a private channel without the flag is rare/indicates some strange use-case where they're gonna use the real SCID for something.
if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels { | ||
// Note that the behavior here should be identical to the above block - we | ||
// should NOT reveal the existence or non-existence of a private channel if | ||
// we don't allow forwards outbound over them. | ||
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None)); | ||
break Some(("Refusing to forward to a private channel based on our config.", 0x4000 | 10, None)); |
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.
Likely before this PR, though not sure if there is test coverage for that.
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.
When I comment this line out I get failures in ln::priv_short_conf_tests::test_priv_forwarding_rejection
and ln::priv_short_conf_tests::test_scid_alias_on_pub_channel
d0e8402
to
34a3821
Compare
Left one question #1351 (comment) and CI's sad. I'm pretty much ACK otherwise |
34a3821
to
09e2174
Compare
(0x4000|10, Vec::new()) | ||
} | ||
} | ||
|
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.
nit: extra line
Test coverage looks good. Only outstanding comment is #1351 (comment). |
Hmm, that's confusing, I believe that is talking about it in the init flags, the part talking about channel types has no such indication - https://github.com/lightning/bolts/pull/910/files#diff-ed04ca2c673fd6aabde69389511fa9ee60cb44d6b2ef6c88b549ffaa753d6afeR216 |
SGTM |
7ff3033
to
40ba604
Compare
Squashed down the existing fixup commits, added some new ones to address Jeff's last comment and rename the scid_privacy feature. |
I'm ACK after Jeff's comments are addressed |
40ba604
to
b2314b8
Compare
Addressed all the comments, let me know if I should squash. |
b2314b8
to
3d4d0bd
Compare
Yes, please squash |
3d4d0bd
to
4d9a5c2
Compare
1.51 (and other earlier versions of `rustc`) appear to refuse to accept our documentation links due to a bogus failure to resolve `ChannelTypeFeatures::supports_scid_privacy`.
As we add new supported channel types, inbound channels which use new features may cause backwards-compatibility issues for clients. If a new channel is opened using new features while a client still wishes to ensure support for downgrading to a previous version of LDK, that new channel may cause the `ChannelManager` to fail deserialization due to unsupported feature flags. By exposing the channel type flags to the user in channel requests, users wishing to support downgrading to previous versions of LDK can reject channels which use channel features which previous versions of LDK do not understand.
This does not, however, ever send the scid_alias feature bit for outgoing channels, as that would cause the immediately prior version of LDK to be unable to read channel data.
Because negotiating `scid_alias` for all of our channels will cause us to create channels which LDK versions prior to 0.0.106 do not understand, we disable `scid_alias` negotiation by default.
This reduces unwraps in channelmanager by a good bit, providing robustness for the upcoming 0conf changes which allow SCIDs to be missing after a channel is in use, making `get_channel_update_for_unicast` more fallible. This also serves as a useful refactor for the next commit, consolidating the channel_update creation sites which are changed in the next commit.
When we fail an HTLC which was destined for a channel that the HTLC sender didn't know the real SCID for, we should ensure we continue to use the alias in the channel_update we provide them. Otherwise we will leak the channel's real SCID to HTLC senders.
There's not a lot of reason to keep it given its used in one place outside of tests, and this lets us clean up some of the byte_utils calls that are still lying around.
4d9a5c2
to
c47acd7
Compare
Pushed an additional commit at the start to bump the CI rustc version in |
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 c47acd7
(Good with the scid_privacy
renaming)
/// | ||
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager | ||
/// [`DecodeError:InvalidValue`]: crate::ln::msgs::DecodeError::InvalidValue | ||
pub negotiate_scid_privacy: bool, |
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.
nit: You can precise we only store for now the latest announced SCID alias by our counterparty. That's an implementation details that user might be interested to know (e.g if they would like to rotate scid alias for each invoice)
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.
Good point, but feel like that belongs on ChannelDetails::inbound_scid_alias
and ChannelDetails::get_inbound_payment_scid
Added an extra commit with more docs (the relevant code is already upstream). |
This is based on #1311 and implements the
SCIDAlias
channel type as well as ensures we don't leak "real" SCIDs via HTLC failures (as pointed out by Val at #1311 (comment))