Skip to content

Implement the SCIDAlias Channel Type and provide SCID Privacy #1351

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

Merged
merged 9 commits into from
Mar 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,7 @@ jobs:
check_commits:
runs-on: ubuntu-latest
env:
# rustc 1.53 regressed and panics when building our (perfectly fine) docs.
# See https://github.com/rust-lang/rust/issues/84738
TOOLCHAIN: 1.52.1
TOOLCHAIN: 1.57.0
steps:
- name: Checkout source code
uses: actions/checkout@v3
Expand Down
1 change: 1 addition & 0 deletions fuzz/src/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
forwarding_info: None,
},
funding_txo: Some(OutPoint { txid: bitcoin::Txid::from_slice(&[0; 32]).unwrap(), index: 0 }),
channel_type: None,
short_channel_id: Some(scid),
inbound_scid_alias: None,
channel_value_satoshis: slice_to_be64(get_slice!(8)),
Expand Down
55 changes: 46 additions & 9 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,31 @@ impl<Signer: Sign> Channel<Signer> {
self.channel_transaction_parameters.opt_anchors.is_some()
}

fn get_initial_channel_type(config: &UserConfig) -> ChannelTypeFeatures {
// The default channel type (ie the first one we try) depends on whether the channel is
// public - if it is, we just go with `only_static_remotekey` as it's the only option
// available. If it's private, we first try `scid_privacy` as it provides better privacy
// with no other changes, and fall back to `only_static_remotekey`
let mut ret = ChannelTypeFeatures::only_static_remote_key();
if !config.channel_options.announced_channel && config.own_channel_config.negotiate_scid_privacy {
ret.set_scid_privacy_required();
}
ret
}

/// 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 maybe_handle_error_without_close(&mut self, chain_hash: BlockHash) -> Result<msgs::OpenChannel, ()> {
if !self.is_outbound() || self.channel_state != ChannelState::OurInitSent as u32 { return Err(()); }
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

if self.channel_type == ChannelTypeFeatures::only_static_remote_key() {
// We've exhausted our options
return Err(());
}
self.channel_type = ChannelTypeFeatures::only_static_remote_key(); // We only currently support two types
Ok(self.get_open_channel(chain_hash))
}

// Constructors:
pub fn new_outbound<K: Deref, F: Deref>(
fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures,
Expand Down Expand Up @@ -967,10 +992,7 @@ impl<Signer: Sign> Channel<Signer> {
#[cfg(any(test, fuzzing))]
historical_inbound_htlc_fulfills: HashSet::new(),

// We currently only actually support one channel type, so don't retry with new types
// on error messages. When we support more we'll need fallback support (assuming we
// want to support old types).
channel_type: ChannelTypeFeatures::only_static_remote_key(),
channel_type: Self::get_initial_channel_type(&config),
})
}

Expand Down Expand Up @@ -1009,15 +1031,26 @@ impl<Signer: Sign> Channel<Signer> {
L::Target: Logger,
{
let opt_anchors = false; // TODO - should be based on features
let announced_channel = if (msg.channel_flags & 1) == 1 { true } else { false };

// First check the channel type is known, failing before we do anything else if we don't
// support this channel type.
let channel_type = if let Some(channel_type) = &msg.channel_type {
if channel_type.supports_any_optional_bits() {
return Err(ChannelError::Close("Channel Type field contained optional bits - this is not allowed".to_owned()));
}
if *channel_type != ChannelTypeFeatures::only_static_remote_key() {
return Err(ChannelError::Close("Channel Type was not understood".to_owned()));
// We currently only allow two channel types, so write it all out here - we allow
// `only_static_remote_key` in all contexts, and further allow
// `static_remote_key|scid_privacy` if the channel is not publicly announced.
let mut allowed_type = ChannelTypeFeatures::only_static_remote_key();
if *channel_type != allowed_type {
allowed_type.set_scid_privacy_required();
if *channel_type != allowed_type {
return Err(ChannelError::Close("Channel Type was not understood".to_owned()));
}
if announced_channel {
return Err(ChannelError::Close("SCID Alias/Privacy Channel Type cannot be set on a public channel".to_owned()));
}
}
channel_type.clone()
} else {
Expand Down Expand Up @@ -1098,14 +1131,13 @@ impl<Signer: Sign> Channel<Signer> {

// Convert things into internal flags and prep our state:

let announce = if (msg.channel_flags & 1) == 1 { true } else { false };
if config.peer_channel_config_limits.force_announced_channel_preference {
if local_config.announced_channel != announce {
if local_config.announced_channel != announced_channel {
return Err(ChannelError::Close("Peer tried to open channel but their announcement preference is different from ours".to_owned()));
}
}
// we either accept their preference or the preferences match
local_config.announced_channel = announce;
local_config.announced_channel = announced_channel;

let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(msg.funding_satoshis);
if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
Expand Down Expand Up @@ -4232,6 +4264,11 @@ impl<Signer: Sign> Channel<Signer> {
self.user_id
}

/// Gets the channel's type
pub fn get_channel_type(&self) -> &ChannelTypeFeatures {
&self.channel_type
}

/// Guaranteed to be Some after both FundingLocked messages have been exchanged (and, thus,
/// is_usable() returns true).
/// Allowed in any state (including after shutdown)
Expand Down
Loading