mirrors/rust-lightning
1
0
Fork 0
mirror of https://github.com/lightningdevkit/rust-lightning.git synced 2025-02-22 14:33:08 +01:00
Code Issues Projects Releases Packages Wiki Activity

Merge pull request #3564 from TheBlueMatt/2025-01-revoked-htlc-not-pinnable

Browse source 
Set correct `counterparty_spendable_height` on c.p. revoked HTLCs
This commit is contained in:
Matt Corallo 2025-01-30 18:54:51 +00:00 • committed by GitHub
commit 1434e9c1e2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 88 additions and 54 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
lightning/src/chain/channelmonitor.rs
View file

@ -3564,11 +3564,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
return (claimable_outpoints, to_counterparty_output_info);
}
let revk_htlc_outp = RevokedHTLCOutput::build(per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, htlc.amount_msat / 1000, htlc.clone(), &self.onchain_tx_handler.channel_transaction_parameters.channel_type_features);
let counterparty_spendable_height = if htlc.offered {
htlc.cltv_expiry
} else {
height
};
let justice_package = PackageTemplate::build_package(
commitment_txid,
transaction_output_index,
PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp),
htlc.cltv_expiry,
counterparty_spendable_height,
);
claimable_outpoints.push(justice_package);
}

46
lightning/src/chain/package.rs
View file

@ -699,8 +699,13 @@ impl PackageSolvingData {
match self {
PackageSolvingData::RevokedOutput(RevokedOutput { .. }) =>
PackageMalleability::Malleable(AggregationCluster::Unpinnable),
PackageSolvingData::RevokedHTLCOutput(..) =>
PackageMalleability::Malleable(AggregationCluster::Pinnable),
PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { htlc, .. }) => {
if htlc.offered {
PackageMalleability::Malleable(AggregationCluster::Unpinnable)
} else {
PackageMalleability::Malleable(AggregationCluster::Pinnable)
}
},
PackageSolvingData::CounterpartyOfferedHTLCOutput(..) =>
PackageMalleability::Malleable(AggregationCluster::Unpinnable),
PackageSolvingData::CounterpartyReceivedHTLCOutput(..) =>
@ -771,10 +776,12 @@ pub struct PackageTemplate {
/// Block height at which our counterparty can potentially claim this output as well (assuming
/// they have the keys or information required to do so).
///
/// This is used primarily by external consumers to decide when an output becomes "pinnable"
/// because the counterparty can potentially spend it. It is also used internally by
/// [`Self::get_height_timer`] to identify when an output must be claimed by, depending on the
/// type of output.
/// This is used primarily to decide when an output becomes "pinnable" because the counterparty
/// can potentially spend it. It is also used internally by [`Self::get_height_timer`] to
/// identify when an output must be claimed by, depending on the type of output.
///
/// Note that for revoked counterparty HTLC outputs the value may be zero in some cases where
/// we upgraded from LDK 0.1 or prior.
counterparty_spendable_height: u32,
// Cache of package feerate committed at previous (re)broadcast. If bumping resources
// (either claimed output value or external utxo), it will keep increasing until holder
@ -834,17 +841,17 @@ impl PackageTemplate {
// Now check that we only merge packages if they are both unpinnable or both
// pinnable.
let self_pinnable = self_cluster == AggregationCluster::Pinnable ||
self.counterparty_spendable_height() <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
self.counterparty_spendable_height <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
let other_pinnable = other_cluster == AggregationCluster::Pinnable ||
other.counterparty_spendable_height() <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
other.counterparty_spendable_height <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
if self_pinnable && other_pinnable {
return true;
}
let self_unpinnable = self_cluster == AggregationCluster::Unpinnable &&
self.counterparty_spendable_height() > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
self.counterparty_spendable_height > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
let other_unpinnable = other_cluster == AggregationCluster::Unpinnable &&
other.counterparty_spendable_height() > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
other.counterparty_spendable_height > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
if self_unpinnable && other_unpinnable {
return true;
}
@ -855,13 +862,6 @@ impl PackageTemplate {
pub(crate) fn is_malleable(&self) -> bool {
matches!(self.malleability, PackageMalleability::Malleable(..))
}
/// The height at which our counterparty may be able to spend this output.
///
/// This is an important limit for aggregation as after this height our counterparty may be
/// able to pin transactions spending this output in the mempool.
pub(crate) fn counterparty_spendable_height(&self) -> u32 {
self.counterparty_spendable_height
}
pub(crate) fn previous_feerate(&self) -> u64 {
self.feerate_previous
}
@ -1225,6 +1225,18 @@ impl Readable for PackageTemplate {
(4, _height_original, option), // Written with a dummy value since 0.1
(6, height_timer, option),
});
for (_, input) in &inputs {
if let PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { htlc, .. }) = input {
// LDK versions through 0.1 set the wrong counterparty_spendable_height for
// non-offered revoked HTLCs (ie HTLCs we sent to our counterparty which they can
// claim with a preimage immediately). Here we detect this and reset the value to
// zero, as the value is unused except for merging decisions which doesn't care
// about any values below the current height.
if !htlc.offered && htlc.cltv_expiry == counterparty_spendable_height {
counterparty_spendable_height = 0;
}
}
}
Ok(PackageTemplate {
inputs,
malleability,

79
lightning/src/ln/functional_tests.rs
View file

@ -15,7 +15,7 @@ use crate::chain;
use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch};
use crate::chain::chaininterface::LowerBoundedFeeEstimator;
use crate::chain::channelmonitor;
use crate::chain::channelmonitor::{Balance, ChannelMonitorUpdateStep, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
use crate::chain::channelmonitor::{Balance, ChannelMonitorUpdateStep, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE};
use crate::chain::transaction::OutPoint;
use crate::sign::{ecdsa::EcdsaChannelSigner, EntropySource, OutputSpender, SignerProvider};
use crate::events::bump_transaction::WalletSource;
@ -2631,14 +2631,12 @@ fn test_justice_tx_htlc_timeout() {
mine_transaction(&nodes[1], &revoked_local_txn[0]);
{
let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
// The unpinnable, revoked to_self output, and the pinnable, revoked htlc output will
// be claimed in separate transactions.
assert_eq!(node_txn.len(), 2);
for tx in node_txn.iter() {
assert_eq!(tx.input.len(), 1);
check_spends!(tx, revoked_local_txn[0]);
}
assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
// The revoked HTLC output is not pinnable for another `TEST_FINAL_CLTV` blocks, and is
// thus claimed in the same transaction with the revoked to_self output.
assert_eq!(node_txn.len(), 1);
assert_eq!(node_txn[0].input.len(), 2);
check_spends!(node_txn[0], revoked_local_txn[0]);
assert_ne!(node_txn[0].input[0].previous_output, node_txn[0].input[1].previous_output);
node_txn.clear();
}
check_added_monitors!(nodes[1], 1);
@ -2858,28 +2856,26 @@ fn claim_htlc_outputs() {
assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
assert_eq!(node_txn.len(), 2); // Two penalty transactions:
assert_eq!(node_txn[0].input.len(), 1); // Claims the unpinnable, revoked output.
assert_eq!(node_txn[1].input.len(), 2); // Claims both pinnable, revoked HTLC outputs separately.
check_spends!(node_txn[0], revoked_local_txn[0]);
check_spends!(node_txn[1], revoked_local_txn[0]);
assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[1].previous_output);
assert_ne!(node_txn[1].input[0].previous_output, node_txn[1].input[1].previous_output);
assert_eq!(node_txn.len(), 2); // ChannelMonitor: penalty txn
// The ChannelMonitor should claim the accepted HTLC output separately from the offered
// HTLC and to_self outputs.
let accepted_claim = node_txn.iter().filter(|tx| tx.input.len() == 1).next().unwrap();
let offered_to_self_claim = node_txn.iter().filter(|tx| tx.input.len() == 2).next().unwrap();
check_spends!(accepted_claim, revoked_local_txn[0]);
check_spends!(offered_to_self_claim, revoked_local_txn[0]);
assert_eq!(accepted_claim.input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
let mut witness_lens = BTreeSet::new();
witness_lens.insert(node_txn[0].input[0].witness.last().unwrap().len());
witness_lens.insert(node_txn[1].input[0].witness.last().unwrap().len());
witness_lens.insert(node_txn[1].input[1].witness.last().unwrap().len());
assert_eq!(witness_lens.len(), 3);
witness_lens.insert(offered_to_self_claim.input[0].witness.last().unwrap().len());
witness_lens.insert(offered_to_self_claim.input[1].witness.last().unwrap().len());
assert_eq!(witness_lens.len(), 2);
assert_eq!(*witness_lens.iter().skip(0).next().unwrap(), 77); // revoked to_local
assert_eq!(*witness_lens.iter().skip(1).next().unwrap(), OFFERED_HTLC_SCRIPT_WEIGHT); // revoked offered HTLC
assert_eq!(*witness_lens.iter().skip(2).next().unwrap(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // revoked received HTLC
assert_eq!(*witness_lens.iter().skip(1).next().unwrap(), OFFERED_HTLC_SCRIPT_WEIGHT);
// Finally, mine the penalty transactions and check that we get an HTLC failure after
// Finally, mine the penalty transaction and check that we get an HTLC failure after
// ANTI_REORG_DELAY confirmations.
mine_transaction(&nodes[1], &node_txn[0]);
mine_transaction(&nodes[1], &node_txn[1]);
mine_transaction(&nodes[1], accepted_claim);
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
expect_payment_failed!(nodes[1], payment_hash_2, false);
}
@ -5042,8 +5038,7 @@ fn test_static_spendable_outputs_timeout_tx() {
check_spends!(spend_txn[2], node_txn[0], commitment_tx[0]); // All outputs
}
#[test]
fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() {
fn do_test_static_spendable_outputs_justice_tx_revoked_commitment_tx(split_tx: bool) {
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]);
@ -5059,20 +5054,28 @@ fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() {
claim_payment(&nodes[0], &vec!(&nodes[1])[..], payment_preimage);
if split_tx {
connect_blocks(&nodes[1], TEST_FINAL_CLTV - COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE + 1);
}
mine_transaction(&nodes[1], &revoked_local_txn[0]);
check_closed_broadcast!(nodes[1], true);
check_added_monitors!(nodes[1], 1);
check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [nodes[0].node.get_our_node_id()], 100000);
// The unpinnable, revoked to_self output and the pinnable, revoked HTLC output will be claimed
// in separate transactions.
// If the HTLC expires in more than COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE blocks, we'll
// claim both the revoked and HTLC outputs in one transaction, otherwise we'll split them as we
// consider the HTLC output as pinnable and want to claim pinnable and unpinnable outputs
// separately.
let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
assert_eq!(node_txn.len(), 2);
assert_eq!(node_txn.len(), if split_tx { 2 } else { 1 });
for tx in node_txn.iter() {
assert_eq!(tx.input.len(), 1);
assert_eq!(tx.input.len(), if split_tx { 1 } else { 2 });
check_spends!(tx, revoked_local_txn[0]);
}
assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
if split_tx {
assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
}
mine_transaction(&nodes[1], &node_txn[0]);
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
@ -5082,6 +5085,12 @@ fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() {
check_spends!(spend_txn[0], node_txn[0]);
}
#[test]
fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() {
do_test_static_spendable_outputs_justice_tx_revoked_commitment_tx(true);
do_test_static_spendable_outputs_justice_tx_revoked_commitment_tx(false);
}
#[test]
fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() {
let mut chanmon_cfgs = create_chanmon_cfgs(2);
@ -5114,6 +5123,10 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() {
check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);
assert_ne!(revoked_htlc_txn[0].lock_time, LockTime::ZERO); // HTLC-Timeout
// In order to connect `revoked_htlc_txn[0]` we must first advance the chain by
// `TEST_FINAL_CLTV` blocks as otherwise the transaction is consensus-invalid due to its
// locktime.
connect_blocks(&nodes[1], TEST_FINAL_CLTV);
// B will generate justice tx from A's revoked commitment/HTLC tx
connect_block(&nodes[1], &create_dummy_block(nodes[1].best_block_hash(), 42, vec![revoked_local_txn[0].clone(), revoked_htlc_txn[0].clone()]));
check_closed_broadcast!(nodes[1], true);

10
lightning/src/ln/monitor_tests.rs
View file

@ -10,7 +10,7 @@
//! Further functional tests which test blockchain reorganizations.
use crate::sign::{ecdsa::EcdsaChannelSigner, OutputSpender, SpendableOutputDescriptor};
use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ARCHIVAL_DELAY_BLOCKS,LATENCY_GRACE_PERIOD_BLOCKS, Balance, BalanceSource, ChannelMonitorUpdateStep};
use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ARCHIVAL_DELAY_BLOCKS,LATENCY_GRACE_PERIOD_BLOCKS, COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE, Balance, BalanceSource, ChannelMonitorUpdateStep};
use crate::chain::transaction::OutPoint;
use crate::chain::chaininterface::{ConfirmationTarget, LowerBoundedFeeEstimator, compute_feerate_sat_per_1000_weight};
use crate::events::bump_transaction::{BumpTransactionEvent, WalletSource};
@ -1734,6 +1734,12 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
assert_eq!(revoked_htlc_success.lock_time, LockTime::ZERO);
assert_ne!(revoked_htlc_timeout.lock_time, LockTime::ZERO);
// First connect blocks until the HTLC expires with
// `COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE` blocks, making us consider all the HTLCs
// pinnable claims, which the remainder of the test assumes.
connect_blocks(&nodes[0], TEST_FINAL_CLTV - COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE);
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(&nodes[0],
[HTLCDestination::FailedPayment { payment_hash: failed_payment_hash }]);
// A will generate justice tx from B's revoked commitment/HTLC tx
mine_transaction(&nodes[0], &revoked_local_txn[0]);
check_closed_broadcast!(nodes[0], true);
@ -1846,8 +1852,6 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()));
connect_blocks(&nodes[0], revoked_htlc_timeout.lock_time.to_consensus_u32() - nodes[0].best_block_info().1);
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(&nodes[0],
[HTLCDestination::FailedPayment { payment_hash: failed_payment_hash }]);
// As time goes on A may split its revocation claim transaction into multiple.
let as_fewer_input_rbf = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
for tx in as_fewer_input_rbf.iter() {