Skip to content

Router fixes #1398

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 4 commits into from
Apr 1, 2022
Merged

Router fixes #1398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37a947b
Fix build warnings without grind_signatures
jkczyz Mar 30, 2022
30b6873
Correctly pick middle hop to victimize
jkczyz Mar 30, 2022
f9cdf93
Select best route by lowest total cost
jkczyz Mar 30, 2022
8ddfe66
Don't consider a path as having hit HTLC-min if it isn't sufficient
TheBlueMatt Mar 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
35 changes: 22 additions & 13 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6289,44 +6289,39 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>

#[cfg(test)]
mod tests {
use bitcoin::util::bip143;
use bitcoin::consensus::encode::serialize;
use bitcoin::blockdata::script::{Script, Builder};
use bitcoin::blockdata::transaction::{Transaction, TxOut, SigHashType};
use bitcoin::blockdata::transaction::{Transaction, TxOut};
use bitcoin::blockdata::constants::genesis_block;
use bitcoin::blockdata::opcodes;
use bitcoin::network::constants::Network;
use bitcoin::hashes::hex::FromHex;
use hex;
use ln::{PaymentPreimage, PaymentHash};
use ln::PaymentHash;
use ln::channelmanager::{HTLCSource, PaymentId};
use ln::channel::{Channel,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,HTLCCandidate,HTLCInitiator,TxCreationKeys};
use ln::channel::{Channel, InboundHTLCOutput, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator};
use ln::channel::MAX_FUNDING_SATOSHIS;
use ln::features::InitFeatures;
use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
use ln::script::ShutdownScript;
use ln::chan_utils;
use ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters, htlc_success_tx_weight, htlc_timeout_tx_weight};
use ln::chan_utils::{htlc_success_tx_weight, htlc_timeout_tx_weight};
use chain::BestBlock;
use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
use chain::keysinterface::{InMemorySigner, Recipient, KeyMaterial, KeysInterface, BaseSign};
use chain::keysinterface::{InMemorySigner, Recipient, KeyMaterial, KeysInterface};
use chain::transaction::OutPoint;
use util::config::UserConfig;
use util::enforcing_trait_impls::EnforcingSigner;
use util::errors::APIError;
use util::test_utils;
use util::test_utils::OnGetShutdownScriptpubkey;
use util::logger::Logger;
use bitcoin::secp256k1::{Secp256k1, Message, Signature, All};
use bitcoin::secp256k1::{Secp256k1, Signature};
use bitcoin::secp256k1::ffi::Signature as FFISignature;
use bitcoin::secp256k1::key::{SecretKey,PublicKey};
use bitcoin::secp256k1::recovery::RecoverableSignature;
use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::hashes::Hash;
use bitcoin::hash_types::{Txid, WPubkeyHash};
use bitcoin::hash_types::WPubkeyHash;
use core::num::NonZeroU8;
use bitcoin::bech32::u5;
use sync::Arc;
use prelude::*;

struct TestFeeEstimator {
Expand Down Expand Up @@ -6380,7 +6375,8 @@ mod tests {
fn sign_invoice(&self, _hrp_bytes: &[u8], _invoice_data: &[u5], _recipient: Recipient) -> Result<RecoverableSignature, ()> { panic!(); }
}

fn public_from_secret_hex(secp_ctx: &Secp256k1<All>, hex: &str) -> PublicKey {
#[cfg(not(feature = "grind_signatures"))]
fn public_from_secret_hex(secp_ctx: &Secp256k1<bitcoin::secp256k1::All>, hex: &str) -> PublicKey {
PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&hex::decode(hex).unwrap()[..]).unwrap())
}

Expand Down Expand Up @@ -6669,6 +6665,19 @@ mod tests {
#[cfg(not(feature = "grind_signatures"))]
#[test]
fn outbound_commitment_test() {
use bitcoin::util::bip143;
use bitcoin::consensus::encode::serialize;
use bitcoin::blockdata::transaction::SigHashType;
use bitcoin::hashes::hex::FromHex;
use bitcoin::hash_types::Txid;
use bitcoin::secp256k1::Message;
use chain::keysinterface::BaseSign;
use ln::PaymentPreimage;
use ln::channel::{HTLCOutputInCommitment ,TxCreationKeys};
use ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
use util::logger::Logger;
use sync::Arc;

// Test vectors from BOLT 3 Appendices C and F (anchors):
let feeest = TestFeeEstimator{fee_est: 15000};
let logger : Arc<Logger> = Arc::new(test_utils::TestLogger::new());
Expand Down
1 change: 1 addition & 0 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ impl core::hash::Hash for HTLCSource {
}
}
}
#[cfg(not(feature = "grind_signatures"))]
#[cfg(test)]
impl HTLCSource {
pub fn dummy() -> Self {
Expand Down
33 changes: 21 additions & 12 deletions lightning/src/routing/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,10 @@ impl<'a> PaymentPath<'a> {
return result;
}

fn get_cost_msat(&self) -> u64 {
self.get_total_fee_paid_msat().saturating_add(self.get_path_penalty_msat())
}

// If the amount transferred by the path is updated, the fees should be adjusted. Any other way
// to change fees may result in an inconsistency.
//
Expand Down Expand Up @@ -912,14 +916,20 @@ where L::Target: Logger {
let over_path_minimum_msat = amount_to_transfer_over_msat >= $candidate.htlc_minimum_msat() &&
amount_to_transfer_over_msat >= $next_hops_path_htlc_minimum_msat;

#[allow(unused_comparisons)] // $next_hops_path_htlc_minimum_msat is 0 in some calls so rustc complains
let may_overpay_to_meet_path_minimum_msat =
((amount_to_transfer_over_msat < $candidate.htlc_minimum_msat() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a rather convoluted comparison. Do you think something like what you do in line 930, having each boolean value be assigned to a variable (maybe with some comments for each one) would improve legibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I imagine it can be broken up into the two variables where one is $candidate.htlc_minimum_msat() falls between amount_to_transfer_over_msat and recommended_value_msat and the other is the same but for $next_hops_path_htlc_minimum_msat. Do you have any recommendations on naming either?

Copy link
Contributor

Choose a reason for hiding this comment

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

is_htlc_minimum_msat_within_tolerance_range perhaps?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that doesn't seem any more clear to me - its not a "tolerance" range more of a "well, its within the range where we should try to meet it instead of ignoring the channel" range.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, nomenclature aside, the logic LGTM, so I'm gonna approve and then re-ack if we wanna change this part

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets land it - its blocking release and its probably clearer than it was given there's now some comment verbiage lol.

recommended_value_msat > $candidate.htlc_minimum_msat()) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw I think this can be >= and below.

Would it be a good first issue to add a test for this case?

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I dunno if that's a good first issue, but an issue yes. Kinda convoluted to hit those cases, though a trivial "make sure we don't run twice if someone disabled a channel by setting the min-HTLC to 21m BTC" test shouldn't be too much work.

(amount_to_transfer_over_msat < $next_hops_path_htlc_minimum_msat &&
recommended_value_msat > $next_hops_path_htlc_minimum_msat));

// If HTLC minimum is larger than the amount we're going to transfer, we shouldn't
// bother considering this channel.
// Since we're choosing amount_to_transfer_over_msat as maximum possible, it can
// be only reduced later (not increased), so this channel should just be skipped
// as not sufficient.
if !over_path_minimum_msat && doesnt_exceed_cltv_delta_limit {
// bother considering this channel. If retrying with recommended_value_msat may
// allow us to hit the HTLC minimum limit, set htlc_minimum_limit so that we go
// around again with a higher amount.
if contributes_sufficient_value && doesnt_exceed_cltv_delta_limit && may_overpay_to_meet_path_minimum_msat {
hit_minimum_limit = true;
} else if contributes_sufficient_value && doesnt_exceed_cltv_delta_limit {
} else if contributes_sufficient_value && doesnt_exceed_cltv_delta_limit && over_path_minimum_msat {
// Note that low contribution here (limited by available_liquidity_msat)
// might violate htlc_minimum_msat on the hops which are next along the
// payment path (upstream to the payee). To avoid that, we recompute
Expand Down Expand Up @@ -1390,7 +1400,7 @@ where L::Target: Logger {
// If we weren't capped by hitting a liquidity limit on a channel in the path,
// we'll probably end up picking the same path again on the next iteration.
// Decrease the available liquidity of a hop in the middle of the path.
let victim_scid = payment_path.hops[(payment_path.hops.len() - 1) / 2].0.candidate.short_channel_id();
let victim_scid = payment_path.hops[(payment_path.hops.len()) / 2].0.candidate.short_channel_id();
log_trace!(logger, "Disabling channel {} for future path building iterations to avoid duplicates.", victim_scid);
let victim_liquidity = bookkept_channels_liquidity_available_msat.get_mut(&victim_scid).unwrap();
*victim_liquidity = 0;
Expand Down Expand Up @@ -1502,9 +1512,8 @@ where L::Target: Logger {
// prefer lower cost paths.
cur_route.sort_unstable_by(|a, b| {
a.get_value_msat().cmp(&b.get_value_msat())
// Reverse ordering for fees, so we drop higher-fee paths first
.then_with(|| b.get_total_fee_paid_msat().saturating_add(b.get_path_penalty_msat())
.cmp(&a.get_total_fee_paid_msat().saturating_add(a.get_path_penalty_msat())))
// Reverse ordering for cost, so we drop higher-cost paths first
.then_with(|| b.get_cost_msat().cmp(&a.get_cost_msat()))
});

// We should make sure that at least 1 path left.
Expand Down Expand Up @@ -1548,8 +1557,8 @@ where L::Target: Logger {
}

// Step (9).
// Select the best route by lowest total fee.
drawn_routes.sort_unstable_by_key(|paths| paths.iter().map(|path| path.get_total_fee_paid_msat()).sum::<u64>());
// Select the best route by lowest total cost.
drawn_routes.sort_unstable_by_key(|paths| paths.iter().map(|path| path.get_cost_msat()).sum::<u64>());
let mut selected_paths = Vec::<Vec<Result<RouteHop, LightningError>>>::new();
for payment_path in drawn_routes.first().unwrap() {
let mut path = payment_path.hops.iter().map(|(payment_hop, node_features)| {
Expand Down