-
Notifications
You must be signed in to change notification settings - Fork 420
Properly consider blinded paths in InFlightHtlcs #4072
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -323,6 +323,18 @@ where | |
type ScoreParams = <S::Target as ScoreLookUp>::ScoreParams; | ||
#[rustfmt::skip] | ||
fn channel_penalty_msat(&self, candidate: &CandidateRouteHop, usage: ChannelUsage, score_params: &Self::ScoreParams) -> u64 { | ||
if let CandidateRouteHop::Blinded(blinded_candidate) = candidate { | ||
if let Some(used_liquidity) = self.inflight_htlcs.used_blinded_liquidity_msat( | ||
*blinded_candidate.source_node_id, blinded_candidate.hint.blinding_point(), | ||
) { | ||
let usage = ChannelUsage { | ||
inflight_htlc_msat: usage.inflight_htlc_msat.saturating_add(used_liquidity), | ||
..usage | ||
}; | ||
|
||
return self.scorer.channel_penalty_msat(candidate, usage, score_params); | ||
} | ||
} | ||
let target = match candidate.target() { | ||
Some(target) => target, | ||
None => return self.scorer.channel_penalty_msat(candidate, usage, score_params), | ||
|
@@ -350,45 +362,64 @@ where | |
/// A data structure for tracking in-flight HTLCs. May be used during pathfinding to account for | ||
/// in-use channel liquidity. | ||
#[derive(Clone)] | ||
pub struct InFlightHtlcs( | ||
pub struct InFlightHtlcs { | ||
// A map with liquidity value (in msat) keyed by a short channel id and the direction the HTLC | ||
// is traveling in. The direction boolean is determined by checking if the HTLC source's public | ||
// key is less than its destination. See `InFlightHtlcs::used_liquidity_msat` for more | ||
// details. | ||
HashMap<(u64, bool), u64>, | ||
); | ||
unblinded_hops: HashMap<(u64, bool), u64>, | ||
/// A map with liquidity value (in msat) keyed by the introduction point of a blinded path and | ||
/// the blinding point. In general blinding points should be globally unique, but just in case | ||
/// we add the introduction point as well. | ||
blinded_hops: HashMap<(NodeId, PublicKey), u64>, | ||
} | ||
|
||
impl InFlightHtlcs { | ||
/// Constructs an empty `InFlightHtlcs`. | ||
#[rustfmt::skip] | ||
pub fn new() -> Self { InFlightHtlcs(new_hash_map()) } | ||
pub fn new() -> Self { | ||
InFlightHtlcs { unblinded_hops: new_hash_map(), blinded_hops: new_hash_map() } | ||
} | ||
|
||
/// Takes in a path with payer's node id and adds the path's details to `InFlightHtlcs`. | ||
#[rustfmt::skip] | ||
pub fn process_path(&mut self, path: &Path, payer_node_id: PublicKey) { | ||
if path.hops.is_empty() { return }; | ||
if path.hops.is_empty() { | ||
return; | ||
} | ||
|
||
let mut cumulative_msat = 0; | ||
if let Some(tail) = &path.blinded_tail { | ||
cumulative_msat += tail.final_value_msat; | ||
if tail.hops.len() > 1 { | ||
// Single-hop blinded paths aren't really "blinded" paths, as they terminate at the | ||
// introduction point. In that case, we don't need to track anything. | ||
Comment on lines
+392
to
+394
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm ok I would have thought a single-hop blinded path is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, its confusing... |
||
let last_hop = path.hops.last().unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The last unblinded hop could be the last trampoline hop instead, IIUC |
||
let intro_node = NodeId::from_pubkey(&last_hop.pubkey); | ||
// The amount we send into the blinded path is the sum of the blinded path final | ||
// amount and the fee we pay in it, which is the `fee_msat` of the last hop. | ||
let blinded_path_sent_amt = last_hop.fee_msat + cumulative_msat; | ||
self.blinded_hops | ||
.entry((intro_node, tail.blinding_point)) | ||
.and_modify(|used_liquidity_msat| *used_liquidity_msat += blinded_path_sent_amt) | ||
.or_insert(blinded_path_sent_amt); | ||
} | ||
} | ||
|
||
// total_inflight_map needs to be direction-sensitive when keeping track of the HTLC value | ||
// that is held up. However, the `hops` array, which is a path returned by `find_route` in | ||
// the router excludes the payer node. In the following lines, the payer's information is | ||
// hardcoded with an inflight value of 0 so that we can correctly represent the first hop | ||
// in our sliding window of two. | ||
let reversed_hops_with_payer = path.hops.iter().rev().skip(1) | ||
.map(|hop| hop.pubkey) | ||
.chain(core::iter::once(payer_node_id)); | ||
let reversed_hops = path.hops.iter().rev().skip(1).map(|hop| hop.pubkey); | ||
let reversed_hops_with_payer = reversed_hops.chain(core::iter::once(payer_node_id)); | ||
|
||
// Taking the reversed vector from above, we zip it with just the reversed hops list to | ||
// work "backwards" of the given path, since the last hop's `fee_msat` actually represents | ||
// the total amount sent. | ||
for (next_hop, prev_hop) in path.hops.iter().rev().zip(reversed_hops_with_payer) { | ||
cumulative_msat += next_hop.fee_msat; | ||
self.0 | ||
.entry((next_hop.short_channel_id, NodeId::from_pubkey(&prev_hop) < NodeId::from_pubkey(&next_hop.pubkey))) | ||
let direction = NodeId::from_pubkey(&prev_hop) < NodeId::from_pubkey(&next_hop.pubkey); | ||
self.unblinded_hops | ||
.entry((next_hop.short_channel_id, direction)) | ||
.and_modify(|used_liquidity_msat| *used_liquidity_msat += cumulative_msat) | ||
.or_insert(cumulative_msat); | ||
} | ||
|
@@ -399,7 +430,7 @@ impl InFlightHtlcs { | |
pub fn add_inflight_htlc( | ||
&mut self, source: &NodeId, target: &NodeId, channel_scid: u64, used_msat: u64, | ||
) { | ||
self.0 | ||
self.unblinded_hops | ||
.entry((channel_scid, source < target)) | ||
.and_modify(|used_liquidity_msat| *used_liquidity_msat += used_msat) | ||
.or_insert(used_msat); | ||
|
@@ -410,19 +441,14 @@ impl InFlightHtlcs { | |
pub fn used_liquidity_msat( | ||
&self, source: &NodeId, target: &NodeId, channel_scid: u64, | ||
) -> Option<u64> { | ||
self.0.get(&(channel_scid, source < target)).map(|v| *v) | ||
self.unblinded_hops.get(&(channel_scid, source < target)).map(|v| *v) | ||
} | ||
} | ||
|
||
impl Writeable for InFlightHtlcs { | ||
#[rustfmt::skip] | ||
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> { self.0.write(writer) } | ||
} | ||
|
||
impl Readable for InFlightHtlcs { | ||
fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> { | ||
let infight_map: HashMap<(u64, bool), u64> = Readable::read(reader)?; | ||
Ok(Self(infight_map)) | ||
/// Returns liquidity in msat given the blinded path introduction point and blinding point. | ||
pub fn used_blinded_liquidity_msat( | ||
&self, introduction_point: NodeId, blinding_point: PublicKey, | ||
) -> Option<u64> { | ||
self.blinded_hops.get(&(introduction_point, blinding_point)).map(|v| *v) | ||
} | ||
} | ||
|
||
|
@@ -3900,8 +3926,9 @@ mod tests { | |
use crate::routing::gossip::{EffectiveCapacity, NetworkGraph, NodeId, P2PGossipSync}; | ||
use crate::routing::router::{ | ||
add_random_cltv_offset, build_route_from_hops_internal, default_node_features, get_route, | ||
BlindedTail, CandidateRouteHop, InFlightHtlcs, Path, PaymentParameters, PublicHopCandidate, | ||
Route, RouteHint, RouteHintHop, RouteHop, RouteParameters, RoutingFees, | ||
BlindedPathCandidate, BlindedTail, CandidateRouteHop, InFlightHtlcs, Path, | ||
PaymentParameters, PublicHopCandidate, Route, RouteHint, RouteHintHop, RouteHop, | ||
RouteParameters, RoutingFees, ScorerAccountingForInFlightHtlcs, | ||
DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA, MAX_PATH_LENGTH_ESTIMATE, | ||
}; | ||
use crate::routing::scoring::{ | ||
|
@@ -3933,7 +3960,7 @@ mod tests { | |
|
||
use crate::io::Cursor; | ||
use crate::prelude::*; | ||
use crate::sync::Arc; | ||
use crate::sync::{Arc, Mutex}; | ||
|
||
#[rustfmt::skip] | ||
fn get_channel_details(short_channel_id: Option<u64>, node_id: PublicKey, | ||
|
@@ -7970,9 +7997,9 @@ mod tests { | |
|
||
#[test] | ||
#[rustfmt::skip] | ||
fn blinded_path_inflight_processing() { | ||
// Ensure we'll score the channel that's inbound to a blinded path's introduction node, and | ||
// account for the blinded tail's final amount_msat. | ||
fn one_hop_blinded_path_inflight_processing() { | ||
// Ensure we'll score the channel that's inbound to a one-hop blinded path's introduction | ||
// node, and account for the blinded tail's final amount_msat. | ||
let mut inflight_htlcs = InFlightHtlcs::new(); | ||
let path = Path { | ||
hops: vec![RouteHop { | ||
|
@@ -8002,8 +8029,108 @@ mod tests { | |
}), | ||
}; | ||
inflight_htlcs.process_path(&path, ln_test_utils::pubkey(44)); | ||
assert_eq!(*inflight_htlcs.0.get(&(42, true)).unwrap(), 301); | ||
assert_eq!(*inflight_htlcs.0.get(&(43, false)).unwrap(), 201); | ||
assert_eq!(*inflight_htlcs.unblinded_hops.get(&(42, true)).unwrap(), 301); | ||
assert_eq!(*inflight_htlcs.unblinded_hops.get(&(43, false)).unwrap(), 201); | ||
assert!(inflight_htlcs.blinded_hops.is_empty()); | ||
} | ||
|
||
struct UsageTrackingScorer(Mutex<Option<ChannelUsage>>); | ||
|
||
impl ScoreLookUp for UsageTrackingScorer { | ||
type ScoreParams = (); | ||
fn channel_penalty_msat(&self, _: &CandidateRouteHop, usage: ChannelUsage, _: &()) -> u64 { | ||
let mut inner = self.0.lock().unwrap(); | ||
assert!(inner.is_none()); | ||
*inner = Some(usage); | ||
0 | ||
} | ||
} | ||
|
||
#[test] | ||
fn blinded_path_inflight_processing() { | ||
// Ensure we'll score the channel that's inbound to a blinded path's introduction node, and | ||
// account for the blinded tail's final amount_msat as well as track the blinded path | ||
// in-flight. | ||
let mut inflight_htlcs = InFlightHtlcs::new(); | ||
let blinding_point = ln_test_utils::pubkey(48); | ||
let mut blinded_hops = Vec::new(); | ||
for i in 0..2 { | ||
blinded_hops.push(BlindedHop { | ||
blinded_node_id: ln_test_utils::pubkey(49 + i as u8), | ||
encrypted_payload: Vec::new(), | ||
}); | ||
} | ||
let intro_point = ln_test_utils::pubkey(43); | ||
let path = Path { | ||
hops: vec![ | ||
RouteHop { | ||
pubkey: ln_test_utils::pubkey(42), | ||
node_features: NodeFeatures::empty(), | ||
short_channel_id: 42, | ||
channel_features: ChannelFeatures::empty(), | ||
fee_msat: 100, | ||
cltv_expiry_delta: 0, | ||
maybe_announced_channel: false, | ||
}, | ||
RouteHop { | ||
pubkey: intro_point, | ||
node_features: NodeFeatures::empty(), | ||
short_channel_id: 43, | ||
channel_features: ChannelFeatures::empty(), | ||
fee_msat: 1, | ||
cltv_expiry_delta: 0, | ||
maybe_announced_channel: false, | ||
}, | ||
], | ||
blinded_tail: Some(BlindedTail { | ||
trampoline_hops: vec![], | ||
hops: blinded_hops.clone(), | ||
blinding_point, | ||
excess_final_cltv_expiry_delta: 0, | ||
final_value_msat: 200, | ||
}), | ||
}; | ||
inflight_htlcs.process_path(&path, ln_test_utils::pubkey(44)); | ||
assert_eq!(*inflight_htlcs.unblinded_hops.get(&(42, true)).unwrap(), 301); | ||
assert_eq!(*inflight_htlcs.unblinded_hops.get(&(43, false)).unwrap(), 201); | ||
let intro_node_id = NodeId::from_pubkey(&ln_test_utils::pubkey(43)); | ||
assert_eq!( | ||
*inflight_htlcs.blinded_hops.get(&(intro_node_id, blinding_point)).unwrap(), | ||
201 | ||
); | ||
|
||
let tracking_scorer = UsageTrackingScorer(Mutex::new(None)); | ||
let inflight_scorer = | ||
ScorerAccountingForInFlightHtlcs::new(&tracking_scorer, &inflight_htlcs); | ||
|
||
let blinded_payinfo = BlindedPayInfo { | ||
fee_base_msat: 100, | ||
fee_proportional_millionths: 500, | ||
htlc_minimum_msat: 1000, | ||
htlc_maximum_msat: 100_000_000, | ||
cltv_expiry_delta: 15, | ||
features: BlindedHopFeatures::empty(), | ||
}; | ||
let blinded_path = BlindedPaymentPath::from_blinded_path_and_payinfo( | ||
intro_point, | ||
blinding_point, | ||
blinded_hops, | ||
blinded_payinfo, | ||
); | ||
|
||
let candidate = CandidateRouteHop::Blinded(BlindedPathCandidate { | ||
source_node_id: &intro_node_id, | ||
hint: &blinded_path, | ||
hint_idx: 0, | ||
source_node_counter: 0, | ||
}); | ||
let empty_usage = ChannelUsage { | ||
amount_msat: 42, | ||
inflight_htlc_msat: 0, | ||
effective_capacity: EffectiveCapacity::HintMaxHTLC { amount_msat: 500 }, | ||
}; | ||
inflight_scorer.channel_penalty_msat(&candidate, empty_usage, &()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this value be checked? |
||
assert_eq!(tracking_scorer.0.lock().unwrap().unwrap().inflight_htlc_msat, 201); | ||
} | ||
|
||
#[test] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,7 @@ use crate::prelude::hash_map::Entry; | |
use crate::prelude::*; | ||
use crate::routing::gossip::{DirectedChannelInfo, EffectiveCapacity, NetworkGraph, NodeId}; | ||
use crate::routing::log_approx; | ||
use crate::routing::router::{CandidateRouteHop, Path, PublicHopCandidate}; | ||
use crate::routing::router::{BlindedPathCandidate, CandidateRouteHop, Path, PublicHopCandidate}; | ||
use crate::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; | ||
use crate::util::logger::Logger; | ||
use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer}; | ||
|
@@ -1682,6 +1682,17 @@ where | |
CandidateRouteHop::PublicHop(PublicHopCandidate { info, short_channel_id }) => { | ||
(short_channel_id, info.target()) | ||
}, | ||
CandidateRouteHop::Blinded(BlindedPathCandidate { hint, .. }) => { | ||
let total_inflight_amount_msat = | ||
usage.amount_msat.saturating_add(usage.inflight_htlc_msat); | ||
if usage.amount_msat > hint.payinfo.htlc_maximum_msat { | ||
return u64::max_value(); | ||
} else if total_inflight_amount_msat > hint.payinfo.htlc_maximum_msat { | ||
return score_params.considered_impossible_penalty_msat; | ||
Comment on lines
+1688
to
+1691
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In both cases, total amount flowing over the channel exceeds our current estimate of the channel's available liquidity, so why return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is consistent with what we do elsewhere - we only use Of course for blinded paths it maybe shouldn't matter anyway - the blinded path candidates should be within the context of a single payment (because we should have a unique blinded path for each payment) so if we have no option but to over-saturate a blinded path probably we're screwed. Its still worth trying, imo, because the recipient may have given us a blinded path with a too-low |
||
} else { | ||
return 0; | ||
} | ||
}, | ||
_ => return 0, | ||
}; | ||
let source = candidate.source(); | ||
|
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 this method should take into account trampoline hops, might be easy to do here