Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Auto-close on inbound/outbound update_fee delay #1056

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 43 additions & 6 deletions lightning/src/chain/channelmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ use ln::chan_utils::{CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLC
use ln::channelmanager::HTLCSource;
use chain;
use chain::{BestBlock, WatchedOutput};
use chain::chaininterface::{BroadcasterInterface, FeeEstimator};
use chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
use chain::transaction::{OutPoint, TransactionData};
use chain::keysinterface::{SpendableOutputDescriptor, StaticPaymentOutputDescriptor, DelayedPaymentOutputDescriptor, Sign, KeysInterface};
use chain::onchaintx::OnchainTxHandler;
Expand Down Expand Up @@ -698,6 +698,10 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
/// spending CSV for revocable outputs).
htlcs_resolved_on_chain: Vec<IrrevocablyResolvedHTLC>,

// If this pre-signed channel feerate is inferior by this rate to the current observed
// feerate, we might force-close a channel with inbound fulfilled HTLCs.
spikes_force_close_rate: u32,

// We simply modify best_block in Channel's block_connected so that serialization is
// consistent but hopefully the users' copy handles block_connected in a consistent way.
// (we do *not*, however, update them in update_monitor to ensure any local user copies keep
Expand Down Expand Up @@ -934,6 +938,7 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
(3, self.htlcs_resolved_on_chain, vec_type),
(5, self.pending_monitor_events, vec_type),
(7, self.funding_spend_seen, required),
(9, self.spikes_force_close_rate, required),
});

Ok(())
Expand All @@ -947,7 +952,8 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
funding_redeemscript: Script, channel_value_satoshis: u64,
commitment_transaction_number_obscure_factor: u64,
initial_holder_commitment_tx: HolderCommitmentTransaction,
best_block: BestBlock) -> ChannelMonitor<Signer> {
best_block: BestBlock,
spikes_force_close_rate: u32) -> ChannelMonitor<Signer> {

assert!(commitment_transaction_number_obscure_factor <= (1 << 48));
let payment_key_hash = WPubkeyHash::hash(&keys.pubkeys().payment_point.serialize());
Expand Down Expand Up @@ -1036,6 +1042,8 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
funding_spend_confirmed: None,
htlcs_resolved_on_chain: Vec::new(),

spikes_force_close_rate,

best_block,

secp_ctx,
Expand Down Expand Up @@ -1989,6 +1997,18 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
self.current_holder_commitment_number
}

/// Return the sum in satoshis of the fulfilled *non-dust* inbound HTLCs pending on this
/// channel.
pub(crate) fn get_fulfilled_htlc_sum(&self) -> u64 {
let mut sum_sats = 0;
for (htlc, _, _) in self.current_holder_commitment_tx.htlc_outputs.iter() {
if htlc.transaction_output_index.is_some() && htlc.offered == false && self.payment_preimages.contains_key(&htlc.payment_hash) {
sum_sats += htlc.amount_msat;
}
}
sum_sats
}

/// Attempts to claim a counterparty commitment transaction's outputs using the revocation key and
/// data in counterparty_claimable_outpoints. Will directly claim any HTLC outputs which expire at a
/// height > height + CLTV_SHARED_CLAIM_BUFFER. In any case, will install monitoring for
Expand Down Expand Up @@ -2443,7 +2463,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
log_trace!(logger, "Processing {} matched transactions for block at height {}.", txn_matched.len(), conf_height);
debug_assert!(self.best_block.height() >= conf_height);

let should_broadcast = self.should_broadcast_holder_commitment_txn(logger);
let fulfilled_htlc_high_exposure = 5_000_000 < self.get_fulfilled_htlc_sum();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we constify or make it a new config knob ?

I think we should give their say to node operators when it's question of risk evaluation. I pick up the same value than max_dust_htlc_exposure. Maybe we could pick one unified "funds at risk" value though at the same time it doesn't exactly cover the same risks...

Long-term, I think we should let node operators fine-tune their channels operations in function of their own risks analysis, instead of substituting to them (though likely too early).

let should_broadcast = self.should_broadcast_holder_commitment_txn(if self.should_force_close_fee_spikes(fee_estimator, self.current_holder_commitment_tx.feerate_per_kw) { true } else { false }, fulfilled_htlc_high_exposure, logger);
if should_broadcast {
let funding_outp = HolderFundingOutput::build(self.funding_redeemscript.clone());
let commitment_package = PackageTemplate::build_package(self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, PackageSolvingData::HolderFundingOutput(funding_outp), self.best_block.height(), false, self.best_block.height());
Expand Down Expand Up @@ -2634,7 +2655,18 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
false
}

fn should_broadcast_holder_commitment_txn<L: Deref>(&self, logger: &L) -> bool where L::Target: Logger {
fn should_force_close_fee_spikes<F: Deref>(&self, fee_estimator: &F, current_feerate: u32) -> bool where F::Target: FeeEstimator {

let new_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
// If the new feerate is superior by `spikes_force_close_rate` of current feerate,
// the channel could be preventively force-closed if there are HTLCs at risk.
if new_feerate > current_feerate * self.spikes_force_close_rate / 1000 {
return true;
}
false
}

fn should_broadcast_holder_commitment_txn<L: Deref>(&self, spikes_force_close: bool, fulfilled_htlc_high_exposure: bool, logger: &L) -> bool where L::Target: Logger {
// We need to consider all HTLCs which are:
// * in any unrevoked counterparty commitment transaction, as they could broadcast said
// transactions and we'd end up in a race, or
Expand Down Expand Up @@ -2674,7 +2706,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
// with CHECK_CLTV_EXPIRY_SANITY_2.
let htlc_outbound = $holder_tx == htlc.offered;
if ( htlc_outbound && htlc.cltv_expiry + LATENCY_GRACE_PERIOD_BLOCKS <= height) ||
(!htlc_outbound && htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER && self.payment_preimages.contains_key(&htlc.payment_hash)) {
((!htlc_outbound && (htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER || (spikes_force_close && fulfilled_htlc_high_exposure))) && self.payment_preimages.contains_key(&htlc.payment_hash)) {
log_info!(logger, "Force-closing channel due to {} HTLC timeout, HTLC expiry is {}", if htlc_outbound { "outbound" } else { "inbound "}, htlc.cltv_expiry);
return true;
}
Expand Down Expand Up @@ -3219,11 +3251,13 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
let mut funding_spend_confirmed = None;
let mut htlcs_resolved_on_chain = Some(Vec::new());
let mut funding_spend_seen = Some(false);
let mut spikes_force_close_rate = 0;
read_tlv_fields!(reader, {
(1, funding_spend_confirmed, option),
(3, htlcs_resolved_on_chain, vec_type),
(5, pending_monitor_events, vec_type),
(7, funding_spend_seen, option),
(9, spikes_force_close_rate, required),
});

let mut secp_ctx = Secp256k1::new();
Expand Down Expand Up @@ -3277,6 +3311,8 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
funding_spend_confirmed,
htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),

spikes_force_close_rate,

best_block,

secp_ctx,
Expand Down Expand Up @@ -3513,7 +3549,8 @@ mod tests {
(OutPoint { txid: Txid::from_slice(&[43; 32]).unwrap(), index: 0 }, Script::new()),
&channel_parameters,
Script::new(), 46, 0,
HolderCommitmentTransaction::dummy(), best_block);
HolderCommitmentTransaction::dummy(), best_block,
1300);

monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..10])).unwrap();
let dummy_txid = dummy_tx.txid();
Expand Down
6 changes: 4 additions & 2 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2014,7 +2014,8 @@ impl<Signer: Sign> Channel<Signer> {
&self.channel_transaction_parameters,
funding_redeemscript.clone(), self.channel_value_satoshis,
obscure_factor,
holder_commitment_tx, best_block);
holder_commitment_tx, best_block,
self.config.spikes_force_close_rate);

channel_monitor.provide_latest_counterparty_commitment_tx(counterparty_initial_commitment_txid, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger);

Expand Down Expand Up @@ -2091,7 +2092,8 @@ impl<Signer: Sign> Channel<Signer> {
&self.channel_transaction_parameters,
funding_redeemscript.clone(), self.channel_value_satoshis,
obscure_factor,
holder_commitment_tx, best_block);
holder_commitment_tx, best_block,
self.config.spikes_force_close_rate);

channel_monitor.provide_latest_counterparty_commitment_tx(counterparty_initial_bitcoin_tx.txid, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger);

Expand Down
27 changes: 27 additions & 0 deletions lightning/src/ln/functional_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9603,3 +9603,30 @@ fn test_max_dust_htlc_exposure() {
do_test_max_dust_htlc_exposure(false, ExposureEvent::AtUpdateFeeOutbound, false);
do_test_max_dust_htlc_exposure(false, ExposureEvent::AtUpdateFeeOutbound, true);
}

#[test]
fn test_fee_spikes_force_close() {

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);

create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());

// Route a payment above default `fulfilled_htlc_high_exposure`
let payment_preimage = route_payment(&nodes[0], &vec!(&nodes[1])[..], 10_000_000).0;
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
nodes[1].node.claim_funds(payment_preimage);
// Increase feerate * 2, above default `spikes_force_close_rate`
{
let mut feerate_lock = chanmon_cfgs[1].fee_estimator.sat_per_kw.lock().unwrap();
*feerate_lock += *feerate_lock;
}
// Confirms a block to trigger force-close.
connect_blocks(&nodes[1], 1);
check_added_monitors!(nodes[1], 1);
check_closed_broadcast!(nodes[1], true).unwrap();
check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed);
check_added_monitors!(nodes[1], 1);
}
9 changes: 9 additions & 0 deletions lightning/src/util/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,13 @@ pub struct ChannelConfig {
/// [`Normal`]: crate::chain::chaininterface::ConfirmationTarget::Normal
/// [`Background`]: crate::chain::chaininterface::ConfirmationTarget::Background
pub force_close_avoidance_max_fee_satoshis: u64,
/// When we detect a fee spikes and receive a block, we force-close immediately channel
/// with pending non-dust inbound HTLCs, of which the preimage is known.
///
/// To disable the force-close on fee spikes mechanism, select 0 as a rate.
///
/// Default value: 30%
pub spikes_force_close_rate: u32,
}

impl Default for ChannelConfig {
Expand All @@ -259,6 +266,7 @@ impl Default for ChannelConfig {
commit_upfront_shutdown_pubkey: true,
max_dust_htlc_exposure_msat: 5_000_000,
force_close_avoidance_max_fee_satoshis: 1000,
spikes_force_close_rate: 1300,
}
}
}
Expand All @@ -269,6 +277,7 @@ impl_writeable_tlv_based!(ChannelConfig, {
(2, cltv_expiry_delta, required),
(3, force_close_avoidance_max_fee_satoshis, (default_value, 1000)),
(4, announced_channel, required),
(5, spikes_force_close_rate, (default_value, 1300)),
(6, commit_upfront_shutdown_pubkey, required),
(8, forwarding_fee_base_msat, required),
});
Expand Down