Skip to content

Commit d63e6f1

Browse files
wpaulinoTheBlueMatt
wpaulino
authored and
TheBlueMatt
committed
Only claim HTLCs with matching payment hash upon preimage monitor update
Previously, we'd attempt to claim all HTLCs that have expired or that we have the preimage for on each preimage monitor update. This happened due to reusing the code path (`get_counterparty_output_claim_info`) used when producing all claims for a newly confirmed counterparty commitment. Unfortunately, this can result in invalid claim transactions and ultimately in loss of funds (if the HTLC expires and the counterparty claims it via the timeout), as it didn't consider that some of those HTLCs may have already been claimed by a separate transaction. This commit changes the behavior when handling preimage monitor updates only. We will now only attempt to claim HTLCs for the specific preimage that we learned via the monitor update. This is safe to do, as even if a preimage HTLC claim transaction is reorged out, the `OnchainTxHandler` is responsible for continuous claiming attempts until we see a reorg of the corresponding commitment transaction. Backport of 46f0d31 Conflicts resolved in: * lightning/src/chain/channelmonitor.rs Silent conflicts resolved in: * lightning/src/ln/monitor_tests.rs
1 parent 29993e3 commit d63e6f1

File tree

2 files changed

+185
-57
lines changed

2 files changed

+185
-57
lines changed

‎lightning/src/chain/channelmonitor.rs‎

Lines changed: 103 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -3118,7 +3118,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
31183118
// First check if a counterparty commitment transaction has been broadcasted:
31193119
macro_rules! claim_htlcs {
31203120
($commitment_number: expr, $txid: expr, $htlcs: expr) => {
3121-
let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info($commitment_number, $txid, None, $htlcs, confirmed_spend_height);
3121+
let htlc_claim_reqs = self.get_counterparty_output_claims_for_preimage(*payment_preimage, $commitment_number, $txid, $htlcs, confirmed_spend_height);
31223122
let conf_target = self.closure_conf_target();
31233123
self.onchain_tx_handler.update_claims_view_from_requests(htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster, conf_target, fee_estimator, logger);
31243124
}
@@ -3728,7 +3728,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
37283728
(htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref()))
37293729
), logger);
37303730
let (htlc_claim_reqs, counterparty_output_info) =
3731-
self.get_counterparty_output_claim_info(commitment_number, commitment_txid, Some(tx), per_commitment_option, Some(height));
3731+
self.get_counterparty_output_claim_info(commitment_number, commitment_txid, tx, per_commitment_claimable_data, Some(height));
37323732
to_counterparty_output_info = counterparty_output_info;
37333733
for req in htlc_claim_reqs {
37343734
claimable_outpoints.push(req);
@@ -3738,75 +3738,121 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
37383738
(claimable_outpoints, to_counterparty_output_info)
37393739
}
37403740

3741+
fn get_point_for_commitment_number(&self, commitment_number: u64) -> Option<PublicKey> {
3742+
let per_commitment_points = &self.their_cur_per_commitment_points?;
3743+
3744+
// If the counterparty commitment tx is the latest valid state, use their latest
3745+
// per-commitment point
3746+
if per_commitment_points.0 == commitment_number {
3747+
Some(per_commitment_points.1)
3748+
} else if let Some(point) = per_commitment_points.2.as_ref() {
3749+
// If counterparty commitment tx is the state previous to the latest valid state, use
3750+
// their previous per-commitment point (non-atomicity of revocation means it's valid for
3751+
// them to temporarily have two valid commitment txns from our viewpoint)
3752+
if per_commitment_points.0 == commitment_number + 1 {
3753+
Some(*point)
3754+
} else {
3755+
None
3756+
}
3757+
} else {
3758+
None
3759+
}
3760+
}
3761+
3762+
fn get_counterparty_output_claims_for_preimage(
3763+
&self, preimage: PaymentPreimage, commitment_number: u64,
3764+
commitment_txid: Txid,
3765+
per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>,
3766+
confirmation_height: Option<u32>,
3767+
) -> Vec<PackageTemplate> {
3768+
let per_commitment_claimable_data = match per_commitment_option {
3769+
Some(outputs) => outputs,
3770+
None => return Vec::new(),
3771+
};
3772+
let per_commitment_point = match self.get_point_for_commitment_number(commitment_number) {
3773+
Some(point) => point,
3774+
None => return Vec::new(),
3775+
};
3776+
3777+
let matching_payment_hash = PaymentHash::from(preimage);
3778+
per_commitment_claimable_data
3779+
.iter()
3780+
.filter_map(|(htlc, _)| {
3781+
if let Some(transaction_output_index) = htlc.transaction_output_index {
3782+
if htlc.offered && htlc.payment_hash == matching_payment_hash {
3783+
let htlc_data = PackageSolvingData::CounterpartyOfferedHTLCOutput(
3784+
CounterpartyOfferedHTLCOutput::build(
3785+
per_commitment_point,
3786+
self.counterparty_commitment_params.counterparty_delayed_payment_base_key,
3787+
self.counterparty_commitment_params.counterparty_htlc_base_key,
3788+
preimage,
3789+
htlc.clone(),
3790+
self.onchain_tx_handler.channel_type_features().clone(),
3791+
confirmation_height,
3792+
),
3793+
);
3794+
Some(PackageTemplate::build_package(
3795+
commitment_txid,
3796+
transaction_output_index,
3797+
htlc_data,
3798+
htlc.cltv_expiry,
3799+
))
3800+
} else {
3801+
None
3802+
}
3803+
} else {
3804+
None
3805+
}
3806+
})
3807+
.collect()
3808+
}
3809+
37413810
/// Returns the HTLC claim package templates and the counterparty output info
37423811
fn get_counterparty_output_claim_info(
37433812
&self, commitment_number: u64, commitment_txid: Txid,
3744-
tx: Option<&Transaction>,
3745-
per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>,
3813+
tx: &Transaction,
3814+
per_commitment_claimable_data: &[(HTLCOutputInCommitment, Option<Box<HTLCSource>>)],
37463815
confirmation_height: Option<u32>,
37473816
) -> (Vec<PackageTemplate>, CommitmentTxCounterpartyOutputInfo) {
37483817
let mut claimable_outpoints = Vec::new();
37493818
let mut to_counterparty_output_info: CommitmentTxCounterpartyOutputInfo = None;
37503819

3751-
let per_commitment_claimable_data = match per_commitment_option {
3752-
Some(outputs) => outputs,
3753-
None => return (claimable_outpoints, to_counterparty_output_info),
3754-
};
3755-
let per_commitment_points = match self.their_cur_per_commitment_points {
3756-
Some(points) => points,
3820+
let per_commitment_point = match self.get_point_for_commitment_number(commitment_number) {
3821+
Some(point) => point,
37573822
None => return (claimable_outpoints, to_counterparty_output_info),
37583823
};
37593824

3760-
let per_commitment_point =
3761-
// If the counterparty commitment tx is the latest valid state, use their latest
3762-
// per-commitment point
3763-
if per_commitment_points.0 == commitment_number { &per_commitment_points.1 }
3764-
else if let Some(point) = per_commitment_points.2.as_ref() {
3765-
// If counterparty commitment tx is the state previous to the latest valid state, use
3766-
// their previous per-commitment point (non-atomicity of revocation means it's valid for
3767-
// them to temporarily have two valid commitment txns from our viewpoint)
3768-
if per_commitment_points.0 == commitment_number + 1 {
3769-
point
3770-
} else { return (claimable_outpoints, to_counterparty_output_info); }
3771-
} else { return (claimable_outpoints, to_counterparty_output_info); };
3772-
3773-
if let Some(transaction) = tx {
3774-
let revocation_pubkey = RevocationKey::from_basepoint(
3775-
&self.onchain_tx_handler.secp_ctx,
3776-
&self.holder_revocation_basepoint,
3777-
&per_commitment_point,
3778-
);
3779-
3780-
let delayed_key = DelayedPaymentKey::from_basepoint(
3781-
&self.onchain_tx_handler.secp_ctx,
3782-
&self.counterparty_commitment_params.counterparty_delayed_payment_base_key,
3783-
&per_commitment_point,
3784-
);
3785-
3786-
let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(
3787-
&revocation_pubkey,
3788-
self.counterparty_commitment_params.on_counterparty_tx_csv,
3789-
&delayed_key,
3790-
)
3791-
.to_p2wsh();
3792-
for (idx, outp) in transaction.output.iter().enumerate() {
3793-
if outp.script_pubkey == revokeable_p2wsh {
3794-
to_counterparty_output_info =
3795-
Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value));
3796-
}
3825+
let revocation_pubkey = RevocationKey::from_basepoint(
3826+
&self.onchain_tx_handler.secp_ctx,
3827+
&self.holder_revocation_basepoint,
3828+
&per_commitment_point,
3829+
);
3830+
let delayed_key = DelayedPaymentKey::from_basepoint(
3831+
&self.onchain_tx_handler.secp_ctx,
3832+
&self.counterparty_commitment_params.counterparty_delayed_payment_base_key,
3833+
&per_commitment_point,
3834+
);
3835+
let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(
3836+
&revocation_pubkey,
3837+
self.counterparty_commitment_params.on_counterparty_tx_csv,
3838+
&delayed_key,
3839+
)
3840+
.to_p2wsh();
3841+
for (idx, outp) in tx.output.iter().enumerate() {
3842+
if outp.script_pubkey == revokeable_p2wsh {
3843+
to_counterparty_output_info =
3844+
Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value));
37973845
}
37983846
}
37993847

38003848
for &(ref htlc, _) in per_commitment_claimable_data.iter() {
38013849
if let Some(transaction_output_index) = htlc.transaction_output_index {
3802-
if let Some(transaction) = tx {
3803-
if transaction_output_index as usize >= transaction.output.len()
3804-
|| transaction.output[transaction_output_index as usize].value
3805-
!= htlc.to_bitcoin_amount()
3806-
{
3807-
// per_commitment_data is corrupt or our commitment signing key leaked!
3808-
return (claimable_outpoints, to_counterparty_output_info);
3809-
}
3850+
if transaction_output_index as usize >= tx.output.len()
3851+
|| tx.output[transaction_output_index as usize].value
3852+
!= htlc.to_bitcoin_amount()
3853+
{
3854+
// per_commitment_data is corrupt or our commitment signing key leaked!
3855+
return (claimable_outpoints, to_counterparty_output_info);
38103856
}
38113857
let preimage = if htlc.offered {
38123858
if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) {
@@ -3821,7 +3867,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
38213867
let counterparty_htlc_outp = if htlc.offered {
38223868
PackageSolvingData::CounterpartyOfferedHTLCOutput(
38233869
CounterpartyOfferedHTLCOutput::build(
3824-
*per_commitment_point,
3870+
per_commitment_point,
38253871
self.counterparty_commitment_params.counterparty_delayed_payment_base_key,
38263872
self.counterparty_commitment_params.counterparty_htlc_base_key,
38273873
preimage.unwrap(),
@@ -3833,7 +3879,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
38333879
} else {
38343880
PackageSolvingData::CounterpartyReceivedHTLCOutput(
38353881
CounterpartyReceivedHTLCOutput::build(
3836-
*per_commitment_point,
3882+
per_commitment_point,
38373883
self.counterparty_commitment_params.counterparty_delayed_payment_base_key,
38383884
self.counterparty_commitment_params.counterparty_htlc_base_key,
38393885
htlc.clone(),

‎lightning/src/ln/monitor_tests.rs‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3693,3 +3693,85 @@ fn test_lost_timeout_monitor_events() {
36933693
do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, false);
36943694
do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, true);
36953695
}
3696+
3697+
#[test]
3698+
fn test_ladder_preimage_htlc_claims() {
3699+
// Tests that when we learn of a preimage via a monitor update we only claim HTLCs with the
3700+
// corresponding payment hash. This test is a reproduction of a scenario that happened in
3701+
// production where the second HTLC claim also included the first HTLC (even though it was
3702+
// already claimed) resulting in an invalid claim transaction.
3703+
let chanmon_cfgs = create_chanmon_cfgs(2);
3704+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
3705+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
3706+
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
3707+
3708+
let node_id_0 = nodes[0].node.get_our_node_id();
3709+
let node_id_1 = nodes[1].node.get_our_node_id();
3710+
3711+
let (_, _, channel_id, _) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
3712+
3713+
let (payment_preimage1, payment_hash1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
3714+
let (payment_preimage2, payment_hash2, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
3715+
3716+
nodes[0].node.force_close_broadcasting_latest_txn(&channel_id, &node_id_1, "test".to_string()).unwrap();
3717+
check_added_monitors(&nodes[0], 1);
3718+
check_closed_broadcast(&nodes[0], 1, true);
3719+
let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) };
3720+
check_closed_event(&nodes[0], 1, reason, false, &[node_id_1], 1_000_000);
3721+
3722+
let commitment_tx = {
3723+
let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
3724+
assert_eq!(txn.len(), 1);
3725+
txn.remove(0)
3726+
};
3727+
mine_transaction(&nodes[0], &commitment_tx);
3728+
mine_transaction(&nodes[1], &commitment_tx);
3729+
3730+
check_closed_broadcast(&nodes[1], 1, true);
3731+
check_added_monitors(&nodes[1], 1);
3732+
check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false, &[node_id_0], 1_000_000);
3733+
3734+
nodes[1].node.claim_funds(payment_preimage1);
3735+
expect_payment_claimed!(&nodes[1], payment_hash1, 1_000_000);
3736+
check_added_monitors(&nodes[1], 1);
3737+
3738+
let (htlc1, htlc_claim_tx1) = {
3739+
let mut txn = nodes[1].tx_broadcaster.txn_broadcast();
3740+
assert_eq!(txn.len(), 1);
3741+
let htlc_claim_tx = txn.remove(0);
3742+
assert_eq!(htlc_claim_tx.input.len(), 1);
3743+
check_spends!(htlc_claim_tx, commitment_tx);
3744+
(htlc_claim_tx.input[0].previous_output, htlc_claim_tx)
3745+
};
3746+
mine_transaction(&nodes[0], &htlc_claim_tx1);
3747+
mine_transaction(&nodes[1], &htlc_claim_tx1);
3748+
3749+
connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
3750+
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
3751+
3752+
expect_payment_sent(&nodes[0], payment_preimage1, None, true, false);
3753+
check_added_monitors(&nodes[0], 1);
3754+
3755+
nodes[1].node.claim_funds(payment_preimage2);
3756+
expect_payment_claimed!(&nodes[1], payment_hash2, 1_000_000);
3757+
check_added_monitors(&nodes[1], 1);
3758+
3759+
let (htlc2, htlc_claim_tx2) = {
3760+
let mut txn = nodes[1].tx_broadcaster.txn_broadcast();
3761+
assert_eq!(txn.len(), 1, "{:?}", txn.iter().map(|tx| tx.compute_txid()).collect::<Vec<_>>());
3762+
let htlc_claim_tx = txn.remove(0);
3763+
assert_eq!(htlc_claim_tx.input.len(), 1);
3764+
check_spends!(htlc_claim_tx, commitment_tx);
3765+
(htlc_claim_tx.input[0].previous_output, htlc_claim_tx)
3766+
};
3767+
assert_ne!(htlc1, htlc2);
3768+
3769+
mine_transaction(&nodes[0], &htlc_claim_tx2);
3770+
mine_transaction(&nodes[1], &htlc_claim_tx2);
3771+
3772+
connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
3773+
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
3774+
3775+
expect_payment_sent(&nodes[0], payment_preimage2, None, true, false);
3776+
check_added_monitors(&nodes[0], 1);
3777+
}

0 commit comments

Comments
 (0)