-
Notifications
You must be signed in to change notification settings - Fork 411
Introduce new BumpTransactionEvent variant HTLCResolution #1825
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.
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
Changes from all commits
af02d10
d618bf1
baf2dec
d7027c2
6874126
d0847bd
0aaba2c
c17ce2e
8170c84
ec1f334
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,7 +53,7 @@ use crate::util::ser::{Readable, ReadableArgs, MaybeReadable, Writer, Writeable, | |
use crate::util::byte_utils; | ||
use crate::util::events::Event; | ||
#[cfg(anchors)] | ||
use crate::util::events::{AnchorDescriptor, BumpTransactionEvent}; | ||
use crate::util::events::{AnchorDescriptor, HTLCDescriptor, BumpTransactionEvent}; | ||
|
||
use crate::prelude::*; | ||
use core::{cmp, mem}; | ||
|
@@ -647,6 +647,7 @@ struct IrrevocablyResolvedHTLC { | |
/// was not present in the confirmed commitment transaction), HTLC-Success, or HTLC-Timeout | ||
/// transaction. | ||
resolving_txid: Option<Txid>, // Added as optional, but always filled in, in 0.0.110 | ||
resolving_tx: Option<Transaction>, | ||
/// Only set if the HTLC claim was ours using a payment preimage | ||
payment_preimage: Option<PaymentPreimage>, | ||
} | ||
|
@@ -662,6 +663,7 @@ impl Writeable for IrrevocablyResolvedHTLC { | |
(0, mapped_commitment_tx_output_idx, required), | ||
(1, self.resolving_txid, option), | ||
(2, self.payment_preimage, option), | ||
(3, self.resolving_tx, option), | ||
}); | ||
Ok(()) | ||
} | ||
|
@@ -672,15 +674,18 @@ impl Readable for IrrevocablyResolvedHTLC { | |
let mut mapped_commitment_tx_output_idx = 0; | ||
let mut resolving_txid = None; | ||
let mut payment_preimage = None; | ||
let mut resolving_tx = None; | ||
read_tlv_fields!(reader, { | ||
(0, mapped_commitment_tx_output_idx, required), | ||
(1, resolving_txid, option), | ||
(2, payment_preimage, option), | ||
(3, resolving_tx, option), | ||
}); | ||
Ok(Self { | ||
commitment_tx_output_idx: if mapped_commitment_tx_output_idx == u32::max_value() { None } else { Some(mapped_commitment_tx_output_idx) }, | ||
resolving_txid, | ||
payment_preimage, | ||
resolving_tx, | ||
}) | ||
} | ||
} | ||
|
@@ -1526,6 +1531,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
if let Some(v) = htlc.transaction_output_index { v } else { return None; }; | ||
|
||
let mut htlc_spend_txid_opt = None; | ||
let mut htlc_spend_tx_opt = None; | ||
let mut holder_timeout_spend_pending = None; | ||
let mut htlc_spend_pending = None; | ||
let mut holder_delayed_output_pending = None; | ||
|
@@ -1534,15 +1540,19 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
OnchainEvent::HTLCUpdate { commitment_tx_output_idx, htlc_value_satoshis, .. } | ||
if commitment_tx_output_idx == Some(htlc_commitment_tx_output_idx) => { | ||
debug_assert!(htlc_spend_txid_opt.is_none()); | ||
wpaulino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
htlc_spend_txid_opt = event.transaction.as_ref().map(|tx| tx.txid()); | ||
htlc_spend_txid_opt = Some(&event.txid); | ||
debug_assert!(htlc_spend_tx_opt.is_none()); | ||
htlc_spend_tx_opt = event.transaction.as_ref(); | ||
debug_assert!(holder_timeout_spend_pending.is_none()); | ||
debug_assert_eq!(htlc_value_satoshis.unwrap(), htlc.amount_msat / 1000); | ||
holder_timeout_spend_pending = Some(event.confirmation_threshold()); | ||
}, | ||
OnchainEvent::HTLCSpendConfirmation { commitment_tx_output_idx, preimage, .. } | ||
if commitment_tx_output_idx == htlc_commitment_tx_output_idx => { | ||
debug_assert!(htlc_spend_txid_opt.is_none()); | ||
htlc_spend_txid_opt = event.transaction.as_ref().map(|tx| tx.txid()); | ||
htlc_spend_txid_opt = Some(&event.txid); | ||
debug_assert!(htlc_spend_tx_opt.is_none()); | ||
htlc_spend_tx_opt = event.transaction.as_ref(); | ||
debug_assert!(htlc_spend_pending.is_none()); | ||
htlc_spend_pending = Some((event.confirmation_threshold(), preimage.is_some())); | ||
}, | ||
|
@@ -1558,19 +1568,32 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
let htlc_resolved = self.htlcs_resolved_on_chain.iter() | ||
.find(|v| if v.commitment_tx_output_idx == Some(htlc_commitment_tx_output_idx) { | ||
debug_assert!(htlc_spend_txid_opt.is_none()); | ||
htlc_spend_txid_opt = v.resolving_txid; | ||
htlc_spend_txid_opt = v.resolving_txid.as_ref(); | ||
debug_assert!(htlc_spend_tx_opt.is_none()); | ||
htlc_spend_tx_opt = v.resolving_tx.as_ref(); | ||
true | ||
} else { false }); | ||
debug_assert!(holder_timeout_spend_pending.is_some() as u8 + htlc_spend_pending.is_some() as u8 + htlc_resolved.is_some() as u8 <= 1); | ||
|
||
let htlc_commitment_outpoint = BitcoinOutPoint::new(confirmed_txid.unwrap(), htlc_commitment_tx_output_idx); | ||
let htlc_output_to_spend = | ||
if let Some(txid) = htlc_spend_txid_opt { | ||
debug_assert!( | ||
self.onchain_tx_handler.channel_transaction_parameters.opt_anchors.is_none(), | ||
"This code needs updating for anchors"); | ||
BitcoinOutPoint::new(txid, 0) | ||
// Because HTLC transactions either only have 1 input and 1 output (pre-anchors) or | ||
// are signed with SIGHASH_SINGLE|ANYONECANPAY under BIP-0143 (post-anchors), we can | ||
// locate the correct output by ensuring its adjacent input spends the HTLC output | ||
// in the commitment. | ||
if let Some(ref tx) = htlc_spend_tx_opt { | ||
let htlc_input_idx_opt = tx.input.iter().enumerate() | ||
.find(|(_, input)| input.previous_output == htlc_commitment_outpoint) | ||
.map(|(idx, _)| idx as u32); | ||
debug_assert!(htlc_input_idx_opt.is_some()); | ||
BitcoinOutPoint::new(*txid, htlc_input_idx_opt.unwrap_or(0)) | ||
} else { | ||
debug_assert!(!self.onchain_tx_handler.opt_anchors()); | ||
BitcoinOutPoint::new(*txid, 0) | ||
} | ||
} else { | ||
BitcoinOutPoint::new(confirmed_txid.unwrap(), htlc_commitment_tx_output_idx) | ||
htlc_commitment_outpoint | ||
}; | ||
let htlc_output_spend_pending = self.onchain_tx_handler.is_output_spend_pending(&htlc_output_to_spend); | ||
|
||
|
@@ -1594,8 +1617,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
} = &event.event { | ||
if event.transaction.as_ref().map(|tx| tx.input.iter().any(|inp| { | ||
if let Some(htlc_spend_txid) = htlc_spend_txid_opt { | ||
Some(tx.txid()) == htlc_spend_txid_opt || | ||
inp.previous_output.txid == htlc_spend_txid | ||
tx.txid() == *htlc_spend_txid || inp.previous_output.txid == *htlc_spend_txid | ||
} else { | ||
Some(inp.previous_output.txid) == confirmed_txid && | ||
inp.previous_output.vout == htlc_commitment_tx_output_idx | ||
|
@@ -2403,6 +2425,27 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
pending_htlcs, | ||
})); | ||
}, | ||
ClaimEvent::BumpHTLC { | ||
target_feerate_sat_per_1000_weight, htlcs, | ||
} => { | ||
let mut htlc_descriptors = Vec::with_capacity(htlcs.len()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I think we can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can probably replace most |
||
for htlc in htlcs { | ||
htlc_descriptors.push(HTLCDescriptor { | ||
channel_keys_id: self.channel_keys_id, | ||
channel_value_satoshis: self.channel_value_satoshis, | ||
channel_parameters: self.onchain_tx_handler.channel_transaction_parameters.clone(), | ||
commitment_txid: htlc.commitment_txid, | ||
per_commitment_number: htlc.per_commitment_number, | ||
htlc: htlc.htlc, | ||
preimage: htlc.preimage, | ||
counterparty_sig: htlc.counterparty_sig, | ||
}); | ||
} | ||
ret.push(Event::BumpTransaction(BumpTransactionEvent::HTLCResolution { | ||
target_feerate_sat_per_1000_weight, | ||
htlc_descriptors, | ||
})); | ||
} | ||
} | ||
} | ||
ret | ||
|
@@ -2623,31 +2666,49 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
} | ||
|
||
/// Attempts to claim a counterparty HTLC-Success/HTLC-Timeout's outputs using the revocation key | ||
fn check_spend_counterparty_htlc<L: Deref>(&mut self, tx: &Transaction, commitment_number: u64, height: u32, logger: &L) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) where L::Target: Logger { | ||
let htlc_txid = tx.txid(); | ||
if tx.input.len() != 1 || tx.output.len() != 1 || tx.input[0].witness.len() != 5 { | ||
return (Vec::new(), None) | ||
} | ||
|
||
macro_rules! ignore_error { | ||
( $thing : expr ) => { | ||
match $thing { | ||
Ok(a) => a, | ||
Err(_) => return (Vec::new(), None) | ||
} | ||
}; | ||
} | ||
|
||
fn check_spend_counterparty_htlc<L: Deref>( | ||
&mut self, tx: &Transaction, commitment_number: u64, commitment_txid: &Txid, height: u32, logger: &L | ||
) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) where L::Target: Logger { | ||
let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); }; | ||
let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret)); | ||
let per_commitment_key = match SecretKey::from_slice(&secret) { | ||
Ok(key) => key, | ||
Err(_) => return (Vec::new(), None) | ||
}; | ||
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key); | ||
|
||
log_error!(logger, "Got broadcast of revoked counterparty HTLC transaction, spending {}:{}", htlc_txid, 0); | ||
let revk_outp = RevokedOutput::build(per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, tx.output[0].value, self.counterparty_commitment_params.on_counterparty_tx_csv); | ||
let justice_package = PackageTemplate::build_package(htlc_txid, 0, PackageSolvingData::RevokedOutput(revk_outp), height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32, true, height); | ||
let claimable_outpoints = vec!(justice_package); | ||
let outputs = vec![(0, tx.output[0].clone())]; | ||
(claimable_outpoints, Some((htlc_txid, outputs))) | ||
let htlc_txid = tx.txid(); | ||
let mut claimable_outpoints = vec![]; | ||
let mut outputs_to_watch = None; | ||
// Previously, we would only claim HTLCs from revoked HTLC transactions if they had 1 input | ||
// with a witness of 5 elements and 1 output. This wasn't enough for anchor outputs, as the | ||
// counterparty can now aggregate multiple HTLCs into a single transaction thanks to | ||
// `SIGHASH_SINGLE` remote signatures, leading us to not claim any HTLCs upon seeing a | ||
// confirmed revoked HTLC transaction (for more details, see | ||
// https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-April/003561.html). | ||
// | ||
// We make sure we're not vulnerable to this case by checking all inputs of the transaction, | ||
// and claim those which spend the commitment transaction, have a witness of 5 elements, and | ||
// have a corresponding output at the same index within the transaction. | ||
for (idx, input) in tx.input.iter().enumerate() { | ||
wpaulino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if input.previous_output.txid == *commitment_txid && input.witness.len() == 5 && tx.output.get(idx).is_some() { | ||
wpaulino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
log_error!(logger, "Got broadcast of revoked counterparty HTLC transaction, spending {}:{}", htlc_txid, idx); | ||
let revk_outp = RevokedOutput::build( | ||
per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, | ||
self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, | ||
tx.output[idx].value, self.counterparty_commitment_params.on_counterparty_tx_csv | ||
); | ||
let justice_package = PackageTemplate::build_package( | ||
htlc_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp), | ||
height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32, true, height | ||
); | ||
claimable_outpoints.push(justice_package); | ||
if outputs_to_watch.is_none() { | ||
outputs_to_watch = Some((htlc_txid, vec![])); | ||
} | ||
outputs_to_watch.as_mut().unwrap().1.push((idx as u32, tx.output[idx].clone())); | ||
} | ||
} | ||
(claimable_outpoints, outputs_to_watch) | ||
} | ||
|
||
// Returns (1) `PackageTemplate`s that can be given to the OnchainTxHandler, so that the handler can | ||
|
@@ -2661,18 +2722,28 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
|
||
for &(ref htlc, _, _) in holder_tx.htlc_outputs.iter() { | ||
if let Some(transaction_output_index) = htlc.transaction_output_index { | ||
let htlc_output = if htlc.offered { | ||
HolderHTLCOutput::build_offered(htlc.amount_msat, htlc.cltv_expiry) | ||
let (htlc_output, aggregable) = if htlc.offered { | ||
let htlc_output = HolderHTLCOutput::build_offered( | ||
htlc.amount_msat, htlc.cltv_expiry, self.onchain_tx_handler.opt_anchors() | ||
); | ||
(htlc_output, false) | ||
} else { | ||
let payment_preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) { | ||
preimage.clone() | ||
} else { | ||
let payment_preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) { | ||
preimage.clone() | ||
} else { | ||
// We can't build an HTLC-Success transaction without the preimage | ||
continue; | ||
}; | ||
HolderHTLCOutput::build_accepted(payment_preimage, htlc.amount_msat) | ||
// We can't build an HTLC-Success transaction without the preimage | ||
continue; | ||
}; | ||
let htlc_package = PackageTemplate::build_package(holder_tx.txid, transaction_output_index, PackageSolvingData::HolderHTLCOutput(htlc_output), htlc.cltv_expiry, false, conf_height); | ||
let htlc_output = HolderHTLCOutput::build_accepted( | ||
payment_preimage, htlc.amount_msat, self.onchain_tx_handler.opt_anchors() | ||
); | ||
(htlc_output, self.onchain_tx_handler.opt_anchors()) | ||
}; | ||
let htlc_package = PackageTemplate::build_package( | ||
holder_tx.txid, transaction_output_index, | ||
wpaulino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
PackageSolvingData::HolderHTLCOutput(htlc_output), | ||
htlc.cltv_expiry, aggregable, conf_height | ||
); | ||
claim_requests.push(htlc_package); | ||
} | ||
} | ||
|
@@ -2905,9 +2976,9 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
|
||
if tx.input.len() == 1 { | ||
// Assuming our keys were not leaked (in which case we're screwed no matter what), | ||
// commitment transactions and HTLC transactions will all only ever have one input, | ||
// which is an easy way to filter out any potential non-matching txn for lazy | ||
// filters. | ||
// commitment transactions and HTLC transactions will all only ever have one input | ||
// (except for HTLC transactions for channels with anchor outputs), which is an easy | ||
// way to filter out any potential non-matching txn for lazy filters. | ||
wpaulino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let prevout = &tx.input[0].previous_output; | ||
if prevout.txid == self.funding_info.0.txid && prevout.vout == self.funding_info.0.index as u32 { | ||
let mut balance_spendable_csv = None; | ||
|
@@ -2945,22 +3016,33 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
commitment_tx_to_counterparty_output, | ||
}, | ||
}); | ||
} else { | ||
if let Some(&commitment_number) = self.counterparty_commitment_txn_on_chain.get(&prevout.txid) { | ||
let (mut new_outpoints, new_outputs_option) = self.check_spend_counterparty_htlc(&tx, commitment_number, height, &logger); | ||
} | ||
} | ||
if tx.input.len() >= 1 { | ||
// While all commitment transactions have one input, HTLC transactions may have more | ||
// if the HTLC was present in an anchor channel. HTLCs can also be resolved in a few | ||
// other ways which can have more than one output. | ||
wpaulino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for tx_input in &tx.input { | ||
let commitment_txid = tx_input.previous_output.txid; | ||
if let Some(&commitment_number) = self.counterparty_commitment_txn_on_chain.get(&commitment_txid) { | ||
let (mut new_outpoints, new_outputs_option) = self.check_spend_counterparty_htlc( | ||
&tx, commitment_number, &commitment_txid, height, &logger | ||
); | ||
claimable_outpoints.append(&mut new_outpoints); | ||
if let Some(new_outputs) = new_outputs_option { | ||
watch_outputs.push(new_outputs); | ||
} | ||
// Since there may be multiple HTLCs (all from the same commitment) being | ||
// claimed by the counterparty within the same transaction, and | ||
// `check_spend_counterparty_htlc` already checks for all of them, we can | ||
// safely break from our loop. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is assumption is broken, and per se our code will still miss the detection and claim of revoked second-stage HTLC transactions. With There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay after looking more, the code is correct, even if the comment is wrong or confusing on the
Note, here we're checking the second-stage HTLC transactions, and we're filtering out the input/output pair to be claimed based on the spent commitment transaction id. Post-anchor output, second-stage HTLC transactions are signed with
This is a correct, the claim of HTLCs is a per-ChannelMonitor responsibility, and I don't think there is a way open by malleability where the aggregated HTLC claims crafted by our counterparty could blind our parsing logic. Note, it makes the logic dependent on the implementation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Since the
Doesn't seem like there's much we can do other than document it? Even then, I would imagine all LDK users want to use our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
More documentation of our assumptions is always better. I think with the extended flexibility offered by our interfaces we have far less visibility on how it could be re-implemented by users rather than a monolithic Lightning node directly consuming from standard Core interfaces. This is raising the bar for funds safety, as implicit assumptions of our low-level code could be silently broken. Beyond, we might have in the future to re-implement a |
||
break; | ||
} | ||
} | ||
} | ||
// While all commitment/HTLC-Success/HTLC-Timeout transactions have one input, HTLCs | ||
// can also be resolved in a few other ways which can have more than one output. Thus, | ||
// we call is_resolving_htlc_output here outside of the tx.input.len() == 1 check. | ||
self.is_resolving_htlc_output(&tx, height, &block_hash, &logger); | ||
self.is_resolving_htlc_output(&tx, height, &block_hash, &logger); | ||
wpaulino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
self.is_paying_spendable_output(&tx, height, &block_hash, &logger); | ||
self.is_paying_spendable_output(&tx, height, &block_hash, &logger); | ||
} | ||
} | ||
|
||
if height > self.best_block.height() { | ||
|
@@ -3074,7 +3156,9 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
htlc_value_satoshis, | ||
})); | ||
self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { | ||
commitment_tx_output_idx, resolving_txid: Some(entry.txid), | ||
commitment_tx_output_idx, | ||
resolving_txid: Some(entry.txid), | ||
resolving_tx: entry.transaction, | ||
payment_preimage: None, | ||
}); | ||
}, | ||
|
@@ -3087,7 +3171,9 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> { | |
}, | ||
OnchainEvent::HTLCSpendConfirmation { commitment_tx_output_idx, preimage, .. } => { | ||
self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { | ||
commitment_tx_output_idx: Some(commitment_tx_output_idx), resolving_txid: Some(entry.txid), | ||
commitment_tx_output_idx: Some(commitment_tx_output_idx), | ||
resolving_txid: Some(entry.txid), | ||
resolving_tx: entry.transaction, | ||
payment_preimage: preimage, | ||
}); | ||
}, | ||
|
Uh oh!
There was an error while loading. Please reload this page.