Skip to content

Commit

Permalink
Mark counterparty revoked offered HTLCs as Unpinnable
Browse files Browse the repository at this point in the history 
If the counterparty broadcasts a revoked transaction with offered
HTLCs, the output is not immediately pinnable as the counterparty
cannot claim the HTLC until the CLTV expires and they use an
HTLC-Timeout path.

Here we properly set these packages as `Unpinnable`, changing some
transaction generation during tests.
  • Loading branch information
@TheBlueMatt
TheBlueMatt committed Jan 27, 2025
1 parent 1be6840 commit 215995c
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 38 deletions.
9 changes: 7 additions & 2 deletions lightning/src/chain/package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(..) =>
Expand Down
78 changes: 45 additions & 33 deletions lightning/src/ln/functional_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -2509,14 +2509,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);
Expand Down Expand Up @@ -2736,28 +2734,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);
}
Expand Down Expand Up @@ -4920,8 +4916,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]);
Expand All @@ -4937,20 +4932,27 @@ 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 within COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE, 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);
Expand All @@ -4960,6 +4962,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);
Expand Down Expand Up @@ -4992,6 +5000,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);
Expand Down
10 changes: 7 additions & 3 deletions lightning/src/ln/monitor_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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() {
Expand Down

0 comments on commit 215995c

Please sign in to comment.