Skip to content

Only generate a post-close lock ChannelMonitorUpdate if we need one #3619

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 3, 2025
Merged

Only generate a post-close lock ChannelMonitorUpdate if we need one #3619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f0e69d
Cancel claims signed by a remote `ChannelMonitor` when reorging
TheBlueMatt Feb 25, 2025
489d70a
Drop return value from `provide_latest_holder_commitment_tx`
TheBlueMatt Feb 25, 2025
cd2e169
Only generate a post-close lock ChannelMonitorUpdate if we need one
TheBlueMatt Feb 25, 2025
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
83 changes: 40 additions & 43 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1536,8 +1536,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
fn provide_latest_holder_commitment_tx(
&self, holder_commitment_tx: HolderCommitmentTransaction,
htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
) -> Result<(), ()> {
self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new(), Vec::new()).map_err(|_| ())
) {
self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new(), Vec::new())
}

/// This is used to provide payment preimage(s) out-of-band during startup without updating the
Expand Down Expand Up @@ -1774,10 +1774,14 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
self.inner.lock().unwrap().get_cur_holder_commitment_number()
}

/// Gets whether we've been notified that this channel is closed by the `ChannelManager` (i.e.
/// via a [`ChannelMonitorUpdateStep::ChannelForceClosed`]).
pub(crate) fn offchain_closed(&self) -> bool {
self.inner.lock().unwrap().lockdown_from_offchain
/// Fetches whether this monitor has marked the channel as closed and will refuse any further
/// updates to the commitment transactions.
///
/// It can be marked closed in a few different ways, including via a
/// [`ChannelMonitorUpdateStep::ChannelForceClosed`] or if the channel has been closed
/// on-chain.
pub(crate) fn no_further_updates_allowed(&self) -> bool {
self.inner.lock().unwrap().no_further_updates_allowed()
}

/// Gets the `node_id` of the counterparty for this channel.
Expand Down Expand Up @@ -2938,7 +2942,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
/// is important that any clones of this channel monitor (including remote clones) by kept
/// up-to-date as our holder commitment transaction is updated.
/// Panics if set_on_holder_tx_csv has never been called.
fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, mut htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], nondust_htlc_sources: Vec<HTLCSource>) -> Result<(), &'static str> {
fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, mut htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], nondust_htlc_sources: Vec<HTLCSource>) {
if htlc_outputs.iter().any(|(_, s, _)| s.is_some()) {
// If we have non-dust HTLCs in htlc_outputs, ensure they match the HTLCs in the
// `holder_commitment_tx`. In the future, we'll no longer provide the redundant data
Expand Down Expand Up @@ -3015,10 +3019,6 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
self.counterparty_fulfilled_htlcs.insert(*claimed_htlc_id, *claimed_preimage);
}
if self.holder_tx_signed {
return Err("Latest holder commitment signed has already been signed, update is rejected");
}
Ok(())
}

/// Provides a payment_hash->payment_preimage mapping. Will be automatically pruned when all
Expand Down Expand Up @@ -3239,11 +3239,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs, claimed_htlcs, nondust_htlc_sources } => {
log_trace!(logger, "Updating ChannelMonitor with latest holder commitment transaction info");
if self.lockdown_from_offchain { panic!(); }
if let Err(e) = self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs, nondust_htlc_sources.clone()) {
log_error!(logger, "Providing latest holder commitment transaction failed/was refused:");
log_error!(logger, " {}", e);
ret = Err(());
}
self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs, nondust_htlc_sources.clone());
}
ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid, htlc_outputs, commitment_number, their_per_commitment_point, .. } => {
log_trace!(logger, "Updating ChannelMonitor with latest counterparty commitment transaction info");
Expand Down Expand Up @@ -3323,12 +3319,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}

if ret.is_ok() && (self.funding_spend_seen || self.lockdown_from_offchain || self.holder_tx_signed) && is_pre_close_update {
if ret.is_ok() && self.no_further_updates_allowed() && is_pre_close_update {
log_error!(logger, "Refusing Channel Monitor Update as counterparty attempted to update commitment after funding was spent");
Err(())
} else { ret }
}

fn no_further_updates_allowed(&self) -> bool {
self.funding_spend_seen || self.lockdown_from_offchain || self.holder_tx_signed
}

fn get_latest_update_id(&self) -> u64 {
self.latest_update_id
}
Expand Down Expand Up @@ -3915,35 +3915,32 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}
}
if self.holder_tx_signed {
// If we've signed, we may have broadcast either commitment (prev or current), and
// attempted to claim from it immediately without waiting for a confirmation.
if self.current_holder_commitment_tx.txid != *confirmed_commitment_txid {
// Cancel any pending claims for any holder commitments in case they had previously
// confirmed or been signed (in which case we will start attempting to claim without
// waiting for confirmation).
if self.current_holder_commitment_tx.txid != *confirmed_commitment_txid {
log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}",
self.current_holder_commitment_tx.txid);
let mut outpoint = BitcoinOutPoint { txid: self.current_holder_commitment_tx.txid, vout: 0 };
for (htlc, _, _) in &self.current_holder_commitment_tx.htlc_outputs {
if let Some(vout) = htlc.transaction_output_index {
outpoint.vout = vout;
self.onchain_tx_handler.abandon_claim(&outpoint);
}
}
}
if let Some(prev_holder_commitment_tx) = &self.prev_holder_signed_commitment_tx {
if prev_holder_commitment_tx.txid != *confirmed_commitment_txid {
log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}",
self.current_holder_commitment_tx.txid);
let mut outpoint = BitcoinOutPoint { txid: self.current_holder_commitment_tx.txid, vout: 0 };
for (htlc, _, _) in &self.current_holder_commitment_tx.htlc_outputs {
prev_holder_commitment_tx.txid);
let mut outpoint = BitcoinOutPoint { txid: prev_holder_commitment_tx.txid, vout: 0 };
for (htlc, _, _) in &prev_holder_commitment_tx.htlc_outputs {
if let Some(vout) = htlc.transaction_output_index {
outpoint.vout = vout;
self.onchain_tx_handler.abandon_claim(&outpoint);
}
}
}
if let Some(prev_holder_commitment_tx) = &self.prev_holder_signed_commitment_tx {
if prev_holder_commitment_tx.txid != *confirmed_commitment_txid {
log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}",
prev_holder_commitment_tx.txid);
let mut outpoint = BitcoinOutPoint { txid: prev_holder_commitment_tx.txid, vout: 0 };
for (htlc, _, _) in &prev_holder_commitment_tx.htlc_outputs {
if let Some(vout) = htlc.transaction_output_index {
outpoint.vout = vout;
self.onchain_tx_handler.abandon_claim(&outpoint);
}
}
}
}
} else {
// No previous claim.
}
}

Expand Down Expand Up @@ -4279,7 +4276,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}

if self.lockdown_from_offchain || self.funding_spend_seen || self.holder_tx_signed {
if self.no_further_updates_allowed() {
// Fail back HTLCs on backwards channels if they expire within
// `LATENCY_GRACE_PERIOD_BLOCKS` blocks and the channel is closed (i.e. we're at a
// point where no further off-chain updates will be accepted). If we haven't seen the
Expand Down Expand Up @@ -5437,7 +5434,7 @@ mod tests {
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs);

monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect());
monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"1").to_byte_array()),
preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger);
monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"2").to_byte_array()),
Expand Down Expand Up @@ -5475,7 +5472,7 @@ mod tests {
let mut htlcs = preimages_slice_to_htlcs!(preimages[0..5]);
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs);
monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect());
secret[0..32].clone_from_slice(&<Vec<u8>>::from_hex("2273e227a5b7449b6e70f1fb4652864038b1cbf9cd7c043a7d6456b7fc275ad8").unwrap());
monitor.provide_secret(281474976710653, secret.clone()).unwrap();
assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 12);
Expand All @@ -5486,7 +5483,7 @@ mod tests {
let mut htlcs = preimages_slice_to_htlcs!(preimages[0..3]);
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs);
monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx,
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect());
secret[0..32].clone_from_slice(&<Vec<u8>>::from_hex("27cddaa5624534cb6cb9d7da077cf2b22ab21e9b506fd4998a51d54502e99116").unwrap());
monitor.provide_secret(281474976710652, secret.clone()).unwrap();
assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 5);
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13781,8 +13781,8 @@ where
// claim.
// Note that a `ChannelMonitor` is created with `update_id` 0 and after we
// provide it with a closure update its `update_id` will be at 1.
if !monitor.offchain_closed() || monitor.get_latest_update_id() > 1 {
should_queue_fc_update = !monitor.offchain_closed();
if !monitor.no_further_updates_allowed() || monitor.get_latest_update_id() > 1 {
should_queue_fc_update = !monitor.no_further_updates_allowed();
let mut latest_update_id = monitor.get_latest_update_id();
if should_queue_fc_update {
latest_update_id += 1;
Expand Down
Loading