Skip to content

Commit 02cb99c

Browse files
committed
Add fuzz coverage of (potential) fee update messages
1 parent f7628b3 commit 02cb99c

File tree

4 files changed

+121
-20
lines changed

4 files changed

+121
-20
lines changed

fuzz/src/chanmon_consistency.rs

Lines changed: 70 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget,
3737
use lightning::chain::keysinterface::{KeysInterface, InMemorySigner};
3838
use lightning::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
3939
use lightning::ln::channelmanager::{ChainParameters, ChannelManager, PaymentSendFailure, ChannelManagerReadArgs};
40+
use lightning::ln::channel::CHAN_STUCK_FEE_INCREASE_MULTIPLE;
4041
use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
4142
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, DecodeError, UpdateAddHTLC, Init};
4243
use lightning::util::enforcing_trait_impls::{EnforcingSigner, INITIAL_REVOKED_COMMITMENT_NUMBER};
@@ -57,16 +58,27 @@ use bitcoin::secp256k1::recovery::RecoverableSignature;
5758
use bitcoin::secp256k1::Secp256k1;
5859

5960
use std::mem;
60-
use std::cmp::Ordering;
61+
use std::cmp::{self, Ordering};
6162
use std::collections::{HashSet, hash_map, HashMap};
6263
use std::sync::{Arc,Mutex};
6364
use std::sync::atomic;
6465
use std::io::Cursor;
6566

66-
struct FuzzEstimator {}
67+
const MAX_FEE: u32 = 10_000;
68+
struct FuzzEstimator {
69+
ret_val: atomic::AtomicU32,
70+
}
6771
impl FeeEstimator for FuzzEstimator {
68-
fn get_est_sat_per_1000_weight(&self, _: ConfirmationTarget) -> u32 {
69-
253
72+
fn get_est_sat_per_1000_weight(&self, conf_target: ConfirmationTarget) -> u32 {
73+
// We force-close channels if our counterparty sends us a feerate which is a small multiple
74+
// of our HighPriority fee estimate or smaller than our Background fee estimate. Thus, we
75+
// always return a HighPriority feerate here which is >= the maximum Normal feerate and a
76+
// Background feerate which is <= the minimum Normal feerate.
77+
match conf_target {
78+
ConfirmationTarget::HighPriority => MAX_FEE,
79+
ConfirmationTarget::Background => 253,
80+
ConfirmationTarget::Normal => cmp::min(self.ret_val.load(atomic::Ordering::Acquire), MAX_FEE),
81+
}
7082
}
7183
}
7284

@@ -131,7 +143,7 @@ impl chain::Watch<EnforcingSigner> for TestChainMonitor {
131143
};
132144
let deserialized_monitor = <(BlockHash, channelmonitor::ChannelMonitor<EnforcingSigner>)>::
133145
read(&mut Cursor::new(&map_entry.get().1), &*self.keys).unwrap().1;
134-
deserialized_monitor.update_monitor(&update, &&TestBroadcaster{}, &&FuzzEstimator{}, &self.logger).unwrap();
146+
deserialized_monitor.update_monitor(&update, &&TestBroadcaster{}, &&FuzzEstimator { ret_val: atomic::AtomicU32::new(253) }, &self.logger).unwrap();
135147
let mut ser = VecWriter(Vec::new());
136148
deserialized_monitor.write(&mut ser).unwrap();
137149
map_entry.insert((update.update_id, ser.0));
@@ -328,14 +340,13 @@ fn send_hop_payment(source: &ChanMan, middle: &ChanMan, middle_chan_id: u64, des
328340

329341
#[inline]
330342
pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
331-
let fee_est = Arc::new(FuzzEstimator{});
332343
let broadcast = Arc::new(TestBroadcaster{});
333344

334345
macro_rules! make_node {
335-
($node_id: expr) => { {
346+
($node_id: expr, $fee_estimator: expr) => { {
336347
let logger: Arc<dyn Logger> = Arc::new(test_logger::TestLogger::new($node_id.to_string(), out.clone()));
337348
let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU32::new(0), revoked_commitments: Mutex::new(HashMap::new()) });
338-
let monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), fee_est.clone(), Arc::new(TestPersister{}), Arc::clone(&keys_manager)));
349+
let monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), $fee_estimator.clone(), Arc::new(TestPersister{}), Arc::clone(&keys_manager)));
339350

340351
let mut config = UserConfig::default();
341352
config.channel_options.forwarding_fee_proportional_millionths = 0;
@@ -345,16 +356,16 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
345356
network,
346357
best_block: BestBlock::from_genesis(network),
347358
};
348-
(ChannelManager::new(fee_est.clone(), monitor.clone(), broadcast.clone(), Arc::clone(&logger), keys_manager.clone(), config, params),
359+
(ChannelManager::new($fee_estimator.clone(), monitor.clone(), broadcast.clone(), Arc::clone(&logger), keys_manager.clone(), config, params),
349360
monitor, keys_manager)
350361
} }
351362
}
352363

353364
macro_rules! reload_node {
354-
($ser: expr, $node_id: expr, $old_monitors: expr, $keys_manager: expr) => { {
365+
($ser: expr, $node_id: expr, $old_monitors: expr, $keys_manager: expr, $fee_estimator: expr) => { {
355366
let keys_manager = Arc::clone(& $keys_manager);
356367
let logger: Arc<dyn Logger> = Arc::new(test_logger::TestLogger::new($node_id.to_string(), out.clone()));
357-
let chain_monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), fee_est.clone(), Arc::new(TestPersister{}), Arc::clone(& $keys_manager)));
368+
let chain_monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), $fee_estimator.clone(), Arc::new(TestPersister{}), Arc::clone(& $keys_manager)));
358369

359370
let mut config = UserConfig::default();
360371
config.channel_options.forwarding_fee_proportional_millionths = 0;
@@ -373,7 +384,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
373384

374385
let read_args = ChannelManagerReadArgs {
375386
keys_manager,
376-
fee_estimator: fee_est.clone(),
387+
fee_estimator: $fee_estimator.clone(),
377388
chain_monitor: chain_monitor.clone(),
378389
tx_broadcaster: broadcast.clone(),
379390
logger,
@@ -488,11 +499,18 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
488499
} }
489500
}
490501

502+
let fee_est_a = Arc::new(FuzzEstimator { ret_val: atomic::AtomicU32::new(253) });
503+
let mut last_htlc_clear_fee_a = 253;
504+
let fee_est_b = Arc::new(FuzzEstimator { ret_val: atomic::AtomicU32::new(253) });
505+
let mut last_htlc_clear_fee_b = 253;
506+
let fee_est_c = Arc::new(FuzzEstimator { ret_val: atomic::AtomicU32::new(253) });
507+
let mut last_htlc_clear_fee_c = 253;
508+
491509
// 3 nodes is enough to hit all the possible cases, notably unknown-source-unknown-dest
492510
// forwarding.
493-
let (node_a, mut monitor_a, keys_manager_a) = make_node!(0);
494-
let (node_b, mut monitor_b, keys_manager_b) = make_node!(1);
495-
let (node_c, mut monitor_c, keys_manager_c) = make_node!(2);
511+
let (node_a, mut monitor_a, keys_manager_a) = make_node!(0, fee_est_a);
512+
let (node_b, mut monitor_b, keys_manager_b) = make_node!(1, fee_est_b);
513+
let (node_c, mut monitor_c, keys_manager_c) = make_node!(2, fee_est_c);
496514

497515
let mut nodes = [node_a, node_b, node_c];
498516

@@ -630,7 +648,6 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
630648
events::MessageSendEvent::UpdateHTLCs { node_id, updates: CommitmentUpdate { update_add_htlcs, update_fail_htlcs, update_fulfill_htlcs, update_fail_malformed_htlcs, update_fee, commitment_signed } } => {
631649
for dest in nodes.iter() {
632650
if dest.get_our_node_id() == node_id {
633-
assert!(update_fee.is_none());
634651
for update_add in update_add_htlcs.iter() {
635652
if !$corrupt_forward {
636653
dest.handle_update_add_htlc(&nodes[$node].get_our_node_id(), update_add);
@@ -654,6 +671,9 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
654671
for update_fail_malformed in update_fail_malformed_htlcs.iter() {
655672
dest.handle_update_fail_malformed_htlc(&nodes[$node].get_our_node_id(), update_fail_malformed);
656673
}
674+
if let Some(msg) = update_fee {
675+
dest.handle_update_fee(&nodes[$node].get_our_node_id(), &msg);
676+
}
657677
let processed_change = !update_add_htlcs.is_empty() || !update_fulfill_htlcs.is_empty() ||
658678
!update_fail_htlcs.is_empty() || !update_fail_malformed_htlcs.is_empty();
659679
if $limit_events != ProcessMessages::AllMessages && processed_change {
@@ -918,7 +938,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
918938
node_a_ser.0.clear();
919939
nodes[0].write(&mut node_a_ser).unwrap();
920940
}
921-
let (new_node_a, new_monitor_a) = reload_node!(node_a_ser, 0, monitor_a, keys_manager_a);
941+
let (new_node_a, new_monitor_a) = reload_node!(node_a_ser, 0, monitor_a, keys_manager_a, fee_est_a);
922942
nodes[0] = new_node_a;
923943
monitor_a = new_monitor_a;
924944
},
@@ -937,7 +957,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
937957
bc_events.clear();
938958
cb_events.clear();
939959
}
940-
let (new_node_b, new_monitor_b) = reload_node!(node_b_ser, 1, monitor_b, keys_manager_b);
960+
let (new_node_b, new_monitor_b) = reload_node!(node_b_ser, 1, monitor_b, keys_manager_b, fee_est_b);
941961
nodes[1] = new_node_b;
942962
monitor_b = new_monitor_b;
943963
},
@@ -951,7 +971,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
951971
node_c_ser.0.clear();
952972
nodes[2].write(&mut node_c_ser).unwrap();
953973
}
954-
let (new_node_c, new_monitor_c) = reload_node!(node_c_ser, 2, monitor_c, keys_manager_c);
974+
let (new_node_c, new_monitor_c) = reload_node!(node_c_ser, 2, monitor_c, keys_manager_c, fee_est_c);
955975
nodes[2] = new_node_c;
956976
monitor_c = new_monitor_c;
957977
},
@@ -1013,6 +1033,33 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
10131033
0x6c => { send_hop_payment(&nodes[0], &nodes[1], chan_a, &nodes[2], chan_b, 1, &mut payment_id); },
10141034
0x6d => { send_hop_payment(&nodes[2], &nodes[1], chan_b, &nodes[0], chan_a, 1, &mut payment_id); },
10151035

1036+
0x80 => {
1037+
let max_feerate = last_htlc_clear_fee_a * CHAN_STUCK_FEE_INCREASE_MULTIPLE as u32;
1038+
if fee_est_a.ret_val.fetch_add(250, atomic::Ordering::AcqRel) + 250 > max_feerate {
1039+
fee_est_a.ret_val.store(max_feerate, atomic::Ordering::Release);
1040+
}
1041+
nodes[0].maybe_update_chan_fees();
1042+
},
1043+
0x81 => { fee_est_a.ret_val.store(253, atomic::Ordering::Release); nodes[0].maybe_update_chan_fees(); },
1044+
1045+
0x84 => {
1046+
let max_feerate = last_htlc_clear_fee_b * CHAN_STUCK_FEE_INCREASE_MULTIPLE as u32;
1047+
if fee_est_b.ret_val.fetch_add(250, atomic::Ordering::AcqRel) + 250 > max_feerate {
1048+
fee_est_b.ret_val.store(max_feerate, atomic::Ordering::Release);
1049+
}
1050+
nodes[1].maybe_update_chan_fees();
1051+
},
1052+
0x85 => { fee_est_b.ret_val.store(253, atomic::Ordering::Release); nodes[1].maybe_update_chan_fees(); },
1053+
1054+
0x88 => {
1055+
let max_feerate = last_htlc_clear_fee_c * CHAN_STUCK_FEE_INCREASE_MULTIPLE as u32;
1056+
if fee_est_c.ret_val.fetch_add(250, atomic::Ordering::AcqRel) + 250 > max_feerate {
1057+
fee_est_c.ret_val.store(max_feerate, atomic::Ordering::Release);
1058+
}
1059+
nodes[2].maybe_update_chan_fees();
1060+
},
1061+
0x89 => { fee_est_c.ret_val.store(253, atomic::Ordering::Release); nodes[2].maybe_update_chan_fees(); },
1062+
10161063
0xff => {
10171064
// Test that no channel is in a stuck state where neither party can send funds even
10181065
// after we resolve all pending events.
@@ -1068,6 +1115,10 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
10681115
assert!(
10691116
send_payment(&nodes[1], &nodes[2], chan_b, 10_000_000, &mut payment_id) ||
10701117
send_payment(&nodes[2], &nodes[1], chan_b, 10_000_000, &mut payment_id));
1118+
1119+
last_htlc_clear_fee_a = fee_est_a.ret_val.load(atomic::Ordering::Acquire);
1120+
last_htlc_clear_fee_b = fee_est_b.ret_val.load(atomic::Ordering::Acquire);
1121+
last_htlc_clear_fee_c = fee_est_c.ret_val.load(atomic::Ordering::Acquire);
10711122
},
10721123
_ => test_return!(),
10731124
}

lightning/src/ln/channel.rs

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,21 @@ pub struct CounterpartyForwardingInfo {
301301
pub cltv_expiry_delta: u16,
302302
}
303303

304+
/// When a node increases the feerate on an outbound channel, it can cause the channel to get
305+
/// "stuck" if the other side holds most of the funds, and suddenly is unable to send an HTLC as it
306+
/// would effectively debit the channel initiator to pay for the new HTLC's fee. If the initiator's
307+
/// balance is right at or near the channel reserve, neither side will be able to send an HTLC.
308+
/// Thus, before sending an HTLC when we are the initiator, we check that the feerate can increase
309+
/// by this multiple without hitting this case, before sending.
310+
/// This multiple is effectively the maximum feerate "jump" we expect until more HTLCs flow over
311+
/// the channel. Sadly, there isn't really a good number for this - if we expect to have no new
312+
/// HTLCs for days we may need this to suffice for feerate increases across days, but that may
313+
/// leave the channel less usable as we hold a bigger reserve.
314+
#[cfg(fuzzing)]
315+
pub const CHAN_STUCK_FEE_INCREASE_MULTIPLE: u64 = 2;
316+
#[cfg(not(fuzzing))]
317+
const CHAN_STUCK_FEE_INCREASE_MULTIPLE: u64 = 2;
318+
304319
// TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking
305320
// has been completed, and then turn into a Channel to get compiler-time enforcement of things like
306321
// calling channel_id() before we're set up or things like get_outbound_funding_signed on an
@@ -4102,7 +4117,7 @@ impl<Signer: Sign> Channel<Signer> {
41024117
// `2 *` and extra HTLC are for the fee spike buffer.
41034118
let commit_tx_fee_msat = if self.is_outbound() {
41044119
let htlc_candidate = HTLCCandidate::new(amount_msat, HTLCInitiator::LocalOffered);
4105-
2 * self.next_local_commit_tx_fee_msat(htlc_candidate, Some(()))
4120+
CHAN_STUCK_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_candidate, Some(()))
41064121
} else { 0 };
41074122
if pending_value_to_self_msat - amount_msat < commit_tx_fee_msat {
41084123
return Err(ChannelError::Ignore(format!("Cannot send value that would not leave enough to pay for fees. Pending value to self: {}. local_commit_tx_fee {}", pending_value_to_self_msat, commit_tx_fee_msat)));

lightning/src/ln/channelmanager.rs

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2453,6 +2453,37 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
24532453
(retain_channel, should_persist, ret_err)
24542454
}
24552455

2456+
#[cfg(fuzzing)]
2457+
/// In chanmon_consistency we want to sometimes do the channel fee updates done in
2458+
/// timer_tick_occurred, but we can't generate the disabled channel updates as it considers
2459+
/// these a fuzz failure (as they usually indicate a channel force-close, which is exactly what
2460+
/// it wants to detect). Thus, we have a variant exposed here for its benefit.
2461+
pub fn maybe_update_chan_fees(&self) {
2462+
PersistenceNotifierGuard::optionally_notify(&self.total_consistency_lock, &self.persistence_notifier, || {
2463+
let mut should_persist = NotifyOption::SkipPersist;
2464+
2465+
let new_feerate = self.fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
2466+
2467+
let mut handle_errors = Vec::new();
2468+
{
2469+
let mut channel_state_lock = self.channel_state.lock().unwrap();
2470+
let channel_state = &mut *channel_state_lock;
2471+
let pending_msg_events = &mut channel_state.pending_msg_events;
2472+
let short_to_id = &mut channel_state.short_to_id;
2473+
channel_state.by_id.retain(|chan_id, chan| {
2474+
let (retain_channel, chan_needs_persist, err) = self.update_channel_fee(short_to_id, pending_msg_events, chan_id, chan, new_feerate);
2475+
if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
2476+
if err.is_err() {
2477+
handle_errors.push(err);
2478+
}
2479+
retain_channel
2480+
});
2481+
}
2482+
2483+
should_persist
2484+
});
2485+
}
2486+
24562487
/// If a peer is disconnected we mark any channels with that peer as 'disabled'.
24572488
/// After some time, if channels are still disabled we need to broadcast a ChannelUpdate
24582489
/// to inform the network about the uselessness of these channels.

lightning/src/ln/mod.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,11 @@ pub mod peer_channel_encryptor;
3333
#[cfg(not(feature = "fuzztarget"))]
3434
pub(crate) mod peer_channel_encryptor;
3535

36+
#[cfg(feature = "fuzztarget")]
37+
pub mod channel;
38+
#[cfg(not(feature = "fuzztarget"))]
3639
mod channel;
40+
3741
mod onion_utils;
3842
mod wire;
3943

0 commit comments

Comments
 (0)