Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support ZeroConf channel type. #1505

Merged
merged 1 commit into from
Jun 2, 2022
Merged

Support ZeroConf channel type. #1505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 40 additions & 9 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,16 +1081,22 @@ impl<Signer: Sign> Channel<Signer> {
if channel_type.supports_any_optional_bits() {
return Err(ChannelError::Close("Channel Type field contained optional bits - this is not allowed".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 {

if channel_type.requires_unknown_bits() {
return Err(ChannelError::Close("Channel Type field contains unknown bits".to_owned()));
}

// We currently only allow four channel types, so write it all out here - we allow
// `only_static_remote_key` or `static_remote_key | zero_conf` in all contexts, and
// further allow `static_remote_key | scid_privacy` or
// `static_remote_key | scid_privacy | zero_conf`, if the channel is not
// publicly announced.
if *channel_type != ChannelTypeFeatures::only_static_remote_key() {
if !channel_type.requires_scid_privacy() && !channel_type.requires_zero_conf() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wont this change imply we accept a channel_type that has some other unknown bit set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, right. Fixed this with 7f8bab5.

return Err(ChannelError::Close("Channel Type was not understood".to_owned()));
}
if announced_channel {

if channel_type.requires_scid_privacy() && announced_channel {
return Err(ChannelError::Close("SCID Alias/Privacy Channel Type cannot be set on a public channel".to_owned()));
}
}
Expand Down Expand Up @@ -6407,7 +6413,7 @@ mod tests {
use ln::channelmanager::{HTLCSource, PaymentId};
use ln::channel::{Channel, InboundHTLCOutput, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator};
use ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS};
use ln::features::InitFeatures;
use ln::features::{InitFeatures, ChannelTypeFeatures};
use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
use ln::script::ShutdownScript;
use ln::chan_utils;
Expand Down Expand Up @@ -7722,4 +7728,29 @@ mod tests {
assert_eq!(chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_secret, &base_secret).unwrap(),
SecretKey::from_slice(&hex::decode("d09ffff62ddb2297ab000cc85bcb4283fdeb6aa052affbc9dddcf33b61078110").unwrap()[..]).unwrap());
}

#[test]
fn test_zero_conf_channel_type_support() {
let feeest = TestFeeEstimator{fee_est: 15000};
let secp_ctx = Secp256k1::new();
let seed = [42; 32];
let network = Network::Testnet;
let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
let logger = test_utils::TestLogger::new();

let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
let config = UserConfig::default();
let node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider,
node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();

let mut channel_type_features = ChannelTypeFeatures::only_static_remote_key();
channel_type_features.set_zero_conf_required();

let mut open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
open_channel_msg.channel_type = Some(channel_type_features);
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
let res = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider,
node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger, 42);
assert!(res.is_ok());
}
}
22 changes: 21 additions & 1 deletion lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4159,6 +4159,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
/// [`Event::ChannelClosed::user_channel_id`] to allow tracking of which events correspond
/// with which `accept_inbound_channel`/`accept_inbound_channel_from_trusted_peer_0conf` call.
///
/// Note that this method will return an error and reject the channel, if it requires support
/// for zero confirmations. Instead, `accept_inbound_channel_from_trusted_peer_0conf` must be
/// used to accept such channels.
///
/// [`Event::OpenChannelRequest`]: events::Event::OpenChannelRequest
/// [`Event::ChannelClosed::user_channel_id`]: events::Event::ChannelClosed::user_channel_id
pub fn accept_inbound_channel(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, user_channel_id: u64) -> Result<(), APIError> {
Expand Down Expand Up @@ -4200,7 +4204,20 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
if *counterparty_node_id != channel.get().get_counterparty_node_id() {
return Err(APIError::APIMisuseError { err: "The passed counterparty_node_id doesn't match the channel's counterparty node_id".to_owned() });
}
if accept_0conf { channel.get_mut().set_0conf(); }
if accept_0conf {
channel.get_mut().set_0conf();
} else if channel.get().get_channel_type().requires_zero_conf() {
let send_msg_err_event = events::MessageSendEvent::HandleError {
node_id: channel.get().get_counterparty_node_id(),
action: msgs::ErrorAction::SendErrorMessage{
msg: msgs::ErrorMessage { channel_id: temporary_channel_id.clone(), data: "No zero confirmation channels accepted".to_owned(), }
}
};
channel_state.pending_msg_events.push(send_msg_err_event);
let _ = remove_channel!(self, channel_state, channel);
return Err(APIError::APIMisuseError { err: "Please use accept_inbound_channel_from_trusted_peer_0conf to accept channels with zero confirmations.".to_owned() });
}

channel_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel {
node_id: channel.get().get_counterparty_node_id(),
msg: channel.get_mut().accept_inbound_channel(user_channel_id),
Expand Down Expand Up @@ -4242,6 +4259,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
},
hash_map::Entry::Vacant(entry) => {
if !self.default_configuration.manually_accept_inbound_channels {
if channel.get_channel_type().requires_zero_conf() {
return Err(MsgHandleErrInternal::send_err_msg_no_close("No zero confirmation channels accepted".to_owned(), msg.temporary_channel_id.clone()));
}
channel_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel {
node_id: counterparty_node_id.clone(),
msg: channel.accept_inbound_channel(0),
Expand Down
27 changes: 24 additions & 3 deletions lightning/src/ln/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ mod sealed {
,
// Byte 5
,
// Byte 6
,
],
optional_features: [
// Byte 0
Expand All @@ -144,6 +146,8 @@ mod sealed {
,
// Byte 5
ChannelType | SCIDPrivacy,
// Byte 6
ZeroConf,
],
});
define_context!(NodeContext {
Expand Down Expand Up @@ -177,7 +181,7 @@ mod sealed {
// Byte 5
ChannelType | SCIDPrivacy,
// Byte 6
Keysend,
ZeroConf | Keysend,
],
});
define_context!(ChannelContext {
Expand Down Expand Up @@ -218,6 +222,8 @@ mod sealed {
,
// Byte 5
SCIDPrivacy,
// Byte 6
ZeroConf,
],
optional_features: [
// Byte 0
Expand All @@ -232,6 +238,8 @@ mod sealed {
,
// Byte 5
,
// Byte 6
,
],
});

Expand Down Expand Up @@ -402,7 +410,9 @@ mod sealed {
define_feature!(47, SCIDPrivacy, [InitContext, NodeContext, ChannelTypeContext],
"Feature flags for only forwarding with SCID aliasing. Called `option_scid_alias` in the BOLTs",
set_scid_privacy_optional, set_scid_privacy_required, supports_scid_privacy, requires_scid_privacy);

define_feature!(51, ZeroConf, [InitContext, NodeContext, ChannelTypeContext],
"Feature flags for accepting channels with zero confirmations. Called `option_zeroconf` in the BOLTs",
set_zero_conf_optional, set_zero_conf_required, supports_zero_conf, requires_zero_conf);
define_feature!(55, Keysend, [NodeContext],
"Feature flags for keysend payments.", set_keysend_optional, set_keysend_required,
supports_keysend, requires_keysend);
Expand Down Expand Up @@ -852,14 +862,23 @@ mod tests {

assert!(InitFeatures::known().supports_scid_privacy());
assert!(NodeFeatures::known().supports_scid_privacy());
assert!(ChannelTypeFeatures::known().supports_scid_privacy());
assert!(!InitFeatures::known().requires_scid_privacy());
assert!(!NodeFeatures::known().requires_scid_privacy());
assert!(ChannelTypeFeatures::known().requires_scid_privacy());

assert!(InitFeatures::known().supports_wumbo());
assert!(NodeFeatures::known().supports_wumbo());
assert!(!InitFeatures::known().requires_wumbo());
assert!(!NodeFeatures::known().requires_wumbo());

assert!(InitFeatures::known().supports_zero_conf());
assert!(!InitFeatures::known().requires_zero_conf());
assert!(NodeFeatures::known().supports_zero_conf());
assert!(!NodeFeatures::known().requires_zero_conf());
assert!(ChannelTypeFeatures::known().supports_zero_conf());
assert!(ChannelTypeFeatures::known().requires_zero_conf());

let mut init_features = InitFeatures::known();
assert!(init_features.initial_routing_sync());
init_features.clear_initial_routing_sync();
Expand Down Expand Up @@ -899,13 +918,15 @@ mod tests {
// - opt_shutdown_anysegwit
// -
// - option_channel_type | option_scid_alias
assert_eq!(node_features.flags.len(), 6);
// - option_zeroconf
assert_eq!(node_features.flags.len(), 7);
assert_eq!(node_features.flags[0], 0b00000010);
assert_eq!(node_features.flags[1], 0b01010001);
assert_eq!(node_features.flags[2], 0b00001010);
assert_eq!(node_features.flags[3], 0b00001000);
assert_eq!(node_features.flags[4], 0b00000000);
assert_eq!(node_features.flags[5], 0b10100000);
assert_eq!(node_features.flags[6], 0b00001000);
}

// Check that cleared flags are kept blank when converting back:
Expand Down
103 changes: 101 additions & 2 deletions lightning/src/ln/priv_short_conf_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ use chain::keysinterface::{Recipient, KeysInterface};
use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, MIN_CLTV_EXPIRY_DELTA};
use routing::network_graph::RoutingFees;
use routing::router::{PaymentParameters, RouteHint, RouteHintHop};
use ln::features::{InitFeatures, InvoiceFeatures};
use ln::features::{InitFeatures, InvoiceFeatures, ChannelTypeFeatures};
use ln::msgs;
use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, OptionalField, ChannelUpdate};
use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, OptionalField, ChannelUpdate, ErrorAction};
use ln::wire::Encode;
use util::enforcing_trait_impls::EnforcingSigner;
use util::events::{ClosureReason, Event, MessageSendEvent, MessageSendEventsProvider};
Expand Down Expand Up @@ -922,3 +922,102 @@ fn test_0conf_channel_reorg() {
});
check_closed_broadcast!(nodes[1], true);
}

#[test]
fn test_zero_conf_accept_reject() {
let mut channel_type_features = ChannelTypeFeatures::only_static_remote_key();
channel_type_features.set_zero_conf_required();

// 1. Check we reject zero conf channels by default
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap();
let mut open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());

open_channel_msg.channel_type = Some(channel_type_features.clone());

nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_channel_msg);

let msg_events = nodes[1].node.get_and_clear_pending_msg_events();
match msg_events[0] {
MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg, .. }, .. } => {
assert_eq!(msg.data, "No zero confirmation channels accepted".to_owned());
},
_ => panic!(),
}

// 2. Check we can manually accept zero conf channels via the right method
let mut manually_accept_conf = UserConfig::default();
manually_accept_conf.manually_accept_inbound_channels = true;

let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs,
&[None, Some(manually_accept_conf.clone())]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

// 2.1 First try the non-0conf method to manually accept
nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42,
Some(manually_accept_conf)).unwrap();
let mut open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel,
nodes[1].node.get_our_node_id());

open_channel_msg.channel_type = Some(channel_type_features.clone());

nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(),
&open_channel_msg);

// Assert that `nodes[1]` has no `MessageSendEvent::SendAcceptChannel` in the `msg_events`.
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

let events = nodes[1].node.get_and_clear_pending_events();

match events[0] {
Event::OpenChannelRequest { temporary_channel_id, .. } => {
// Assert we fail to accept via the non-0conf method
assert!(nodes[1].node.accept_inbound_channel(&temporary_channel_id,
&nodes[0].node.get_our_node_id(), 0).is_err());
},
_ => panic!(),
}

let msg_events = nodes[1].node.get_and_clear_pending_msg_events();
match msg_events[0] {
MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg, .. }, .. } => {
assert_eq!(msg.data, "No zero confirmation channels accepted".to_owned());
},
_ => panic!(),
}

// 2.2 Try again with the 0conf method to manually accept
nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42,
Some(manually_accept_conf)).unwrap();
let mut open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel,
nodes[1].node.get_our_node_id());

open_channel_msg.channel_type = Some(channel_type_features);

nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(),
&open_channel_msg);

let events = nodes[1].node.get_and_clear_pending_events();

match events[0] {
Event::OpenChannelRequest { temporary_channel_id, .. } => {
// Assert we can accept via the 0conf method
assert!(nodes[1].node.accept_inbound_channel_from_trusted_peer_0conf(
&temporary_channel_id, &nodes[0].node.get_our_node_id(), 0).is_ok());
},
_ => panic!(),
}

// Check we would send accept
let msg_events = nodes[1].node.get_and_clear_pending_msg_events();
match msg_events[0] {
MessageSendEvent::SendAcceptChannel { .. } => {},
_ => panic!(),
}
}
9 changes: 8 additions & 1 deletion lightning/src/util/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,8 @@ pub enum Event {
/// transaction.
claim_from_onchain_tx: bool,
},
/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
/// Used to indicate that a previously opened channel with the given `channel_id` is in the
/// process of closure.
ChannelClosed {
/// The channel_id of the channel which has been closed. Note that on-chain transactions
/// resolving the channel are likely still awaiting confirmation.
Expand Down Expand Up @@ -466,6 +467,12 @@ pub enum Event {
/// the resulting [`ChannelManager`] will not be readable by versions of LDK prior to
/// 0.0.106.
///
/// Furthermore, note that if [`ChannelTypeFeatures::supports_zero_conf`] returns true on this type,
/// the resulting [`ChannelManager`] will not be readable by versions of LDK prior to
/// 0.0.107. Channels setting this type also need to get manually accepted via
/// [`crate::ln::channelmanager::ChannelManager::accept_inbound_channel_from_trusted_peer_0conf`],
/// or will be rejected otherwise.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so its not technically "rejected otherwise" the accept_inbound_channel call fails, and the channel is still there, which kinda sucks. Maybe we just force-close automatically if you call accept_inbound_channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, if the user calls the wrong method and the channel keeps 'lingering' in our channel_state, we would basically have the same situation as before he called. I'm not sure how this would be different from the 'normal' operation of lingering incoming channels when we set manually_accept_channels?
If we want to get rid of such stale request, we might generally need some kind of garbage collection for the state?

Also not entirely sure what 'force-close' would mean in this context? We'd probably just send an error over the wire and remove the channel from channel_state?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, if the user calls the wrong method and the channel keeps 'lingering' in our channel_state, we would basically have the same situation as before he called. I'm not sure how this would be different from the 'normal' operation of lingering incoming channels when we set manually_accept_channels?

Correct, though its pretty different in that the user handled the OpenChannelRequest event, they don't expect to have to handle it twice. In general, our users aren't going to manually handle an error - if we return an APIError here, the users' code probably just won't do anything else. The counterparty, however, may be waiting for an error to retry the channel open (this is the expected behavior for a channel_type "negotiation").

We can (generally) assume that users will try to handle OpenChannelRequest, my concern her is that they won't handle an error handling it.

If we want to get rid of such stale request, we might generally need some kind of garbage collection for the state?

Agreed - in general we should eventually time-out requests, but such requests may take some time to process (external API calls to third-party providers are not unlikely, I believe), so we have to be kinda conservative here, likely even too conservative for moving the channel_type negotiation forward.

Also not entirely sure what 'force-close' would mean in this context? We'd probably just send an error over the wire and remove the channel from channel_state?

Yep, that. I generally use "force-close" to mean "send error message and close channel", though, indeed, there's no broadcast associated here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I now reject the channel in accept_inbound_channel if it is 0conf. Also added a note explaining this behavior.

///
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
channel_type: ChannelTypeFeatures,
},
Expand Down