Skip to content

Commit 2c09663

Browse files
committed
Return Result<Signature> instead of modifying args in ChannelKeys
This cleans up sign_local_commitment somewhat by returning a Result<Signaure, ()> over the local commitment transaction instead of modifying the struct which was passed in. This is the first step in making LocalCommitmentTransaction a completely pub struct, using it just to communicate enough information to the user to allow them to construct a signaure instead of having it contain a bunch of logic. This should make it much easier to implement a custom ChannelKeys by disconnecting the local commitment transaction signing from our own datastructures.
1 parent 7e11701 commit 2c09663

File tree

6 files changed

+49
-38
lines changed

6 files changed

+49
-38
lines changed

lightning/src/chain/keysinterface.rs

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -216,21 +216,20 @@ pub trait ChannelKeys : Send+Clone {
216216
/// making the callee generate it via some util function we expose)!
217217
fn sign_remote_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()>;
218218

219-
/// Create a signature for a local commitment transaction
219+
/// Create a signature for a local commitment transaction. This will only ever be called with
220+
/// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees
221+
/// that it will not be called multiple times.
220222
///
221223
/// TODO: Document the things someone using this interface should enforce before signing.
222224
/// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
223-
/// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method
224-
/// making the callee generate it via some util function we expose)!
225-
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
225+
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;
226226

227-
/// Create a signature for a local commitment transaction without enforcing one-time signing.
228-
///
229-
/// Testing revocation logic by our test framework needs to sign multiple local commitment
230-
/// transactions. This unsafe test-only version doesn't enforce one-time signing security
231-
/// requirement.
227+
/// Same as sign_local_commitment, but exists only for tests to get access to local commitment
228+
/// transactions which will be broadcasted later, after the channel has moved on to a newer
229+
/// state. Thus, needs its own method as sign_local_commitment may enforce that we only ever
230+
/// get called once.
232231
#[cfg(test)]
233-
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>);
232+
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;
234233

235234
/// Signs a transaction created by build_htlc_transaction. If the transaction is an
236235
/// HTLC-Success transaction, preimage must be set!
@@ -363,21 +362,21 @@ impl ChannelKeys for InMemoryChannelKeys {
363362
Ok((commitment_sig, htlc_sigs))
364363
}
365364

366-
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
365+
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
367366
let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
368367
let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
369368
let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
370369

371-
local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
370+
Ok(local_commitment_tx.get_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx))
372371
}
373372

374373
#[cfg(test)]
375-
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
374+
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
376375
let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
377376
let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing");
378377
let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey);
379378

380-
local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx);
379+
Ok(local_commitment_tx.get_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx))
381380
}
382381

383382
fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {

lightning/src/ln/chan_utils.rs

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -555,7 +555,7 @@ impl LocalCommitmentTransaction {
555555
}
556556

557557
/// Check if LocalCommitmentTransaction has already been signed by us
558-
pub fn has_local_sig(&self) -> bool {
558+
pub(crate) fn has_local_sig(&self) -> bool {
559559
if self.tx.input.len() != 1 { panic!("Commitment transactions must have input count == 1!"); }
560560
if self.tx.input[0].witness.len() == 4 {
561561
assert!(!self.tx.input[0].witness[1].is_empty());
@@ -569,20 +569,23 @@ impl LocalCommitmentTransaction {
569569
}
570570
}
571571

572-
/// Add local signature for LocalCommitmentTransaction, do nothing if signature is already
573-
/// present
572+
/// Gets a local signature for the contained commitment transaction.
574573
///
575574
/// Funding key is your key included in the 2-2 funding_outpoint lock. Should be provided
576575
/// by your ChannelKeys.
577576
/// Funding redeemscript is script locking funding_outpoint. This is the mutlsig script
578577
/// between your own funding key and your counterparty's. Currently, this is provided in
579578
/// ChannelKeys::sign_local_commitment() calls directly.
580579
/// Channel value is amount locked in funding_outpoint.
581-
pub fn add_local_sig<T: secp256k1::Signing>(&mut self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) {
582-
if self.has_local_sig() { return; }
580+
pub fn get_local_sig<T: secp256k1::Signing>(&self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) -> Signature {
583581
let sighash = hash_to_message!(&bip143::SighashComponents::new(&self.tx)
584582
.sighash_all(&self.tx.input[0], funding_redeemscript, channel_value_satoshis)[..]);
585-
let our_sig = secp_ctx.sign(&sighash, funding_key);
583+
secp_ctx.sign(&sighash, funding_key)
584+
}
585+
586+
587+
pub(crate) fn add_local_sig(&mut self, funding_redeemscript: &Script, our_sig: Signature) {
588+
if self.has_local_sig() { return; }
586589

587590
if self.tx.input[0].witness[1].is_empty() {
588591
self.tx.input[0].witness[1] = our_sig.serialize_der().to_vec();
@@ -598,7 +601,7 @@ impl LocalCommitmentTransaction {
598601
/// Get raw transaction without asserting if witness is complete
599602
pub(crate) fn without_valid_witness(&self) -> &Transaction { &self.tx }
600603
/// Get raw transaction with panics if witness is incomplete
601-
pub fn with_valid_witness(&self) -> &Transaction {
604+
pub(crate) fn with_valid_witness(&self) -> &Transaction {
602605
assert!(self.has_local_sig());
603606
&self.tx
604607
}

lightning/src/ln/channel.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4501,7 +4501,8 @@ mod tests {
45014501
assert_eq!(unsigned_tx.1.len(), per_htlc.len());
45024502

45034503
localtx = LocalCommitmentTransaction::new_missing_local_sig(unsigned_tx.0.clone(), &their_signature, &PublicKey::from_secret_key(&secp_ctx, chan.local_keys.funding_key()), chan.their_funding_pubkey(), keys.clone(), chan.feerate_per_kw, per_htlc);
4504-
chan_keys.sign_local_commitment(&mut localtx, &chan.secp_ctx);
4504+
let local_sig = chan_keys.sign_local_commitment(&localtx, &chan.secp_ctx).unwrap();
4505+
localtx.add_local_sig(&redeemscript, local_sig);
45054506

45064507
assert_eq!(serialize(localtx.with_valid_witness())[..],
45074508
hex::decode($tx_hex).unwrap()[..]);

lightning/src/ln/channelmonitor.rs

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,7 @@ pub(crate) enum InputMaterial {
422422
},
423423
Funding {
424424
channel_value: u64,
425+
funding_redeemscript: Script,
425426
}
426427
}
427428

@@ -449,9 +450,10 @@ impl Writeable for InputMaterial {
449450
preimage.write(writer)?;
450451
writer.write_all(&byte_utils::be64_to_array(*amount))?;
451452
},
452-
&InputMaterial::Funding { ref channel_value } => {
453+
&InputMaterial::Funding { ref channel_value, ref funding_redeemscript } => {
453454
writer.write_all(&[3; 1])?;
454455
channel_value.write(writer)?;
456+
funding_redeemscript.write(writer)?;
455457
}
456458
}
457459
Ok(())
@@ -498,9 +500,9 @@ impl Readable for InputMaterial {
498500
}
499501
},
500502
3 => {
501-
let channel_value = Readable::read(reader)?;
502503
InputMaterial::Funding {
503-
channel_value
504+
channel_value: Readable::read(reader)?,
505+
funding_redeemscript: Readable::read(reader)?,
504506
}
505507
}
506508
_ => return Err(DecodeError::InvalidValue),
@@ -1755,7 +1757,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
17551757
pub fn get_latest_local_commitment_txn(&mut self) -> Vec<Transaction> {
17561758
log_trace!(self, "Getting signed latest local commitment transaction!");
17571759
self.local_tx_signed = true;
1758-
if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx() {
1760+
if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(&self.funding_redeemscript) {
17591761
let txid = commitment_tx.txid();
17601762
let mut res = vec![commitment_tx];
17611763
for htlc in self.current_local_commitment_tx.htlc_outputs.iter() {
@@ -1779,7 +1781,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
17791781
#[cfg(test)]
17801782
pub fn unsafe_get_latest_local_commitment_txn(&mut self) -> Vec<Transaction> {
17811783
log_trace!(self, "Getting signed copy of latest local commitment transaction!");
1782-
if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_copy_local_tx() {
1784+
if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_copy_local_tx(&self.funding_redeemscript) {
17831785
let txid = commitment_tx.txid();
17841786
let mut res = vec![commitment_tx];
17851787
for htlc in self.current_local_commitment_tx.htlc_outputs.iter() {
@@ -1857,10 +1859,10 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
18571859
}
18581860
let should_broadcast = self.would_broadcast_at_height(height);
18591861
if should_broadcast {
1860-
claimable_outpoints.push(ClaimRequest { absolute_timelock: height, aggregable: false, outpoint: BitcoinOutPoint { txid: self.funding_info.0.txid.clone(), vout: self.funding_info.0.index as u32 }, witness_data: InputMaterial::Funding { channel_value: self.channel_value_satoshis }});
1862+
claimable_outpoints.push(ClaimRequest { absolute_timelock: height, aggregable: false, outpoint: BitcoinOutPoint { txid: self.funding_info.0.txid.clone(), vout: self.funding_info.0.index as u32 }, witness_data: InputMaterial::Funding { channel_value: self.channel_value_satoshis, funding_redeemscript: self.funding_redeemscript.clone() }});
18611863
}
18621864
if should_broadcast {
1863-
if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx() {
1865+
if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(&self.funding_redeemscript) {
18641866
let (mut new_outpoints, new_outputs, _) = self.broadcast_by_local_state(&commitment_tx, &self.current_local_commitment_tx);
18651867
if !new_outputs.is_empty() {
18661868
watch_outputs.push((self.current_local_commitment_tx.txid.clone(), new_outputs));

lightning/src/ln/onchaintx.rs

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -531,8 +531,8 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
531531
}
532532
return None;
533533
},
534-
&InputMaterial::Funding { ref channel_value } => {
535-
let signed_tx = self.get_fully_signed_local_tx().unwrap();
534+
&InputMaterial::Funding { ref channel_value, ref funding_redeemscript } => {
535+
let signed_tx = self.get_fully_signed_local_tx(funding_redeemscript).unwrap();
536536
let mut amt_outputs = 0;
537537
for outp in signed_tx.output.iter() {
538538
amt_outputs += outp.value;
@@ -785,19 +785,25 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
785785
// have empty local commitment transaction if a ChannelMonitor is asked to force-close just after Channel::get_outbound_funding_created,
786786
// before providing a initial commitment transaction. For outbound channel, init ChannelMonitor at Channel::funding_signed, there is nothing
787787
// to monitor before.
788-
pub(super) fn get_fully_signed_local_tx(&mut self) -> Option<Transaction> {
788+
pub(super) fn get_fully_signed_local_tx(&mut self, funding_redeemscript: &Script) -> Option<Transaction> {
789789
if let Some(ref mut local_commitment) = self.local_commitment {
790-
self.key_storage.sign_local_commitment(local_commitment, &self.secp_ctx);
790+
match self.key_storage.sign_local_commitment(local_commitment, &self.secp_ctx) {
791+
Ok(sig) => local_commitment.add_local_sig(funding_redeemscript, sig),
792+
Err(_) => return None,
793+
}
791794
return Some(local_commitment.with_valid_witness().clone());
792795
}
793796
None
794797
}
795798

796799
#[cfg(test)]
797-
pub(super) fn get_fully_signed_copy_local_tx(&mut self) -> Option<Transaction> {
800+
pub(super) fn get_fully_signed_copy_local_tx(&mut self, funding_redeemscript: &Script) -> Option<Transaction> {
798801
if let Some(ref mut local_commitment) = self.local_commitment {
799802
let mut local_commitment = local_commitment.clone();
800-
self.key_storage.unsafe_sign_local_commitment(&mut local_commitment, &self.secp_ctx);
803+
match self.key_storage.sign_local_commitment(&local_commitment, &self.secp_ctx) {
804+
Ok(sig) => local_commitment.add_local_sig(funding_redeemscript, sig),
805+
Err(_) => return None,
806+
}
801807
return Some(local_commitment.with_valid_witness().clone());
802808
}
803809
None

lightning/src/util/enforcing_trait_impls.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,13 +79,13 @@ impl ChannelKeys for EnforcingChannelKeys {
7979
Ok(self.inner.sign_remote_commitment(feerate_per_kw, commitment_tx, keys, htlcs, to_self_delay, secp_ctx).unwrap())
8080
}
8181

82-
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
82+
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
8383
self.inner.sign_local_commitment(local_commitment_tx, secp_ctx)
8484
}
8585

8686
#[cfg(test)]
87-
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) {
88-
self.inner.unsafe_sign_local_commitment(local_commitment_tx, secp_ctx);
87+
fn unsafe_sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
88+
self.inner.unsafe_sign_local_commitment(local_commitment_tx, secp_ctx)
8989
}
9090

9191
fn sign_htlc_transaction<T: secp256k1::Signing>(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option<PaymentPreimage>, local_csv: u16, secp_ctx: &Secp256k1<T>) {

0 commit comments

Comments
 (0)