From f961daef33ad1e999c83aafbf654db449e0e93e0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 29 Sep 2022 20:26:48 +0000 Subject: [PATCH] Rename APIError::MonitorUpdateFailed to MonitorUpdateInProgress This much more accurately represents the error, indicating that a monitor update is in progress asynchronously and may complete at a later time. --- fuzz/src/chanmon_consistency.rs | 2 +- lightning-invoice/src/payment.rs | 18 +++++----- lightning/src/ln/chanmon_update_fail_tests.rs | 12 +++---- lightning/src/ln/channelmanager.rs | 35 ++++++++++--------- lightning/src/util/errors.rs | 14 ++++---- 5 files changed, 42 insertions(+), 39 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 7944d04ad..a02d003e1 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -270,7 +270,7 @@ fn check_api_err(api_err: APIError) { _ => panic!("{}", err), } }, - APIError::MonitorUpdateFailed => { + APIError::MonitorUpdateInProgress => { // We can (obviously) temp-fail a monitor update }, APIError::IncompatibleShutdownScript { .. } => panic!("Cannot send an incompatible shutdown script"), diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs index b9f448633..e8e296294 100644 --- a/lightning-invoice/src/payment.rs +++ b/lightning-invoice/src/payment.rs @@ -513,11 +513,11 @@ where }, PaymentSendFailure::PartialFailure { failed_paths_retry, payment_id, results } => { // If a `PartialFailure` event returns a result that is an `Ok()`, it means that - // part of our payment is retried. When we receive `MonitorUpdateFailed`, it + // part of our payment is retried. When we receive `MonitorUpdateInProgress`, it // means that we are still waiting for our channel monitor update to be completed. for (result, path) in results.iter().zip(route.paths.into_iter()) { match result { - Ok(_) | Err(APIError::MonitorUpdateFailed) => { + Ok(_) | Err(APIError::MonitorUpdateInProgress) => { self.process_path_inflight_htlcs(payment_hash, path); }, _ => {}, @@ -617,11 +617,11 @@ where }, Err(PaymentSendFailure::PartialFailure { failed_paths_retry, results, .. }) => { // If a `PartialFailure` error contains a result that is an `Ok()`, it means that - // part of our payment is retried. When we receive `MonitorUpdateFailed`, it + // part of our payment is retried. When we receive `MonitorUpdateInProgress`, it // means that we are still waiting for our channel monitor update to complete. for (result, path) in results.iter().zip(route.unwrap().paths.into_iter()) { match result { - Ok(_) | Err(APIError::MonitorUpdateFailed) => { + Ok(_) | Err(APIError::MonitorUpdateInProgress) => { self.process_path_inflight_htlcs(payment_hash, path); }, _ => {}, @@ -796,7 +796,7 @@ mod tests { use std::time::{SystemTime, Duration}; use time_utils::tests::SinceEpoch; use DEFAULT_EXPIRY_TIME; - use lightning::util::errors::APIError::{ChannelUnavailable, MonitorUpdateFailed}; + use lightning::util::errors::APIError::{ChannelUnavailable, MonitorUpdateInProgress}; fn invoice(payment_preimage: PaymentPreimage) -> Invoice { let payment_hash = Sha256::hash(&payment_preimage.0); @@ -1718,7 +1718,7 @@ mod tests { .fails_with_partial_failure( retry.clone(), OnAttempt(1), Some(vec![ - Err(ChannelUnavailable { err: "abc".to_string() }), Err(MonitorUpdateFailed) + Err(ChannelUnavailable { err: "abc".to_string() }), Err(MonitorUpdateInProgress) ])) .expect_send(Amount::ForInvoice(final_value_msat)); @@ -1731,7 +1731,7 @@ mod tests { invoice_payer.pay_invoice(&invoice_to_pay).unwrap(); let inflight_map = invoice_payer.create_inflight_map(); - // Only the second path, which failed with `MonitorUpdateFailed` should be added to our + // Only the second path, which failed with `MonitorUpdateInProgress` should be added to our // inflight map because retries are disabled. assert_eq!(inflight_map.len(), 2); } @@ -1750,7 +1750,7 @@ mod tests { .fails_with_partial_failure( retry.clone(), OnAttempt(1), Some(vec![ - Ok(()), Err(MonitorUpdateFailed) + Ok(()), Err(MonitorUpdateInProgress) ])) .expect_send(Amount::ForInvoice(final_value_msat)); @@ -2044,7 +2044,7 @@ mod tests { } fn fails_on_attempt(self, attempt: usize) -> Self { - let failure = PaymentSendFailure::ParameterError(APIError::MonitorUpdateFailed); + let failure = PaymentSendFailure::ParameterError(APIError::MonitorUpdateInProgress); self.fails_with(failure, OnAttempt(attempt)) } diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index c0648eb2b..15d46b046 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -170,7 +170,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) { chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); { - unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash_1, &Some(payment_secret_1)), false, APIError::MonitorUpdateFailed, {}); + unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash_1, &Some(payment_secret_1)), false, APIError::MonitorUpdateInProgress, {}); check_added_monitors!(nodes[0], 1); } @@ -221,7 +221,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) { let (route, payment_hash_2, _, payment_secret_2) = get_route_and_payment_hash!(&nodes[0], nodes[1], 1000000); { chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); - unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)), false, APIError::MonitorUpdateFailed, {}); + unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)), false, APIError::MonitorUpdateInProgress, {}); check_added_monitors!(nodes[0], 1); } @@ -285,7 +285,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) { let (route, payment_hash_2, payment_preimage_2, payment_secret_2) = get_route_and_payment_hash!(nodes[0], nodes[1], 1000000); { chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); - unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)), false, APIError::MonitorUpdateFailed, {}); + unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash_2, &Some(payment_secret_2)), false, APIError::MonitorUpdateInProgress, {}); check_added_monitors!(nodes[0], 1); } @@ -1962,12 +1962,12 @@ fn test_path_paused_mpp() { chanmon_cfgs[0].persister.set_next_update_ret(Some(ChannelMonitorUpdateStatus::InProgress)); // Now check that we get the right return value, indicating that the first path succeeded but - // the second got a MonitorUpdateFailed err. This implies PaymentSendFailure::PartialFailure as - // some paths succeeded, preventing retry. + // the second got a MonitorUpdateInProgress err. This implies + // PaymentSendFailure::PartialFailure as some paths succeeded, preventing retry. if let Err(PaymentSendFailure::PartialFailure { results, ..}) = nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)) { assert_eq!(results.len(), 2); if let Ok(()) = results[0] {} else { panic!(); } - if let Err(APIError::MonitorUpdateFailed) = results[1] {} else { panic!(); } + if let Err(APIError::MonitorUpdateInProgress) = results[1] {} else { panic!(); } } else { panic!(); } check_added_monitors!(nodes[0], 2); chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index bd3c6d501..dbeee4161 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1166,13 +1166,13 @@ pub enum PaymentSendFailure { /// in over-/re-payment. /// /// The results here are ordered the same as the paths in the route object which was passed to - /// send_payment, and any Errs which are not APIError::MonitorUpdateFailed can be safely - /// retried (though there is currently no API with which to do so). + /// send_payment, and any `Err`s which are not [`APIError::MonitorUpdateInProgress`] can be + /// safely retried via [`ChannelManager::retry_payment`]. /// - /// Any entries which contain Err(APIError::MonitorUpdateFailed) or Ok(()) MUST NOT be retried - /// as they will result in over-/re-payment. These HTLCs all either successfully sent (in the - /// case of Ok(())) or will send once a [`MonitorEvent::Completed`] is provided for the - /// next-hop channel with the latest update_id. + /// Any entries which contain `Err(APIError::MonitorUpdateInprogress)` or `Ok(())` MUST NOT be + /// retried as they will result in over-/re-payment. These HTLCs all either successfully sent + /// (in the case of `Ok(())`) or will send once a [`MonitorEvent::Completed`] is provided for + /// the next-hop channel with the latest update_id. PartialFailure { /// The errors themselves, in the same order as the route hops. results: Vec>, @@ -2469,13 +2469,14 @@ impl ChannelMana insert_outbound_payment!(); }, (ChannelMonitorUpdateStatus::InProgress, Err(_)) => { - // Note that MonitorUpdateFailed here indicates (per function docs) - // that we will resend the commitment update once monitor updating - // is restored. Therefore, we must return an error indicating that - // it is unsafe to retry the payment wholesale, which we do in the - // send_payment check for MonitorUpdateFailed, below. + // Note that MonitorUpdateInProgress here indicates (per function + // docs) that we will resend the commitment update once monitor + // updating completes. Therefore, we must return an error + // indicating that it is unsafe to retry the payment wholesale, + // which we do in the send_payment check for + // MonitorUpdateInProgress, below. insert_outbound_payment!(); // Only do this after possibly break'ing on Perm failure above. - return Err(APIError::MonitorUpdateFailed); + return Err(APIError::MonitorUpdateInProgress); }, _ => unreachable!(), } @@ -2526,12 +2527,12 @@ impl ChannelMana /// PaymentSendFailure for more info. /// /// In general, a path may raise: - /// * APIError::RouteError when an invalid route or forwarding parameter (cltv_delta, fee, + /// * [`APIError::RouteError`] when an invalid route or forwarding parameter (cltv_delta, fee, /// node public key) is specified. - /// * APIError::ChannelUnavailable if the next-hop channel is not available for updates + /// * [`APIError::ChannelUnavailable`] if the next-hop channel is not available for updates /// (including due to previous monitor update failure or new permanent monitor update /// failure). - /// * APIError::MonitorUpdateFailed if a new monitor update failure prevented sending the + /// * [`APIError::MonitorUpdateInProgress`] if a new monitor update failure prevented sending the /// relevant updates. /// /// Note that depending on the type of the PaymentSendFailure the HTLC may have been @@ -2595,8 +2596,8 @@ impl ChannelMana for (res, path) in results.iter().zip(route.paths.iter()) { if res.is_ok() { has_ok = true; } if res.is_err() { has_err = true; } - if let &Err(APIError::MonitorUpdateFailed) = res { - // MonitorUpdateFailed is inherently unsafe to retry, so we call it a + if let &Err(APIError::MonitorUpdateInProgress) = res { + // MonitorUpdateInProgress is inherently unsafe to retry, so we call it a // PartialFailure. has_err = true; has_ok = true; diff --git a/lightning/src/util/errors.rs b/lightning/src/util/errors.rs index 15dd202b9..ad6993542 100644 --- a/lightning/src/util/errors.rs +++ b/lightning/src/util/errors.rs @@ -46,13 +46,15 @@ pub enum APIError { /// A human-readable error message err: String }, - /// An attempt to call watch/update_channel returned a - /// [`ChannelMonitorUpdateStatus::InProgress`] indicating the persistence of a monitor update - /// is awaiting async resolution. Once it resolves the attempted action should complete - /// automatically. + /// An attempt to call [`chain::Watch::watch_channel`]/[`chain::Watch::update_channel`] + /// returned a [`ChannelMonitorUpdateStatus::InProgress`] indicating the persistence of a + /// monitor update is awaiting async resolution. Once it resolves the attempted action should + /// complete automatically. /// + /// [`chain::Watch::watch_channel`]: crate::chain::Watch::watch_channel + /// [`chain::Watch::update_channel`]: crate::chain::Watch::update_channel /// [`ChannelMonitorUpdateStatus::InProgress`]: crate::chain::ChannelMonitorUpdateStatus::InProgress - MonitorUpdateFailed, + MonitorUpdateInProgress, /// [`KeysInterface::get_shutdown_scriptpubkey`] returned a shutdown scriptpubkey incompatible /// with the channel counterparty as negotiated in [`InitFeatures`]. /// @@ -74,7 +76,7 @@ impl fmt::Debug for APIError { APIError::FeeRateTooHigh {ref err, ref feerate} => write!(f, "{} feerate: {}", err, feerate), APIError::RouteError {ref err} => write!(f, "Route error: {}", err), APIError::ChannelUnavailable {ref err} => write!(f, "Channel unavailable: {}", err), - APIError::MonitorUpdateFailed => f.write_str("Client indicated a channel monitor update failed"), + APIError::MonitorUpdateInProgress => f.write_str("Client indicated a channel monitor update is in progress but not yet complete"), APIError::IncompatibleShutdownScript { ref script } => { write!(f, "Provided a scriptpubkey format not accepted by peer: {}", script) }, -- 2.39.5