Skip to content

Support future removal of redundant per-HTLC signatures in CMUs #2101

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 6, 2023
Merged

Support future removal of redundant per-HTLC signatures in CMUs #2101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85b573d
Add an `optional_vec` TLV format which makes sense, unlike vec_type
TheBlueMatt Mar 20, 2023
b72f6b1
Support future removal of redundant per-HTLC data in `ChanMonUpd`s
TheBlueMatt Mar 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 79 additions & 21 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,8 +493,12 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,
pub(crate) enum ChannelMonitorUpdateStep {
LatestHolderCommitmentTXInfo {
commitment_tx: HolderCommitmentTransaction,
/// Note that LDK after 0.0.115 supports this only containing dust HTLCs (implying the
/// `Signature` field is never filled in). At that point, non-dust HTLCs are implied by the
/// HTLC fields in `commitment_tx` and the sources passed via `nondust_htlc_sources`.
htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
claimed_htlcs: Vec<(SentHTLCId, PaymentPreimage)>,
nondust_htlc_sources: Vec<HTLCSource>,
},
LatestCounterpartyCommitmentTXInfo {
commitment_txid: Txid,
Expand Down Expand Up @@ -539,6 +543,7 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep,
(0, commitment_tx, required),
(1, claimed_htlcs, vec_type),
(2, htlc_outputs, vec_type),
(4, nondust_htlc_sources, optional_vec),
},
(1, LatestCounterpartyCommitmentTXInfo) => {
(0, commitment_txid, required),
Expand Down Expand Up @@ -1180,7 +1185,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
&self, holder_commitment_tx: HolderCommitmentTransaction,
htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
) -> Result<(), ()> {
self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new()).map_err(|_| ())
self.inner.lock().unwrap().provide_latest_holder_commitment_tx(holder_commitment_tx, htlc_outputs, &Vec::new(), Vec::new()).map_err(|_| ())
}

/// This is used to provide payment preimage(s) out-of-band during startup without updating the
Expand Down Expand Up @@ -2160,7 +2165,53 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
/// is important that any clones of this channel monitor (including remote clones) by kept
/// up-to-date as our holder commitment transaction is updated.
/// Panics if set_on_holder_tx_csv has never been called.
fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)]) -> Result<(), &'static str> {
fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, mut htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], nondust_htlc_sources: Vec<HTLCSource>) -> Result<(), &'static str> {
if htlc_outputs.iter().any(|(_, s, _)| s.is_some()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehension check: do dust HTLCs not have signatures because there wouldn't be a point since they have no output to ever spend/enforce on-chain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. There's simply nothing to sign.

// If we have non-dust HTLCs in htlc_outputs, ensure they match the HTLCs in the
// `holder_commitment_tx`. In the future, we'll no longer provide the redundant data
// and just pass in source data via `nondust_htlc_sources`.
debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.trust().htlcs().len());
for (a, b) in htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).map(|(h, _, _)| h).zip(holder_commitment_tx.trust().htlcs().iter()) {
debug_assert_eq!(a, b);
}
debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.counterparty_htlc_sigs.len());
for (a, b) in htlc_outputs.iter().filter_map(|(_, s, _)| s.as_ref()).zip(holder_commitment_tx.counterparty_htlc_sigs.iter()) {
debug_assert_eq!(a, b);
}
debug_assert!(nondust_htlc_sources.is_empty());
} else {
// If we don't have any non-dust HTLCs in htlc_outputs, assume they were all passed via
// `nondust_htlc_sources`, building up the final htlc_outputs by combining
// `nondust_htlc_sources` and the `holder_commitment_tx`
#[cfg(debug_assertions)] {
let mut prev = -1;
for htlc in holder_commitment_tx.trust().htlcs().iter() {
assert!(htlc.transaction_output_index.unwrap() as i32 > prev);
prev = htlc.transaction_output_index.unwrap() as i32;
}
}
debug_assert!(htlc_outputs.iter().all(|(htlc, _, _)| htlc.transaction_output_index.is_none()));
debug_assert!(htlc_outputs.iter().all(|(_, sig_opt, _)| sig_opt.is_none()));
debug_assert_eq!(holder_commitment_tx.trust().htlcs().len(), holder_commitment_tx.counterparty_htlc_sigs.len());

let mut sources_iter = nondust_htlc_sources.into_iter();

for (htlc, counterparty_sig) in holder_commitment_tx.trust().htlcs().iter()
.zip(holder_commitment_tx.counterparty_htlc_sigs.iter())
{
if htlc.offered {
let source = sources_iter.next().expect("Non-dust HTLC sources didn't match commitment tx");
#[cfg(debug_assertions)] {
assert!(source.possibly_matches_output(htlc));
}
htlc_outputs.push((htlc.clone(), Some(counterparty_sig.clone()), Some(source)));
} else {
htlc_outputs.push((htlc.clone(), Some(counterparty_sig.clone()), None));
}
}
debug_assert!(sources_iter.next().is_none());
}

let trusted_tx = holder_commitment_tx.trust();
let txid = trusted_tx.txid();
let tx_keys = trusted_tx.keys();
Expand Down Expand Up @@ -2285,10 +2336,10 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&*fee_estimator);
for update in updates.updates.iter() {
match update {
ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs, claimed_htlcs } => {
ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { commitment_tx, htlc_outputs, claimed_htlcs, nondust_htlc_sources } => {
log_trace!(logger, "Updating ChannelMonitor with latest holder commitment transaction info");
if self.lockdown_from_offchain { panic!(); }
if let Err(e) = self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs) {
if let Err(e) = self.provide_latest_holder_commitment_tx(commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs, nondust_htlc_sources.clone()) {
log_error!(logger, "Providing latest holder commitment transaction failed/was refused:");
log_error!(logger, " {}", e);
ret = Err(());
Expand Down Expand Up @@ -4132,7 +4183,7 @@ mod tests {
}
}

macro_rules! preimages_slice_to_htlc_outputs {
macro_rules! preimages_slice_to_htlcs {
($preimages_slice: expr) => {
{
let mut res = Vec::new();
Expand All @@ -4143,21 +4194,20 @@ mod tests {
cltv_expiry: 0,
payment_hash: preimage.1.clone(),
transaction_output_index: Some(idx as u32),
}, None));
}, ()));
}
res
}
}
}
macro_rules! preimages_to_holder_htlcs {
macro_rules! preimages_slice_to_htlc_outputs {
($preimages_slice: expr) => {
{
let mut inp = preimages_slice_to_htlc_outputs!($preimages_slice);
let res: Vec<_> = inp.drain(..).map(|e| { (e.0, None, e.1) }).collect();
res
}
preimages_slice_to_htlcs!($preimages_slice).into_iter().map(|(htlc, _)| (htlc, None)).collect()
}
}
let dummy_sig = crate::util::crypto::sign(&secp_ctx,
&bitcoin::secp256k1::Message::from_slice(&[42; 32]).unwrap(),
&SecretKey::from_slice(&[42; 32]).unwrap());

macro_rules! test_preimages_exist {
($preimages_slice: expr, $monitor: expr) => {
Expand Down Expand Up @@ -4204,13 +4254,15 @@ mod tests {
let shutdown_pubkey = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
let best_block = BestBlock::from_network(Network::Testnet);
let monitor = ChannelMonitor::new(Secp256k1::new(), keys,
Some(ShutdownScript::new_p2wpkh_from_pubkey(shutdown_pubkey).into_inner()), 0, &Script::new(),
(OutPoint { txid: Txid::from_slice(&[43; 32]).unwrap(), index: 0 }, Script::new()),
&channel_parameters,
Script::new(), 46, 0,
HolderCommitmentTransaction::dummy(), best_block, dummy_key);

monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..10])).unwrap();
Some(ShutdownScript::new_p2wpkh_from_pubkey(shutdown_pubkey).into_inner()), 0, &Script::new(),
(OutPoint { txid: Txid::from_slice(&[43; 32]).unwrap(), index: 0 }, Script::new()),
&channel_parameters, Script::new(), 46, 0, HolderCommitmentTransaction::dummy(&mut Vec::new()),
best_block, dummy_key);

let mut htlcs = preimages_slice_to_htlcs!(preimages[0..10]);
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs);
monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
monitor.provide_latest_counterparty_commitment_tx(Txid::from_inner(Sha256::hash(b"1").into_inner()),
preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger);
monitor.provide_latest_counterparty_commitment_tx(Txid::from_inner(Sha256::hash(b"2").into_inner()),
Expand Down Expand Up @@ -4243,15 +4295,21 @@ mod tests {

// Now update holder commitment tx info, pruning only element 18 as we still care about the
// previous commitment tx's preimages too
monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..5])).unwrap();
let mut htlcs = preimages_slice_to_htlcs!(preimages[0..5]);
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs);
monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
secret[0..32].clone_from_slice(&hex::decode("2273e227a5b7449b6e70f1fb4652864038b1cbf9cd7c043a7d6456b7fc275ad8").unwrap());
monitor.provide_secret(281474976710653, secret.clone()).unwrap();
assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 12);
test_preimages_exist!(&preimages[0..10], monitor);
test_preimages_exist!(&preimages[18..20], monitor);

// But if we do it again, we'll prune 5-10
monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..3])).unwrap();
let mut htlcs = preimages_slice_to_htlcs!(preimages[0..3]);
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(&mut htlcs);
monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx,
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), None)).collect()).unwrap();
secret[0..32].clone_from_slice(&hex::decode("27cddaa5624534cb6cb9d7da077cf2b22ab21e9b506fd4998a51d54502e99116").unwrap());
monitor.provide_secret(281474976710652, secret.clone()).unwrap();
assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 5);
Expand Down
12 changes: 8 additions & 4 deletions lightning/src/ln/chan_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -984,7 +984,7 @@ impl_writeable_tlv_based!(HolderCommitmentTransaction, {

impl HolderCommitmentTransaction {
#[cfg(test)]
pub fn dummy() -> Self {
pub fn dummy(htlcs: &mut Vec<(HTLCOutputInCommitment, ())>) -> Self {
let secp_ctx = Secp256k1::new();
let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
let dummy_sig = sign(&secp_ctx, &secp256k1::Message::from_slice(&[42; 32]).unwrap(), &SecretKey::from_slice(&[42; 32]).unwrap());
Expand Down Expand Up @@ -1012,12 +1012,16 @@ impl HolderCommitmentTransaction {
opt_anchors: None,
opt_non_zero_fee_anchors: None,
};
let mut htlcs_with_aux: Vec<(_, ())> = Vec::new();
let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, 0, 0, false, dummy_key.clone(), dummy_key.clone(), keys, 0, &mut htlcs_with_aux, &channel_parameters.as_counterparty_broadcastable());
let mut counterparty_htlc_sigs = Vec::new();
for _ in 0..htlcs.len() {
counterparty_htlc_sigs.push(dummy_sig);
}
let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, 0, 0, false, dummy_key.clone(), dummy_key.clone(), keys, 0, htlcs, &channel_parameters.as_counterparty_broadcastable());
htlcs.sort_by_key(|htlc| htlc.0.transaction_output_index);
HolderCommitmentTransaction {
inner,
counterparty_sig: dummy_sig,
counterparty_htlc_sigs: Vec::new(),
counterparty_htlc_sigs,
holder_sig_first: false
}
}
Expand Down
32 changes: 28 additions & 4 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3122,9 +3122,24 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
return Err(ChannelError::Close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs)));
}

// TODO: Sadly, we pass HTLCs twice to ChannelMonitor: once via the HolderCommitmentTransaction and once via the update
// Up to LDK 0.0.115, HTLC information was required to be duplicated in the
// `htlcs_and_sigs` vec and in the `holder_commitment_tx` itself, both of which were passed
// in the `ChannelMonitorUpdate`. In 0.0.115, support for having a separate set of
// outbound-non-dust-HTLCSources in the `ChannelMonitorUpdate` was added, however for
// backwards compatibility, we never use it in production. To provide test coverage, here,
// we randomly decide (in test/fuzzing builds) to use the new vec sometimes.
#[allow(unused_assignments, unused_mut)]
let mut separate_nondust_htlc_sources = false;
#[cfg(all(feature = "std", any(test, fuzzing)))] {
use core::hash::{BuildHasher, Hasher};
// Get a random value using the only std API to do so - the DefaultHasher
let rand_val = std::collections::hash_map::RandomState::new().build_hasher().finish();
separate_nondust_htlc_sources = rand_val % 2 == 0;
}
Comment on lines +3132 to +3138
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get how this would work well with fuzzing, but for normal tests would you just have to run them a couple times to be confident the new implementation got tested? I guess this may be a temporary solution for testing so it's not a big deal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we have a bit of randomness in tests which avoids having to run every test five times but ensures over time we will hit any issues.


let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len());
let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len());
for (idx, (htlc, source)) in htlcs_cloned.drain(..).enumerate() {
for (idx, (htlc, mut source_opt)) in htlcs_cloned.drain(..).enumerate() {
if let Some(_) = htlc.transaction_output_index {
let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.feerate_per_kw,
self.get_counterparty_selected_contest_delay().unwrap(), &htlc, self.opt_anchors(),
Expand All @@ -3139,10 +3154,18 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
if let Err(_) = self.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &keys.countersignatory_htlc_key) {
return Err(ChannelError::Close("Invalid HTLC tx signature from peer".to_owned()));
}
htlcs_and_sigs.push((htlc, Some(msg.htlc_signatures[idx]), source));
if !separate_nondust_htlc_sources {
htlcs_and_sigs.push((htlc, Some(msg.htlc_signatures[idx]), source_opt.take()));
}
} else {
htlcs_and_sigs.push((htlc, None, source));
htlcs_and_sigs.push((htlc, None, source_opt.take()));
}
if separate_nondust_htlc_sources {
if let Some(source) = source_opt.take() {
nondust_htlc_sources.push(source);
}
}
debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere");
}

let holder_commitment_tx = HolderCommitmentTransaction::new(
Expand Down Expand Up @@ -3205,6 +3228,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
commitment_tx: holder_commitment_tx,
htlc_outputs: htlcs_and_sigs,
claimed_htlcs,
nondust_htlc_sources,
}]
};

Expand Down
16 changes: 14 additions & 2 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,9 +302,9 @@ impl core::hash::Hash for HTLCSource {
}
}
}
#[cfg(not(feature = "grind_signatures"))]
#[cfg(test)]
impl HTLCSource {
#[cfg(not(feature = "grind_signatures"))]
#[cfg(test)]
pub fn dummy() -> Self {
HTLCSource::OutboundRoute {
path: Vec::new(),
Expand All @@ -315,6 +315,18 @@ impl HTLCSource {
payment_params: None,
}
}

#[cfg(debug_assertions)]
/// Checks whether this HTLCSource could possibly match the given HTLC output in a commitment
/// transaction. Useful to ensure different datastructures match up.
pub(crate) fn possibly_matches_output(&self, htlc: &super::chan_utils::HTLCOutputInCommitment) -> bool {
if let HTLCSource::OutboundRoute { first_hop_htlc_msat, .. } = self {
*first_hop_htlc_msat == htlc.amount_msat
} else {
// There's nothing we can check for forwarded HTLCs
true
}
}
}

struct ReceiveError {
Expand Down
Loading