Skip to content

Randomize user_channel_id for inbound channels #1790

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

Merged
merged 3 commits into from
Nov 15, 2022
Merged

Randomize user_channel_id for inbound channels #1790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa10f64
Introduce `init_and_read_tlv_fields` macro
tnull Nov 15, 2022
b2f8df0
Randomize `user_channel_id` for inbound channels
tnull Oct 21, 2022
dc3ff54
Make `user_channel_id` a `u128`
tnull Oct 24, 2022
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
2 changes: 1 addition & 1 deletion fuzz/src/full_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
// Adding new calls to `KeysInterface::get_secure_random_bytes` during startup can change all the
// keys subsequently generated in this test. Rather than regenerating all the messages manually,
// it's easier to just increment the counter here so the keys don't change.
keys_manager.counter.fetch_sub(2, Ordering::AcqRel);
keys_manager.counter.fetch_sub(3, Ordering::AcqRel);
let our_id = PublicKey::from_secret_key(&Secp256k1::signing_only(), &keys_manager.get_node_secret(Recipient::Node).unwrap());
let network_graph = Arc::new(NetworkGraph::new(genesis_block(network).block_hash(), Arc::clone(&logger)));
let gossip_sync = Arc::new(P2PGossipSync::new(Arc::clone(&network_graph), None, Arc::clone(&logger)));
Expand Down
35 changes: 28 additions & 7 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ pub(super) struct Channel<Signer: Sign> {

inbound_handshake_limits_override: Option<ChannelHandshakeLimits>,

user_id: u64,
user_id: u128,

channel_id: [u8; 32],
channel_state: u32,
Expand Down Expand Up @@ -902,7 +902,7 @@ impl<Signer: Sign> Channel<Signer> {
// Constructors:
pub fn new_outbound<K: Deref, F: Deref>(
fee_estimator: &LowerBoundedFeeEstimator<F>, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures,
channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig, current_chain_height: u32,
channel_value_satoshis: u64, push_msat: u64, user_id: u128, config: &UserConfig, current_chain_height: u32,
outbound_scid_alias: u64
) -> Result<Channel<Signer>, APIError>
where K::Target: KeysInterface<Signer = Signer>,
Expand Down Expand Up @@ -1102,7 +1102,7 @@ impl<Signer: Sign> Channel<Signer> {
/// Assumes chain_hash has already been checked and corresponds with what we expect!
pub fn new_from_req<K: Deref, F: Deref, L: Deref>(
fee_estimator: &LowerBoundedFeeEstimator<F>, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures,
msg: &msgs::OpenChannel, user_id: u64, config: &UserConfig, current_chain_height: u32, logger: &L,
msg: &msgs::OpenChannel, user_id: u128, config: &UserConfig, current_chain_height: u32, logger: &L,
outbound_scid_alias: u64
) -> Result<Channel<Signer>, ChannelError>
where K::Target: KeysInterface<Signer = Signer>,
Expand Down Expand Up @@ -4482,7 +4482,7 @@ impl<Signer: Sign> Channel<Signer> {

/// Gets the "user_id" value passed into the construction of this channel. It has no special
/// meaning and exists only to allow users to have a persistent identifier of a channel.
pub fn get_user_id(&self) -> u64 {
pub fn get_user_id(&self) -> u128 {
self.user_id
}

Expand Down Expand Up @@ -5173,7 +5173,7 @@ impl<Signer: Sign> Channel<Signer> {
/// should be sent back to the counterparty node.
///
/// [`msgs::AcceptChannel`]: crate::ln::msgs::AcceptChannel
pub fn accept_inbound_channel(&mut self, user_id: u64) -> msgs::AcceptChannel {
pub fn accept_inbound_channel(&mut self, user_id: u128) -> msgs::AcceptChannel {
if self.is_outbound() {
panic!("Tried to send accept_channel for an outbound channel?");
}
Expand Down Expand Up @@ -6002,7 +6002,11 @@ impl<Signer: Sign> Writeable for Channel<Signer> {

write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);

self.user_id.write(writer)?;
// `user_id` used to be a single u64 value. In order to remain backwards compatible with
// versions prior to 0.0.113, the u128 is serialized as two separate u64 values. We write
// the low bytes now and the optional high bytes later.
let user_id_low = self.user_id as u64;
user_id_low.write(writer)?;

// Version 1 deserializers expected to read parts of the config object here. Version 2
// deserializers (0.0.99) now read config through TLVs, and as we now require them for
Expand Down Expand Up @@ -6249,6 +6253,11 @@ impl<Signer: Sign> Writeable for Channel<Signer> {

let channel_ready_event_emitted = Some(self.channel_ready_event_emitted);

// `user_id` used to be a single u64 value. In order to remain backwards compatible with
// versions prior to 0.0.113, the u128 is serialized as two separate u64 values. Therefore,
// we write the high bytes as an option here.
let user_id_high_opt = Some((self.user_id >> 64) as u64);

write_tlv_fields!(writer, {
(0, self.announcement_sigs, option),
// minimum_depth and counterparty_selected_channel_reserve_satoshis used to have a
Expand All @@ -6272,6 +6281,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
(19, self.latest_inbound_scid_alias, option),
(21, self.outbound_scid_alias, required),
(23, channel_ready_event_emitted, option),
(25, user_id_high_opt, option),
});

Ok(())
Expand All @@ -6285,7 +6295,10 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
let (keys_source, serialized_height) = args;
let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);

let user_id = Readable::read(reader)?;
// `user_id` used to be a single u64 value. In order to remain backwards compatible with
// versions prior to 0.0.113, the u128 is serialized as two separate u64 values. We read
// the low bytes now and the high bytes later.
let user_id_low: u64 = Readable::read(reader)?;

let mut config = Some(LegacyChannelConfig::default());
if ver == 1 {
Expand Down Expand Up @@ -6531,6 +6544,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
let mut outbound_scid_alias = None;
let mut channel_ready_event_emitted = None;

let mut user_id_high_opt: Option<u64> = None;

read_tlv_fields!(reader, {
(0, announcement_sigs, option),
(1, minimum_depth, option),
Expand All @@ -6548,6 +6563,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
(19, latest_inbound_scid_alias, option),
(21, outbound_scid_alias, option),
(23, channel_ready_event_emitted, option),
(25, user_id_high_opt, option),
});

if let Some(preimages) = preimages_opt {
Expand Down Expand Up @@ -6584,6 +6600,11 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
let mut secp_ctx = Secp256k1::new();
secp_ctx.seeded_randomize(&keys_source.get_secure_random_bytes());

// `user_id` used to be a single u64 value. In order to remain backwards
// compatible with versions prior to 0.0.113, the u128 is serialized as two
// separate u64 values.
let user_id = user_id_low as u128 + ((user_id_high_opt.unwrap_or(0) as u128) << 64);

Ok(Channel {
user_id,

Expand Down
159 changes: 119 additions & 40 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ type ShutdownResult = (Option<(OutPoint, ChannelMonitorUpdate)>, Vec<(HTLCSource

struct MsgHandleErrInternal {
err: msgs::LightningError,
chan_id: Option<([u8; 32], u64)>, // If Some a channel of ours has been closed
chan_id: Option<([u8; 32], u128)>, // If Some a channel of ours has been closed
shutdown_finish: Option<(ShutdownResult, Option<msgs::ChannelUpdate>)>,
}
impl MsgHandleErrInternal {
Expand Down Expand Up @@ -325,7 +325,7 @@ impl MsgHandleErrInternal {
Self { err, chan_id: None, shutdown_finish: None }
}
#[inline]
fn from_finish_shutdown(err: String, channel_id: [u8; 32], user_channel_id: u64, shutdown_res: ShutdownResult, channel_update: Option<msgs::ChannelUpdate>) -> Self {
fn from_finish_shutdown(err: String, channel_id: [u8; 32], user_channel_id: u128, shutdown_res: ShutdownResult, channel_update: Option<msgs::ChannelUpdate>) -> Self {
Self {
err: LightningError {
err: err.clone(),
Expand Down Expand Up @@ -1083,8 +1083,9 @@ pub struct ChannelDetails {
///
/// [`outbound_capacity_msat`]: ChannelDetails::outbound_capacity_msat
pub unspendable_punishment_reserve: Option<u64>,
/// The `user_channel_id` passed in to create_channel, or 0 if the channel was inbound.
pub user_channel_id: u64,
/// The `user_channel_id` passed in to create_channel, or a random value if the channel was
/// inbound.
pub user_channel_id: u128,
/// Our total balance. This is the amount we would get if we close the channel.
/// This value is not exact. Due to various in-flight changes and feerate changes, exactly this
/// amount is not likely to be recoverable on close.
Expand Down Expand Up @@ -1719,10 +1720,9 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
///
/// `user_channel_id` will be provided back as in
/// [`Event::FundingGenerationReady::user_channel_id`] to allow tracking of which events
/// correspond with which `create_channel` call. Note that the `user_channel_id` defaults to 0
/// for inbound channels, so you may wish to avoid using 0 for `user_channel_id` here.
/// `user_channel_id` has no meaning inside of LDK, it is simply copied to events and otherwise
/// ignored.
/// correspond with which `create_channel` call. Note that the `user_channel_id` defaults to a
/// randomized value for inbound channels. `user_channel_id` has no meaning inside of LDK, it
/// is simply copied to events and otherwise ignored.
///
/// Raises [`APIError::APIMisuseError`] when `channel_value_satoshis` > 2**24 or `push_msat` is
/// greater than `channel_value_satoshis * 1k` or `channel_value_satoshis < 1000`.
Expand All @@ -1741,7 +1741,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
/// [`Event::FundingGenerationReady::user_channel_id`]: events::Event::FundingGenerationReady::user_channel_id
/// [`Event::FundingGenerationReady::temporary_channel_id`]: events::Event::FundingGenerationReady::temporary_channel_id
/// [`Event::ChannelClosed::channel_id`]: events::Event::ChannelClosed::channel_id
pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_channel_id: u64, override_config: Option<UserConfig>) -> Result<[u8; 32], APIError> {
pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_channel_id: u128, override_config: Option<UserConfig>) -> Result<[u8; 32], APIError> {
if channel_value_satoshis < 1000 {
return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
}
Expand Down Expand Up @@ -4530,7 +4530,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
///
/// [`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> {
pub fn accept_inbound_channel(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, user_channel_id: u128) -> Result<(), APIError> {
self.do_accept_inbound_channel(temporary_channel_id, counterparty_node_id, false, user_channel_id)
}

Expand All @@ -4552,11 +4552,11 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
///
/// [`Event::OpenChannelRequest`]: events::Event::OpenChannelRequest
/// [`Event::ChannelClosed::user_channel_id`]: events::Event::ChannelClosed::user_channel_id
pub fn accept_inbound_channel_from_trusted_peer_0conf(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, user_channel_id: u64) -> Result<(), APIError> {
pub fn accept_inbound_channel_from_trusted_peer_0conf(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, user_channel_id: u128) -> Result<(), APIError> {
self.do_accept_inbound_channel(temporary_channel_id, counterparty_node_id, true, user_channel_id)
}

fn do_accept_inbound_channel(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, accept_0conf: bool, user_channel_id: u64) -> Result<(), APIError> {
fn do_accept_inbound_channel(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, accept_0conf: bool, user_channel_id: u128) -> Result<(), APIError> {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);

let mut channel_state_lock = self.channel_state.lock().unwrap();
Expand Down Expand Up @@ -4604,9 +4604,13 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
return Err(MsgHandleErrInternal::send_err_msg_no_close("No inbound channels accepted".to_owned(), msg.temporary_channel_id.clone()));
}

let mut random_bytes = [0u8; 16];
random_bytes.copy_from_slice(&self.keys_manager.get_secure_random_bytes()[..16]);
let user_channel_id = u128::from_be_bytes(random_bytes);

let outbound_scid_alias = self.create_and_insert_outbound_scid_alias();
let mut channel = match Channel::new_from_req(&self.fee_estimator, &self.keys_manager,
counterparty_node_id.clone(), &their_features, msg, 0, &self.default_configuration,
counterparty_node_id.clone(), &their_features, msg, user_channel_id, &self.default_configuration,
self.best_block.read().unwrap().height(), &self.logger, outbound_scid_alias)
{
Err(e) => {
Expand Down Expand Up @@ -6404,33 +6408,108 @@ impl_writeable_tlv_based!(ChannelCounterparty, {
(11, outbound_htlc_maximum_msat, option),
});

impl_writeable_tlv_based!(ChannelDetails, {
(1, inbound_scid_alias, option),
(2, channel_id, required),
(3, channel_type, option),
(4, counterparty, required),
(5, outbound_scid_alias, option),
(6, funding_txo, option),
(7, config, option),
(8, short_channel_id, option),
(10, channel_value_satoshis, required),
(12, unspendable_punishment_reserve, option),
(14, user_channel_id, required),
(16, balance_msat, required),
(18, outbound_capacity_msat, required),
// Note that by the time we get past the required read above, outbound_capacity_msat will be
// filled in, so we can safely unwrap it here.
(19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)),
(20, inbound_capacity_msat, required),
(22, confirmations_required, option),
(24, force_close_spend_delay, option),
(26, is_outbound, required),
(28, is_channel_ready, required),
(30, is_usable, required),
(32, is_public, required),
(33, inbound_htlc_minimum_msat, option),
(35, inbound_htlc_maximum_msat, option),
});
impl Writeable for ChannelDetails {
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
// `user_channel_id` used to be a single u64 value. In order to remain backwards compatible with
// versions prior to 0.0.113, the u128 is serialized as two separate u64 values.
let user_channel_id_low = self.user_channel_id as u64;
let user_channel_id_high_opt = Some((self.user_channel_id >> 64) as u64);
write_tlv_fields!(writer, {
(1, self.inbound_scid_alias, option),
(2, self.channel_id, required),
(3, self.channel_type, option),
(4, self.counterparty, required),
(5, self.outbound_scid_alias, option),
(6, self.funding_txo, option),
(7, self.config, option),
(8, self.short_channel_id, option),
(10, self.channel_value_satoshis, required),
(12, self.unspendable_punishment_reserve, option),
(14, user_channel_id_low, required),
(16, self.balance_msat, required),
(18, self.outbound_capacity_msat, required),
// Note that by the time we get past the required read above, outbound_capacity_msat will be
// filled in, so we can safely unwrap it here.
(19, self.next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)),
(20, self.inbound_capacity_msat, required),
(22, self.confirmations_required, option),
(24, self.force_close_spend_delay, option),
(26, self.is_outbound, required),
(28, self.is_channel_ready, required),
(30, self.is_usable, required),
(32, self.is_public, required),
(33, self.inbound_htlc_minimum_msat, option),
(35, self.inbound_htlc_maximum_msat, option),
(37, user_channel_id_high_opt, option),
});
Ok(())
}
}

impl Readable for ChannelDetails {
fn read<R: Read>(reader: &mut R) -> Result<Self, DecodeError> {
init_and_read_tlv_fields!(reader, {
(1, inbound_scid_alias, option),
(2, channel_id, required),
(3, channel_type, option),
(4, counterparty, required),
(5, outbound_scid_alias, option),
(6, funding_txo, option),
(7, config, option),
(8, short_channel_id, option),
(10, channel_value_satoshis, required),
(12, unspendable_punishment_reserve, option),
(14, user_channel_id_low, required),
(16, balance_msat, required),
(18, outbound_capacity_msat, required),
// Note that by the time we get past the required read above, outbound_capacity_msat will be
// filled in, so we can safely unwrap it here.
(19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)),
(20, inbound_capacity_msat, required),
(22, confirmations_required, option),
(24, force_close_spend_delay, option),
(26, is_outbound, required),
(28, is_channel_ready, required),
(30, is_usable, required),
(32, is_public, required),
(33, inbound_htlc_minimum_msat, option),
(35, inbound_htlc_maximum_msat, option),
(37, user_channel_id_high_opt, option),
});

// `user_channel_id` used to be a single u64 value. In order to remain backwards compatible with
// versions prior to 0.0.113, the u128 is serialized as two separate u64 values.
let user_channel_id_low: u64 = user_channel_id_low.0.unwrap();
let user_channel_id = user_channel_id_low as u128 +
((user_channel_id_high_opt.unwrap_or(0 as u64) as u128) << 64);

Ok(Self {
inbound_scid_alias,
channel_id: channel_id.0.unwrap(),
channel_type,
counterparty: counterparty.0.unwrap(),
outbound_scid_alias,
funding_txo,
config,
short_channel_id,
channel_value_satoshis: channel_value_satoshis.0.unwrap(),
unspendable_punishment_reserve,
user_channel_id,
balance_msat: balance_msat.0.unwrap(),
outbound_capacity_msat: outbound_capacity_msat.0.unwrap(),
next_outbound_htlc_limit_msat: next_outbound_htlc_limit_msat.0.unwrap(),
inbound_capacity_msat: inbound_capacity_msat.0.unwrap(),
confirmations_required,
force_close_spend_delay,
is_outbound: is_outbound.0.unwrap(),
is_channel_ready: is_channel_ready.0.unwrap(),
is_usable: is_usable.0.unwrap(),
is_public: is_public.0.unwrap(),
inbound_htlc_minimum_msat,
inbound_htlc_maximum_msat,
})
}
}

impl_writeable_tlv_based!(PhantomRouteHints, {
(2, channels, vec_type),
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ macro_rules! check_added_monitors {
}
}

pub fn create_funding_transaction<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, expected_counterparty_node_id: &PublicKey, expected_chan_value: u64, expected_user_chan_id: u64) -> ([u8; 32], Transaction, OutPoint) {
pub fn create_funding_transaction<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, expected_counterparty_node_id: &PublicKey, expected_chan_value: u64, expected_user_chan_id: u128) -> ([u8; 32], Transaction, OutPoint) {
let chan_id = *node.network_chan_count.borrow();

let events = node.node.get_and_clear_pending_events();
Expand Down
Loading