Skip to content

Commit d751d98

Browse files
committed
Don't sign holder HTLCs along with holder commitments
`sign_holder_commitment_and_htlcs` never really made sense. Unlike `sign_counterparty_commitment`, the signatures for holder HTLC transactions may be required much later than the commitment transaction's. While it was nice for us to only reach the signer once to obtain all holder signatures, it's not really ideal anymore as we want our signatures to be random and not reused. We no longer return all holder HTLC signatures and instead defer to obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.
1 parent d842e24 commit d751d98

File tree

5 files changed

+70
-61
lines changed

5 files changed

+70
-61
lines changed

lightning/src/chain/onchaintx.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -1101,13 +1101,13 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
11011101
// before providing a initial commitment transaction. For outbound channel, init ChannelMonitor at Channel::funding_signed, there is nothing
11021102
// to monitor before.
11031103
pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction {
1104-
let (sig, _) = self.signer.sign_holder_commitment_and_htlcs(&self.holder_commitment, &self.secp_ctx).expect("signing holder commitment");
1104+
let sig = self.signer.sign_holder_commitment(&self.holder_commitment, &self.secp_ctx).expect("signing holder commitment");
11051105
self.holder_commitment.add_holder_sig(funding_redeemscript, sig)
11061106
}
11071107

11081108
#[cfg(any(test, feature="unsafe_revoked_tx_signing"))]
11091109
pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction {
1110-
let (sig, _) = self.signer.unsafe_sign_holder_commitment_and_htlcs(&self.holder_commitment, &self.secp_ctx).expect("sign holder commitment");
1110+
let sig = self.signer.unsafe_sign_holder_commitment(&self.holder_commitment, &self.secp_ctx).expect("sign holder commitment");
11111111
self.holder_commitment.add_holder_sig(funding_redeemscript, sig)
11121112
}
11131113

lightning/src/ln/channel.rs

+25-12
Original file line numberDiff line numberDiff line change
@@ -8184,6 +8184,7 @@ mod tests {
81848184
use bitcoin::hashes::hex::FromHex;
81858185
use bitcoin::hash_types::Txid;
81868186
use bitcoin::secp256k1::Message;
8187+
use crate::events::bump_transaction::{ChannelDerivationParameters, HTLCDescriptor};
81878188
use crate::sign::EcdsaChannelSigner;
81888189
use crate::ln::PaymentPreimage;
81898190
use crate::ln::channel::{HTLCOutputInCommitment ,TxCreationKeys};
@@ -8309,22 +8310,22 @@ mod tests {
83098310
&chan.context.holder_signer.as_ref().pubkeys().funding_pubkey,
83108311
chan.context.counterparty_funding_pubkey()
83118312
);
8312-
let (holder_sig, htlc_sigs) = signer.sign_holder_commitment_and_htlcs(&holder_commitment_tx, &secp_ctx).unwrap();
8313+
let holder_sig = signer.sign_holder_commitment(&holder_commitment_tx, &secp_ctx).unwrap();
83138314
assert_eq!(Signature::from_der(&hex::decode($sig_hex).unwrap()[..]).unwrap(), holder_sig, "holder_sig");
83148315

83158316
let funding_redeemscript = chan.context.get_funding_redeemscript();
83168317
let tx = holder_commitment_tx.add_holder_sig(&funding_redeemscript, holder_sig);
83178318
assert_eq!(serialize(&tx)[..], hex::decode($tx_hex).unwrap()[..], "tx");
83188319

83198320
// ((htlc, counterparty_sig), (index, holder_sig))
8320-
let mut htlc_sig_iter = holder_commitment_tx.htlcs().iter().zip(&holder_commitment_tx.counterparty_htlc_sigs).zip(htlc_sigs.iter().enumerate());
8321+
let mut htlc_counterparty_sig_iter = holder_commitment_tx.counterparty_htlc_sigs.iter();
83218322

83228323
$({
83238324
log_trace!(logger, "verifying htlc {}", $htlc_idx);
83248325
let remote_signature = Signature::from_der(&hex::decode($counterparty_htlc_sig_hex).unwrap()[..]).unwrap();
83258326

83268327
let ref htlc = htlcs[$htlc_idx];
8327-
let htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw,
8328+
let mut htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw,
83288329
chan.context.get_counterparty_selected_contest_delay().unwrap(),
83298330
&htlc, $opt_anchors, &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
83308331
let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, $opt_anchors, &keys);
@@ -8344,20 +8345,32 @@ mod tests {
83448345
assert!(preimage.is_some());
83458346
}
83468347

8347-
let htlc_sig = htlc_sig_iter.next().unwrap();
8348+
let htlc_counterparty_sig = htlc_counterparty_sig_iter.next().unwrap();
8349+
let htlc_holder_sig = signer.sign_holder_htlc_transaction(&htlc_tx, 0, &HTLCDescriptor {
8350+
channel_derivation_parameters: ChannelDerivationParameters {
8351+
value_satoshis: chan.context.channel_value_satoshis,
8352+
keys_id: chan.context.channel_keys_id,
8353+
transaction_parameters: chan.context.channel_transaction_parameters.clone(),
8354+
},
8355+
commitment_txid: trusted_tx.txid(),
8356+
per_commitment_number: trusted_tx.commitment_number(),
8357+
per_commitment_point: trusted_tx.per_commitment_point(),
8358+
feerate_per_kw: trusted_tx.feerate_per_kw(),
8359+
htlc: htlc.clone(),
8360+
preimage: preimage.clone(),
8361+
counterparty_sig: *htlc_counterparty_sig,
8362+
}, &secp_ctx).unwrap();
83488363
let num_anchors = if $opt_anchors.supports_anchors_zero_fee_htlc_tx() { 2 } else { 0 };
8349-
assert_eq!((htlc_sig.0).0.transaction_output_index, Some($htlc_idx + num_anchors), "output index");
8364+
assert_eq!(htlc.transaction_output_index, Some($htlc_idx + num_anchors), "output index");
83508365

83518366
let signature = Signature::from_der(&hex::decode($htlc_sig_hex).unwrap()[..]).unwrap();
8352-
assert_eq!(signature, *(htlc_sig.1).1, "htlc sig");
8353-
let index = (htlc_sig.1).0;
8354-
let channel_parameters = chan.context.channel_transaction_parameters.as_holder_broadcastable();
8367+
assert_eq!(signature, htlc_holder_sig, "htlc sig");
83558368
let trusted_tx = holder_commitment_tx.trust();
8356-
log_trace!(logger, "htlc_tx = {}", hex::encode(serialize(&trusted_tx.get_signed_htlc_tx(&channel_parameters, index, &(htlc_sig.0).1, (htlc_sig.1).1, &preimage))));
8357-
assert_eq!(serialize(&trusted_tx.get_signed_htlc_tx(&channel_parameters, index, &(htlc_sig.0).1, (htlc_sig.1).1, &preimage))[..],
8358-
hex::decode($htlc_tx_hex).unwrap()[..], "htlc tx");
8369+
htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness($htlc_idx, htlc_counterparty_sig, &htlc_holder_sig, &preimage);
8370+
log_trace!(logger, "htlc_tx = {}", hex::encode(serialize(&htlc_tx)));
8371+
assert_eq!(serialize(&htlc_tx)[..], hex::decode($htlc_tx_hex).unwrap()[..], "htlc tx");
83598372
})*
8360-
assert!(htlc_sig_iter.next().is_none());
8373+
assert!(htlc_counterparty_sig_iter.next().is_none());
83618374
} }
83628375
}
83638376

lightning/src/ln/monitor_tests.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1680,7 +1680,7 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) {
16801680
// secret to the counterparty. However, because we always immediately take the revocation
16811681
// secret from the keys_manager, we would panic at broadcast as we're trying to sign a
16821682
// transaction which, from the point of view of our keys_manager, is revoked.
1683-
chanmon_cfgs[1].keys_manager.disable_revocation_policy_check = true;
1683+
chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true;
16841684
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
16851685
let mut user_config = test_default_channel_config();
16861686
if anchors {

lightning/src/sign/mod.rs

+12-18
Original file line numberDiff line numberDiff line change
@@ -503,15 +503,15 @@ pub trait EcdsaChannelSigner: ChannelSigner {
503503
///
504504
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
505505
// TODO: Document the things someone using this interface should enforce before signing.
506-
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
507-
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
508-
/// Same as [`sign_holder_commitment_and_htlcs`], but exists only for tests to get access to
509-
/// holder commitment transactions which will be broadcasted later, after the channel has moved
510-
/// on to a newer state. Thus, needs its own method as [`sign_holder_commitment_and_htlcs`] may
511-
/// enforce that we only ever get called once.
506+
fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction,
507+
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>;
508+
/// Same as [`sign_holder_commitment`], but exists only for tests to get access to holder
509+
/// commitment transactions which will be broadcasted later, after the channel has moved on to a
510+
/// newer state. Thus, needs its own method as [`sign_holder_commitment`] may enforce that we
511+
/// only ever get called once.
512512
#[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
513-
fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
514-
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
513+
fn unsafe_sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction,
514+
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>;
515515
/// Create a signature for the given input in a transaction spending an HTLC transaction output
516516
/// or a commitment transaction `to_local` output when our counterparty broadcasts an old state.
517517
///
@@ -1118,27 +1118,21 @@ impl EcdsaChannelSigner for InMemorySigner {
11181118
Ok(())
11191119
}
11201120

1121-
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
1121+
fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
11221122
let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
11231123
let counterparty_keys = self.counterparty_pubkeys().expect(MISSING_PARAMS_ERR);
11241124
let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &counterparty_keys.funding_pubkey);
11251125
let trusted_tx = commitment_tx.trust();
1126-
let sig = trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx);
1127-
let channel_parameters = self.get_channel_parameters().expect(MISSING_PARAMS_ERR);
1128-
let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx)?;
1129-
Ok((sig, htlc_sigs))
1126+
Ok(trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx))
11301127
}
11311128

11321129
#[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
1133-
fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
1130+
fn unsafe_sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
11341131
let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
11351132
let counterparty_keys = self.counterparty_pubkeys().expect(MISSING_PARAMS_ERR);
11361133
let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &counterparty_keys.funding_pubkey);
11371134
let trusted_tx = commitment_tx.trust();
1138-
let sig = trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx);
1139-
let channel_parameters = self.get_channel_parameters().expect(MISSING_PARAMS_ERR);
1140-
let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx)?;
1141-
Ok((sig, htlc_sigs))
1135+
Ok(trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx))
11421136
}
11431137

11441138
fn sign_justice_revoked_output(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {

lightning/src/util/test_channel_signer.rs

+30-28
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,8 @@ impl EcdsaChannelSigner for TestChannelSigner {
155155
Ok(())
156156
}
157157

158-
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
158+
fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
159159
let trusted_tx = self.verify_holder_commitment_tx(commitment_tx, secp_ctx);
160-
let commitment_txid = trusted_tx.txid();
161-
let holder_csv = self.inner.counterparty_selected_contest_delay().unwrap();
162-
163160
let state = self.state.lock().unwrap();
164161
let commitment_number = trusted_tx.commitment_number();
165162
if state.last_holder_revoked_commitment - 1 != commitment_number && state.last_holder_revoked_commitment - 2 != commitment_number {
@@ -168,33 +165,12 @@ impl EcdsaChannelSigner for TestChannelSigner {
168165
state.last_holder_revoked_commitment, commitment_number, self.inner.commitment_seed[0])
169166
}
170167
}
171-
172-
for (this_htlc, sig) in trusted_tx.htlcs().iter().zip(&commitment_tx.counterparty_htlc_sigs) {
173-
assert!(this_htlc.transaction_output_index.is_some());
174-
let keys = trusted_tx.keys();
175-
let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, trusted_tx.feerate_per_kw(), holder_csv, &this_htlc, self.channel_type_features(), &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
176-
177-
let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&this_htlc, self.channel_type_features(), &keys);
178-
179-
let sighash_type = if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
180-
EcdsaSighashType::SinglePlusAnyoneCanPay
181-
} else {
182-
EcdsaSighashType::All
183-
};
184-
let sighash = hash_to_message!(
185-
&sighash::SighashCache::new(&htlc_tx).segwit_signature_hash(
186-
0, &htlc_redeemscript, this_htlc.amount_msat / 1000, sighash_type,
187-
).unwrap()[..]
188-
);
189-
secp_ctx.verify_ecdsa(&sighash, sig, &keys.countersignatory_htlc_key).unwrap();
190-
}
191-
192-
Ok(self.inner.sign_holder_commitment_and_htlcs(commitment_tx, secp_ctx).unwrap())
168+
Ok(self.inner.sign_holder_commitment(commitment_tx, secp_ctx).unwrap())
193169
}
194170

195171
#[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
196-
fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()> {
197-
Ok(self.inner.unsafe_sign_holder_commitment_and_htlcs(commitment_tx, secp_ctx).unwrap())
172+
fn unsafe_sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
173+
Ok(self.inner.unsafe_sign_holder_commitment(commitment_tx, secp_ctx).unwrap())
198174
}
199175

200176
fn sign_justice_revoked_output(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
@@ -209,8 +185,34 @@ impl EcdsaChannelSigner for TestChannelSigner {
209185
&self, htlc_tx: &Transaction, input: usize, htlc_descriptor: &HTLCDescriptor,
210186
secp_ctx: &Secp256k1<secp256k1::All>
211187
) -> Result<Signature, ()> {
188+
let state = self.state.lock().unwrap();
189+
if state.last_holder_revoked_commitment - 1 != htlc_descriptor.per_commitment_number &&
190+
state.last_holder_revoked_commitment - 2 != htlc_descriptor.per_commitment_number
191+
{
192+
if !self.disable_revocation_policy_check {
193+
panic!("can only sign the next two unrevoked commitment numbers, revoked={} vs requested={} for {}",
194+
state.last_holder_revoked_commitment, htlc_descriptor.per_commitment_number, self.inner.commitment_seed[0])
195+
}
196+
}
212197
assert_eq!(htlc_tx.input[input], htlc_descriptor.unsigned_tx_input());
213198
assert_eq!(htlc_tx.output[input], htlc_descriptor.tx_output(secp_ctx));
199+
{
200+
let witness_script = htlc_descriptor.witness_script(secp_ctx);
201+
let sighash_type = if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
202+
EcdsaSighashType::SinglePlusAnyoneCanPay
203+
} else {
204+
EcdsaSighashType::All
205+
};
206+
let sighash = &sighash::SighashCache::new(&*htlc_tx).segwit_signature_hash(
207+
input, &witness_script, htlc_descriptor.htlc.amount_msat / 1000, sighash_type
208+
).unwrap();
209+
let countersignatory_htlc_key = chan_utils::derive_public_key(
210+
&secp_ctx, &htlc_descriptor.per_commitment_point, &self.inner.counterparty_pubkeys().unwrap().htlc_basepoint
211+
);
212+
secp_ctx.verify_ecdsa(
213+
&hash_to_message!(&sighash), &htlc_descriptor.counterparty_sig, &countersignatory_htlc_key
214+
).unwrap();
215+
}
214216
Ok(self.inner.sign_holder_htlc_transaction(htlc_tx, input, htlc_descriptor, secp_ctx).unwrap())
215217
}
216218

0 commit comments

Comments
 (0)