Skip to content

Commit

Permalink
Remove deprecated send_payment_with_route usage from fuzzing
Browse files Browse the repository at this point in the history 
This allows us to make the PaymentSendFailure error type private, as well as
reduce the visibility of the vestigial send_payment_with_route method that was
already made test and fuzz-only in a previous commit.
  • Loading branch information
@valentinewallace
valentinewallace committed Dec 2, 2024
1 parent 9de59ca commit 3956661
Show file tree
Hide file tree
Showing 3 changed files with 113 additions and 95 deletions.
191 changes: 101 additions & 90 deletions fuzz/src/chanmon_consistency.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ use lightning::events::MessageSendEventsProvider;
use lightning::ln::channel::FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
use lightning::ln::channel_state::ChannelDetails;
use lightning::ln::channelmanager::{
ChainParameters, ChannelManager, ChannelManagerReadArgs, PaymentId, PaymentSendFailure,
RecipientOnionFields,
ChainParameters, ChannelManager, ChannelManagerReadArgs, PaymentId, RecipientOnionFields, Retry,
};
use lightning::ln::functional_test_utils::*;
use lightning::ln::msgs::{
Expand All @@ -58,7 +57,9 @@ use lightning::ln::script::ShutdownScript;
use lightning::ln::types::ChannelId;
use lightning::offers::invoice::UnsignedBolt12Invoice;
use lightning::onion_message::messenger::{Destination, MessageRouter, OnionMessagePath};
use lightning::routing::router::{InFlightHtlcs, Path, Route, RouteHop, RouteParameters, Router};
use lightning::routing::router::{
InFlightHtlcs, Path, PaymentParameters, Route, RouteHop, RouteParameters, Router,
};
use lightning::sign::{
EntropySource, InMemorySigner, KeyMaterial, NodeSigner, Recipient, SignerProvider,
};
Expand All @@ -82,6 +83,7 @@ use bitcoin::secp256k1::{self, Message, PublicKey, Scalar, Secp256k1, SecretKey}

use lightning::io::Cursor;
use std::cmp::{self, Ordering};
use std::collections::VecDeque;
use std::mem;
use std::sync::atomic;
use std::sync::{Arc, Mutex};
Expand Down Expand Up @@ -112,13 +114,18 @@ impl FeeEstimator for FuzzEstimator {
}
}

struct FuzzRouter {}
struct FuzzRouter {
pub next_routes: Mutex<VecDeque<Route>>,
}

impl Router for FuzzRouter {
fn find_route(
&self, _payer: &PublicKey, _params: &RouteParameters,
_first_hops: Option<&[&ChannelDetails]>, _inflight_htlcs: InFlightHtlcs,
) -> Result<Route, msgs::LightningError> {
if let Some(route) = self.next_routes.lock().unwrap().pop_front() {
return Ok(route);
}
Err(msgs::LightningError {
err: String::from("Not implemented"),
action: msgs::ErrorAction::IgnoreError,
Expand Down Expand Up @@ -434,6 +441,34 @@ impl KeyProvider {
}
}

// Returns a bool indicating whether the payment failed.
#[inline]
fn check_payment_send_events(
source: &ChanMan, amt: u64, min_sendable: u64, max_sendable: u64,
) -> bool {
let mut payment_failed = false;
let events = source.get_and_clear_pending_events();
assert!(events.len() == 2 || events.len() == 0);
for ev in events {
match ev {
events::Event::PaymentPathFailed {
failure: events::PathFailure::InitialSend { err },
..
} => {
check_api_err(err, amt > max_sendable || amt < min_sendable);
},
events::Event::PaymentFailed { .. } => {},
_ => panic!(),
};
payment_failed = true;
}
// Note that while the max is a strict upper-bound, we can occasionally send substantially
// below the minimum, with some gap which is unusable immediately below the minimum. Thus,
// we don't check against min_value_sendable here.
assert!(payment_failed || (amt <= max_sendable));
payment_failed
}

#[inline]
fn check_api_err(api_err: APIError, sendable_bounds_violated: bool) {
match api_err {
Expand All @@ -460,34 +495,6 @@ fn check_api_err(api_err: APIError, sendable_bounds_violated: bool) {
},
}
}
#[inline]
fn check_payment_err(send_err: PaymentSendFailure, sendable_bounds_violated: bool) {
match send_err {
PaymentSendFailure::ParameterError(api_err) => {
check_api_err(api_err, sendable_bounds_violated)
},
PaymentSendFailure::PathParameterError(per_path_results) => {
for res in per_path_results {
if let Err(api_err) = res {
check_api_err(api_err, sendable_bounds_violated);
}
}
},
PaymentSendFailure::AllFailedResendSafe(per_path_results) => {
for api_err in per_path_results {
check_api_err(api_err, sendable_bounds_violated);
}
},
PaymentSendFailure::PartialFailure { results, .. } => {
for res in results {
if let Err(api_err) = res {
check_api_err(api_err, sendable_bounds_violated);
}
}
},
PaymentSendFailure::DuplicatePayment => panic!(),
}
}

type ChanMan<'a> = ChannelManager<
Arc<TestChainMonitor>,
Expand Down Expand Up @@ -546,34 +553,36 @@ fn send_payment(
.find(|chan| chan.short_channel_id == Some(dest_chan_id))
.map(|chan| (chan.next_outbound_htlc_minimum_msat, chan.next_outbound_htlc_limit_msat))
.unwrap_or((0, 0));
if let Err(err) = source.send_payment_with_route(
Route {
paths: vec![Path {
hops: vec![RouteHop {
pubkey: dest.get_our_node_id(),
node_features: dest.node_features(),
short_channel_id: dest_chan_id,
channel_features: dest.channel_features(),
fee_msat: amt,
cltv_expiry_delta: 200,
maybe_announced_channel: true,
}],
blinded_tail: None,
let mut next_routes = source.router.next_routes.lock().unwrap();
next_routes.push_back(Route {
paths: vec![Path {
hops: vec![RouteHop {
pubkey: dest.get_our_node_id(),
node_features: dest.node_features(),
short_channel_id: dest_chan_id,
channel_features: dest.channel_features(),
fee_msat: amt,
cltv_expiry_delta: 200,
maybe_announced_channel: true,
}],
route_params: None,
},
blinded_tail: None,
}],
route_params: None,
});
let route_params = RouteParameters::from_payment_params_and_value(
PaymentParameters::from_node_id(source.get_our_node_id(), TEST_FINAL_CLTV),
amt,
);
if let Err(err) = source.send_payment(
payment_hash,
RecipientOnionFields::secret_only(payment_secret),
PaymentId(payment_id),
route_params,
Retry::Attempts(0),
) {
check_payment_err(err, amt > max_value_sendable || amt < min_value_sendable);
false
panic!("Errored with {:?} on initial payment send", err);
} else {
// Note that while the max is a strict upper-bound, we can occasionally send substantially
// below the minimum, with some gap which is unusable immediately below the minimum. Thus,
// we don't check against min_value_sendable here.
assert!(amt <= max_value_sendable);
true
check_payment_send_events(source, amt, min_value_sendable, max_value_sendable)
}
}

Expand Down Expand Up @@ -615,54 +624,56 @@ fn send_hop_payment(
.map(|chan| (chan.next_outbound_htlc_minimum_msat, chan.next_outbound_htlc_limit_msat))
.unwrap_or((0, 0));
let first_hop_fee = 50_000;
if let Err(err) = source.send_payment_with_route(
Route {
paths: vec![Path {
hops: vec![
RouteHop {
pubkey: middle.get_our_node_id(),
node_features: middle.node_features(),
short_channel_id: middle_chan_id,
channel_features: middle.channel_features(),
fee_msat: first_hop_fee,
cltv_expiry_delta: 100,
maybe_announced_channel: true,
},
RouteHop {
pubkey: dest.get_our_node_id(),
node_features: dest.node_features(),
short_channel_id: dest_chan_id,
channel_features: dest.channel_features(),
fee_msat: amt,
cltv_expiry_delta: 200,
maybe_announced_channel: true,
},
],
blinded_tail: None,
}],
route_params: None,
},
let mut next_routes = source.router.next_routes.lock().unwrap();
next_routes.push_back(Route {
paths: vec![Path {
hops: vec![
RouteHop {
pubkey: middle.get_our_node_id(),
node_features: middle.node_features(),
short_channel_id: middle_chan_id,
channel_features: middle.channel_features(),
fee_msat: first_hop_fee,
cltv_expiry_delta: 100,
maybe_announced_channel: true,
},
RouteHop {
pubkey: dest.get_our_node_id(),
node_features: dest.node_features(),
short_channel_id: dest_chan_id,
channel_features: dest.channel_features(),
fee_msat: amt,
cltv_expiry_delta: 200,
maybe_announced_channel: true,
},
],
blinded_tail: None,
}],
route_params: None,
});
let route_params = RouteParameters::from_payment_params_and_value(
PaymentParameters::from_node_id(source.get_our_node_id(), TEST_FINAL_CLTV),
amt,
);
if let Err(err) = source.send_payment(
payment_hash,
RecipientOnionFields::secret_only(payment_secret),
PaymentId(payment_id),
route_params,
Retry::Attempts(0),
) {
let sent_amt = amt + first_hop_fee;
check_payment_err(err, sent_amt < min_value_sendable || sent_amt > max_value_sendable);
false
panic!("Errored with {:?} on initial payment send", err);
} else {
// Note that while the max is a strict upper-bound, we can occasionally send substantially
// below the minimum, with some gap which is unusable immediately below the minimum. Thus,
// we don't check against min_value_sendable here.
assert!(amt + first_hop_fee <= max_value_sendable);
true
let sent_amt = amt + first_hop_fee;
check_payment_send_events(source, sent_amt, min_value_sendable, max_value_sendable)
}
}

#[inline]
pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
let out = SearchingOutput::new(underlying_out);
let broadcast = Arc::new(TestBroadcaster {});
let router = FuzzRouter {};
let router = FuzzRouter { next_routes: Mutex::new(VecDeque::new()) };

macro_rules! make_node {
($node_id: expr, $fee_estimator: expr) => {{
Expand Down
15 changes: 11 additions & 4 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,9 @@ use core::time::Duration;
use core::ops::Deref;
use bitcoin::hex::impl_fmt_traits;
// Re-export this for use in the public API.
pub use crate::ln::outbound_payment::{Bolt12PaymentError, PaymentSendFailure, ProbeSendFailure, Retry, RetryableSendFailure, RecipientOnionFields};
pub use crate::ln::outbound_payment::{Bolt12PaymentError, ProbeSendFailure, Retry, RetryableSendFailure, RecipientOnionFields};
#[cfg(test)]
pub(crate) use crate::ln::outbound_payment::PaymentSendFailure;
use crate::ln::script::ShutdownScript;

// We hold various information about HTLC relay in the HTLC objects in Channel itself:
Expand Down Expand Up @@ -2363,7 +2365,9 @@ where
fee_estimator: LowerBoundedFeeEstimator<F>,
chain_monitor: M,
tx_broadcaster: T,
#[allow(unused)]
#[cfg(fuzzing)]
pub router: R,
#[cfg(not(fuzzing))]
router: R,
message_router: MR,

Expand Down Expand Up @@ -4525,8 +4529,11 @@ where
// [`TestRouter::expect_find_route`] instead.
//
// [`TestRouter::expect_find_route`]: crate::util::test_utils::TestRouter::expect_find_route
#[cfg(any(test, fuzzing))]
pub fn send_payment_with_route(&self, route: Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId) -> Result<(), PaymentSendFailure> {
#[cfg(test)]
pub(crate) fn send_payment_with_route(
&self, route: Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields,
payment_id: PaymentId
) -> Result<(), PaymentSendFailure> {
let best_block_height = self.best_block.read().unwrap().height;
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
self.pending_outbound_payments
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/outbound_payment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ pub enum RetryableSendFailure {
/// as the Err() type describing which state the payment is in, see the description of individual
/// enum states for more.
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum PaymentSendFailure {
pub(crate) enum PaymentSendFailure {
/// A parameter which was passed to send_payment was invalid, preventing us from attempting to
/// send the payment at all.
///
Expand Down

0 comments on commit 3956661

Please sign in to comment.