Skip to content

Commit 652f0f1

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #4154 from wpaulino/ladder-preimage-claim-fix
Only claim HTLCs with matching payment hash upon preimage monitor update
2 parents eeb5b14 + 46f0d31 commit 652f0f1

File tree

2 files changed

+202
-51
lines changed

2 files changed

+202
-51
lines changed

‎lightning/src/chain/channelmonitor.rs‎

Lines changed: 120 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -3833,7 +3833,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
38333833
// First check if a counterparty commitment transaction has been broadcasted:
38343834
macro_rules! claim_htlcs {
38353835
($commitment_number: expr, $txid: expr, $htlcs: expr) => {
3836-
let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info(funding_spent, $commitment_number, $txid, None, $htlcs, confirmed_spend_height);
3836+
let htlc_claim_reqs = self.get_counterparty_output_claims_for_preimage(*payment_preimage, funding_spent, $commitment_number, $txid, $htlcs, confirmed_spend_height);
38373837
let conf_target = self.closure_conf_target();
38383838
self.onchain_tx_handler.update_claims_view_from_requests(
38393839
htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster,
@@ -4732,7 +4732,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
47324732
(htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref()))
47334733
), logger);
47344734
let (htlc_claim_reqs, counterparty_output_info) =
4735-
self.get_counterparty_output_claim_info(funding_spent, commitment_number, commitment_txid, Some(commitment_tx), per_commitment_option, Some(height));
4735+
self.get_counterparty_output_claim_info(funding_spent, commitment_number, commitment_txid, commitment_tx, per_commitment_claimable_data, Some(height));
47364736
to_counterparty_output_info = counterparty_output_info;
47374737
for req in htlc_claim_reqs {
47384738
claimable_outpoints.push(req);
@@ -4742,87 +4742,156 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
47424742
(claimable_outpoints, to_counterparty_output_info)
47434743
}
47444744

4745+
fn get_point_for_commitment_number(&self, commitment_number: u64) -> Option<PublicKey> {
4746+
let per_commitment_points = &self.their_cur_per_commitment_points?;
4747+
4748+
// If the counterparty commitment tx is the latest valid state, use their latest
4749+
// per-commitment point
4750+
if per_commitment_points.0 == commitment_number {
4751+
Some(per_commitment_points.1)
4752+
} else if let Some(point) = per_commitment_points.2.as_ref() {
4753+
// If counterparty commitment tx is the state previous to the latest valid state, use
4754+
// their previous per-commitment point (non-atomicity of revocation means it's valid for
4755+
// them to temporarily have two valid commitment txns from our viewpoint)
4756+
if per_commitment_points.0 == commitment_number + 1 {
4757+
Some(*point)
4758+
} else {
4759+
None
4760+
}
4761+
} else {
4762+
None
4763+
}
4764+
}
4765+
4766+
fn get_counterparty_output_claims_for_preimage(
4767+
&self, preimage: PaymentPreimage, funding_spent: &FundingScope, commitment_number: u64,
4768+
commitment_txid: Txid,
4769+
per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>,
4770+
confirmation_height: Option<u32>,
4771+
) -> Vec<PackageTemplate> {
4772+
let per_commitment_claimable_data = match per_commitment_option {
4773+
Some(outputs) => outputs,
4774+
None => return Vec::new(),
4775+
};
4776+
let per_commitment_point = match self.get_point_for_commitment_number(commitment_number) {
4777+
Some(point) => point,
4778+
None => return Vec::new(),
4779+
};
4780+
4781+
let matching_payment_hash = PaymentHash::from(preimage);
4782+
per_commitment_claimable_data
4783+
.iter()
4784+
.filter_map(|(htlc, _)| {
4785+
if let Some(transaction_output_index) = htlc.transaction_output_index {
4786+
if htlc.offered && htlc.payment_hash == matching_payment_hash {
4787+
let htlc_data = PackageSolvingData::CounterpartyOfferedHTLCOutput(
4788+
CounterpartyOfferedHTLCOutput::build(
4789+
per_commitment_point,
4790+
preimage,
4791+
htlc.clone(),
4792+
funding_spent.channel_parameters.clone(),
4793+
confirmation_height,
4794+
),
4795+
);
4796+
Some(PackageTemplate::build_package(
4797+
commitment_txid,
4798+
transaction_output_index,
4799+
htlc_data,
4800+
htlc.cltv_expiry,
4801+
))
4802+
} else {
4803+
None
4804+
}
4805+
} else {
4806+
None
4807+
}
4808+
})
4809+
.collect()
4810+
}
4811+
47454812
/// Returns the HTLC claim package templates and the counterparty output info
4746-
#[rustfmt::skip]
47474813
fn get_counterparty_output_claim_info(
47484814
&self, funding_spent: &FundingScope, commitment_number: u64, commitment_txid: Txid,
4749-
tx: Option<&Transaction>,
4750-
per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>,
4815+
tx: &Transaction,
4816+
per_commitment_claimable_data: &[(HTLCOutputInCommitment, Option<Box<HTLCSource>>)],
47514817
confirmation_height: Option<u32>,
47524818
) -> (Vec<PackageTemplate>, CommitmentTxCounterpartyOutputInfo) {
47534819
let mut claimable_outpoints = Vec::new();
47544820
let mut to_counterparty_output_info: CommitmentTxCounterpartyOutputInfo = None;
47554821

4756-
let per_commitment_claimable_data = match per_commitment_option {
4757-
Some(outputs) => outputs,
4758-
None => return (claimable_outpoints, to_counterparty_output_info),
4759-
};
4760-
let per_commitment_points = match self.their_cur_per_commitment_points {
4761-
Some(points) => points,
4822+
let per_commitment_point = match self.get_point_for_commitment_number(commitment_number) {
4823+
Some(point) => point,
47624824
None => return (claimable_outpoints, to_counterparty_output_info),
47634825
};
47644826

4765-
let per_commitment_point =
4766-
// If the counterparty commitment tx is the latest valid state, use their latest
4767-
// per-commitment point
4768-
if per_commitment_points.0 == commitment_number { &per_commitment_points.1 }
4769-
else if let Some(point) = per_commitment_points.2.as_ref() {
4770-
// If counterparty commitment tx is the state previous to the latest valid state, use
4771-
// their previous per-commitment point (non-atomicity of revocation means it's valid for
4772-
// them to temporarily have two valid commitment txns from our viewpoint)
4773-
if per_commitment_points.0 == commitment_number + 1 {
4774-
point
4775-
} else { return (claimable_outpoints, to_counterparty_output_info); }
4776-
} else { return (claimable_outpoints, to_counterparty_output_info); };
4777-
4778-
if let Some(transaction) = tx {
4779-
let revocation_pubkey = RevocationKey::from_basepoint(
4780-
&self.onchain_tx_handler.secp_ctx, &self.holder_revocation_basepoint, &per_commitment_point);
4781-
4782-
let delayed_key = DelayedPaymentKey::from_basepoint(&self.onchain_tx_handler.secp_ctx, &self.counterparty_commitment_params.counterparty_delayed_payment_base_key, &per_commitment_point);
4783-
4784-
let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(&revocation_pubkey,
4785-
self.counterparty_commitment_params.on_counterparty_tx_csv,
4786-
&delayed_key).to_p2wsh();
4787-
for (idx, outp) in transaction.output.iter().enumerate() {
4788-
if outp.script_pubkey == revokeable_p2wsh {
4789-
to_counterparty_output_info =
4790-
Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value));
4791-
}
4827+
let revocation_pubkey = RevocationKey::from_basepoint(
4828+
&self.onchain_tx_handler.secp_ctx,
4829+
&self.holder_revocation_basepoint,
4830+
&per_commitment_point,
4831+
);
4832+
let delayed_key = DelayedPaymentKey::from_basepoint(
4833+
&self.onchain_tx_handler.secp_ctx,
4834+
&self.counterparty_commitment_params.counterparty_delayed_payment_base_key,
4835+
&per_commitment_point,
4836+
);
4837+
let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(
4838+
&revocation_pubkey,
4839+
self.counterparty_commitment_params.on_counterparty_tx_csv,
4840+
&delayed_key,
4841+
)
4842+
.to_p2wsh();
4843+
for (idx, outp) in tx.output.iter().enumerate() {
4844+
if outp.script_pubkey == revokeable_p2wsh {
4845+
to_counterparty_output_info =
4846+
Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value));
47924847
}
47934848
}
47944849

4795-
for &(ref htlc, _) in per_commitment_claimable_data.iter() {
4850+
for &(ref htlc, _) in per_commitment_claimable_data.iter() {
47964851
if let Some(transaction_output_index) = htlc.transaction_output_index {
4797-
if let Some(transaction) = tx {
4798-
if transaction_output_index as usize >= transaction.output.len() ||
4799-
transaction.output[transaction_output_index as usize].value != htlc.to_bitcoin_amount() {
4800-
// per_commitment_data is corrupt or our commitment signing key leaked!
4801-
return (claimable_outpoints, to_counterparty_output_info);
4802-
}
4852+
if transaction_output_index as usize >= tx.output.len()
4853+
|| tx.output[transaction_output_index as usize].value
4854+
!= htlc.to_bitcoin_amount()
4855+
{
4856+
// per_commitment_data is corrupt or our commitment signing key leaked!
4857+
return (claimable_outpoints, to_counterparty_output_info);
48034858
}
4804-
let preimage = if htlc.offered { if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) { Some(*p) } else { None } } else { None };
4859+
let preimage = if htlc.offered {
4860+
if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) {
4861+
Some(*p)
4862+
} else {
4863+
None
4864+
}
4865+
} else {
4866+
None
4867+
};
48054868
if preimage.is_some() || !htlc.offered {
48064869
let counterparty_htlc_outp = if htlc.offered {
48074870
PackageSolvingData::CounterpartyOfferedHTLCOutput(
48084871
CounterpartyOfferedHTLCOutput::build(
4809-
*per_commitment_point, preimage.unwrap(),
4872+
per_commitment_point,
4873+
preimage.unwrap(),
48104874
htlc.clone(),
48114875
funding_spent.channel_parameters.clone(),
48124876
confirmation_height,
4813-
)
4877+
),
48144878
)
48154879
} else {
48164880
PackageSolvingData::CounterpartyReceivedHTLCOutput(
48174881
CounterpartyReceivedHTLCOutput::build(
4818-
*per_commitment_point,
4882+
per_commitment_point,
48194883
htlc.clone(),
48204884
funding_spent.channel_parameters.clone(),
48214885
confirmation_height,
4822-
)
4886+
),
48234887
)
48244888
};
4825-
let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry);
4889+
let counterparty_package = PackageTemplate::build_package(
4890+
commitment_txid,
4891+
transaction_output_index,
4892+
counterparty_htlc_outp,
4893+
htlc.cltv_expiry,
4894+
);
48264895
claimable_outpoints.push(counterparty_package);
48274896
}
48284897
}

‎lightning/src/ln/monitor_tests.rs‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3836,3 +3836,85 @@ fn test_lost_timeout_monitor_events() {
38363836
do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, false, true);
38373837
do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, true, true);
38383838
}
3839+
3840+
#[test]
3841+
fn test_ladder_preimage_htlc_claims() {
3842+
// Tests that when we learn of a preimage via a monitor update we only claim HTLCs with the
3843+
// corresponding payment hash. This test is a reproduction of a scenario that happened in
3844+
// production where the second HTLC claim also included the first HTLC (even though it was
3845+
// already claimed) resulting in an invalid claim transaction.
3846+
let chanmon_cfgs = create_chanmon_cfgs(2);
3847+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
3848+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
3849+
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
3850+
3851+
let node_id_0 = nodes[0].node.get_our_node_id();
3852+
let node_id_1 = nodes[1].node.get_our_node_id();
3853+
3854+
let (_, _, channel_id, _) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
3855+
3856+
let (payment_preimage1, payment_hash1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
3857+
let (payment_preimage2, payment_hash2, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
3858+
3859+
nodes[0].node.force_close_broadcasting_latest_txn(&channel_id, &node_id_1, "test".to_string()).unwrap();
3860+
check_added_monitors(&nodes[0], 1);
3861+
check_closed_broadcast(&nodes[0], 1, true);
3862+
let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message: "test".to_string() };
3863+
check_closed_event(&nodes[0], 1, reason, false, &[node_id_1], 1_000_000);
3864+
3865+
let commitment_tx = {
3866+
let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
3867+
assert_eq!(txn.len(), 1);
3868+
txn.remove(0)
3869+
};
3870+
mine_transaction(&nodes[0], &commitment_tx);
3871+
mine_transaction(&nodes[1], &commitment_tx);
3872+
3873+
check_closed_broadcast(&nodes[1], 1, true);
3874+
check_added_monitors(&nodes[1], 1);
3875+
check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false, &[node_id_0], 1_000_000);
3876+
3877+
nodes[1].node.claim_funds(payment_preimage1);
3878+
expect_payment_claimed!(&nodes[1], payment_hash1, 1_000_000);
3879+
check_added_monitors(&nodes[1], 1);
3880+
3881+
let (htlc1, htlc_claim_tx1) = {
3882+
let mut txn = nodes[1].tx_broadcaster.txn_broadcast();
3883+
assert_eq!(txn.len(), 1);
3884+
let htlc_claim_tx = txn.remove(0);
3885+
assert_eq!(htlc_claim_tx.input.len(), 1);
3886+
check_spends!(htlc_claim_tx, commitment_tx);
3887+
(htlc_claim_tx.input[0].previous_output, htlc_claim_tx)
3888+
};
3889+
mine_transaction(&nodes[0], &htlc_claim_tx1);
3890+
mine_transaction(&nodes[1], &htlc_claim_tx1);
3891+
3892+
connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
3893+
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
3894+
3895+
expect_payment_sent(&nodes[0], payment_preimage1, None, true, false);
3896+
check_added_monitors(&nodes[0], 1);
3897+
3898+
nodes[1].node.claim_funds(payment_preimage2);
3899+
expect_payment_claimed!(&nodes[1], payment_hash2, 1_000_000);
3900+
check_added_monitors(&nodes[1], 1);
3901+
3902+
let (htlc2, htlc_claim_tx2) = {
3903+
let mut txn = nodes[1].tx_broadcaster.txn_broadcast();
3904+
assert_eq!(txn.len(), 1, "{:?}", txn.iter().map(|tx| tx.compute_txid()).collect::<Vec<_>>());
3905+
let htlc_claim_tx = txn.remove(0);
3906+
assert_eq!(htlc_claim_tx.input.len(), 1);
3907+
check_spends!(htlc_claim_tx, commitment_tx);
3908+
(htlc_claim_tx.input[0].previous_output, htlc_claim_tx)
3909+
};
3910+
assert_ne!(htlc1, htlc2);
3911+
3912+
mine_transaction(&nodes[0], &htlc_claim_tx2);
3913+
mine_transaction(&nodes[1], &htlc_claim_tx2);
3914+
3915+
connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
3916+
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
3917+
3918+
expect_payment_sent(&nodes[0], payment_preimage2, None, true, false);
3919+
check_added_monitors(&nodes[0], 1);
3920+
}

0 commit comments

Comments
 (0)