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.

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

Optional compact blinded path creation #3080

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
3 changes: 1 addition & 2 deletions fuzz/src/chanmon_consistency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ use bitcoin::hashes::sha256d::Hash as Sha256dHash;
use bitcoin::hash_types::BlockHash;

use lightning::blinded_path::BlindedPath;
use lightning::blinded_path::message::ForwardNode;
use lightning::blinded_path::payment::ReceiveTlvs;
use lightning::chain;
use lightning::chain::{BestBlock, ChannelMonitorUpdateStatus, chainmonitor, channelmonitor, Confirm, Watch};
Expand Down Expand Up @@ -124,7 +123,7 @@ impl MessageRouter for FuzzRouter {
}

fn create_blinded_paths<T: secp256k1::Signing + secp256k1::Verification>(
&self, _recipient: PublicKey, _peers: Vec<ForwardNode>, _secp_ctx: &Secp256k1<T>,
&self, _recipient: PublicKey, _peers: Vec<PublicKey>, _secp_ctx: &Secp256k1<T>,
) -> Result<Vec<BlindedPath>, ()> {
unreachable!()
}
Expand Down
3 changes: 1 addition & 2 deletions fuzz/src/full_stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ use bitcoin::hashes::sha256d::Hash as Sha256dHash;
use bitcoin::hash_types::{Txid, BlockHash};

use lightning::blinded_path::BlindedPath;
use lightning::blinded_path::message::ForwardNode;
use lightning::blinded_path::payment::ReceiveTlvs;
use lightning::chain;
use lightning::chain::{BestBlock, ChannelMonitorUpdateStatus, Confirm, Listen};
Expand Down Expand Up @@ -162,7 +161,7 @@ impl MessageRouter for FuzzRouter {
}

fn create_blinded_paths<T: secp256k1::Signing + secp256k1::Verification>(
&self, _recipient: PublicKey, _peers: Vec<ForwardNode>, _secp_ctx: &Secp256k1<T>,
&self, _recipient: PublicKey, _peers: Vec<PublicKey>, _secp_ctx: &Secp256k1<T>,
) -> Result<Vec<BlindedPath>, ()> {
unreachable!()
}
Expand Down
3 changes: 1 addition & 2 deletions fuzz/src/onion_message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use bitcoin::secp256k1::ecdsa::RecoverableSignature;
use bitcoin::secp256k1::schnorr;

use lightning::blinded_path::{BlindedPath, EmptyNodeIdLookUp};
use lightning::blinded_path::message::ForwardNode;
use lightning::ln::features::InitFeatures;
use lightning::ln::msgs::{self, DecodeError, OnionMessageHandler};
use lightning::ln::script::ShutdownScript;
Expand Down Expand Up @@ -89,7 +88,7 @@ impl MessageRouter for TestMessageRouter {
}

fn create_blinded_paths<T: secp256k1::Signing + secp256k1::Verification>(
&self, _recipient: PublicKey, _peers: Vec<ForwardNode>, _secp_ctx: &Secp256k1<T>,
&self, _recipient: PublicKey, _peers: Vec<PublicKey>, _secp_ctx: &Secp256k1<T>,
) -> Result<Vec<BlindedPath>, ()> {
unreachable!()
}
Expand Down
115 changes: 94 additions & 21 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1554,8 +1554,9 @@ where
/// #
/// # fn example<T: AChannelManager>(channel_manager: T) -> Result<(), Bolt12SemanticError> {
/// # let channel_manager = channel_manager.get_cm();
/// # let absolute_expiry = None;
/// let offer = channel_manager
/// .create_offer_builder()?
/// .create_offer_builder(absolute_expiry)?
/// # ;
/// # // Needed for compiling for c_bindings
/// # let builder: lightning::offers::offer::OfferBuilder<_, _> = offer.into();
Expand Down Expand Up @@ -2287,6 +2288,19 @@ const MAX_UNFUNDED_CHANNEL_PEERS: usize = 50;
/// many peers we reject new (inbound) connections.
const MAX_NO_CHANNEL_PEERS: usize = 250;

/// The maximum expiration from the current time where an [`Offer`] or [`Refund`] is considered
/// short-lived, while anything with a greater expiration is considered long-lived.
///
/// Using [`ChannelManager::create_offer_builder`] or [`ChannelManager::create_refund_builder`],
/// will included a [`BlindedPath`] created using:
Comment on lines +2294 to +2295
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
/// Using [`ChannelManager::create_offer_builder`] or [`ChannelManager::create_refund_builder`],
/// will included a [`BlindedPath`] created using:
/// Using [`ChannelManager::create_offer_builder`] or [`ChannelManager::create_refund_builder`]
/// will include a [`BlindedPath`] created using:

/// - [`MessageRouter::create_compact_blinded_paths`] when short-lived, and
/// - [`MessageRouter::create_blinded_paths`] when long-lived.
///
/// Using compact [`BlindedPath`]s may provide better privacy as the [`MessageRouter`] could select
/// more hops. However, since they use short channel ids instead of pubkeys, they are more likely to
/// become invalid over time as channels are closed. Thus, they are only suitable for short-term use.
pub const MAX_SHORT_LIVED_RELATIVE_EXPIRY: Duration = Duration::from_secs(60 * 60 * 24);

/// Used by [`ChannelManager::list_recent_payments`] to express the status of recent payments.
/// These include payments that have yet to find a successful path, or have unresolved HTLCs.
#[derive(Debug, PartialEq)]
Expand Down Expand Up @@ -8240,16 +8254,15 @@ where

macro_rules! create_offer_builder { ($self: ident, $builder: ty) => {
/// Creates an [`OfferBuilder`] such that the [`Offer`] it builds is recognized by the
/// [`ChannelManager`] when handling [`InvoiceRequest`] messages for the offer. The offer will
/// not have an expiration unless otherwise set on the builder.
/// [`ChannelManager`] when handling [`InvoiceRequest`] messages for the offer. The offer's
/// expiration will be `absolute_expiry` if `Some`, otherwise it will not expire.
///
/// # Privacy
///
/// Uses [`MessageRouter::create_blinded_paths`] to construct a [`BlindedPath`] for the offer.
/// However, if one is not found, uses a one-hop [`BlindedPath`] with
/// [`ChannelManager::get_our_node_id`] as the introduction node instead. In the latter case,
/// the node must be announced, otherwise, there is no way to find a path to the introduction in
/// order to send the [`InvoiceRequest`].
/// Uses [`MessageRouter`] to construct a [`BlindedPath`] for the offer based on the given
/// `absolute_expiry` according to [`MAX_SHORT_LIVED_RELATIVE_EXPIRY`]. See those docs for
/// privacy implications as well as those of the parameterized [`Router`], which implements
/// [`MessageRouter`].
///
/// Also, uses a derived signing pubkey in the offer for recipient privacy.
///
Expand All @@ -8264,19 +8277,27 @@ macro_rules! create_offer_builder { ($self: ident, $builder: ty) => {
///
/// [`Offer`]: crate::offers::offer::Offer
/// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest
pub fn create_offer_builder(&$self) -> Result<$builder, Bolt12SemanticError> {
pub fn create_offer_builder(
&$self, absolute_expiry: Option<Duration>
) -> Result<$builder, Bolt12SemanticError> {
let node_id = $self.get_our_node_id();
let expanded_key = &$self.inbound_payment_key;
let entropy = &*$self.entropy_source;
let secp_ctx = &$self.secp_ctx;

let path = $self.create_blinded_path().map_err(|_| Bolt12SemanticError::MissingPaths)?;
let path = $self.create_blinded_path_using_absolute_expiry(absolute_expiry)
.map_err(|_| Bolt12SemanticError::MissingPaths)?;
let builder = OfferBuilder::deriving_signing_pubkey(
node_id, expanded_key, entropy, secp_ctx
)
.chain_hash($self.chain_hash)
.path(path);

let builder = match absolute_expiry {
None => builder,
Some(absolute_expiry) => builder.absolute_expiry(absolute_expiry),
};

Ok(builder.into())
}
} }
Expand Down Expand Up @@ -8304,11 +8325,10 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {
///
/// # Privacy
///
/// Uses [`MessageRouter::create_blinded_paths`] to construct a [`BlindedPath`] for the refund.
/// However, if one is not found, uses a one-hop [`BlindedPath`] with
/// [`ChannelManager::get_our_node_id`] as the introduction node instead. In the latter case,
/// the node must be announced, otherwise, there is no way to find a path to the introduction in
/// order to send the [`Bolt12Invoice`].
/// Uses [`MessageRouter`] to construct a [`BlindedPath`] for the refund based on the given
/// `absolute_expiry` according to [`MAX_SHORT_LIVED_RELATIVE_EXPIRY`]. See those docs for
/// privacy implications as well as those of the parameterized [`Router`], which implements
/// [`MessageRouter`].
///
/// Also, uses a derived payer id in the refund for payer privacy.
///
Expand Down Expand Up @@ -8337,7 +8357,8 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {
let entropy = &*$self.entropy_source;
let secp_ctx = &$self.secp_ctx;

let path = $self.create_blinded_path().map_err(|_| Bolt12SemanticError::MissingPaths)?;
let path = $self.create_blinded_path_using_absolute_expiry(Some(absolute_expiry))
.map_err(|_| Bolt12SemanticError::MissingPaths)?;
let builder = RefundBuilder::deriving_payer_id(
node_id, expanded_key, entropy, secp_ctx, amount_msats, payment_id
)?
Expand Down Expand Up @@ -8406,10 +8427,9 @@ where
///
/// # Privacy
///
/// Uses a one-hop [`BlindedPath`] for the reply path with [`ChannelManager::get_our_node_id`]
/// as the introduction node and a derived payer id for payer privacy. As such, currently, the
/// node must be announced. Otherwise, there is no way to find a path to the introduction node
/// in order to send the [`Bolt12Invoice`].
/// For payer privacy, uses a derived payer id and uses [`MessageRouter::create_blinded_paths`]
/// to construct a [`BlindedPath`] for the reply path. For further privacy implications, see the
/// docs of the parameterized [`Router`], which implements [`MessageRouter`].
///
/// # Limitations
///
Expand Down Expand Up @@ -8686,6 +8706,38 @@ where
inbound_payment::get_payment_preimage(payment_hash, payment_secret, &self.inbound_payment_key)
}

/// Creates a blinded path by delegating to [`MessageRouter`] based on the path's intended
/// lifetime.
///
/// Whether or not the path is compact depends on whether the path is short-lived or long-lived,
/// respectively, based on the given `absolute_expiry` as seconds since the Unix epoch. See
/// [`MAX_SHORT_LIVED_RELATIVE_EXPIRY`].
fn create_blinded_path_using_absolute_expiry(
&self, absolute_expiry: Option<Duration>
) -> Result<BlindedPath, ()> {
let now = self.duration_since_epoch();
let max_short_lived_absolute_expiry = now.saturating_add(MAX_SHORT_LIVED_RELATIVE_EXPIRY);

if absolute_expiry.unwrap_or(Duration::MAX) <= max_short_lived_absolute_expiry {
self.create_compact_blinded_path()
} else {
self.create_blinded_path()
}
}

pub(super) fn duration_since_epoch(&self) -> Duration {
#[cfg(not(feature = "std"))]
let now = Duration::from_secs(
self.highest_seen_timestamp.load(Ordering::Acquire) as u64
);
#[cfg(feature = "std")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Until we upgrade to rust-bitcoin 0.32, we should avoid assuming we can access time even with std :(.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... we are already doing so in timer_tick_occurred and OffersMessageHandler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I'd missed that, we need to fix that (or mutiny is gonna be mad...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there anything blocking us from upgrading now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just someone doing the work, I think. @tnull should chime in though, it may be that BDK is on 0.31 and we want to ship an 0.31 release first so that we can sync with the existing BDK code before jumping to 0.32.

Copy link
Contributor

@tnull tnull Jun 5, 2024

Choose a reason for hiding this comment

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

I discussed that with the BDK team in the meeting last week. BDK is close to finishing the upgrade and it will land ~next release, at the very least before the feature that keeps LDK Node from upgrading (data model backwards compat). So we're free to (and in fact should ASAP) upgrade to 0.32. I can see to prioritize it by the end of the week. Would this be sufficient to have it land it first, so that we don't have to deal with the things mentioned above 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.

Yeah, preferably so. I'm not sure what our alternative is, though I don't think we're in any big rush to land this. Presumably we'll want it for the next release but waiting on 0.32 shouldn't hold up the review at least.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to fix this for the next release one way or another (#3097). Given its already a bug IMO we should just land this and then handle the fixups as a part of #3097.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, now tracking here: #3100

Will see to pick it up soon if nobody is already working on it.

let now = std::time::SystemTime::now()
.duration_since(std::time::SystemTime::UNIX_EPOCH)
.expect("SystemTime::now() should come after SystemTime::UNIX_EPOCH");

now
}

/// Creates a blinded path by delegating to [`MessageRouter::create_blinded_paths`].
///
/// Errors if the `MessageRouter` errors or returns an empty `Vec`.
Expand All @@ -8696,6 +8748,27 @@ where
let peers = self.per_peer_state.read().unwrap()
.iter()
.map(|(node_id, peer_state)| (node_id, peer_state.lock().unwrap()))
.filter(|(_, peer)| peer.is_connected)
.filter(|(_, peer)| peer.latest_features.supports_onion_messages())
.map(|(node_id, _)| *node_id)
.collect::<Vec<_>>();

self.router
.create_blinded_paths(recipient, peers, secp_ctx)
.and_then(|paths| paths.into_iter().next().ok_or(()))
}

/// Creates a blinded path by delegating to [`MessageRouter::create_compact_blinded_paths`].
///
/// Errors if the `MessageRouter` errors or returns an empty `Vec`.
fn create_compact_blinded_path(&self) -> Result<BlindedPath, ()> {
let recipient = self.get_our_node_id();
let secp_ctx = &self.secp_ctx;

let peers = self.per_peer_state.read().unwrap()
.iter()
.map(|(node_id, peer_state)| (node_id, peer_state.lock().unwrap()))
.filter(|(_, peer)| peer.is_connected)
.filter(|(_, peer)| peer.latest_features.supports_onion_messages())
.map(|(node_id, peer)| ForwardNode {
node_id: *node_id,
Expand All @@ -8708,7 +8781,7 @@ where
.collect::<Vec<_>>();

self.router
.create_blinded_paths(recipient, peers, secp_ctx)
.create_compact_blinded_paths(recipient, peers, secp_ctx)
.and_then(|paths| paths.into_iter().next().ok_or(()))
}

Expand Down
47 changes: 23 additions & 24 deletions lightning/src/ln/functional_test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3259,30 +3259,34 @@ pub fn create_network<'a, 'b: 'a, 'c: 'b>(node_count: usize, cfgs: &'b Vec<NodeC

for i in 0..node_count {
for j in (i+1)..node_count {
let node_id_i = nodes[i].node.get_our_node_id();
let node_id_j = nodes[j].node.get_our_node_id();

let init_i = msgs::Init {
features: nodes[i].init_features(&node_id_j),
networks: None,
remote_network_address: None,
};
let init_j = msgs::Init {
features: nodes[j].init_features(&node_id_i),
networks: None,
remote_network_address: None,
};

nodes[i].node.peer_connected(&node_id_j, &init_j, true).unwrap();
nodes[j].node.peer_connected(&node_id_i, &init_i, false).unwrap();
nodes[i].onion_messenger.peer_connected(&node_id_j, &init_j, true).unwrap();
nodes[j].onion_messenger.peer_connected(&node_id_i, &init_i, false).unwrap();
connect_nodes(&nodes[i], &nodes[j]);
}
}

nodes
}

fn connect_nodes<'a, 'b: 'a, 'c: 'b>(node_a: &Node<'a, 'b, 'c>, node_b: &Node<'a, 'b, 'c>) {
let node_id_a = node_a.node.get_our_node_id();
let node_id_b = node_b.node.get_our_node_id();

let init_a = msgs::Init {
features: node_a.init_features(&node_id_b),
networks: None,
remote_network_address: None,
};
let init_b = msgs::Init {
features: node_b.init_features(&node_id_a),
networks: None,
remote_network_address: None,
};

node_a.node.peer_connected(&node_id_b, &init_b, true).unwrap();
node_b.node.peer_connected(&node_id_a, &init_a, false).unwrap();
node_a.onion_messenger.peer_connected(&node_id_b, &init_b, true).unwrap();
node_b.onion_messenger.peer_connected(&node_id_a, &init_a, false).unwrap();
}

pub fn connect_dummy_node<'a, 'b: 'a, 'c: 'b>(node: &Node<'a, 'b, 'c>) {
let node_id_dummy = PublicKey::from_slice(&[2; 33]).unwrap();

Expand Down Expand Up @@ -3643,13 +3647,8 @@ pub fn reconnect_nodes<'a, 'b, 'c, 'd>(args: ReconnectArgs<'a, 'b, 'c, 'd>) {
pending_cell_htlc_claims, pending_cell_htlc_fails, pending_raa,
pending_responding_commitment_signed, pending_responding_commitment_signed_dup_monitor,
} = args;
node_a.node.peer_connected(&node_b.node.get_our_node_id(), &msgs::Init {
features: node_b.node.init_features(), networks: None, remote_network_address: None
}, true).unwrap();
connect_nodes(node_a, node_b);
let reestablish_1 = get_chan_reestablish_msgs!(node_a, node_b);
node_b.node.peer_connected(&node_a.node.get_our_node_id(), &msgs::Init {
features: node_a.node.init_features(), networks: None, remote_network_address: None
}, false).unwrap();
let reestablish_2 = get_chan_reestablish_msgs!(node_b, node_a);

if send_channel_ready.0 {
Expand Down
Loading
Loading