Skip to content

Commit 9d2a190

Browse files
committed
Fold sign_holder_commitment_htlc_transactions into sign_holder_commitment
Signing the commitment transaction is almost always followed by signing the attached HTLC transactions, so fold the signing operations into a single method.
1 parent 9c9c881 commit 9d2a190

File tree

4 files changed

+29
-55
lines changed

4 files changed

+29
-55
lines changed

lightning/src/chain/keysinterface.rs

Lines changed: 6 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ pub trait ChannelKeys : Send+Clone {
239239
/// An external signer implementation should check that the commitment has not been revoked.
240240
//
241241
// TODO: Document the things someone using this interface should enforce before signing.
242-
fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;
242+
fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()>;
243243

244244
/// Same as sign_holder_commitment, but exists only for tests to get access to holder commitment
245245
/// transactions which will be broadcasted later, after the channel has moved on to a newer
@@ -248,18 +248,6 @@ pub trait ChannelKeys : Send+Clone {
248248
#[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
249249
fn unsafe_sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;
250250

251-
/// Create a signature for each HTLC transaction spending a holder's commitment transaction.
252-
///
253-
/// Unlike sign_holder_commitment, this may be called multiple times with *different*
254-
/// commitment_tx values. While this will never be called with a revoked
255-
/// commitment_tx, it is possible that it is called with the second-latest
256-
/// commitment_tx (only if we haven't yet revoked it) if some watchtower/secondary
257-
/// ChannelMonitor decided to broadcast before it had been updated to the latest.
258-
///
259-
/// Either an Err should be returned, or a Vec with one entry for each HTLC which exists in
260-
/// commitment_tx.
261-
fn sign_holder_commitment_htlc_transactions<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Vec<Signature>, ()>;
262-
263251
/// Create a signature for the given input in a transaction spending an HTLC or commitment
264252
/// transaction output when our counterparty broadcasts an old state.
265253
///
@@ -492,11 +480,14 @@ impl ChannelKeys for InMemoryChannelKeys {
492480
Ok((commitment_sig, htlc_sigs))
493481
}
494482

495-
fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
483+
fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
496484
let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
497485
let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey);
498486
let sig = commitment_tx.trust().built_transaction().sign(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, secp_ctx);
499-
Ok(sig)
487+
let channel_parameters = self.get_channel_parameters();
488+
let trusted_tx = commitment_tx.trust();
489+
let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), secp_ctx)?;
490+
Ok((sig, htlc_sigs))
500491
}
501492

502493
#[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
@@ -506,12 +497,6 @@ impl ChannelKeys for InMemoryChannelKeys {
506497
Ok(commitment_tx.trust().built_transaction().sign(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx))
507498
}
508499

509-
fn sign_holder_commitment_htlc_transactions<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Vec<Signature>, ()> {
510-
let channel_parameters = self.get_channel_parameters();
511-
let trusted_tx = commitment_tx.trust();
512-
trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), secp_ctx)
513-
}
514-
515500
fn sign_justice_transaction<T: secp256k1::Signing + secp256k1::Verification>(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &Option<HTLCOutputInCommitment>, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
516501
let revocation_key = match chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key) {
517502
Ok(revocation_key) => revocation_key,

lightning/src/ln/channel.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4723,15 +4723,13 @@ mod tests {
47234723
&chan.holder_keys.pubkeys().funding_pubkey,
47244724
chan.counterparty_funding_pubkey()
47254725
);
4726-
let holder_sig = chan_keys.sign_holder_commitment(&holder_commitment_tx, &secp_ctx).unwrap();
4726+
let (holder_sig, htlc_sigs) = chan_keys.sign_holder_commitment(&holder_commitment_tx, &secp_ctx).unwrap();
47274727
assert_eq!(Signature::from_der(&hex::decode($sig_hex).unwrap()[..]).unwrap(), holder_sig, "holder_sig");
47284728

47294729
let funding_redeemscript = chan.get_funding_redeemscript();
47304730
let tx = holder_commitment_tx.add_holder_sig(&funding_redeemscript, holder_sig);
47314731
assert_eq!(serialize(&tx)[..], hex::decode($tx_hex).unwrap()[..], "tx");
47324732

4733-
let htlc_sigs = chan_keys.sign_holder_commitment_htlc_transactions(&holder_commitment_tx, &secp_ctx).unwrap();
4734-
47354733
// ((htlc, counterparty_sig), (index, holder_sig))
47364734
let mut htlc_sig_iter = holder_commitment_tx.htlcs().iter().zip(&holder_commitment_tx.counterparty_htlc_sigs).zip(htlc_sigs.iter().enumerate());
47374735

lightning/src/ln/onchaintx.rs

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -890,20 +890,29 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
890890

891891
pub(crate) fn provide_latest_holder_tx(&mut self, tx: HolderCommitmentTransaction) {
892892
self.prev_holder_commitment = self.holder_commitment.take();
893+
self.holder_htlc_sigs = None;
893894
self.holder_commitment = Some(tx);
894895
}
895896

897+
// Normally holder HTLCs are signed at the same time as the holder commitment tx. However,
898+
// in some configurations, the holder commitment tx has been signed and broadcast by a replica of
899+
// the ChannelMonitor, so we handle that case here.
896900
fn sign_latest_holder_htlcs(&mut self) {
897-
if let Some(ref holder_commitment) = self.holder_commitment {
898-
if let Ok(sigs) = self.key_storage.sign_holder_commitment_htlc_transactions(holder_commitment, &self.secp_ctx) {
901+
if self.holder_htlc_sigs.is_none() {
902+
if let Some(ref holder_commitment) = self.holder_commitment {
903+
let (_sig, sigs) = self.key_storage.sign_holder_commitment(holder_commitment, &self.secp_ctx).expect("sign holder commitment");
899904
self.holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, sigs));
900905
}
901906
}
902907
}
903908

909+
// Normally only the latest commitment tx and HTLCs need to be signed. However, in some
910+
// configurations we may have updated our holder commtiment but a replica of the ChannelMonitor
911+
// broadcast the previous one before we sync with it. We handle that case here.
904912
fn sign_prev_holder_htlcs(&mut self) {
905-
if let Some(ref holder_commitment) = self.prev_holder_commitment {
906-
if let Ok(sigs) = self.key_storage.sign_holder_commitment_htlc_transactions(holder_commitment, &self.secp_ctx) {
913+
if self.prev_holder_htlc_sigs.is_none() {
914+
if let Some(ref holder_commitment) = self.prev_holder_commitment {
915+
let (_sig, sigs) = self.key_storage.sign_holder_commitment(holder_commitment, &self.secp_ctx).expect("sign previous holder commitment");
907916
self.prev_holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, sigs));
908917
}
909918
}
@@ -925,8 +934,9 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
925934
// to monitor before.
926935
pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Option<Transaction> {
927936
if let Some(ref mut holder_commitment) = self.holder_commitment {
928-
match self.key_storage.sign_holder_commitment(&holder_commitment, &self.secp_ctx) {
929-
Ok(sig) => {
937+
match self.key_storage.sign_holder_commitment(holder_commitment, &self.secp_ctx) {
938+
Ok((sig, htlc_sigs)) => {
939+
self.holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, htlc_sigs));
930940
Some(holder_commitment.add_holder_sig(funding_redeemscript, sig))
931941
},
932942
Err(_) => return None,
@@ -940,7 +950,8 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
940950
pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Option<Transaction> {
941951
if let Some(ref mut holder_commitment) = self.holder_commitment {
942952
match self.key_storage.sign_holder_commitment(holder_commitment, &self.secp_ctx) {
943-
Ok(sig) => {
953+
Ok((sig, htlc_sigs)) => {
954+
self.holder_htlc_sigs = Some(Self::extract_holder_sigs(holder_commitment, htlc_sigs));
944955
Some(holder_commitment.add_holder_sig(funding_redeemscript, sig))
945956
},
946957
Err(_) => return None,
@@ -966,7 +977,7 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
966977
}
967978
}
968979
}
969-
if self.prev_holder_commitment.is_some() {
980+
if htlc_tx.is_none() && self.prev_holder_commitment.is_some() {
970981
let commitment_txid = self.prev_holder_commitment.as_ref().unwrap().trust().txid();
971982
if commitment_txid == outp.txid {
972983
self.sign_prev_holder_htlcs();

lightning/src/util/enforcing_trait_impls.rs

Lines changed: 3 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,13 @@
88
// licenses.
99

1010
use ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, HolderCommitmentTransaction, CommitmentTransaction, ChannelTransactionParameters, TrustedCommitmentTransaction};
11-
use ln::{chan_utils, msgs};
11+
use ln::msgs;
1212
use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys};
1313

1414
use std::cmp;
1515
use std::sync::{Mutex, Arc};
1616

17-
use bitcoin::blockdata::transaction::{Transaction, SigHashType};
18-
use bitcoin::util::bip143;
17+
use bitcoin::blockdata::transaction::Transaction;
1918

2019
use bitcoin::secp256k1;
2120
use bitcoin::secp256k1::key::{SecretKey, PublicKey};
@@ -72,7 +71,7 @@ impl ChannelKeys for EnforcingChannelKeys {
7271
Ok(self.inner.sign_counterparty_commitment(commitment_tx, secp_ctx).unwrap())
7372
}
7473

75-
fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
74+
fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
7675
self.verify_holder_commitment_tx(commitment_tx, secp_ctx);
7776

7877
// TODO: enforce the ChannelKeys contract - error if this commitment was already revoked
@@ -85,25 +84,6 @@ impl ChannelKeys for EnforcingChannelKeys {
8584
Ok(self.inner.unsafe_sign_holder_commitment(commitment_tx, secp_ctx).unwrap())
8685
}
8786

88-
fn sign_holder_commitment_htlc_transactions<T: secp256k1::Signing + secp256k1::Verification>(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Vec<Signature>, ()> {
89-
let trusted_tx = self.verify_holder_commitment_tx(commitment_tx, secp_ctx);
90-
let commitment_txid = trusted_tx.txid();
91-
let holder_csv = self.inner.counterparty_selected_contest_delay();
92-
93-
for (this_htlc, sig) in trusted_tx.htlcs().iter().zip(&commitment_tx.counterparty_htlc_sigs) {
94-
assert!(this_htlc.transaction_output_index.is_some());
95-
let keys = trusted_tx.keys();
96-
let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, trusted_tx.feerate_per_kw(), holder_csv, &this_htlc, &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
97-
98-
let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&this_htlc, &keys);
99-
100-
let sighash = hash_to_message!(&bip143::SigHashCache::new(&htlc_tx).signature_hash(0, &htlc_redeemscript, this_htlc.amount_msat / 1000, SigHashType::All)[..]);
101-
secp_ctx.verify(&sighash, sig, &keys.countersignatory_htlc_key).unwrap();
102-
}
103-
104-
Ok(self.inner.sign_holder_commitment_htlc_transactions(commitment_tx, secp_ctx).unwrap())
105-
}
106-
10787
fn sign_justice_transaction<T: secp256k1::Signing + secp256k1::Verification>(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &Option<HTLCOutputInCommitment>, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
10888
Ok(self.inner.sign_justice_transaction(justice_tx, input, amount, per_commitment_key, htlc, secp_ctx).unwrap())
10989
}

0 commit comments

Comments
 (0)