Skip to content

Track claimed outbound HTLCs in ChannelMonitors #2048

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, 2023
Merged

Track claimed outbound HTLCs in ChannelMonitors #2048

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
33c36d0
Avoid removing stale preimages when hashes collide in fuzzing
TheBlueMatt Mar 2, 2023
0ad1f4c
Track claimed outbound HTLCs in ChannelMonitors
TheBlueMatt Feb 22, 2023
75527db
Remove unused compat block in `provide_latest_holder_commitment_tx`
TheBlueMatt Mar 1, 2023
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
125 changes: 87 additions & 38 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use crate::ln::{PaymentHash, PaymentPreimage};
use crate::ln::msgs::DecodeError;
use crate::ln::chan_utils;
use crate::ln::chan_utils::{CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLCClaim, ChannelTransactionParameters, HolderCommitmentTransaction};
use crate::ln::channelmanager::HTLCSource;
use crate::ln::channelmanager::{HTLCSource, SentHTLCId};
use crate::chain;
use crate::chain::{BestBlock, WatchedOutput};
use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator};
Expand Down Expand Up @@ -494,6 +494,7 @@ pub(crate) enum ChannelMonitorUpdateStep {
LatestHolderCommitmentTXInfo {
commitment_tx: HolderCommitmentTransaction,
htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
claimed_htlcs: Vec<(SentHTLCId, PaymentPreimage)>,
},
LatestCounterpartyCommitmentTXInfo {
commitment_txid: Txid,
Expand Down Expand Up @@ -536,6 +537,7 @@ impl ChannelMonitorUpdateStep {
impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep,
(0, LatestHolderCommitmentTXInfo) => {
(0, commitment_tx, required),
(1, claimed_htlcs, vec_type),
(2, htlc_outputs, vec_type),
},
(1, LatestCounterpartyCommitmentTXInfo) => {
Expand Down Expand Up @@ -750,6 +752,8 @@ pub(crate) struct ChannelMonitorImpl<Signer: WriteableEcdsaChannelSigner> {
/// Serialized to disk but should generally not be sent to Watchtowers.
counterparty_hash_commitment_number: HashMap<PaymentHash, u64>,

counterparty_fulfilled_htlcs: HashMap<SentHTLCId, PaymentPreimage>,

// We store two holder commitment transactions to avoid any race conditions where we may update
// some monitors (potentially on watchtowers) but then fail to update others, resulting in the
// various monitors for one channel being out of sync, and us broadcasting a holder
Expand Down Expand Up @@ -1033,6 +1037,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signe
(9, self.counterparty_node_id, option),
(11, self.confirmed_commitment_tx_counterparty_output, option),
(13, self.spendable_txids_confirmed, vec_type),
(15, self.counterparty_fulfilled_htlcs, required),
});

Ok(())
Expand Down Expand Up @@ -1120,6 +1125,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
counterparty_claimable_outpoints: HashMap::new(),
counterparty_commitment_txn_on_chain: HashMap::new(),
counterparty_hash_commitment_number: HashMap::new(),
counterparty_fulfilled_htlcs: HashMap::new(),

prev_holder_signed_commitment_tx: None,
current_holder_commitment_tx: holder_commitment_tx,
Expand Down Expand Up @@ -1174,7 +1180,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
&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).map_err(|_| ())
self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new()).map_err(|_| ())
}

/// This is used to provide payment preimage(s) out-of-band during startup without updating the
Expand Down Expand Up @@ -1810,9 +1816,10 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
/// `ChannelMonitor`. This is used to determine if an HTLC was removed from the channel prior
/// to the `ChannelManager` having been persisted.
///
/// This is similar to [`Self::get_pending_outbound_htlcs`] except it includes HTLCs which were
/// resolved by this `ChannelMonitor`.
pub(crate) fn get_all_current_outbound_htlcs(&self) -> HashMap<HTLCSource, HTLCOutputInCommitment> {
/// This is similar to [`Self::get_pending_or_resolved_outbound_htlcs`] except it includes
/// HTLCs which were resolved on-chain (i.e. where the final HTLC resolution was done by an
/// event from this `ChannelMonitor`).
pub(crate) fn get_all_current_outbound_htlcs(&self) -> HashMap<HTLCSource, (HTLCOutputInCommitment, Option<PaymentPreimage>)> {
let mut res = HashMap::new();
// Just examine the available counterparty commitment transactions. See docs on
// `fail_unbroadcast_htlcs`, below, for justification.
Expand All @@ -1822,7 +1829,8 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
if let Some(ref latest_outpoints) = us.counterparty_claimable_outpoints.get($txid) {
for &(ref htlc, ref source_option) in latest_outpoints.iter() {
if let &Some(ref source) = source_option {
res.insert((**source).clone(), htlc.clone());
res.insert((**source).clone(), (htlc.clone(),
us.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).cloned()));
}
}
}
Expand All @@ -1837,9 +1845,14 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
res
}

/// Gets the set of outbound HTLCs which are pending resolution in this channel.
/// Gets the set of outbound HTLCs which are pending resolution in this channel or which were
/// resolved with a preimage from our counterparty.
///
/// This is used to reconstruct pending outbound payments on restart in the ChannelManager.
pub(crate) fn get_pending_outbound_htlcs(&self) -> HashMap<HTLCSource, HTLCOutputInCommitment> {
///
/// Currently, the preimage is unused, however if it is present in the relevant internal state
/// an HTLC is always included even if it has been resolved.
pub(crate) fn get_pending_or_resolved_outbound_htlcs(&self) -> HashMap<HTLCSource, (HTLCOutputInCommitment, Option<PaymentPreimage>)> {
let us = self.inner.lock().unwrap();
// We're only concerned with the confirmation count of HTLC transactions, and don't
// actually care how many confirmations a commitment transaction may or may not have. Thus,
Expand Down Expand Up @@ -1887,8 +1900,10 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
Some(commitment_tx_output_idx) == htlc.transaction_output_index
} else { false }
});
if !htlc_update_confd {
res.insert(source.clone(), htlc.clone());
let counterparty_resolved_preimage_opt =
us.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).cloned();
if !htlc_update_confd || counterparty_resolved_preimage_opt.is_some() {
res.insert(source.clone(), (htlc.clone(), counterparty_resolved_preimage_opt));
}
}
}
Expand Down Expand Up @@ -1970,6 +1985,9 @@ macro_rules! fail_unbroadcast_htlcs {
}
}
if matched_htlc { continue; }
if $self.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).is_some() {
continue;
}
$self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
if entry.height != $commitment_tx_conf_height { return true; }
match entry.event {
Expand Down Expand Up @@ -2041,8 +2059,23 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
// Prune HTLCs from the previous counterparty commitment tx so we don't generate failure/fulfill
// events for now-revoked/fulfilled HTLCs.
if let Some(txid) = self.prev_counterparty_commitment_txid.take() {
for &mut (_, ref mut source) in self.counterparty_claimable_outpoints.get_mut(&txid).unwrap() {
*source = None;
if self.current_counterparty_commitment_txid.unwrap() != txid {
let cur_claimables = self.counterparty_claimable_outpoints.get(
&self.current_counterparty_commitment_txid.unwrap()).unwrap();
for (_, ref source_opt) in self.counterparty_claimable_outpoints.get(&txid).unwrap() {
if let Some(source) = source_opt {
if !cur_claimables.iter()
.any(|(_, cur_source_opt)| cur_source_opt == source_opt)
{
self.counterparty_fulfilled_htlcs.remove(&SentHTLCId::from_source(source));
}
}
}
for &mut (_, ref mut source_opt) in self.counterparty_claimable_outpoints.get_mut(&txid).unwrap() {
*source_opt = None;
}
} else {
assert!(cfg!(fuzzing), "Commitment txids are unique outside of fuzzing, where hashes can collide");
}
}

Expand Down Expand Up @@ -2127,28 +2160,37 @@ impl<Signer: WriteableEcdsaChannelSigner> 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, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>) -> Result<(), &'static str> {
// block for Rust 1.34 compat
let mut new_holder_commitment_tx = {
let trusted_tx = holder_commitment_tx.trust();
let txid = trusted_tx.txid();
let tx_keys = trusted_tx.keys();
self.current_holder_commitment_number = trusted_tx.commitment_number();
HolderSignedTx {
txid,
revocation_key: tx_keys.revocation_key,
a_htlc_key: tx_keys.broadcaster_htlc_key,
b_htlc_key: tx_keys.countersignatory_htlc_key,
delayed_payment_key: tx_keys.broadcaster_delayed_payment_key,
per_commitment_point: tx_keys.per_commitment_point,
htlc_outputs,
to_self_value_sat: holder_commitment_tx.to_broadcaster_value_sat(),
feerate_per_kw: trusted_tx.feerate_per_kw(),
}
fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)]) -> Result<(), &'static str> {
let trusted_tx = holder_commitment_tx.trust();
let txid = trusted_tx.txid();
let tx_keys = trusted_tx.keys();
self.current_holder_commitment_number = trusted_tx.commitment_number();
let mut new_holder_commitment_tx = HolderSignedTx {
txid,
revocation_key: tx_keys.revocation_key,
a_htlc_key: tx_keys.broadcaster_htlc_key,
b_htlc_key: tx_keys.countersignatory_htlc_key,
delayed_payment_key: tx_keys.broadcaster_delayed_payment_key,
per_commitment_point: tx_keys.per_commitment_point,
htlc_outputs,
to_self_value_sat: holder_commitment_tx.to_broadcaster_value_sat(),
feerate_per_kw: trusted_tx.feerate_per_kw(),
};
self.onchain_tx_handler.provide_latest_holder_tx(holder_commitment_tx);
mem::swap(&mut new_holder_commitment_tx, &mut self.current_holder_commitment_tx);
self.prev_holder_signed_commitment_tx = Some(new_holder_commitment_tx);
for (claimed_htlc_id, claimed_preimage) in claimed_htlcs {
#[cfg(debug_assertions)] {
let cur_counterparty_htlcs = self.counterparty_claimable_outpoints.get(
&self.current_counterparty_commitment_txid.unwrap()).unwrap();
assert!(cur_counterparty_htlcs.iter().any(|(_, source_opt)| {
if let Some(source) = source_opt {
SentHTLCId::from_source(source) == *claimed_htlc_id
} else { false }
}));
}
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");
}
Expand Down Expand Up @@ -2243,10 +2285,10 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&*fee_estimator);
for update in updates.updates.iter() {
match update {
ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs } => {
ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs, claimed_htlcs } => {
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()) {
if let Err(e) = self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs) {
log_error!(logger, "Providing latest holder commitment transaction failed/was refused:");
log_error!(logger, " {}", e);
ret = Err(());
Expand Down Expand Up @@ -3868,6 +3910,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
let mut counterparty_node_id = None;
let mut confirmed_commitment_tx_counterparty_output = None;
let mut spendable_txids_confirmed = Some(Vec::new());
let mut counterparty_fulfilled_htlcs = Some(HashMap::new());
read_tlv_fields!(reader, {
(1, funding_spend_confirmed, option),
(3, htlcs_resolved_on_chain, vec_type),
Expand All @@ -3876,6 +3919,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
(9, counterparty_node_id, option),
(11, confirmed_commitment_tx_counterparty_output, option),
(13, spendable_txids_confirmed, vec_type),
(15, counterparty_fulfilled_htlcs, option),
});

Ok((best_block.block_hash(), ChannelMonitor::from_impl(ChannelMonitorImpl {
Expand Down Expand Up @@ -3904,6 +3948,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
counterparty_claimable_outpoints,
counterparty_commitment_txn_on_chain,
counterparty_hash_commitment_number,
counterparty_fulfilled_htlcs: counterparty_fulfilled_htlcs.unwrap(),

prev_holder_signed_commitment_tx,
current_holder_commitment_tx,
Expand Down Expand Up @@ -4077,7 +4122,6 @@ mod tests {
let fee_estimator = TestFeeEstimator { sat_per_kw: Mutex::new(253) };

let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
let dummy_tx = Transaction { version: 0, lock_time: PackedLockTime::ZERO, input: Vec::new(), output: Vec::new() };

let mut preimages = Vec::new();
{
Expand Down Expand Up @@ -4167,11 +4211,10 @@ mod tests {
HolderCommitmentTransaction::dummy(), best_block, dummy_key);

monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..10])).unwrap();
let dummy_txid = dummy_tx.txid();
monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger);
monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[15..20]), 281474976710654, dummy_key, &logger);
monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[17..20]), 281474976710653, dummy_key, &logger);
monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[18..20]), 281474976710652, dummy_key, &logger);
monitor.provide_latest_counterparty_commitment_tx(Txid::from_inner(Sha256::hash(b"1").into_inner()),
preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger);
monitor.provide_latest_counterparty_commitment_tx(Txid::from_inner(Sha256::hash(b"2").into_inner()),
preimages_slice_to_htlc_outputs!(preimages[15..20]), 281474976710654, dummy_key, &logger);
for &(ref preimage, ref hash) in preimages.iter() {
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&fee_estimator);
monitor.provide_payment_preimage(hash, preimage, &broadcaster, &bounded_fee_estimator, &logger);
Expand All @@ -4185,13 +4228,19 @@ mod tests {
test_preimages_exist!(&preimages[0..10], monitor);
test_preimages_exist!(&preimages[15..20], monitor);

monitor.provide_latest_counterparty_commitment_tx(Txid::from_inner(Sha256::hash(b"3").into_inner()),
preimages_slice_to_htlc_outputs!(preimages[17..20]), 281474976710653, dummy_key, &logger);

// Now provide a further secret, pruning preimages 15-17
secret[0..32].clone_from_slice(&hex::decode("c7518c8ae4660ed02894df8976fa1a3659c1a8b4b5bec0c4b872abeba4cb8964").unwrap());
monitor.provide_secret(281474976710654, secret.clone()).unwrap();
assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 13);
test_preimages_exist!(&preimages[0..10], monitor);
test_preimages_exist!(&preimages[17..20], monitor);

monitor.provide_latest_counterparty_commitment_tx(Txid::from_inner(Sha256::hash(b"4").into_inner()),
preimages_slice_to_htlc_outputs!(preimages[18..20]), 281474976710652, dummy_key, &logger);

// Now update holder commitment tx info, pruning only element 18 as we still care about the
// previous commitment tx's preimages too
monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..5])).unwrap();
Expand Down
32 changes: 22 additions & 10 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use crate::ln::features::{ChannelTypeFeatures, InitFeatures};
use crate::ln::msgs;
use crate::ln::msgs::{DecodeError, OptionalField, DataLossProtect};
use crate::ln::script::{self, ShutdownScript};
use crate::ln::channelmanager::{self, CounterpartyForwardingInfo, PendingHTLCStatus, HTLCSource, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
use crate::ln::channelmanager::{self, CounterpartyForwardingInfo, PendingHTLCStatus, HTLCSource, SentHTLCId, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
use crate::ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, htlc_success_tx_weight, htlc_timeout_tx_weight, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor, ClosingTransaction};
use crate::ln::chan_utils;
use crate::ln::onion_utils::HTLCFailReason;
Expand Down Expand Up @@ -192,6 +192,7 @@ enum OutboundHTLCState {

#[derive(Clone)]
enum OutboundHTLCOutcome {
/// LDK version 0.0.105+ will always fill in the preimage here.
Success(Option<PaymentPreimage>),
Failure(HTLCFailReason),
}
Expand Down Expand Up @@ -3159,15 +3160,6 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
}
}

self.latest_monitor_update_id += 1;
let mut monitor_update = ChannelMonitorUpdate {
update_id: self.latest_monitor_update_id,
updates: vec![ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo {
commitment_tx: holder_commitment_tx,
htlc_outputs: htlcs_and_sigs
}]
};

for htlc in self.pending_inbound_htlcs.iter_mut() {
let new_forward = if let &InboundHTLCState::RemoteAnnounced(ref forward_info) = &htlc.state {
Some(forward_info.clone())
Expand All @@ -3179,18 +3171,38 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
need_commitment = true;
}
}
let mut claimed_htlcs = Vec::new();
for htlc in self.pending_outbound_htlcs.iter_mut() {
if let &mut OutboundHTLCState::RemoteRemoved(ref mut outcome) = &mut htlc.state {
log_trace!(logger, "Updating HTLC {} to AwaitingRemoteRevokeToRemove due to commitment_signed in channel {}.",
log_bytes!(htlc.payment_hash.0), log_bytes!(self.channel_id));
// Grab the preimage, if it exists, instead of cloning
let mut reason = OutboundHTLCOutcome::Success(None);
mem::swap(outcome, &mut reason);
if let OutboundHTLCOutcome::Success(Some(preimage)) = reason {
// If a user (a) receives an HTLC claim using LDK 0.0.104 or before, then (b)
// upgrades to LDK 0.0.114 or later before the HTLC is fully resolved, we could
// have a `Success(None)` reason. In this case we could forget some HTLC
// claims, but such an upgrade is unlikely and including claimed HTLCs here
// fixes a bug which the user was exposed to on 0.0.104 when they started the
// claim anyway.
claimed_htlcs.push((SentHTLCId::from_source(&htlc.source), preimage));
}
htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(reason);
need_commitment = true;
}
}

self.latest_monitor_update_id += 1;
let mut monitor_update = ChannelMonitorUpdate {
update_id: self.latest_monitor_update_id,
updates: vec![ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo {
commitment_tx: holder_commitment_tx,
htlc_outputs: htlcs_and_sigs,
claimed_htlcs,
}]
};

self.cur_holder_commitment_transaction_number -= 1;
// Note that if we need_commitment & !AwaitingRemoteRevoke we'll call
// build_commitment_no_status_check() next which will reset this to RAAFirst.
Expand Down
Loading