Skip to content

Commit 7544030

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #1106 from TheBlueMatt/2021-10-no-perm-err-broadcast
Do not broadcast commitment txn on Permanent mon update failure
2 parents 7bc52aa + 52934e0 commit 7544030

19 files changed

+566
-476
lines changed

‎fuzz/src/chanmon_consistency.rs

+21-16
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ use bitcoin::hashes::sha256::Hash as Sha256;
3232
use bitcoin::hash_types::{BlockHash, WPubkeyHash};
3333

3434
use lightning::chain;
35-
use lightning::chain::{BestBlock, ChannelMonitorUpdateErr, chainmonitor, channelmonitor, Confirm, Watch};
35+
use lightning::chain::{BestBlock, ChannelMonitorUpdateStatus, chainmonitor, channelmonitor, Confirm, Watch};
3636
use lightning::chain::channelmonitor::{ChannelMonitor, MonitorEvent};
3737
use lightning::chain::transaction::OutPoint;
3838
use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
@@ -124,7 +124,7 @@ impl TestChainMonitor {
124124
}
125125
}
126126
impl chain::Watch<EnforcingSigner> for TestChainMonitor {
127-
fn watch_channel(&self, funding_txo: OutPoint, monitor: channelmonitor::ChannelMonitor<EnforcingSigner>) -> Result<(), chain::ChannelMonitorUpdateErr> {
127+
fn watch_channel(&self, funding_txo: OutPoint, monitor: channelmonitor::ChannelMonitor<EnforcingSigner>) -> chain::ChannelMonitorUpdateStatus {
128128
let mut ser = VecWriter(Vec::new());
129129
monitor.write(&mut ser).unwrap();
130130
if let Some(_) = self.latest_monitors.lock().unwrap().insert(funding_txo, (monitor.get_latest_update_id(), ser.0)) {
@@ -134,7 +134,7 @@ impl chain::Watch<EnforcingSigner> for TestChainMonitor {
134134
self.chain_monitor.watch_channel(funding_txo, monitor)
135135
}
136136

137-
fn update_channel(&self, funding_txo: OutPoint, update: channelmonitor::ChannelMonitorUpdate) -> Result<(), chain::ChannelMonitorUpdateErr> {
137+
fn update_channel(&self, funding_txo: OutPoint, update: channelmonitor::ChannelMonitorUpdate) -> chain::ChannelMonitorUpdateStatus {
138138
let mut map_lock = self.latest_monitors.lock().unwrap();
139139
let mut map_entry = match map_lock.entry(funding_txo) {
140140
hash_map::Entry::Occupied(entry) => entry,
@@ -270,7 +270,7 @@ fn check_api_err(api_err: APIError) {
270270
_ => panic!("{}", err),
271271
}
272272
},
273-
APIError::MonitorUpdateFailed => {
273+
APIError::MonitorUpdateInProgress => {
274274
// We can (obviously) temp-fail a monitor update
275275
},
276276
APIError::IncompatibleShutdownScript { .. } => panic!("Cannot send an incompatible shutdown script"),
@@ -363,7 +363,9 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
363363
let logger: Arc<dyn Logger> = Arc::new(test_logger::TestLogger::new($node_id.to_string(), out.clone()));
364364
let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU32::new(0), enforcement_states: Mutex::new(HashMap::new()) });
365365
let monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), $fee_estimator.clone(),
366-
Arc::new(TestPersister { update_ret: Mutex::new(Ok(())) }), Arc::clone(&keys_manager)));
366+
Arc::new(TestPersister {
367+
update_ret: Mutex::new(ChannelMonitorUpdateStatus::Completed)
368+
}), Arc::clone(&keys_manager)));
367369

368370
let mut config = UserConfig::default();
369371
config.channel_config.forwarding_fee_proportional_millionths = 0;
@@ -383,7 +385,9 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
383385
let keys_manager = Arc::clone(& $keys_manager);
384386
let logger: Arc<dyn Logger> = Arc::new(test_logger::TestLogger::new($node_id.to_string(), out.clone()));
385387
let chain_monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), $fee_estimator.clone(),
386-
Arc::new(TestPersister { update_ret: Mutex::new(Ok(())) }), Arc::clone(& $keys_manager)));
388+
Arc::new(TestPersister {
389+
update_ret: Mutex::new(ChannelMonitorUpdateStatus::Completed)
390+
}), Arc::clone(& $keys_manager)));
387391

388392
let mut config = UserConfig::default();
389393
config.channel_config.forwarding_fee_proportional_millionths = 0;
@@ -412,7 +416,8 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
412416

413417
let res = (<(BlockHash, ChanMan)>::read(&mut Cursor::new(&$ser.0), read_args).expect("Failed to read manager").1, chain_monitor.clone());
414418
for (funding_txo, mon) in monitors.drain() {
415-
assert!(chain_monitor.chain_monitor.watch_channel(funding_txo, mon).is_ok());
419+
assert_eq!(chain_monitor.chain_monitor.watch_channel(funding_txo, mon),
420+
ChannelMonitorUpdateStatus::Completed);
416421
}
417422
res
418423
} }
@@ -889,12 +894,12 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
889894
// bit-twiddling mutations to have similar effects. This is probably overkill, but no
890895
// harm in doing so.
891896

892-
0x00 => *monitor_a.persister.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure),
893-
0x01 => *monitor_b.persister.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure),
894-
0x02 => *monitor_c.persister.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure),
895-
0x04 => *monitor_a.persister.update_ret.lock().unwrap() = Ok(()),
896-
0x05 => *monitor_b.persister.update_ret.lock().unwrap() = Ok(()),
897-
0x06 => *monitor_c.persister.update_ret.lock().unwrap() = Ok(()),
897+
0x00 => *monitor_a.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::InProgress,
898+
0x01 => *monitor_b.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::InProgress,
899+
0x02 => *monitor_c.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::InProgress,
900+
0x04 => *monitor_a.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed,
901+
0x05 => *monitor_b.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed,
902+
0x06 => *monitor_c.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed,
898903

899904
0x08 => {
900905
if let Some((id, _)) = monitor_a.latest_monitors.lock().unwrap().get(&chan_1_funding) {
@@ -1119,9 +1124,9 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
11191124
// after we resolve all pending events.
11201125
// First make sure there are no pending monitor updates, resetting the error state
11211126
// and calling force_channel_monitor_updated for each monitor.
1122-
*monitor_a.persister.update_ret.lock().unwrap() = Ok(());
1123-
*monitor_b.persister.update_ret.lock().unwrap() = Ok(());
1124-
*monitor_c.persister.update_ret.lock().unwrap() = Ok(());
1127+
*monitor_a.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed;
1128+
*monitor_b.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed;
1129+
*monitor_c.persister.update_ret.lock().unwrap() = ChannelMonitorUpdateStatus::Completed;
11251130

11261131
if let Some((id, _)) = monitor_a.latest_monitors.lock().unwrap().get(&chan_1_funding) {
11271132
monitor_a.chain_monitor.force_channel_monitor_updated(chan_1_funding, *id);

‎fuzz/src/full_stack.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ use bitcoin::hashes::sha256::Hash as Sha256;
2929
use bitcoin::hash_types::{Txid, BlockHash, WPubkeyHash};
3030

3131
use lightning::chain;
32-
use lightning::chain::{BestBlock, Confirm, Listen};
32+
use lightning::chain::{BestBlock, ChannelMonitorUpdateStatus, Confirm, Listen};
3333
use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
3434
use lightning::chain::chainmonitor;
3535
use lightning::chain::transaction::OutPoint;
@@ -389,7 +389,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
389389

390390
let broadcast = Arc::new(TestBroadcaster{ txn_broadcasted: Mutex::new(Vec::new()) });
391391
let monitor = Arc::new(chainmonitor::ChainMonitor::new(None, broadcast.clone(), Arc::clone(&logger), fee_est.clone(),
392-
Arc::new(TestPersister { update_ret: Mutex::new(Ok(())) })));
392+
Arc::new(TestPersister { update_ret: Mutex::new(ChannelMonitorUpdateStatus::Completed) })));
393393

394394
let keys_manager = Arc::new(KeyProvider { node_secret: our_network_key.clone(), inbound_payment_key: KeyMaterial(inbound_payment_key.try_into().unwrap()), counter: AtomicU64::new(0) });
395395
let mut config = UserConfig::default();

‎fuzz/src/utils/test_persister.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,14 @@ use lightning::util::enforcing_trait_impls::EnforcingSigner;
77
use std::sync::Mutex;
88

99
pub struct TestPersister {
10-
pub update_ret: Mutex<Result<(), chain::ChannelMonitorUpdateErr>>,
10+
pub update_ret: Mutex<chain::ChannelMonitorUpdateStatus>,
1111
}
1212
impl chainmonitor::Persist<EnforcingSigner> for TestPersister {
13-
fn persist_new_channel(&self, _funding_txo: OutPoint, _data: &channelmonitor::ChannelMonitor<EnforcingSigner>, _update_id: MonitorUpdateId) -> Result<(), chain::ChannelMonitorUpdateErr> {
13+
fn persist_new_channel(&self, _funding_txo: OutPoint, _data: &channelmonitor::ChannelMonitor<EnforcingSigner>, _update_id: MonitorUpdateId) -> chain::ChannelMonitorUpdateStatus {
1414
self.update_ret.lock().unwrap().clone()
1515
}
1616

17-
fn update_persisted_channel(&self, _funding_txo: OutPoint, _update: &Option<channelmonitor::ChannelMonitorUpdate>, _data: &channelmonitor::ChannelMonitor<EnforcingSigner>, _update_id: MonitorUpdateId) -> Result<(), chain::ChannelMonitorUpdateErr> {
17+
fn update_persisted_channel(&self, _funding_txo: OutPoint, _update: &Option<channelmonitor::ChannelMonitorUpdate>, _data: &channelmonitor::ChannelMonitor<EnforcingSigner>, _update_id: MonitorUpdateId) -> chain::ChannelMonitorUpdateStatus {
1818
self.update_ret.lock().unwrap().clone()
1919
}
2020
}

‎lightning-invoice/src/payment.rs

+9-9
Original file line numberDiff line numberDiff line change
@@ -474,11 +474,11 @@ where
474474
},
475475
PaymentSendFailure::PartialFailure { failed_paths_retry, payment_id, results } => {
476476
// If a `PartialFailure` event returns a result that is an `Ok()`, it means that
477-
// part of our payment is retried. When we receive `MonitorUpdateFailed`, it
477+
// part of our payment is retried. When we receive `MonitorUpdateInProgress`, it
478478
// means that we are still waiting for our channel monitor update to be completed.
479479
for (result, path) in results.iter().zip(route.paths.into_iter()) {
480480
match result {
481-
Ok(_) | Err(APIError::MonitorUpdateFailed) => {
481+
Ok(_) | Err(APIError::MonitorUpdateInProgress) => {
482482
self.process_path_inflight_htlcs(payment_hash, path);
483483
},
484484
_ => {},
@@ -578,11 +578,11 @@ where
578578
},
579579
Err(PaymentSendFailure::PartialFailure { failed_paths_retry, results, .. }) => {
580580
// If a `PartialFailure` error contains a result that is an `Ok()`, it means that
581-
// part of our payment is retried. When we receive `MonitorUpdateFailed`, it
581+
// part of our payment is retried. When we receive `MonitorUpdateInProgress`, it
582582
// means that we are still waiting for our channel monitor update to complete.
583583
for (result, path) in results.iter().zip(route.unwrap().paths.into_iter()) {
584584
match result {
585-
Ok(_) | Err(APIError::MonitorUpdateFailed) => {
585+
Ok(_) | Err(APIError::MonitorUpdateInProgress) => {
586586
self.process_path_inflight_htlcs(payment_hash, path);
587587
},
588588
_ => {},
@@ -782,7 +782,7 @@ mod tests {
782782
use std::time::{SystemTime, Duration};
783783
use time_utils::tests::SinceEpoch;
784784
use DEFAULT_EXPIRY_TIME;
785-
use lightning::util::errors::APIError::{ChannelUnavailable, MonitorUpdateFailed};
785+
use lightning::util::errors::APIError::{ChannelUnavailable, MonitorUpdateInProgress};
786786

787787
fn invoice(payment_preimage: PaymentPreimage) -> Invoice {
788788
let payment_hash = Sha256::hash(&payment_preimage.0);
@@ -1683,7 +1683,7 @@ mod tests {
16831683
.fails_with_partial_failure(
16841684
retry.clone(), OnAttempt(1),
16851685
Some(vec![
1686-
Err(ChannelUnavailable { err: "abc".to_string() }), Err(MonitorUpdateFailed)
1686+
Err(ChannelUnavailable { err: "abc".to_string() }), Err(MonitorUpdateInProgress)
16871687
]))
16881688
.expect_send(Amount::ForInvoice(final_value_msat));
16891689

@@ -1695,7 +1695,7 @@ mod tests {
16951695
invoice_payer.pay_invoice(&invoice_to_pay).unwrap();
16961696
let inflight_map = invoice_payer.create_inflight_map();
16971697

1698-
// Only the second path, which failed with `MonitorUpdateFailed` should be added to our
1698+
// Only the second path, which failed with `MonitorUpdateInProgress` should be added to our
16991699
// inflight map because retries are disabled.
17001700
assert_eq!(inflight_map.0.len(), 2);
17011701
}
@@ -1714,7 +1714,7 @@ mod tests {
17141714
.fails_with_partial_failure(
17151715
retry.clone(), OnAttempt(1),
17161716
Some(vec![
1717-
Ok(()), Err(MonitorUpdateFailed)
1717+
Ok(()), Err(MonitorUpdateInProgress)
17181718
]))
17191719
.expect_send(Amount::ForInvoice(final_value_msat));
17201720

@@ -2039,7 +2039,7 @@ mod tests {
20392039
}
20402040

20412041
fn fails_on_attempt(self, attempt: usize) -> Self {
2042-
let failure = PaymentSendFailure::ParameterError(APIError::MonitorUpdateFailed);
2042+
let failure = PaymentSendFailure::ParameterError(APIError::MonitorUpdateInProgress);
20432043
self.fails_with(failure, OnAttempt(attempt))
20442044
}
20452045

‎lightning-persister/src/lib.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ mod tests {
138138
use bitcoin::blockdata::block::{Block, BlockHeader};
139139
use bitcoin::hashes::hex::FromHex;
140140
use bitcoin::{Txid, TxMerkleNode};
141-
use lightning::chain::ChannelMonitorUpdateErr;
141+
use lightning::chain::ChannelMonitorUpdateStatus;
142142
use lightning::chain::chainmonitor::Persist;
143143
use lightning::chain::transaction::OutPoint;
144144
use lightning::{check_closed_broadcast, check_closed_event, check_added_monitors};
@@ -270,7 +270,7 @@ mod tests {
270270
index: 0
271271
};
272272
match persister.persist_new_channel(test_txo, &added_monitors[0].1, update_id.2) {
273-
Err(ChannelMonitorUpdateErr::PermanentFailure) => {},
273+
ChannelMonitorUpdateStatus::PermanentFailure => {},
274274
_ => panic!("unexpected result from persisting new channel")
275275
}
276276

@@ -307,7 +307,7 @@ mod tests {
307307
index: 0
308308
};
309309
match persister.persist_new_channel(test_txo, &added_monitors[0].1, update_id.2) {
310-
Err(ChannelMonitorUpdateErr::PermanentFailure) => {},
310+
ChannelMonitorUpdateStatus::PermanentFailure => {},
311311
_ => panic!("unexpected result from persisting new channel")
312312
}
313313

0 commit comments

Comments
 (0)