Skip to content

Commit

Permalink
Merge pull request #2015 from TheBlueMatt/2023-02-no-dumb-redundant-f…
Browse files Browse the repository at this point in the history 
…ields
  • Loading branch information
@TheBlueMatt
TheBlueMatt authored Feb 28, 2023
2 parents b5e5435 + f03b7cd commit b8bea74
Show file tree
Hide file tree
Showing 11 changed files with 147 additions and 95 deletions.
2 changes: 0 additions & 2 deletions fuzz/src/full_stack.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,6 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
let params = RouteParameters {
payment_params,
final_value_msat,
final_cltv_expiry_delta: 42,
};
let random_seed_bytes: [u8; 32] = keys_manager.get_secure_random_bytes();
let route = match find_route(&our_id, &params, &network_graph, None, Arc::clone(&logger), &scorer, &random_seed_bytes) {
Expand All @@ -537,7 +536,6 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
let params = RouteParameters {
payment_params,
final_value_msat,
final_cltv_expiry_delta: 42,
};
let random_seed_bytes: [u8; 32] = keys_manager.get_secure_random_bytes();
let mut route = match find_route(&our_id, &params, &network_graph, None, Arc::clone(&logger), &scorer, &random_seed_bytes) {
Expand Down
1 change: 0 additions & 1 deletion fuzz/src/router.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,6 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
payment_params: PaymentParameters::from_node_id(*target, final_cltv_expiry_delta)
.with_route_hints(last_hops.clone()),
final_value_msat,
final_cltv_expiry_delta,
};
let _ = find_route(&our_pubkey, &route_params, &net_graph,
first_hops.map(|c| c.iter().collect::<Vec<_>>()).as_ref().map(|a| a.as_slice()),
Expand Down
1 change: 0 additions & 1 deletion lightning-invoice/src/payment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ fn pay_invoice_using_amount<P: Deref>(
let route_params = RouteParameters {
payment_params,
final_value_msat: amount_msats,
final_cltv_expiry_delta: invoice.min_final_cltv_expiry_delta() as u32,
};

payer.send_payment(payment_hash, &payment_secret, payment_id, route_params, retry_strategy)
Expand Down
2 changes: 0 additions & 2 deletions lightning-invoice/src/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,6 @@ mod test {
let route_params = RouteParameters {
payment_params,
final_value_msat: invoice.amount_milli_satoshis().unwrap(),
final_cltv_expiry_delta: invoice.min_final_cltv_expiry_delta() as u32,
};
let first_hops = nodes[0].node.list_usable_channels();
let network_graph = &node_cfgs[0].network_graph;
Expand Down Expand Up @@ -1050,7 +1049,6 @@ mod test {
let params = RouteParameters {
payment_params,
final_value_msat: invoice.amount_milli_satoshis().unwrap(),
final_cltv_expiry_delta: invoice.min_final_cltv_expiry_delta() as u32,
};
let first_hops = nodes[0].node.list_usable_channels();
let network_graph = &node_cfgs[0].network_graph;
Expand Down
20 changes: 13 additions & 7 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6745,27 +6745,36 @@ impl Readable for HTLCSource {
0 => {
let mut session_priv: crate::util::ser::RequiredWrapper<SecretKey> = crate::util::ser::RequiredWrapper(None);
let mut first_hop_htlc_msat: u64 = 0;
let mut path = Some(Vec::new());
let mut path: Option<Vec<RouteHop>> = Some(Vec::new());
let mut payment_id = None;
let mut payment_secret = None;
let mut payment_params = None;
let mut payment_params: Option<PaymentParameters> = None;
read_tlv_fields!(reader, {
(0, session_priv, required),
(1, payment_id, option),
(2, first_hop_htlc_msat, required),
(3, payment_secret, option),
(4, path, vec_type),
(5, payment_params, option),
(5, payment_params, (option: ReadableArgs, 0)),
});
if payment_id.is_none() {
// For backwards compat, if there was no payment_id written, use the session_priv bytes
// instead.
payment_id = Some(PaymentId(*session_priv.0.unwrap().as_ref()));
}
if path.is_none() || path.as_ref().unwrap().is_empty() {
return Err(DecodeError::InvalidValue);
}
let path = path.unwrap();
if let Some(params) = payment_params.as_mut() {
if params.final_cltv_expiry_delta == 0 {
params.final_cltv_expiry_delta = path.last().unwrap().cltv_expiry_delta;
}
}
Ok(HTLCSource::OutboundRoute {
session_priv: session_priv.0.unwrap(),
first_hop_htlc_msat,
path: path.unwrap(),
path,
payment_id: payment_id.unwrap(),
payment_secret,
payment_params,
Expand Down Expand Up @@ -7963,7 +7972,6 @@ mod tests {
let route_params = RouteParameters {
payment_params: PaymentParameters::for_keysend(expected_route.last().unwrap().node.get_our_node_id(), TEST_FINAL_CLTV),
final_value_msat: 100_000,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};
let route = find_route(
&nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph,
Expand Down Expand Up @@ -8054,7 +8062,6 @@ mod tests {
let route_params = RouteParameters {
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
final_value_msat: 10_000,
final_cltv_expiry_delta: 40,
};
let network_graph = nodes[0].network_graph.clone();
let first_hops = nodes[0].node.list_usable_channels();
Expand Down Expand Up @@ -8097,7 +8104,6 @@ mod tests {
let route_params = RouteParameters {
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
final_value_msat: 10_000,
final_cltv_expiry_delta: 40,
};
let network_graph = nodes[0].network_graph.clone();
let first_hops = nodes[0].node.list_usable_channels();
Expand Down
2 changes: 0 additions & 2 deletions lightning/src/ln/functional_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9241,7 +9241,6 @@ fn test_keysend_payments_to_public_node() {
let route_params = RouteParameters {
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
final_value_msat: 10000,
final_cltv_expiry_delta: 40,
};
let scorer = test_utils::TestScorer::new();
let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes();
Expand Down Expand Up @@ -9272,7 +9271,6 @@ fn test_keysend_payments_to_private_node() {
let route_params = RouteParameters {
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
final_value_msat: 10000,
final_cltv_expiry_delta: 40,
};
let network_graph = nodes[0].network_graph.clone();
let first_hops = nodes[0].node.list_usable_channels();
Expand Down
27 changes: 4 additions & 23 deletions lightning/src/ln/outbound_payment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use bitcoin::secp256k1::{self, Secp256k1, SecretKey};
use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
use crate::ln::channelmanager::{ChannelDetails, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, PaymentId};
use crate::ln::channelmanager::MIN_FINAL_CLTV_EXPIRY_DELTA as LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA;
use crate::ln::onion_utils::HTLCFailReason;
use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RouteParameters, RoutePath, Router};
use crate::util::errors::APIError;
Expand All @@ -25,6 +24,7 @@ use crate::util::logger::Logger;
use crate::util::time::Time;
#[cfg(all(not(feature = "no-std"), test))]
use crate::util::time::tests::SinceEpoch;
use crate::util::ser::ReadableArgs;

use core::cmp;
use core::fmt::{self, Display, Formatter};
Expand Down Expand Up @@ -528,12 +528,6 @@ impl OutboundPayments {
if pending_amt_msat < total_msat {
retry_id_route_params = Some((*pmt_id, RouteParameters {
final_value_msat: *total_msat - *pending_amt_msat,
final_cltv_expiry_delta:
if let Some(delta) = params.final_cltv_expiry_delta { delta }
else {
debug_assert!(false, "We always set the final_cltv_expiry_delta when a path fails");
LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA.into()
},
payment_params: params.clone(),
}));
break
Expand Down Expand Up @@ -976,9 +970,6 @@ impl OutboundPayments {
Some(RouteParameters {
payment_params: payment_params.clone(),
final_value_msat: pending_amt_unsent,
final_cltv_expiry_delta:
if let Some(delta) = payment_params.final_cltv_expiry_delta { delta }
else { max_unsent_cltv_delta },
})
} else { None }
} else { None },
Expand Down Expand Up @@ -1179,23 +1170,14 @@ impl OutboundPayments {
// `payment_params`) back to the user.
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
if let Some(params) = payment.get_mut().payment_parameters() {
if params.final_cltv_expiry_delta.is_none() {
// This should be rare, but a user could provide None for the payment data, and
// we need it when we go to retry the payment, so fill it in.
params.final_cltv_expiry_delta = Some(path_last_hop.cltv_expiry_delta);
}
retry = Some(RouteParameters {
payment_params: params.clone(),
final_value_msat: path_last_hop.fee_msat,
final_cltv_expiry_delta: params.final_cltv_expiry_delta.unwrap(),
});
} else if let Some(params) = payment_params {
retry = Some(RouteParameters {
payment_params: params.clone(),
final_value_msat: path_last_hop.fee_msat,
final_cltv_expiry_delta:
if let Some(delta) = params.final_cltv_expiry_delta { delta }
else { path_last_hop.cltv_expiry_delta },
});
}

Expand Down Expand Up @@ -1330,7 +1312,9 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
(0, session_privs, required),
(1, pending_fee_msat, option),
(2, payment_hash, required),
(3, payment_params, option),
// Note that while we "default" payment_param's final CLTV expiry delta to 0 we should
// never see it - `payment_params` was added here after the field was added/required.
(3, payment_params, (option: ReadableArgs, 0)),
(4, payment_secret, option),
(5, keysend_preimage, option),
(6, total_msat, required),
Expand Down Expand Up @@ -1386,7 +1370,6 @@ mod tests {
let expired_route_params = RouteParameters {
payment_params,
final_value_msat: 0,
final_cltv_expiry_delta: 0,
};
let pending_events = Mutex::new(Vec::new());
if on_retry {
Expand Down Expand Up @@ -1428,7 +1411,6 @@ mod tests {
let route_params = RouteParameters {
payment_params,
final_value_msat: 0,
final_cltv_expiry_delta: 0,
};
router.expect_find_route(route_params.clone(),
Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError }));
Expand Down Expand Up @@ -1471,7 +1453,6 @@ mod tests {
let route_params = RouteParameters {
payment_params: payment_params.clone(),
final_value_msat: 0,
final_cltv_expiry_delta: 0,
};
let failed_scid = 42;
let route = Route {
Expand Down
26 changes: 7 additions & 19 deletions lightning/src/ln/payment_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ fn mpp_retry() {
let mut route_params = RouteParameters {
payment_params: route.payment_params.clone().unwrap(),
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};

nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
Expand Down Expand Up @@ -297,7 +296,6 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
let route_params = RouteParameters {
payment_params: route.payment_params.clone().unwrap(),
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
check_added_monitors!(nodes[0], 1);
Expand Down Expand Up @@ -1387,12 +1385,12 @@ fn do_test_intercepted_payment(test: InterceptTest) {
let route_params = RouteParameters {
payment_params,
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};
let route = get_route(
&nodes[0].node.get_our_node_id(), &route_params.payment_params,
&nodes[0].network_graph.read_only(), None, route_params.final_value_msat,
route_params.final_cltv_expiry_delta, nodes[0].logger, &scorer, &random_seed_bytes
route_params.payment_params.final_cltv_expiry_delta, nodes[0].logger, &scorer,
&random_seed_bytes,
).unwrap();

let (payment_hash, payment_secret) = nodes[2].node.create_inbound_payment(Some(amt_msat), 60 * 60, None).unwrap();
Expand Down Expand Up @@ -1577,7 +1575,6 @@ fn do_automatic_retries(test: AutoRetry) {
let route_params = RouteParameters {
payment_params,
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};
let (_, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], amt_msat);

Expand Down Expand Up @@ -1787,7 +1784,6 @@ fn auto_retry_partial_failure() {
let route_params = RouteParameters {
payment_params,
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};

// Ensure the first monitor update (for the initial send path1 over chan_1) succeeds, but the
Expand Down Expand Up @@ -1860,12 +1856,12 @@ fn auto_retry_partial_failure() {
let mut payment_params = route_params.payment_params.clone();
payment_params.previously_failed_channels.push(chan_2_id);
nodes[0].router.expect_find_route(RouteParameters {
payment_params, final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV
payment_params, final_value_msat: amt_msat / 2,
}, Ok(retry_1_route));
let mut payment_params = route_params.payment_params.clone();
payment_params.previously_failed_channels.push(chan_3_id);
nodes[0].router.expect_find_route(RouteParameters {
payment_params, final_value_msat: amt_msat / 4, final_cltv_expiry_delta: TEST_FINAL_CLTV
payment_params, final_value_msat: amt_msat / 4,
}, Ok(retry_2_route));

// Send a payment that will partially fail on send, then partially fail on retry, then succeed.
Expand Down Expand Up @@ -1999,7 +1995,6 @@ fn auto_retry_zero_attempts_send_error() {
let route_params = RouteParameters {
payment_params,
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};

chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::PermanentFailure);
Expand Down Expand Up @@ -2039,7 +2034,6 @@ fn fails_paying_after_rejected_by_payee() {
let route_params = RouteParameters {
payment_params,
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};

nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
Expand Down Expand Up @@ -2086,7 +2080,6 @@ fn retry_multi_path_single_failed_payment() {
let route_params = RouteParameters {
payment_params: payment_params.clone(),
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};

let chans = nodes[0].node.list_usable_channels();
Expand Down Expand Up @@ -2121,7 +2114,7 @@ fn retry_multi_path_single_failed_payment() {
payment_params: pay_params,
// Note that the second request here requests the amount we originally failed to send,
// not the amount remaining on the full payment, which should be changed.
final_value_msat: 100_000_001, final_cltv_expiry_delta: TEST_FINAL_CLTV
final_value_msat: 100_000_001,
}, Ok(route.clone()));

{
Expand Down Expand Up @@ -2180,7 +2173,6 @@ fn immediate_retry_on_failure() {
let route_params = RouteParameters {
payment_params,
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};

let chans = nodes[0].node.list_usable_channels();
Expand All @@ -2207,7 +2199,6 @@ fn immediate_retry_on_failure() {
pay_params.previously_failed_channels.push(chans[0].short_channel_id.unwrap());
nodes[0].router.expect_find_route(RouteParameters {
payment_params: pay_params, final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV
}, Ok(route.clone()));

nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
Expand Down Expand Up @@ -2270,7 +2261,6 @@ fn no_extra_retries_on_back_to_back_fail() {
let route_params = RouteParameters {
payment_params,
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};

let mut route = Route {
Expand Down Expand Up @@ -2316,7 +2306,7 @@ fn no_extra_retries_on_back_to_back_fail() {
route.paths[0][1].fee_msat = amt_msat;
nodes[0].router.expect_find_route(RouteParameters {
payment_params: second_payment_params,
final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV,
final_value_msat: amt_msat,
}, Ok(route.clone()));

nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
Expand Down Expand Up @@ -2471,7 +2461,6 @@ fn test_simple_partial_retry() {
let route_params = RouteParameters {
payment_params,
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};

let mut route = Route {
Expand Down Expand Up @@ -2516,7 +2505,7 @@ fn test_simple_partial_retry() {
route.paths.remove(0);
nodes[0].router.expect_find_route(RouteParameters {
payment_params: second_payment_params,
final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV,
final_value_msat: amt_msat / 2,
}, Ok(route.clone()));

nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
Expand Down Expand Up @@ -2637,7 +2626,6 @@ fn test_threaded_payment_retries() {
let mut route_params = RouteParameters {
payment_params,
final_value_msat: amt_msat,
final_cltv_expiry_delta: TEST_FINAL_CLTV,
};

let mut route = Route {
Expand Down
Loading

0 comments on commit b8bea74

Please sign in to comment.