From 73bc0f61b912ab0617c77192328faedc4abf1502 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 5 May 2024 23:10:39 +0000 Subject: [PATCH 1/7] Add `ChannelError::close` constructor --- lightning/src/ln/channel.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 716eba7cf50..14ebc663f95 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -733,6 +733,12 @@ impl fmt::Display for ChannelError { } } +impl ChannelError { + pub(super) fn close(err: String) -> Self { + ChannelError::Close(err.clone()) + } +} + pub(super) struct WithChannelContext<'a, L: Deref> where L::Target: Logger { pub logger: &'a L, pub peer_id: Option, From 3e09d9937e67b4f34c033c313f527235a8428a67 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 5 May 2024 23:52:08 +0000 Subject: [PATCH 2/7] Use `ChannelError::close` constructor when building a close variant In the next commit we'll add a second field to `ChannelError::Close` so here we prep by converting existing calls to the constructor function, which is almost a full-file sed. --- lightning/src/ln/channel.rs | 276 ++++++++++++++--------------- lightning/src/ln/channelmanager.rs | 34 ++-- 2 files changed, 155 insertions(+), 155 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 14ebc663f95..37ce1082e0a 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -773,7 +773,7 @@ macro_rules! secp_check { ($res: expr, $err: expr) => { match $res { Ok(thing) => thing, - Err(_) => return Err(ChannelError::Close($err)), + Err(_) => return Err(ChannelError::close($err)), } }; } @@ -1404,90 +1404,90 @@ impl ChannelContext where SP::Target: SignerProvider { let pubkeys = holder_signer.pubkeys().clone(); if config.channel_handshake_config.our_to_self_delay < BREAKDOWN_TIMEOUT { - return Err(ChannelError::Close(format!("Configured with an unreasonable our_to_self_delay ({}) putting user funds at risks. It must be greater than {}", config.channel_handshake_config.our_to_self_delay, BREAKDOWN_TIMEOUT))); + return Err(ChannelError::close(format!("Configured with an unreasonable our_to_self_delay ({}) putting user funds at risks. It must be greater than {}", config.channel_handshake_config.our_to_self_delay, BREAKDOWN_TIMEOUT))); } // Check sanity of message fields: if channel_value_satoshis > config.channel_handshake_limits.max_funding_satoshis { - return Err(ChannelError::Close(format!( + return Err(ChannelError::close(format!( "Per our config, funding must be at most {}. It was {}. Peer contribution: {}. Our contribution: {}", config.channel_handshake_limits.max_funding_satoshis, channel_value_satoshis, open_channel_fields.funding_satoshis, our_funding_satoshis))); } if channel_value_satoshis >= TOTAL_BITCOIN_SUPPLY_SATOSHIS { - return Err(ChannelError::Close(format!("Funding must be smaller than the total bitcoin supply. It was {}", channel_value_satoshis))); + return Err(ChannelError::close(format!("Funding must be smaller than the total bitcoin supply. It was {}", channel_value_satoshis))); } if msg_channel_reserve_satoshis > channel_value_satoshis { - return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must be no greater than channel_value_satoshis: {}", msg_channel_reserve_satoshis, channel_value_satoshis))); + return Err(ChannelError::close(format!("Bogus channel_reserve_satoshis ({}). Must be no greater than channel_value_satoshis: {}", msg_channel_reserve_satoshis, channel_value_satoshis))); } let full_channel_value_msat = (channel_value_satoshis - msg_channel_reserve_satoshis) * 1000; if msg_push_msat > full_channel_value_msat { - return Err(ChannelError::Close(format!("push_msat {} was larger than channel amount minus reserve ({})", msg_push_msat, full_channel_value_msat))); + return Err(ChannelError::close(format!("push_msat {} was larger than channel amount minus reserve ({})", msg_push_msat, full_channel_value_msat))); } if open_channel_fields.dust_limit_satoshis > channel_value_satoshis { - return Err(ChannelError::Close(format!("dust_limit_satoshis {} was larger than channel_value_satoshis {}. Peer never wants payout outputs?", open_channel_fields.dust_limit_satoshis, channel_value_satoshis))); + return Err(ChannelError::close(format!("dust_limit_satoshis {} was larger than channel_value_satoshis {}. Peer never wants payout outputs?", open_channel_fields.dust_limit_satoshis, channel_value_satoshis))); } if open_channel_fields.htlc_minimum_msat >= full_channel_value_msat { - return Err(ChannelError::Close(format!("Minimum htlc value ({}) was larger than full channel value ({})", open_channel_fields.htlc_minimum_msat, full_channel_value_msat))); + return Err(ChannelError::close(format!("Minimum htlc value ({}) was larger than full channel value ({})", open_channel_fields.htlc_minimum_msat, full_channel_value_msat))); } Channel::::check_remote_fee(&channel_type, fee_estimator, open_channel_fields.commitment_feerate_sat_per_1000_weight, None, &&logger)?; let max_counterparty_selected_contest_delay = u16::min(config.channel_handshake_limits.their_to_self_delay, MAX_LOCAL_BREAKDOWN_TIMEOUT); if open_channel_fields.to_self_delay > max_counterparty_selected_contest_delay { - return Err(ChannelError::Close(format!("They wanted our payments to be delayed by a needlessly long period. Upper limit: {}. Actual: {}", max_counterparty_selected_contest_delay, open_channel_fields.to_self_delay))); + return Err(ChannelError::close(format!("They wanted our payments to be delayed by a needlessly long period. Upper limit: {}. Actual: {}", max_counterparty_selected_contest_delay, open_channel_fields.to_self_delay))); } if open_channel_fields.max_accepted_htlcs < 1 { - return Err(ChannelError::Close("0 max_accepted_htlcs makes for a useless channel".to_owned())); + return Err(ChannelError::close("0 max_accepted_htlcs makes for a useless channel".to_owned())); } if open_channel_fields.max_accepted_htlcs > MAX_HTLCS { - return Err(ChannelError::Close(format!("max_accepted_htlcs was {}. It must not be larger than {}", open_channel_fields.max_accepted_htlcs, MAX_HTLCS))); + return Err(ChannelError::close(format!("max_accepted_htlcs was {}. It must not be larger than {}", open_channel_fields.max_accepted_htlcs, MAX_HTLCS))); } // Now check against optional parameters as set by config... if channel_value_satoshis < config.channel_handshake_limits.min_funding_satoshis { - return Err(ChannelError::Close(format!("Funding satoshis ({}) is less than the user specified limit ({})", channel_value_satoshis, config.channel_handshake_limits.min_funding_satoshis))); + return Err(ChannelError::close(format!("Funding satoshis ({}) is less than the user specified limit ({})", channel_value_satoshis, config.channel_handshake_limits.min_funding_satoshis))); } if open_channel_fields.htlc_minimum_msat > config.channel_handshake_limits.max_htlc_minimum_msat { - return Err(ChannelError::Close(format!("htlc_minimum_msat ({}) is higher than the user specified limit ({})", open_channel_fields.htlc_minimum_msat, config.channel_handshake_limits.max_htlc_minimum_msat))); + return Err(ChannelError::close(format!("htlc_minimum_msat ({}) is higher than the user specified limit ({})", open_channel_fields.htlc_minimum_msat, config.channel_handshake_limits.max_htlc_minimum_msat))); } if open_channel_fields.max_htlc_value_in_flight_msat < config.channel_handshake_limits.min_max_htlc_value_in_flight_msat { - return Err(ChannelError::Close(format!("max_htlc_value_in_flight_msat ({}) is less than the user specified limit ({})", open_channel_fields.max_htlc_value_in_flight_msat, config.channel_handshake_limits.min_max_htlc_value_in_flight_msat))); + return Err(ChannelError::close(format!("max_htlc_value_in_flight_msat ({}) is less than the user specified limit ({})", open_channel_fields.max_htlc_value_in_flight_msat, config.channel_handshake_limits.min_max_htlc_value_in_flight_msat))); } if msg_channel_reserve_satoshis > config.channel_handshake_limits.max_channel_reserve_satoshis { - return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is higher than the user specified limit ({})", msg_channel_reserve_satoshis, config.channel_handshake_limits.max_channel_reserve_satoshis))); + return Err(ChannelError::close(format!("channel_reserve_satoshis ({}) is higher than the user specified limit ({})", msg_channel_reserve_satoshis, config.channel_handshake_limits.max_channel_reserve_satoshis))); } if open_channel_fields.max_accepted_htlcs < config.channel_handshake_limits.min_max_accepted_htlcs { - return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", open_channel_fields.max_accepted_htlcs, config.channel_handshake_limits.min_max_accepted_htlcs))); + return Err(ChannelError::close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", open_channel_fields.max_accepted_htlcs, config.channel_handshake_limits.min_max_accepted_htlcs))); } if open_channel_fields.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { - return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", open_channel_fields.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); + return Err(ChannelError::close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", open_channel_fields.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); } if open_channel_fields.dust_limit_satoshis > MAX_CHAN_DUST_LIMIT_SATOSHIS { - return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", open_channel_fields.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); + return Err(ChannelError::close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", open_channel_fields.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); } // Convert things into internal flags and prep our state: if config.channel_handshake_limits.force_announced_channel_preference { if config.channel_handshake_config.announced_channel != announced_channel { - return Err(ChannelError::Close("Peer tried to open channel but their announcement preference is different from ours".to_owned())); + return Err(ChannelError::close("Peer tried to open channel but their announcement preference is different from ours".to_owned())); } } if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { // Protocol level safety check in place, although it should never happen because // of `MIN_THEIR_CHAN_RESERVE_SATOSHIS` - return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); + return Err(ChannelError::close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); } if holder_selected_channel_reserve_satoshis * 1000 >= full_channel_value_msat { - return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({})msats. Channel value is ({} - {})msats.", holder_selected_channel_reserve_satoshis * 1000, full_channel_value_msat, msg_push_msat))); + return Err(ChannelError::close(format!("Suitable channel reserve not found. remote_channel_reserve was ({})msats. Channel value is ({} - {})msats.", holder_selected_channel_reserve_satoshis * 1000, full_channel_value_msat, msg_push_msat))); } if msg_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { log_debug!(logger, "channel_reserve_satoshis ({}) is smaller than our dust limit ({}). We can broadcast stale states without any risk, implying this channel is very insecure for our counterparty.", msg_channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS); } if holder_selected_channel_reserve_satoshis < open_channel_fields.dust_limit_satoshis { - return Err(ChannelError::Close(format!("Dust limit ({}) too high for the channel reserve we require the remote to keep ({})", open_channel_fields.dust_limit_satoshis, holder_selected_channel_reserve_satoshis))); + return Err(ChannelError::close(format!("Dust limit ({}) too high for the channel reserve we require the remote to keep ({})", open_channel_fields.dust_limit_satoshis, holder_selected_channel_reserve_satoshis))); } // check if the funder's amount for the initial commitment tx is sufficient @@ -1500,14 +1500,14 @@ impl ChannelContext where SP::Target: SignerProvider { let funders_amount_msat = open_channel_fields.funding_satoshis * 1000 - msg_push_msat; let commitment_tx_fee = commit_tx_fee_msat(open_channel_fields.commitment_feerate_sat_per_1000_weight, MIN_AFFORDABLE_HTLC_COUNT, &channel_type) / 1000; if (funders_amount_msat / 1000).saturating_sub(anchor_outputs_value) < commitment_tx_fee { - return Err(ChannelError::Close(format!("Funding amount ({} sats) can't even pay fee for initial commitment transaction fee of {} sats.", (funders_amount_msat / 1000).saturating_sub(anchor_outputs_value), commitment_tx_fee))); + return Err(ChannelError::close(format!("Funding amount ({} sats) can't even pay fee for initial commitment transaction fee of {} sats.", (funders_amount_msat / 1000).saturating_sub(anchor_outputs_value), commitment_tx_fee))); } let to_remote_satoshis = funders_amount_msat / 1000 - commitment_tx_fee - anchor_outputs_value; // While it's reasonable for us to not meet the channel reserve initially (if they don't // want to push much to us), our counterparty should always have more than our reserve. if to_remote_satoshis < holder_selected_channel_reserve_satoshis { - return Err(ChannelError::Close("Insufficient funding amount for initial reserve".to_owned())); + return Err(ChannelError::close("Insufficient funding amount for initial reserve".to_owned())); } let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() { @@ -1518,14 +1518,14 @@ impl ChannelContext where SP::Target: SignerProvider { None } else { if !script::is_bolt2_compliant(&script, their_features) { - return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {}", script))) + return Err(ChannelError::close(format!("Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {}", script))) } Some(script.clone()) } }, // Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel &None => { - return Err(ChannelError::Close("Peer is signaling upfront_shutdown but we don't get any script. Use 0-length script to opt-out".to_owned())); + return Err(ChannelError::close("Peer is signaling upfront_shutdown but we don't get any script. Use 0-length script to opt-out".to_owned())); } } } else { None }; @@ -1533,19 +1533,19 @@ impl ChannelContext where SP::Target: SignerProvider { let shutdown_scriptpubkey = if config.channel_handshake_config.commit_upfront_shutdown_pubkey { match signer_provider.get_shutdown_scriptpubkey() { Ok(scriptpubkey) => Some(scriptpubkey), - Err(_) => return Err(ChannelError::Close("Failed to get upfront shutdown scriptpubkey".to_owned())), + Err(_) => return Err(ChannelError::close("Failed to get upfront shutdown scriptpubkey".to_owned())), } } else { None }; if let Some(shutdown_scriptpubkey) = &shutdown_scriptpubkey { if !shutdown_scriptpubkey.is_compatible(&their_features) { - return Err(ChannelError::Close(format!("Provided a scriptpubkey format not accepted by peer: {}", shutdown_scriptpubkey))); + return Err(ChannelError::close(format!("Provided a scriptpubkey format not accepted by peer: {}", shutdown_scriptpubkey))); } } let destination_script = match signer_provider.get_destination_script(channel_keys_id) { Ok(script) => script, - Err(_) => return Err(ChannelError::Close("Failed to get destination script".to_owned())), + Err(_) => return Err(ChannelError::close("Failed to get destination script".to_owned())), }; let mut secp_ctx = Secp256k1::new(); @@ -3512,7 +3512,7 @@ impl Channel where return Ok(()); } } - return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {}", feerate_per_kw, lower_limit))); + return Err(ChannelError::close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {}", feerate_per_kw, lower_limit))); } Ok(()) } @@ -3962,7 +3962,7 @@ impl Channel where } // If we reconnected before sending our `channel_ready` they may still resend theirs. ChannelState::ChannelReady(_) => check_reconnection = true, - _ => return Err(ChannelError::Close("Peer sent a channel_ready at a strange time".to_owned())), + _ => return Err(ChannelError::close("Peer sent a channel_ready at a strange time".to_owned())), } if check_reconnection { // They probably disconnected/reconnected and re-sent the channel_ready, which is @@ -3985,7 +3985,7 @@ impl Channel where ).expect("We already advanced, so previous secret keys should have been validated already"))) }; if expected_point != Some(msg.next_per_commitment_point) { - return Err(ChannelError::Close("Peer sent a reconnect channel_ready with a different point".to_owned())); + return Err(ChannelError::close("Peer sent a reconnect channel_ready with a different point".to_owned())); } return Ok(None); } @@ -4003,32 +4003,32 @@ impl Channel where fee_estimator: &LowerBoundedFeeEstimator, ) -> Result<(), ChannelError> where F::Target: FeeEstimator { if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { - return Err(ChannelError::Close("Got add HTLC message when channel was not in an operational state".to_owned())); + return Err(ChannelError::close("Got add HTLC message when channel was not in an operational state".to_owned())); } // If the remote has sent a shutdown prior to adding this HTLC, then they are in violation of the spec. if self.context.channel_state.is_remote_shutdown_sent() { - return Err(ChannelError::Close("Got add HTLC message when channel was not in an operational state".to_owned())); + return Err(ChannelError::close("Got add HTLC message when channel was not in an operational state".to_owned())); } if self.context.channel_state.is_peer_disconnected() { - return Err(ChannelError::Close("Peer sent update_add_htlc when we needed a channel_reestablish".to_owned())); + return Err(ChannelError::close("Peer sent update_add_htlc when we needed a channel_reestablish".to_owned())); } if msg.amount_msat > self.context.channel_value_satoshis * 1000 { - return Err(ChannelError::Close("Remote side tried to send more than the total value of the channel".to_owned())); + return Err(ChannelError::close("Remote side tried to send more than the total value of the channel".to_owned())); } if msg.amount_msat == 0 { - return Err(ChannelError::Close("Remote side tried to send a 0-msat HTLC".to_owned())); + return Err(ChannelError::close("Remote side tried to send a 0-msat HTLC".to_owned())); } if msg.amount_msat < self.context.holder_htlc_minimum_msat { - return Err(ChannelError::Close(format!("Remote side tried to send less than our minimum HTLC value. Lower limit: ({}). Actual: ({})", self.context.holder_htlc_minimum_msat, msg.amount_msat))); + return Err(ChannelError::close(format!("Remote side tried to send less than our minimum HTLC value. Lower limit: ({}). Actual: ({})", self.context.holder_htlc_minimum_msat, msg.amount_msat))); } let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator); let htlc_stats = self.context.get_pending_htlc_stats(None, dust_exposure_limiting_feerate); if htlc_stats.pending_inbound_htlcs + 1 > self.context.holder_max_accepted_htlcs as usize { - return Err(ChannelError::Close(format!("Remote tried to push more than our max accepted HTLCs ({})", self.context.holder_max_accepted_htlcs))); + return Err(ChannelError::close(format!("Remote tried to push more than our max accepted HTLCs ({})", self.context.holder_max_accepted_htlcs))); } if htlc_stats.pending_inbound_htlcs_value_msat + msg.amount_msat > self.context.holder_max_htlc_value_in_flight_msat { - return Err(ChannelError::Close(format!("Remote HTLC add would put them over our max HTLC value ({})", self.context.holder_max_htlc_value_in_flight_msat))); + return Err(ChannelError::close(format!("Remote HTLC add would put them over our max HTLC value ({})", self.context.holder_max_htlc_value_in_flight_msat))); } // Check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet @@ -4057,7 +4057,7 @@ impl Channel where let pending_remote_value_msat = self.context.channel_value_satoshis * 1000 - pending_value_to_self_msat; if pending_remote_value_msat < msg.amount_msat { - return Err(ChannelError::Close("Remote HTLC add would overdraw remaining funds".to_owned())); + return Err(ChannelError::close("Remote HTLC add would overdraw remaining funds".to_owned())); } // Check that the remote can afford to pay for this HTLC on-chain at the current @@ -4073,10 +4073,10 @@ impl Channel where 0 }; if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(anchor_outputs_value_msat) < remote_commit_tx_fee_msat { - return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned())); + return Err(ChannelError::close("Remote HTLC add would not leave enough to pay for fees".to_owned())); }; if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(remote_commit_tx_fee_msat).saturating_sub(anchor_outputs_value_msat) < self.context.holder_selected_channel_reserve_satoshis * 1000 { - return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value".to_owned())); + return Err(ChannelError::close("Remote HTLC add would put them under remote reserve value".to_owned())); } } @@ -4090,14 +4090,14 @@ impl Channel where let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered); let local_commit_tx_fee_msat = self.context.next_local_commit_tx_fee_msat(htlc_candidate, None); if self.context.value_to_self_msat < self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat + anchor_outputs_value_msat { - return Err(ChannelError::Close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned())); + return Err(ChannelError::close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned())); } } if self.context.next_counterparty_htlc_id != msg.htlc_id { - return Err(ChannelError::Close(format!("Remote skipped HTLC ID (skipped ID: {})", self.context.next_counterparty_htlc_id))); + return Err(ChannelError::close(format!("Remote skipped HTLC ID (skipped ID: {})", self.context.next_counterparty_htlc_id))); } if msg.cltv_expiry >= 500000000 { - return Err(ChannelError::Close("Remote provided CLTV expiry in seconds instead of block height".to_owned())); + return Err(ChannelError::close("Remote provided CLTV expiry in seconds instead of block height".to_owned())); } if self.context.channel_state.is_local_shutdown_sent() { @@ -4131,32 +4131,32 @@ impl Channel where Some(payment_preimage) => { let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array()); if payment_hash != htlc.payment_hash { - return Err(ChannelError::Close(format!("Remote tried to fulfill HTLC ({}) with an incorrect preimage", htlc_id))); + return Err(ChannelError::close(format!("Remote tried to fulfill HTLC ({}) with an incorrect preimage", htlc_id))); } OutboundHTLCOutcome::Success(Some(payment_preimage)) } }; match htlc.state { OutboundHTLCState::LocalAnnounced(_) => - return Err(ChannelError::Close(format!("Remote tried to fulfill/fail HTLC ({}) before it had been committed", htlc_id))), + return Err(ChannelError::close(format!("Remote tried to fulfill/fail HTLC ({}) before it had been committed", htlc_id))), OutboundHTLCState::Committed => { htlc.state = OutboundHTLCState::RemoteRemoved(outcome); }, OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) | OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) | OutboundHTLCState::RemoteRemoved(_) => - return Err(ChannelError::Close(format!("Remote tried to fulfill/fail HTLC ({}) that they'd already fulfilled/failed", htlc_id))), + return Err(ChannelError::close(format!("Remote tried to fulfill/fail HTLC ({}) that they'd already fulfilled/failed", htlc_id))), } return Ok(htlc); } } - Err(ChannelError::Close("Remote tried to fulfill/fail an HTLC we couldn't find".to_owned())) + Err(ChannelError::close("Remote tried to fulfill/fail an HTLC we couldn't find".to_owned())) } pub fn update_fulfill_htlc(&mut self, msg: &msgs::UpdateFulfillHTLC) -> Result<(HTLCSource, u64, Option), ChannelError> { if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { - return Err(ChannelError::Close("Got fulfill HTLC message when channel was not in an operational state".to_owned())); + return Err(ChannelError::close("Got fulfill HTLC message when channel was not in an operational state".to_owned())); } if self.context.channel_state.is_peer_disconnected() { - return Err(ChannelError::Close("Peer sent update_fulfill_htlc when we needed a channel_reestablish".to_owned())); + return Err(ChannelError::close("Peer sent update_fulfill_htlc when we needed a channel_reestablish".to_owned())); } self.mark_outbound_htlc_removed(msg.htlc_id, Some(msg.payment_preimage), None).map(|htlc| (htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat)) @@ -4164,10 +4164,10 @@ impl Channel where pub fn update_fail_htlc(&mut self, msg: &msgs::UpdateFailHTLC, fail_reason: HTLCFailReason) -> Result<(), ChannelError> { if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { - return Err(ChannelError::Close("Got fail HTLC message when channel was not in an operational state".to_owned())); + return Err(ChannelError::close("Got fail HTLC message when channel was not in an operational state".to_owned())); } if self.context.channel_state.is_peer_disconnected() { - return Err(ChannelError::Close("Peer sent update_fail_htlc when we needed a channel_reestablish".to_owned())); + return Err(ChannelError::close("Peer sent update_fail_htlc when we needed a channel_reestablish".to_owned())); } self.mark_outbound_htlc_removed(msg.htlc_id, None, Some(fail_reason))?; @@ -4176,10 +4176,10 @@ impl Channel where pub fn update_fail_malformed_htlc(&mut self, msg: &msgs::UpdateFailMalformedHTLC, fail_reason: HTLCFailReason) -> Result<(), ChannelError> { if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { - return Err(ChannelError::Close("Got fail malformed HTLC message when channel was not in an operational state".to_owned())); + return Err(ChannelError::close("Got fail malformed HTLC message when channel was not in an operational state".to_owned())); } if self.context.channel_state.is_peer_disconnected() { - return Err(ChannelError::Close("Peer sent update_fail_malformed_htlc when we needed a channel_reestablish".to_owned())); + return Err(ChannelError::close("Peer sent update_fail_malformed_htlc when we needed a channel_reestablish".to_owned())); } self.mark_outbound_htlc_removed(msg.htlc_id, None, Some(fail_reason))?; @@ -4190,13 +4190,13 @@ impl Channel where where L::Target: Logger { if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { - return Err(ChannelError::Close("Got commitment signed message when channel was not in an operational state".to_owned())); + return Err(ChannelError::close("Got commitment signed message when channel was not in an operational state".to_owned())); } if self.context.channel_state.is_peer_disconnected() { - return Err(ChannelError::Close("Peer sent commitment_signed when we needed a channel_reestablish".to_owned())); + return Err(ChannelError::close("Peer sent commitment_signed when we needed a channel_reestablish".to_owned())); } if self.context.channel_state.is_both_sides_shutdown() && self.context.last_sent_closing_fee.is_some() { - return Err(ChannelError::Close("Peer sent commitment_signed after we'd started exchanging closing_signeds".to_owned())); + return Err(ChannelError::close("Peer sent commitment_signed after we'd started exchanging closing_signeds".to_owned())); } let funding_script = self.context.get_funding_redeemscript(); @@ -4214,7 +4214,7 @@ impl Channel where log_bytes!(self.context.counterparty_funding_pubkey().serialize()), encode::serialize_hex(&bitcoin_tx.transaction), log_bytes!(sighash[..]), encode::serialize_hex(&funding_script), &self.context.channel_id()); if let Err(_) = self.context.secp_ctx.verify_ecdsa(&sighash, &msg.signature, &self.context.counterparty_funding_pubkey()) { - return Err(ChannelError::Close("Invalid commitment tx signature from peer".to_owned())); + return Err(ChannelError::close("Invalid commitment tx signature from peer".to_owned())); } bitcoin_tx.txid }; @@ -4229,7 +4229,7 @@ impl Channel where debug_assert!(!self.context.is_outbound()); let counterparty_reserve_we_require_msat = self.context.holder_selected_channel_reserve_satoshis * 1000; if commitment_stats.remote_balance_msat < commitment_stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat { - return Err(ChannelError::Close("Funding remote cannot afford proposed new fee".to_owned())); + return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned())); } } #[cfg(any(test, fuzzing))] @@ -4251,7 +4251,7 @@ impl Channel where } if msg.htlc_signatures.len() != commitment_stats.num_nondust_htlcs { - return Err(ChannelError::Close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs))); + return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs))); } // Up to LDK 0.0.115, HTLC information was required to be duplicated in the @@ -4284,7 +4284,7 @@ impl Channel where log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(keys.countersignatory_htlc_key.to_public_key().serialize()), encode::serialize_hex(&htlc_tx), log_bytes!(htlc_sighash[..]), encode::serialize_hex(&htlc_redeemscript), &self.context.channel_id()); if let Err(_) = self.context.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &keys.countersignatory_htlc_key.to_public_key()) { - return Err(ChannelError::Close("Invalid HTLC tx signature from peer".to_owned())); + return Err(ChannelError::close("Invalid HTLC tx signature from peer".to_owned())); } if !separate_nondust_htlc_sources { htlcs_and_sigs.push((htlc, Some(msg.htlc_signatures[idx]), source_opt.take())); @@ -4309,7 +4309,7 @@ impl Channel where ); self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.outbound_htlc_preimages) - .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?; + .map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?; // Update state now that we've passed all the can-fail calls... let mut need_commitment = false; @@ -4562,20 +4562,20 @@ impl Channel where where F::Target: FeeEstimator, L::Target: Logger, { if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { - return Err(ChannelError::Close("Got revoke/ACK message when channel was not in an operational state".to_owned())); + return Err(ChannelError::close("Got revoke/ACK message when channel was not in an operational state".to_owned())); } if self.context.channel_state.is_peer_disconnected() { - return Err(ChannelError::Close("Peer sent revoke_and_ack when we needed a channel_reestablish".to_owned())); + return Err(ChannelError::close("Peer sent revoke_and_ack when we needed a channel_reestablish".to_owned())); } if self.context.channel_state.is_both_sides_shutdown() && self.context.last_sent_closing_fee.is_some() { - return Err(ChannelError::Close("Peer sent revoke_and_ack after we'd started exchanging closing_signeds".to_owned())); + return Err(ChannelError::close("Peer sent revoke_and_ack after we'd started exchanging closing_signeds".to_owned())); } let secret = secp_check!(SecretKey::from_slice(&msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret".to_owned()); if let Some(counterparty_prev_commitment_point) = self.context.counterparty_prev_commitment_point { if PublicKey::from_secret_key(&self.context.secp_ctx, &secret) != counterparty_prev_commitment_point { - return Err(ChannelError::Close("Got a revoke commitment secret which didn't correspond to their current pubkey".to_owned())); + return Err(ChannelError::close("Got a revoke commitment secret which didn't correspond to their current pubkey".to_owned())); } } @@ -4587,7 +4587,7 @@ impl Channel where // lot of work, and there's some chance this is all a misunderstanding anyway. // We have to do *something*, though, since our signer may get mad at us for otherwise // jumping a remote commitment number, so best to just force-close and move on. - return Err(ChannelError::Close("Received an unexpected revoke_and_ack".to_owned())); + return Err(ChannelError::close("Received an unexpected revoke_and_ack".to_owned())); } #[cfg(any(test, fuzzing))] @@ -4601,7 +4601,7 @@ impl Channel where ecdsa.validate_counterparty_revocation( self.context.cur_counterparty_commitment_transaction_number + 1, &secret - ).map_err(|_| ChannelError::Close("Failed to validate revocation from peer".to_owned()))?; + ).map_err(|_| ChannelError::close("Failed to validate revocation from peer".to_owned()))?; }, // TODO (taproot|arik) #[cfg(taproot)] @@ -4609,7 +4609,7 @@ impl Channel where }; self.context.commitment_secrets.provide_secret(self.context.cur_counterparty_commitment_transaction_number + 1, msg.per_commitment_secret) - .map_err(|_| ChannelError::Close("Previous secrets did not match new one".to_owned()))?; + .map_err(|_| ChannelError::close("Previous secrets did not match new one".to_owned()))?; self.context.latest_monitor_update_id += 1; let mut monitor_update = ChannelMonitorUpdate { update_id: self.context.latest_monitor_update_id, @@ -5116,10 +5116,10 @@ impl Channel where where F::Target: FeeEstimator, L::Target: Logger { if self.context.is_outbound() { - return Err(ChannelError::Close("Non-funding remote tried to update channel fee".to_owned())); + return Err(ChannelError::close("Non-funding remote tried to update channel fee".to_owned())); } if self.context.channel_state.is_peer_disconnected() { - return Err(ChannelError::Close("Peer sent update_fee when we needed a channel_reestablish".to_owned())); + return Err(ChannelError::close("Peer sent update_fee when we needed a channel_reestablish".to_owned())); } Channel::::check_remote_fee(&self.context.channel_type, fee_estimator, msg.feerate_per_kw, Some(self.context.feerate_per_kw), logger)?; @@ -5130,11 +5130,11 @@ impl Channel where let htlc_stats = self.context.get_pending_htlc_stats(None, dust_exposure_limiting_feerate); let max_dust_htlc_exposure_msat = self.context.get_max_dust_htlc_exposure_msat(dust_exposure_limiting_feerate); if htlc_stats.on_holder_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { - return Err(ChannelError::Close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our own transactions (totaling {} msat)", + return Err(ChannelError::close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our own transactions (totaling {} msat)", msg.feerate_per_kw, htlc_stats.on_holder_tx_dust_exposure_msat))); } if htlc_stats.on_counterparty_tx_dust_exposure_msat > max_dust_htlc_exposure_msat { - return Err(ChannelError::Close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our counterparty's transactions (totaling {} msat)", + return Err(ChannelError::close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our counterparty's transactions (totaling {} msat)", msg.feerate_per_kw, htlc_stats.on_counterparty_tx_dust_exposure_msat))); } Ok(()) @@ -5293,21 +5293,21 @@ impl Channel where // While BOLT 2 doesn't indicate explicitly we should error this channel here, it // almost certainly indicates we are going to end up out-of-sync in some way, so we // just close here instead of trying to recover. - return Err(ChannelError::Close("Peer sent a loose channel_reestablish not after reconnect".to_owned())); + return Err(ChannelError::close("Peer sent a loose channel_reestablish not after reconnect".to_owned())); } if msg.next_local_commitment_number >= INITIAL_COMMITMENT_NUMBER || msg.next_remote_commitment_number >= INITIAL_COMMITMENT_NUMBER || msg.next_local_commitment_number == 0 { - return Err(ChannelError::Close("Peer sent an invalid channel_reestablish to force close in a non-standard way".to_owned())); + return Err(ChannelError::close("Peer sent an invalid channel_reestablish to force close in a non-standard way".to_owned())); } let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number - 1; if msg.next_remote_commitment_number > 0 { let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.context.secp_ctx); let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret) - .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; + .map_err(|_| ChannelError::close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) { - return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned())); + return Err(ChannelError::close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned())); } if msg.next_remote_commitment_number > our_commitment_transaction { macro_rules! log_and_panic { @@ -5351,7 +5351,7 @@ impl Channel where if !self.context.channel_state.is_our_channel_ready() || self.context.channel_state.is_monitor_update_in_progress() { if msg.next_remote_commitment_number != 0 { - return Err(ChannelError::Close("Peer claimed they saw a revoke_and_ack but we haven't sent channel_ready yet".to_owned())); + return Err(ChannelError::close("Peer claimed they saw a revoke_and_ack but we haven't sent channel_ready yet".to_owned())); } // Short circuit the whole handler as there is nothing we can resend them return Ok(ReestablishResponses { @@ -5389,7 +5389,7 @@ impl Channel where } } else { debug_assert!(false, "All values should have been handled in the four cases above"); - return Err(ChannelError::Close(format!( + return Err(ChannelError::close(format!( "Peer attempted to reestablish channel expecting a future local commitment transaction: {} (received) vs {} (expected)", msg.next_remote_commitment_number, our_commitment_transaction @@ -5452,13 +5452,13 @@ impl Channel where }) } } else if msg.next_local_commitment_number < next_counterparty_commitment_number { - Err(ChannelError::Close(format!( + Err(ChannelError::close(format!( "Peer attempted to reestablish channel with a very old remote commitment transaction: {} (received) vs {} (expected)", msg.next_local_commitment_number, next_counterparty_commitment_number, ))) } else { - Err(ChannelError::Close(format!( + Err(ChannelError::close(format!( "Peer attempted to reestablish channel with a future remote commitment transaction: {} (received) vs {} (expected)", msg.next_local_commitment_number, next_counterparty_commitment_number, @@ -5533,7 +5533,7 @@ impl Channel where pub fn timer_check_closing_negotiation_progress(&mut self) -> Result<(), ChannelError> { if self.closing_negotiation_ready() { if self.context.closing_signed_in_flight { - return Err(ChannelError::Close("closing_signed negotiation failed to finish within two timer ticks".to_owned())); + return Err(ChannelError::close("closing_signed negotiation failed to finish within two timer ticks".to_owned())); } else { self.context.closing_signed_in_flight = true; } @@ -5578,7 +5578,7 @@ impl Channel where ChannelSignerType::Ecdsa(ecdsa) => { let sig = ecdsa .sign_closing_transaction(&closing_tx, &self.context.secp_ctx) - .map_err(|()| ChannelError::Close("Failed to get signature for closing transaction.".to_owned()))?; + .map_err(|()| ChannelError::close("Failed to get signature for closing transaction.".to_owned()))?; self.context.last_sent_closing_fee = Some((total_fee_satoshis, sig.clone())); Ok((Some(msgs::ClosingSigned { @@ -5624,17 +5624,17 @@ impl Channel where ) -> Result<(Option, Option, Vec<(HTLCSource, PaymentHash)>), ChannelError> { if self.context.channel_state.is_peer_disconnected() { - return Err(ChannelError::Close("Peer sent shutdown when we needed a channel_reestablish".to_owned())); + return Err(ChannelError::close("Peer sent shutdown when we needed a channel_reestablish".to_owned())); } if self.context.channel_state.is_pre_funded_state() { // Spec says we should fail the connection, not the channel, but that's nonsense, there // are plenty of reasons you may want to fail a channel pre-funding, and spec says you // can do that via error message without getting a connection fail anyway... - return Err(ChannelError::Close("Peer sent shutdown pre-funding generation".to_owned())); + return Err(ChannelError::close("Peer sent shutdown pre-funding generation".to_owned())); } for htlc in self.context.pending_inbound_htlcs.iter() { if let InboundHTLCState::RemoteAnnounced(_) = htlc.state { - return Err(ChannelError::Close("Got shutdown with remote pending HTLCs".to_owned())); + return Err(ChannelError::close("Got shutdown with remote pending HTLCs".to_owned())); } } assert!(!matches!(self.context.channel_state, ChannelState::ShutdownComplete)); @@ -5662,10 +5662,10 @@ impl Channel where assert!(send_shutdown); let shutdown_scriptpubkey = match signer_provider.get_shutdown_scriptpubkey() { Ok(scriptpubkey) => scriptpubkey, - Err(_) => return Err(ChannelError::Close("Failed to get shutdown scriptpubkey".to_owned())), + Err(_) => return Err(ChannelError::close("Failed to get shutdown scriptpubkey".to_owned())), }; if !shutdown_scriptpubkey.is_compatible(their_features) { - return Err(ChannelError::Close(format!("Provided a scriptpubkey format not accepted by peer: {}", shutdown_scriptpubkey))); + return Err(ChannelError::close(format!("Provided a scriptpubkey format not accepted by peer: {}", shutdown_scriptpubkey))); } self.context.shutdown_scriptpubkey = Some(shutdown_scriptpubkey); true @@ -5747,20 +5747,20 @@ impl Channel where where F::Target: FeeEstimator { if !self.context.channel_state.is_both_sides_shutdown() { - return Err(ChannelError::Close("Remote end sent us a closing_signed before both sides provided a shutdown".to_owned())); + return Err(ChannelError::close("Remote end sent us a closing_signed before both sides provided a shutdown".to_owned())); } if self.context.channel_state.is_peer_disconnected() { - return Err(ChannelError::Close("Peer sent closing_signed when we needed a channel_reestablish".to_owned())); + return Err(ChannelError::close("Peer sent closing_signed when we needed a channel_reestablish".to_owned())); } if !self.context.pending_inbound_htlcs.is_empty() || !self.context.pending_outbound_htlcs.is_empty() { - return Err(ChannelError::Close("Remote end sent us a closing_signed while there were still pending HTLCs".to_owned())); + return Err(ChannelError::close("Remote end sent us a closing_signed while there were still pending HTLCs".to_owned())); } if msg.fee_satoshis > TOTAL_BITCOIN_SUPPLY_SATOSHIS { // this is required to stop potential overflow in build_closing_transaction - return Err(ChannelError::Close("Remote tried to send us a closing tx with > 21 million BTC fee".to_owned())); + return Err(ChannelError::close("Remote tried to send us a closing tx with > 21 million BTC fee".to_owned())); } if self.context.is_outbound() && self.context.last_sent_closing_fee.is_none() { - return Err(ChannelError::Close("Remote tried to send a closing_signed when we were supposed to propose the first one".to_owned())); + return Err(ChannelError::close("Remote tried to send a closing_signed when we were supposed to propose the first one".to_owned())); } if self.context.channel_state.is_monitor_update_in_progress() { @@ -5771,7 +5771,7 @@ impl Channel where let funding_redeemscript = self.context.get_funding_redeemscript(); let (mut closing_tx, used_total_fee) = self.build_closing_transaction(msg.fee_satoshis, false); if used_total_fee != msg.fee_satoshis { - return Err(ChannelError::Close(format!("Remote sent us a closing_signed with a fee other than the value they can claim. Fee in message: {}. Actual closing tx fee: {}", msg.fee_satoshis, used_total_fee))); + return Err(ChannelError::close(format!("Remote sent us a closing_signed with a fee other than the value they can claim. Fee in message: {}. Actual closing tx fee: {}", msg.fee_satoshis, used_total_fee))); } let sighash = closing_tx.trust().get_sighash_all(&funding_redeemscript, self.context.channel_value_satoshis); @@ -5788,7 +5788,7 @@ impl Channel where for outp in closing_tx.trust().built_transaction().output.iter() { if !outp.script_pubkey.is_witness_program() && outp.value < Amount::from_sat(MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS) { - return Err(ChannelError::Close("Remote sent us a closing_signed with a dust output. Always use segwit closing scripts!".to_owned())); + return Err(ChannelError::close("Remote sent us a closing_signed with a dust output. Always use segwit closing scripts!".to_owned())); } } @@ -5834,7 +5834,7 @@ impl Channel where ChannelSignerType::Ecdsa(ecdsa) => { let sig = ecdsa .sign_closing_transaction(&closing_tx, &self.context.secp_ctx) - .map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?; + .map_err(|_| ChannelError::close("External signer refused to sign closing transaction".to_owned()))?; let (signed_tx, shutdown_result) = if $new_fee == msg.fee_satoshis { let shutdown_result = ShutdownResult { closure_reason, @@ -5876,7 +5876,7 @@ impl Channel where if let Some(msgs::ClosingSignedFeeRange { min_fee_satoshis, max_fee_satoshis }) = msg.fee_range { if msg.fee_satoshis < min_fee_satoshis || msg.fee_satoshis > max_fee_satoshis { - return Err(ChannelError::Close(format!("Peer sent a bogus closing_signed - suggested fee of {} sat was not in their desired range of {} sat - {} sat", msg.fee_satoshis, min_fee_satoshis, max_fee_satoshis))); + return Err(ChannelError::close(format!("Peer sent a bogus closing_signed - suggested fee of {} sat was not in their desired range of {} sat - {} sat", msg.fee_satoshis, min_fee_satoshis, max_fee_satoshis))); } if max_fee_satoshis < our_min_fee { return Err(ChannelError::Warn(format!("Unable to come to consensus about closing feerate, remote's max fee ({} sat) was smaller than our min fee ({} sat)", max_fee_satoshis, our_min_fee))); @@ -5892,7 +5892,7 @@ impl Channel where propose_fee!(cmp::min(max_fee_satoshis, our_max_fee)); } else { if msg.fee_satoshis < our_min_fee || msg.fee_satoshis > our_max_fee { - return Err(ChannelError::Close(format!("Peer sent a bogus closing_signed - suggested fee of {} sat was not in our desired range of {} sat - {} sat after we informed them of our range.", + return Err(ChannelError::close(format!("Peer sent a bogus closing_signed - suggested fee of {} sat was not in our desired range of {} sat - {} sat after we informed them of our range.", msg.fee_satoshis, our_min_fee, our_max_fee))); } // The proposed fee is in our acceptable range, accept it and broadcast! @@ -5908,7 +5908,7 @@ impl Channel where } else if last_fee < our_max_fee { propose_fee!(our_max_fee); } else { - return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wants something ({} sat) higher than our max fee ({} sat)", msg.fee_satoshis, our_max_fee))); + return Err(ChannelError::close(format!("Unable to come to consensus about closing feerate, remote wants something ({} sat) higher than our max fee ({} sat)", msg.fee_satoshis, our_max_fee))); } } else { if msg.fee_satoshis > our_min_fee { @@ -5916,7 +5916,7 @@ impl Channel where } else if last_fee > our_min_fee { propose_fee!(our_min_fee); } else { - return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wants something ({} sat) lower than our min fee ({} sat)", msg.fee_satoshis, our_min_fee))); + return Err(ChannelError::close(format!("Unable to come to consensus about closing feerate, remote wants something ({} sat) lower than our min fee ({} sat)", msg.fee_satoshis, our_min_fee))); } } } else { @@ -6666,12 +6666,12 @@ impl Channel where let msghash = hash_to_message!(&Sha256d::hash(&announcement.encode()[..])[..]); if self.context.secp_ctx.verify_ecdsa(&msghash, &msg.node_signature, &self.context.get_counterparty_node_id()).is_err() { - return Err(ChannelError::Close(format!( + return Err(ChannelError::close(format!( "Bad announcement_signatures. Failed to verify node_signature. UnsignedChannelAnnouncement used for verification is {:?}. their_node_key is {:?}", &announcement, self.context.get_counterparty_node_id()))); } if self.context.secp_ctx.verify_ecdsa(&msghash, &msg.bitcoin_signature, self.context.counterparty_funding_pubkey()).is_err() { - return Err(ChannelError::Close(format!( + return Err(ChannelError::close(format!( "Bad announcement_signatures. Failed to verify bitcoin_signature. UnsignedChannelAnnouncement used for verification is {:?}. their_bitcoin_key is ({:?})", &announcement, self.context.counterparty_funding_pubkey()))); } @@ -7396,72 +7396,72 @@ impl OutboundV1Channel where SP::Target: SignerProvider { // Check sanity of message fields: if !self.context.is_outbound() { - return Err(ChannelError::Close("Got an accept_channel message from an inbound peer".to_owned())); + return Err(ChannelError::close("Got an accept_channel message from an inbound peer".to_owned())); } if !matches!(self.context.channel_state, ChannelState::NegotiatingFunding(flags) if flags == NegotiatingFundingFlags::OUR_INIT_SENT) { - return Err(ChannelError::Close("Got an accept_channel message at a strange time".to_owned())); + return Err(ChannelError::close("Got an accept_channel message at a strange time".to_owned())); } if msg.common_fields.dust_limit_satoshis > 21000000 * 100000000 { - return Err(ChannelError::Close(format!("Peer never wants payout outputs? dust_limit_satoshis was {}", msg.common_fields.dust_limit_satoshis))); + return Err(ChannelError::close(format!("Peer never wants payout outputs? dust_limit_satoshis was {}", msg.common_fields.dust_limit_satoshis))); } if msg.channel_reserve_satoshis > self.context.channel_value_satoshis { - return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than ({})", msg.channel_reserve_satoshis, self.context.channel_value_satoshis))); + return Err(ChannelError::close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than ({})", msg.channel_reserve_satoshis, self.context.channel_value_satoshis))); } if msg.common_fields.dust_limit_satoshis > self.context.holder_selected_channel_reserve_satoshis { - return Err(ChannelError::Close(format!("Dust limit ({}) is bigger than our channel reserve ({})", msg.common_fields.dust_limit_satoshis, self.context.holder_selected_channel_reserve_satoshis))); + return Err(ChannelError::close(format!("Dust limit ({}) is bigger than our channel reserve ({})", msg.common_fields.dust_limit_satoshis, self.context.holder_selected_channel_reserve_satoshis))); } if msg.channel_reserve_satoshis > self.context.channel_value_satoshis - self.context.holder_selected_channel_reserve_satoshis { - return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than channel value minus our reserve ({})", + return Err(ChannelError::close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than channel value minus our reserve ({})", msg.channel_reserve_satoshis, self.context.channel_value_satoshis - self.context.holder_selected_channel_reserve_satoshis))); } let full_channel_value_msat = (self.context.channel_value_satoshis - msg.channel_reserve_satoshis) * 1000; if msg.common_fields.htlc_minimum_msat >= full_channel_value_msat { - return Err(ChannelError::Close(format!("Minimum htlc value ({}) is full channel value ({})", msg.common_fields.htlc_minimum_msat, full_channel_value_msat))); + return Err(ChannelError::close(format!("Minimum htlc value ({}) is full channel value ({})", msg.common_fields.htlc_minimum_msat, full_channel_value_msat))); } let max_delay_acceptable = u16::min(peer_limits.their_to_self_delay, MAX_LOCAL_BREAKDOWN_TIMEOUT); if msg.common_fields.to_self_delay > max_delay_acceptable { - return Err(ChannelError::Close(format!("They wanted our payments to be delayed by a needlessly long period. Upper limit: {}. Actual: {}", max_delay_acceptable, msg.common_fields.to_self_delay))); + return Err(ChannelError::close(format!("They wanted our payments to be delayed by a needlessly long period. Upper limit: {}. Actual: {}", max_delay_acceptable, msg.common_fields.to_self_delay))); } if msg.common_fields.max_accepted_htlcs < 1 { - return Err(ChannelError::Close("0 max_accepted_htlcs makes for a useless channel".to_owned())); + return Err(ChannelError::close("0 max_accepted_htlcs makes for a useless channel".to_owned())); } if msg.common_fields.max_accepted_htlcs > MAX_HTLCS { - return Err(ChannelError::Close(format!("max_accepted_htlcs was {}. It must not be larger than {}", msg.common_fields.max_accepted_htlcs, MAX_HTLCS))); + return Err(ChannelError::close(format!("max_accepted_htlcs was {}. It must not be larger than {}", msg.common_fields.max_accepted_htlcs, MAX_HTLCS))); } // Now check against optional parameters as set by config... if msg.common_fields.htlc_minimum_msat > peer_limits.max_htlc_minimum_msat { - return Err(ChannelError::Close(format!("htlc_minimum_msat ({}) is higher than the user specified limit ({})", msg.common_fields.htlc_minimum_msat, peer_limits.max_htlc_minimum_msat))); + return Err(ChannelError::close(format!("htlc_minimum_msat ({}) is higher than the user specified limit ({})", msg.common_fields.htlc_minimum_msat, peer_limits.max_htlc_minimum_msat))); } if msg.common_fields.max_htlc_value_in_flight_msat < peer_limits.min_max_htlc_value_in_flight_msat { - return Err(ChannelError::Close(format!("max_htlc_value_in_flight_msat ({}) is less than the user specified limit ({})", msg.common_fields.max_htlc_value_in_flight_msat, peer_limits.min_max_htlc_value_in_flight_msat))); + return Err(ChannelError::close(format!("max_htlc_value_in_flight_msat ({}) is less than the user specified limit ({})", msg.common_fields.max_htlc_value_in_flight_msat, peer_limits.min_max_htlc_value_in_flight_msat))); } if msg.channel_reserve_satoshis > peer_limits.max_channel_reserve_satoshis { - return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is higher than the user specified limit ({})", msg.channel_reserve_satoshis, peer_limits.max_channel_reserve_satoshis))); + return Err(ChannelError::close(format!("channel_reserve_satoshis ({}) is higher than the user specified limit ({})", msg.channel_reserve_satoshis, peer_limits.max_channel_reserve_satoshis))); } if msg.common_fields.max_accepted_htlcs < peer_limits.min_max_accepted_htlcs { - return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.common_fields.max_accepted_htlcs, peer_limits.min_max_accepted_htlcs))); + return Err(ChannelError::close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.common_fields.max_accepted_htlcs, peer_limits.min_max_accepted_htlcs))); } if msg.common_fields.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { - return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.common_fields.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); + return Err(ChannelError::close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.common_fields.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); } if msg.common_fields.dust_limit_satoshis > MAX_CHAN_DUST_LIMIT_SATOSHIS { - return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.common_fields.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); + return Err(ChannelError::close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.common_fields.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); } if msg.common_fields.minimum_depth > peer_limits.max_minimum_depth { - return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", peer_limits.max_minimum_depth, msg.common_fields.minimum_depth))); + return Err(ChannelError::close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", peer_limits.max_minimum_depth, msg.common_fields.minimum_depth))); } if let Some(ty) = &msg.common_fields.channel_type { if *ty != self.context.channel_type { - return Err(ChannelError::Close("Channel Type in accept_channel didn't match the one sent in open_channel.".to_owned())); + return Err(ChannelError::close("Channel Type in accept_channel didn't match the one sent in open_channel.".to_owned())); } } else if their_features.supports_channel_type() { // Assume they've accepted the channel type as they said they understand it. } else { let channel_type = ChannelTypeFeatures::from_init(&their_features); if channel_type != ChannelTypeFeatures::only_static_remote_key() { - return Err(ChannelError::Close("Only static_remote_key is supported for non-negotiated channel types".to_owned())); + return Err(ChannelError::close("Only static_remote_key is supported for non-negotiated channel types".to_owned())); } self.context.channel_type = channel_type.clone(); self.context.channel_transaction_parameters.channel_type_features = channel_type; @@ -7475,14 +7475,14 @@ impl OutboundV1Channel where SP::Target: SignerProvider { None } else { if !script::is_bolt2_compliant(&script, their_features) { - return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {}", script))); + return Err(ChannelError::close(format!("Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {}", script))); } Some(script.clone()) } }, // Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel &None => { - return Err(ChannelError::Close("Peer is signaling upfront_shutdown but we don't get any script. Use 0-length script to opt-out".to_owned())); + return Err(ChannelError::close("Peer is signaling upfront_shutdown but we don't get any script. Use 0-length script to opt-out".to_owned())); } } } else { None }; @@ -7532,10 +7532,10 @@ impl OutboundV1Channel where SP::Target: SignerProvider { L::Target: Logger { if !self.context.is_outbound() { - return Err((self, ChannelError::Close("Received funding_signed for an inbound channel?".to_owned()))); + return Err((self, ChannelError::close("Received funding_signed for an inbound channel?".to_owned()))); } if !matches!(self.context.channel_state, ChannelState::FundingNegotiated) { - return Err((self, ChannelError::Close("Received funding_signed in strange state!".to_owned()))); + return Err((self, ChannelError::close("Received funding_signed in strange state!".to_owned()))); } if self.context.commitment_secrets.get_min_seen_secret() != (1 << 48) || self.context.cur_counterparty_commitment_transaction_number != INITIAL_COMMITMENT_NUMBER || @@ -7561,7 +7561,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { let sighash = initial_commitment_bitcoin_tx.get_sighash_all(&funding_script, self.context.channel_value_satoshis); // They sign our commitment transaction, allowing us to broadcast the tx if we wish. if let Err(_) = self.context.secp_ctx.verify_ecdsa(&sighash, &msg.signature, &self.context.get_counterparty_pubkeys().funding_pubkey) { - return Err((self, ChannelError::Close("Invalid funding_signed signature from peer".to_owned()))); + return Err((self, ChannelError::close("Invalid funding_signed signature from peer".to_owned()))); } } @@ -7576,7 +7576,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { let validated = self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, Vec::new()); if validated.is_err() { - return Err((self, ChannelError::Close("Failed to validate our commitment".to_owned()))); + return Err((self, ChannelError::close("Failed to validate our commitment".to_owned()))); } let funding_redeemscript = self.context.get_funding_redeemscript(); @@ -7648,28 +7648,28 @@ pub(super) fn channel_type_from_open_channel( ) -> Result { if let Some(channel_type) = &common_fields.channel_type { if channel_type.supports_any_optional_bits() { - return Err(ChannelError::Close("Channel Type field contained optional bits - this is not allowed".to_owned())); + return Err(ChannelError::close("Channel Type field contained optional bits - this is not allowed".to_owned())); } // We only support the channel types defined by the `ChannelManager` in // `provided_channel_type_features`. The channel type must always support // `static_remote_key`. if !channel_type.requires_static_remote_key() { - return Err(ChannelError::Close("Channel Type was not understood - we require static remote key".to_owned())); + return Err(ChannelError::close("Channel Type was not understood - we require static remote key".to_owned())); } // Make sure we support all of the features behind the channel type. if !channel_type.is_subset(our_supported_features) { - return Err(ChannelError::Close("Channel Type contains unsupported features".to_owned())); + return Err(ChannelError::close("Channel Type contains unsupported features".to_owned())); } let announced_channel = if (common_fields.channel_flags & 1) == 1 { true } else { false }; if channel_type.requires_scid_privacy() && announced_channel { - return Err(ChannelError::Close("SCID Alias/Privacy Channel Type cannot be set on a public channel".to_owned())); + return Err(ChannelError::close("SCID Alias/Privacy Channel Type cannot be set on a public channel".to_owned())); } Ok(channel_type.clone()) } else { let channel_type = ChannelTypeFeatures::from_init(&their_features); if channel_type != ChannelTypeFeatures::only_static_remote_key() { - return Err(ChannelError::Close("Only static_remote_key is supported for non-negotiated channel types".to_owned())); + return Err(ChannelError::close("Only static_remote_key is supported for non-negotiated channel types".to_owned())); } Ok(channel_type) } @@ -7820,7 +7820,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { L::Target: Logger { if self.context.is_outbound() { - return Err((self, ChannelError::Close("Received funding_created for an outbound channel?".to_owned()))); + return Err((self, ChannelError::close("Received funding_created for an outbound channel?".to_owned()))); } if !matches!( self.context.channel_state, ChannelState::NegotiatingFunding(flags) @@ -7829,7 +7829,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { // BOLT 2 says that if we disconnect before we send funding_signed we SHOULD NOT // remember the channel, so it's safe to just send an error_message here and drop the // channel. - return Err((self, ChannelError::Close("Received funding_created after we got the channel!".to_owned()))); + return Err((self, ChannelError::close("Received funding_created after we got the channel!".to_owned()))); } if self.context.commitment_secrets.get_min_seen_secret() != (1 << 48) || self.context.cur_counterparty_commitment_transaction_number != INITIAL_COMMITMENT_NUMBER || @@ -7865,7 +7865,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { ); if let Err(_) = self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, Vec::new()) { - return Err((self, ChannelError::Close("Failed to validate our commitment".to_owned()))); + return Err((self, ChannelError::close("Failed to validate our commitment".to_owned()))); } // Now that we're past error-generating stuff, update our local state: @@ -8069,7 +8069,7 @@ impl InboundV2Channel where SP::Target: SignerProvider { // First check the channel type is known, failing before we do anything else if we don't // support this channel type. if msg.common_fields.channel_type.is_none() { - return Err(ChannelError::Close(format!("Rejecting V2 channel {} missing channel_type", + return Err(ChannelError::close(format!("Rejecting V2 channel {} missing channel_type", msg.common_fields.temporary_channel_id))) } let channel_type = channel_type_from_open_channel(&msg.common_fields, their_features, our_supported_features)?; diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 992fd3c6f02..d1a8672c5e6 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4217,7 +4217,7 @@ where match find_funding_output(&chan, &funding_transaction) { Ok(found_funding_txo) => funding_txo = found_funding_txo, Err(err) => { - let chan_err = ChannelError::Close(err.to_owned()); + let chan_err = ChannelError::close(err.to_owned()); let api_err = APIError::APIMisuseError { err: err.to_owned() }; return close_chan!(chan_err, api_err, chan); }, @@ -7016,7 +7016,7 @@ where }, Some(mut phase) => { let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id); - let err = ChannelError::Close(err_msg); + let err = ChannelError::close(err_msg); return Err(convert_chan_phase_err!(self, err, &mut phase, &msg.temporary_channel_id).1); }, None => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.temporary_channel_id)) @@ -7031,7 +7031,7 @@ where // `update_maps_on_chan_removal`), we'll remove the existing channel // from `outpoint_to_peer`. Thus, we must first unset the funding outpoint // on the channel. - let err = ChannelError::Close($err.to_owned()); + let err = ChannelError::close($err.to_owned()); chan.unset_funding_info(msg.temporary_channel_id); return Err(convert_chan_phase_err!(self, err, chan, &funded_channel_id, UNFUNDED_CHANNEL).1); } } } @@ -7116,7 +7116,7 @@ where } else { unreachable!(); } Ok(()) } else { - let e = ChannelError::Close("Channel funding outpoint was a duplicate".to_owned()); + let e = ChannelError::close("Channel funding outpoint was a duplicate".to_owned()); // We weren't able to watch the channel to begin with, so no // updates should be made on it. Previously, full_stack_target // found an (unreachable) panic when the monitor update contained @@ -7187,7 +7187,7 @@ where Ok(()) } else { - try_chan_phase_entry!(self, Err(ChannelError::Close( + try_chan_phase_entry!(self, Err(ChannelError::close( "Got a channel_ready message for an unfunded channel!".into())), chan_phase_entry) } }, @@ -7302,7 +7302,7 @@ where (tx, Some(remove_channel_phase!(self, chan_phase_entry)), shutdown_result) } else { (tx, None, shutdown_result) } } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got a closing_signed message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -7402,7 +7402,7 @@ where } try_chan_phase_entry!(self, chan.update_add_htlc(&msg, pending_forward_info, &self.fee_estimator), chan_phase_entry); } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got an update_add_htlc message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -7446,7 +7446,7 @@ where next_user_channel_id = chan.context.get_user_id(); res } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got an update_fulfill_htlc message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -7477,7 +7477,7 @@ where if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { try_chan_phase_entry!(self, chan.update_fail_htlc(&msg, HTLCFailReason::from_msg(msg)), chan_phase_entry); } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got an update_fail_htlc message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -7500,13 +7500,13 @@ where match peer_state.channel_by_id.entry(msg.channel_id) { hash_map::Entry::Occupied(mut chan_phase_entry) => { if (msg.failure_code & 0x8000) == 0 { - let chan_err: ChannelError = ChannelError::Close("Got update_fail_malformed_htlc with BADONION not set".to_owned()); + let chan_err = ChannelError::close("Got update_fail_malformed_htlc with BADONION not set".to_owned()); try_chan_phase_entry!(self, Err(chan_err), chan_phase_entry); } if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { try_chan_phase_entry!(self, chan.update_fail_malformed_htlc(&msg, HTLCFailReason::reason(msg.failure_code, msg.sha256_of_onion.to_vec())), chan_phase_entry); } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got an update_fail_malformed_htlc message for an unfunded channel!".into())), chan_phase_entry); } Ok(()) @@ -7536,7 +7536,7 @@ where } Ok(()) } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got a commitment_signed message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -7732,7 +7732,7 @@ where } htlcs_to_fail } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got a revoke_and_ack message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -7758,7 +7758,7 @@ where let logger = WithChannelContext::from(&self.logger, &chan.context, None); try_chan_phase_entry!(self, chan.update_fee(&self.fee_estimator, &msg, &&logger), chan_phase_entry); } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got an update_fee message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -7793,7 +7793,7 @@ where update_msg: Some(self.get_channel_update_for_broadcast(chan).unwrap()), }); } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got an announcement_signatures message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -7845,7 +7845,7 @@ where } } } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got a channel_update for an unfunded channel!".into())), chan_phase_entry); } }, @@ -7907,7 +7907,7 @@ where } need_lnd_workaround } else { - return try_chan_phase_entry!(self, Err(ChannelError::Close( + return try_chan_phase_entry!(self, Err(ChannelError::close( "Got a channel_reestablish message for an unfunded channel!".into())), chan_phase_entry); } }, From 93011c377c1b6c73a0e93f63669ecf95be18522c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 May 2024 00:02:09 +0000 Subject: [PATCH 3/7] Allow `ChannelError` to specify the `ClosureReason` This will allow us to add more granular failure reasons when returning the general `ChannelError::Close`. --- lightning/src/ln/channel.rs | 8 ++++---- lightning/src/ln/channelmanager.rs | 8 +++----- lightning/src/ln/functional_tests.rs | 10 ++++++++-- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 37ce1082e0a..c1294af6a73 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -710,7 +710,7 @@ pub const MIN_THEIR_CHAN_RESERVE_SATOSHIS: u64 = 1000; pub(super) enum ChannelError { Ignore(String), Warn(String), - Close(String), + Close((String, ClosureReason)), } impl fmt::Debug for ChannelError { @@ -718,7 +718,7 @@ impl fmt::Debug for ChannelError { match self { &ChannelError::Ignore(ref e) => write!(f, "Ignore : {}", e), &ChannelError::Warn(ref e) => write!(f, "Warn : {}", e), - &ChannelError::Close(ref e) => write!(f, "Close : {}", e), + &ChannelError::Close((ref e, _)) => write!(f, "Close : {}", e), } } } @@ -728,14 +728,14 @@ impl fmt::Display for ChannelError { match self { &ChannelError::Ignore(ref e) => write!(f, "{}", e), &ChannelError::Warn(ref e) => write!(f, "{}", e), - &ChannelError::Close(ref e) => write!(f, "{}", e), + &ChannelError::Close((ref e, _)) => write!(f, "{}", e), } } } impl ChannelError { pub(super) fn close(err: String) -> Self { - ChannelError::Close(err.clone()) + ChannelError::Close((err.clone(), ClosureReason::ProcessingError { err })) } } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index d1a8672c5e6..a2647c80cac 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -636,7 +636,7 @@ impl MsgHandleErrInternal { err: msg, action: msgs::ErrorAction::IgnoreError, }, - ChannelError::Close(msg) => LightningError { + ChannelError::Close((msg, _reason)) => LightningError { err: msg.clone(), action: msgs::ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { @@ -2446,11 +2446,10 @@ macro_rules! convert_chan_phase_err { ChannelError::Ignore(msg) => { (false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), *$channel_id)) }, - ChannelError::Close(msg) => { + ChannelError::Close((msg, reason)) => { let logger = WithChannelContext::from(&$self.logger, &$channel.context, None); log_error!(logger, "Closing channel {} due to close-required error: {}", $channel_id, msg); update_maps_on_chan_removal!($self, $channel.context); - let reason = ClosureReason::ProcessingError { err: msg.clone() }; let shutdown_res = $channel.context.force_shutdown(true, reason); let err = MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $channel_update); @@ -4201,10 +4200,9 @@ where Some(ChannelPhase::UnfundedOutboundV1(mut chan)) => { macro_rules! close_chan { ($err: expr, $api_err: expr, $chan: expr) => { { let counterparty; - let err = if let ChannelError::Close(msg) = $err { + let err = if let ChannelError::Close((msg, reason)) = $err { let channel_id = $chan.context.channel_id(); counterparty = chan.context.get_counterparty_node_id(); - let reason = ClosureReason::ProcessingError { err: msg.clone() }; let shutdown_res = $chan.context.force_shutdown(false, reason); MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, shutdown_res, None) } else { unreachable!(); }; diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 7cd0f376d2e..5c9d8f269b1 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -7263,7 +7263,10 @@ fn test_user_configurable_csv_delay() { &low_our_to_self_config, 0, &nodes[0].logger, /*is_0conf=*/false) { match error { - ChannelError::Close(err) => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); }, + ChannelError::Close((err, _)) => { + let regex = regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap(); + assert!(regex.is_match(err.as_str())); + }, _ => panic!("Unexpected event"), } } else { assert!(false); } @@ -7295,7 +7298,10 @@ fn test_user_configurable_csv_delay() { &high_their_to_self_config, 0, &nodes[0].logger, /*is_0conf=*/false) { match error { - ChannelError::Close(err) => { assert!(regex::Regex::new(r"They wanted our payments to be delayed by a needlessly long period\. Upper limit: \d+\. Actual: \d+").unwrap().is_match(err.as_str())); }, + ChannelError::Close((err, _)) => { + let regex = regex::Regex::new(r"They wanted our payments to be delayed by a needlessly long period\. Upper limit: \d+\. Actual: \d+").unwrap(); + assert!(regex.is_match(err.as_str())); + }, _ => panic!("Unexpected event"), } } else { assert!(false); } From 88c291a9bc6df5aa9d2253182a2c934e56c3cfad Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 5 May 2024 23:21:13 +0000 Subject: [PATCH 4/7] Add a new `ClosureReason::PeerFeerateTooLow` Closure due to feerate disagreements are a specific closure reason which admins can understand and tune their config (in the form of their `FeeEstimator`) to avoid, so having a separate `ClosureReason` for it is useful. --- lightning/src/events/mod.rs | 24 ++++++++++++++++++++++++ lightning/src/ln/channel.rs | 7 ++++++- lightning/src/ln/functional_tests.rs | 6 +++--- 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index 45ed895ade7..f054a50ceb8 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -331,6 +331,21 @@ pub enum ClosureReason { FundingBatchClosure, /// One of our HTLCs timed out in a channel, causing us to force close the channel. HTLCsTimedOut, + /// Our peer provided a feerate which violated our required minimum (fetched from our + /// [`FeeEstimator`] either as [`ConfirmationTarget::MinAllowedAnchorChannelRemoteFee`] or + /// [`ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee`]). + /// + /// [`FeeEstimator`]: crate::chain::chaininterface::FeeEstimator + /// [`ConfirmationTarget::MinAllowedAnchorChannelRemoteFee`]: crate::chain::chaininterface::ConfirmationTarget::MinAllowedAnchorChannelRemoteFee + /// [`ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee`]: crate::chain::chaininterface::ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee + PeerFeerateTooLow { + /// The feerate on our channel set by our peer. + peer_feerate_sat_per_kw: u32, + /// The required feerate we enforce, from our [`FeeEstimator`]. + /// + /// [`FeeEstimator`]: crate::chain::chaininterface::FeeEstimator + required_feerate_sat_per_kw: u32, + }, } impl core::fmt::Display for ClosureReason { @@ -355,6 +370,11 @@ impl core::fmt::Display for ClosureReason { ClosureReason::CounterpartyCoopClosedUnfundedChannel => f.write_str("the peer requested the unfunded channel be closed"), ClosureReason::FundingBatchClosure => f.write_str("another channel in the same funding batch closed"), ClosureReason::HTLCsTimedOut => f.write_str("htlcs on the channel timed out"), + ClosureReason::PeerFeerateTooLow { peer_feerate_sat_per_kw, required_feerate_sat_per_kw } => + f.write_fmt(format_args!( + "peer provided a feerate ({} sat/kw) which was below our lower bound ({} sat/kw)", + peer_feerate_sat_per_kw, required_feerate_sat_per_kw, + )), } } } @@ -373,6 +393,10 @@ impl_writeable_tlv_based_enum_upgradable!(ClosureReason, (17, CounterpartyInitiatedCooperativeClosure) => {}, (19, LocallyInitiatedCooperativeClosure) => {}, (21, HTLCsTimedOut) => {}, + (23, PeerFeerateTooLow) => { + (0, peer_feerate_sat_per_kw, required), + (2, required_feerate_sat_per_kw, required), + }, ); /// Intended destination of a failed HTLC as indicated in [`Event::HTLCHandlingFailed`]. diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index c1294af6a73..488141fa8fa 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3512,7 +3512,12 @@ impl Channel where return Ok(()); } } - return Err(ChannelError::close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {}", feerate_per_kw, lower_limit))); + return Err(ChannelError::Close((format!( + "Peer's feerate much too low. Actual: {}. Our expected lower limit: {}", feerate_per_kw, lower_limit + ), ClosureReason::PeerFeerateTooLow { + peer_feerate_sat_per_kw: feerate_per_kw, + required_feerate_sat_per_kw: lower_limit, + }))); } Ok(()) } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 5c9d8f269b1..1e31e52cbf0 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -10408,9 +10408,9 @@ fn accept_busted_but_better_fee() { match events[0] { MessageSendEvent::UpdateHTLCs { updates: msgs::CommitmentUpdate { ref update_fee, .. }, .. } => { nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), update_fee.as_ref().unwrap()); - check_closed_event!(nodes[1], 1, ClosureReason::ProcessingError { - err: "Peer's feerate much too low. Actual: 1000. Our expected lower limit: 5000".to_owned() }, - [nodes[0].node.get_our_node_id()], 100000); + check_closed_event!(nodes[1], 1, ClosureReason::PeerFeerateTooLow { + peer_feerate_sat_per_kw: 1000, required_feerate_sat_per_kw: 5000, + }, [nodes[0].node.get_our_node_id()], 100000); check_closed_broadcast!(nodes[1], true); check_added_monitors!(nodes[1], 1); }, From 66e6ee563bc4e2d2e5337914e24929d271265c06 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 31 May 2024 17:42:49 +0000 Subject: [PATCH 5/7] Skip fee reads in `full_stack_target` when connecting many blocks When we connect 100 blocks in a row, requiring the fuzz input to contain 100 fee estimator results is uneccessary, so add a bool that lets us skip those reads. --- fuzz/src/full_stack.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index bdd29be9129..4b2cf17a2a9 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -71,7 +71,7 @@ use std::cell::RefCell; use std::convert::TryInto; use std::cmp; use std::sync::{Arc, Mutex}; -use std::sync::atomic::{AtomicU64,AtomicUsize,Ordering}; +use std::sync::atomic::{AtomicU64,AtomicUsize,AtomicBool,Ordering}; use bech32::u5; #[inline] @@ -98,6 +98,7 @@ pub fn slice_to_be24(v: &[u8]) -> u32 { struct InputData { data: Vec, read_pos: AtomicUsize, + halt_fee_est_reads: AtomicBool, } impl InputData { fn get_slice(&self, len: usize) -> Option<&[u8]> { @@ -124,6 +125,9 @@ struct FuzzEstimator { } impl FeeEstimator for FuzzEstimator { fn get_est_sat_per_1000_weight(&self, _: ConfirmationTarget) -> u32 { + if self.input.halt_fee_est_reads.load(Ordering::Acquire) { + return 253; + } //TODO: We should actually be testing at least much more than 64k... match self.input.get_slice(2) { Some(slice) => cmp::max(slice_to_be16(slice) as u32, 253), @@ -446,6 +450,7 @@ pub fn do_test(mut data: &[u8], logger: &Arc) { let input = Arc::new(InputData { data: data.to_vec(), read_pos: AtomicUsize::new(0), + halt_fee_est_reads: AtomicBool::new(false), }); let fee_est = Arc::new(FuzzEstimator { input: input.clone(), @@ -703,10 +708,12 @@ pub fn do_test(mut data: &[u8], logger: &Arc) { 11 => { let mut txn = broadcast.txn_broadcasted.lock().unwrap().split_off(0); if !txn.is_empty() { + input.halt_fee_est_reads.store(true, Ordering::Release); loss_detector.connect_block(&txn[..]); for _ in 2..100 { loss_detector.connect_block(&[]); } + input.halt_fee_est_reads.store(false, Ordering::Release); } for tx in txn.drain(..) { loss_detector.funding_txn.push(tx); From 5a1cc288b756fa39bcb33563da67841c2f757173 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 5 May 2024 23:22:23 +0000 Subject: [PATCH 6/7] Force-close channels if their feerate gets stale without any update For quite some time, LDK has force-closed channels if the peer sends us a feerate update which is below our `FeeEstimator`'s concept of a channel lower-bound. This is intended to ensure that channel feerates are always sufficient to get our commitment transaction confirmed on-chain if we do need to force-close. However, we've never checked our channel feerate regularly - if a peer is offline (or just uninterested in updating the channel feerate) and the prevailing feerates on-chain go up, we'll simply ignore it and allow our commitment transaction to sit around with a feerate too low to get confirmed. Here we rectify this oversight by force-closing channels with stale feerates, checking after each block. However, because fee estimators are often buggy and force-closures piss off users, we only do so rather conservatively. Specifically, we only force-close if a channel's feerate is below the minimum `FeeEstimator`-provided minimum across the last day. Further, because fee estimators are often especially buggy on startup (and because peers haven't had a chance to update the channel feerates yet), we don't force-close channels until we have a full day of feerate lower-bound history. This should reduce the incidence of force-closures substantially, but it is expected this will still increase force-closures somewhat substantially depending on the users' `FeeEstimator`. Fixes #993 --- fuzz/src/full_stack.rs | 20 +++++++++++ lightning/src/ln/channel.rs | 20 +++++++++++ lightning/src/ln/channelmanager.rs | 57 +++++++++++++++++++++++++++++- 3 files changed, 96 insertions(+), 1 deletion(-) diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 4b2cf17a2a9..4591888e3f0 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -921,19 +921,32 @@ mod tests { ext_from_hex("0c005e", &mut test); // the funding transaction ext_from_hex("020000000100000000000000000000000000000000000000000000000000000000000000000000000000ffffffff0150c3000000000000220020ae0000000000000000000000000000000000000000000000000000000000000000000000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions, one per line ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // by now client should have sent a channel_ready (CHECK 3: SendChannelReady to 03000000 for chan 3d000000) // inbound read from peer id 0 of len 18 @@ -1303,21 +1316,28 @@ mod tests { ext_from_hex("0c007d", &mut test); // the commitment transaction for channel 3f00000000000000000000000000000000000000000000000000000000000000 ext_from_hex("02000000013a000000000000000000000000000000000000000000000000000000000000000000000000000000800258020000000000002200204b0000000000000000000000000000000000000000000000000000000000000014c0000000000000160014280000000000000000000000000000000000000005000020", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // // connect a block with one transaction of len 94 ext_from_hex("0c005e", &mut test); // the HTLC timeout transaction ext_from_hex("0200000001730000000000000000000000000000000000000000000000000000000000000000000000000000000001a701000000000000220020b20000000000000000000000000000000000000000000000000000000000000000000000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // process the now-pending HTLC forward ext_from_hex("07", &mut test); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 488141fa8fa..75d1c0949e4 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5117,6 +5117,26 @@ impl Channel where } } + pub fn check_for_stale_feerate(&mut self, logger: &L, min_feerate: u32) -> Result<(), ClosureReason> { + if self.context.is_outbound() { + // While its possible our fee is too low for an outbound channel because we've been + // unable to increase the fee, we don't try to force-close directly here. + return Ok(()); + } + if self.context.feerate_per_kw < min_feerate { + log_info!(logger, + "Closing channel as feerate of {} is below required {} (the minimum required rate over the past day)", + self.context.feerate_per_kw, min_feerate + ); + Err(ClosureReason::PeerFeerateTooLow { + peer_feerate_sat_per_kw: self.context.feerate_per_kw, + required_feerate_sat_per_kw: min_feerate, + }) + } else { + Ok(()) + } + } + pub fn update_fee(&mut self, fee_estimator: &LowerBoundedFeeEstimator, msg: &msgs::UpdateFee, logger: &L) -> Result<(), ChannelError> where F::Target: FeeEstimator, L::Target: Logger { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a2647c80cac..ef6089d3c50 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -962,6 +962,11 @@ pub(super) struct InboundChannelRequest { /// accepted. An unaccepted channel that exceeds this limit will be abandoned. const UNACCEPTED_INBOUND_CHANNEL_AGE_LIMIT_TICKS: i32 = 2; +/// The number of blocks of historical feerate estimates we keep around and consider when deciding +/// to force-close a channel for having too-low fees. Also the number of blocks we have to see +/// after startup before we consider force-closing channels for having too-low fees. +const FEERATE_TRACKING_BLOCKS: usize = 144; + /// Stores a PaymentSecret and any other data we may need to validate an inbound payment is /// actually ours and not some duplicate HTLC sent to us by a node along the route. /// @@ -2098,6 +2103,21 @@ where /// Tracks the message events that are to be broadcasted when we are connected to some peer. pending_broadcast_messages: Mutex>, + /// We only want to force-close our channels on peers based on stale feerates when we're + /// confident the feerate on the channel is *really* stale, not just became stale recently. + /// Thus, we store the fee estimates we had as of the last [`FEERATE_TRACKING_BLOCKS`] blocks + /// (after startup completed) here, and only force-close when channels have a lower feerate + /// than we predicted any time in the last [`FEERATE_TRACKING_BLOCKS`] blocks. + /// + /// We only keep this in memory as we assume any feerates we receive immediately after startup + /// may be bunk (as they often are if Bitcoin Core crashes) and want to delay taking any + /// actions for a day anyway. + /// + /// The first element in the pair is the + /// [`ConfirmationTarget::MinAllowedAnchorChannelRemoteFee`] estimate, the second the + /// [`ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee`] estimate. + last_days_feerates: Mutex>, + entropy_source: ES, node_signer: NS, signer_provider: SP, @@ -2875,6 +2895,8 @@ where pending_offers_messages: Mutex::new(Vec::new()), pending_broadcast_messages: Mutex::new(Vec::new()), + last_days_feerates: Mutex::new(VecDeque::new()), + entropy_source, node_signer, signer_provider, @@ -9173,7 +9195,38 @@ where self, || -> NotifyOption { NotifyOption::DoPersist }); *self.best_block.write().unwrap() = BestBlock::new(block_hash, height); - self.do_chain_event(Some(height), |channel| channel.best_block_updated(height, header.time, self.chain_hash, &self.node_signer, &self.default_configuration, &&WithChannelContext::from(&self.logger, &channel.context, None))); + let mut min_anchor_feerate = None; + let mut min_non_anchor_feerate = None; + if self.background_events_processed_since_startup.load(Ordering::Relaxed) { + // If we're past the startup phase, update our feerate cache + let mut last_days_feerates = self.last_days_feerates.lock().unwrap(); + if last_days_feerates.len() >= FEERATE_TRACKING_BLOCKS { + last_days_feerates.pop_front(); + } + let anchor_feerate = self.fee_estimator + .bounded_sat_per_1000_weight(ConfirmationTarget::MinAllowedAnchorChannelRemoteFee); + let non_anchor_feerate = self.fee_estimator + .bounded_sat_per_1000_weight(ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee); + last_days_feerates.push_back((anchor_feerate, non_anchor_feerate)); + if last_days_feerates.len() >= FEERATE_TRACKING_BLOCKS { + min_anchor_feerate = last_days_feerates.iter().map(|(f, _)| f).min().copied(); + min_non_anchor_feerate = last_days_feerates.iter().map(|(_, f)| f).min().copied(); + } + } + + self.do_chain_event(Some(height), |channel| { + let logger = WithChannelContext::from(&self.logger, &channel.context, None); + if channel.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() { + if let Some(feerate) = min_anchor_feerate { + channel.check_for_stale_feerate(&logger, feerate)?; + } + } else { + if let Some(feerate) = min_non_anchor_feerate { + channel.check_for_stale_feerate(&logger, feerate)?; + } + } + channel.best_block_updated(height, header.time, self.chain_hash, &self.node_signer, &self.default_configuration, &&WithChannelContext::from(&self.logger, &channel.context, None)) + }); macro_rules! max_time { ($timestamp: expr) => { @@ -11992,6 +12045,8 @@ where node_signer: args.node_signer, signer_provider: args.signer_provider, + last_days_feerates: Mutex::new(VecDeque::new()), + logger: args.logger, default_configuration: args.default_config, }; From 17b77e0bcf0fd6721ac820df07a92ff697b3f50f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 30 May 2024 20:59:42 +0000 Subject: [PATCH 7/7] Add a test of stale-feerate-force-closure behavior --- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/ln/shutdown_tests.rs | 56 ++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index ef6089d3c50..80b98b2b6f5 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -965,7 +965,7 @@ const UNACCEPTED_INBOUND_CHANNEL_AGE_LIMIT_TICKS: i32 = 2; /// The number of blocks of historical feerate estimates we keep around and consider when deciding /// to force-close a channel for having too-low fees. Also the number of blocks we have to see /// after startup before we consider force-closing channels for having too-low fees. -const FEERATE_TRACKING_BLOCKS: usize = 144; +pub(super) const FEERATE_TRACKING_BLOCKS: usize = 144; /// Stores a PaymentSecret and any other data we may need to validate an inbound payment is /// actually ours and not some duplicate HTLC sent to us by a node along the route. diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index 0e03fd8db13..fe79da413c8 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -1464,3 +1464,59 @@ fn batch_funding_failure() { check_closed_events(&nodes[0], &close); assert_eq!(nodes[0].node.list_channels().len(), 0); } + +#[test] +fn test_force_closure_on_low_stale_fee() { + // Check that we force-close channels if they have a low fee and that has gotten stale (without + // update). + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let chan_id = create_announced_chan_between_nodes(&nodes, 0, 1).2; + + // Start by connecting lots of blocks to give LDK some feerate history + for _ in 0..super::channelmanager::FEERATE_TRACKING_BLOCKS * 2 { + connect_blocks(&nodes[1], 1); + } + + // Now connect a handful of blocks with a "high" feerate + { + let mut feerate_lock = chanmon_cfgs[1].fee_estimator.sat_per_kw.lock().unwrap(); + *feerate_lock *= 2; + } + for _ in 0..super::channelmanager::FEERATE_TRACKING_BLOCKS - 1 { + connect_blocks(&nodes[1], 1); + } + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); + + // Now, note that one more block would cause us to force-close, it won't because we've dropped + // the feerate + { + let mut feerate_lock = chanmon_cfgs[1].fee_estimator.sat_per_kw.lock().unwrap(); + *feerate_lock /= 2; + } + connect_blocks(&nodes[1], super::channelmanager::FEERATE_TRACKING_BLOCKS as u32 * 2); + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); + + // Now, connect another FEERATE_TRACKING_BLOCKS - 1 blocks at a high feerate, note that none of + // these will cause a force-closure because LDK only looks at the minimium feerate over the + // last FEERATE_TRACKING_BLOCKS blocks. + { + let mut feerate_lock = chanmon_cfgs[1].fee_estimator.sat_per_kw.lock().unwrap(); + *feerate_lock *= 2; + } + + for _ in 0..super::channelmanager::FEERATE_TRACKING_BLOCKS - 1 { + connect_blocks(&nodes[1], 1); + } + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); + + // Finally, connect one more block and check the force-close happened. + connect_blocks(&nodes[1], 1); + check_added_monitors!(nodes[1], 1); + check_closed_broadcast(&nodes[1], 1, true); + let reason = ClosureReason::PeerFeerateTooLow { peer_feerate_sat_per_kw: 253, required_feerate_sat_per_kw: 253 * 2 }; + check_closed_events(&nodes[1], &[ExpectedCloseEvent::from_id_reason(chan_id, false, reason)]); +}