From: Matt Corallo Date: Sun, 19 Apr 2020 21:26:41 +0000 (-0400) Subject: Return Result instead of modifying args in ChannelKeys X-Git-Tag: v0.0.12~72^2~4 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=bf74bb625fb92f3a0345bee31fca97487e3aa6e7;p=rust-lightning Return Result instead of modifying args in ChannelKeys This cleans up sign_local_commitment somewhat by returning a Result 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. --- diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index ec76262b1..037748844 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -216,21 +216,20 @@ pub trait ChannelKeys : Send+Clone { /// making the callee generate it via some util function we expose)! fn sign_remote_commitment(&self, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; - /// Create a signature for a local commitment transaction + /// Create a signature for a local commitment transaction. This will only ever be called with + /// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees + /// that it will not be called multiple times. /// /// TODO: Document the things someone using this interface should enforce before signing. /// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and - /// TODO: Ensure test-only version doesn't enforce uniqueness of signature when it's enforced in this method - /// making the callee generate it via some util function we expose)! - fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1); + fn sign_local_commitment(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1) -> Result; - /// Create a signature for a local commitment transaction without enforcing one-time signing. - /// - /// Testing revocation logic by our test framework needs to sign multiple local commitment - /// transactions. This unsafe test-only version doesn't enforce one-time signing security - /// requirement. + /// Same as sign_local_commitment, but exists only for tests to get access to local commitment + /// transactions which will be broadcasted later, after the channel has moved on to a newer + /// state. Thus, needs its own method as sign_local_commitment may enforce that we only ever + /// get called once. #[cfg(test)] - fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1); + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1) -> Result; /// Signs a transaction created by build_htlc_transaction. If the transaction is an /// HTLC-Success transaction, preimage must be set! @@ -363,21 +362,21 @@ impl ChannelKeys for InMemoryChannelKeys { Ok((commitment_sig, htlc_sigs)) } - fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1) { + fn sign_local_commitment(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing"); let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey); - local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx); + Ok(local_commitment_tx.get_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx)) } #[cfg(test)] - fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1) { + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); let remote_channel_pubkeys = self.remote_channel_pubkeys.as_ref().expect("must set remote channel pubkeys before signing"); let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &remote_channel_pubkeys.funding_pubkey); - local_commitment_tx.add_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx); + Ok(local_commitment_tx.get_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx)) } fn sign_htlc_transaction(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1) { diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 43f567933..b899fe494 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -555,7 +555,7 @@ impl LocalCommitmentTransaction { } /// Check if LocalCommitmentTransaction has already been signed by us - pub fn has_local_sig(&self) -> bool { + pub(crate) fn has_local_sig(&self) -> bool { if self.tx.input.len() != 1 { panic!("Commitment transactions must have input count == 1!"); } if self.tx.input[0].witness.len() == 4 { assert!(!self.tx.input[0].witness[1].is_empty()); @@ -569,8 +569,7 @@ impl LocalCommitmentTransaction { } } - /// Add local signature for LocalCommitmentTransaction, do nothing if signature is already - /// present + /// Gets our signature for the contained commitment transaction given our funding private key. /// /// Funding key is your key included in the 2-2 funding_outpoint lock. Should be provided /// by your ChannelKeys. @@ -578,11 +577,15 @@ impl LocalCommitmentTransaction { /// between your own funding key and your counterparty's. Currently, this is provided in /// ChannelKeys::sign_local_commitment() calls directly. /// Channel value is amount locked in funding_outpoint. - pub fn add_local_sig(&mut self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) { - if self.has_local_sig() { return; } + pub fn get_local_sig(&self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1) -> Signature { let sighash = hash_to_message!(&bip143::SighashComponents::new(&self.tx) .sighash_all(&self.tx.input[0], funding_redeemscript, channel_value_satoshis)[..]); - let our_sig = secp_ctx.sign(&sighash, funding_key); + secp_ctx.sign(&sighash, funding_key) + } + + + pub(crate) fn add_local_sig(&mut self, funding_redeemscript: &Script, our_sig: Signature) { + if self.has_local_sig() { return; } if self.tx.input[0].witness[1].is_empty() { self.tx.input[0].witness[1] = our_sig.serialize_der().to_vec(); @@ -598,7 +601,7 @@ impl LocalCommitmentTransaction { /// Get raw transaction without asserting if witness is complete pub(crate) fn without_valid_witness(&self) -> &Transaction { &self.tx } /// Get raw transaction with panics if witness is incomplete - pub fn with_valid_witness(&self) -> &Transaction { + pub(crate) fn with_valid_witness(&self) -> &Transaction { assert!(self.has_local_sig()); &self.tx } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 35f10ef2a..9ea859ecb 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4517,7 +4517,8 @@ mod tests { assert_eq!(unsigned_tx.1.len(), per_htlc.len()); 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); - chan_keys.sign_local_commitment(&mut localtx, &chan.secp_ctx); + let local_sig = chan_keys.sign_local_commitment(&localtx, &chan.secp_ctx).unwrap(); + localtx.add_local_sig(&redeemscript, local_sig); assert_eq!(serialize(localtx.with_valid_witness())[..], hex::decode($tx_hex).unwrap()[..]); diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 5241d97d4..4987f8f3b 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -442,7 +442,9 @@ pub(crate) enum InputMaterial { preimage: Option, amount: u64, }, - Funding {} + Funding { + funding_redeemscript: Script, + } } impl Writeable for InputMaterial { @@ -469,8 +471,9 @@ impl Writeable for InputMaterial { preimage.write(writer)?; writer.write_all(&byte_utils::be64_to_array(*amount))?; }, - &InputMaterial::Funding {} => { + &InputMaterial::Funding { ref funding_redeemscript } => { writer.write_all(&[3; 1])?; + funding_redeemscript.write(writer)?; } } Ok(()) @@ -517,7 +520,9 @@ impl Readable for InputMaterial { } }, 3 => { - InputMaterial::Funding {} + InputMaterial::Funding { + funding_redeemscript: Readable::read(reader)?, + } } _ => return Err(DecodeError::InvalidValue), }; @@ -1771,7 +1776,7 @@ impl ChannelMonitor { pub fn get_latest_local_commitment_txn(&mut self) -> Vec { log_trace!(self, "Getting signed latest local commitment transaction!"); self.local_tx_signed = true; - if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx() { + if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(&self.funding_redeemscript) { let txid = commitment_tx.txid(); let mut res = vec![commitment_tx]; for htlc in self.current_local_commitment_tx.htlc_outputs.iter() { @@ -1795,7 +1800,7 @@ impl ChannelMonitor { #[cfg(test)] pub fn unsafe_get_latest_local_commitment_txn(&mut self) -> Vec { log_trace!(self, "Getting signed copy of latest local commitment transaction!"); - if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_copy_local_tx() { + if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_copy_local_tx(&self.funding_redeemscript) { let txid = commitment_tx.txid(); let mut res = vec![commitment_tx]; for htlc in self.current_local_commitment_tx.htlc_outputs.iter() { @@ -1873,10 +1878,10 @@ impl ChannelMonitor { } let should_broadcast = self.would_broadcast_at_height(height); if should_broadcast { - 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 {}}); + 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 { funding_redeemscript: self.funding_redeemscript.clone() }}); } if should_broadcast { - if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx() { + if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(&self.funding_redeemscript) { let (mut new_outpoints, new_outputs, _) = self.broadcast_by_local_state(&commitment_tx, &self.current_local_commitment_tx); if !new_outputs.is_empty() { watch_outputs.push((self.current_local_commitment_tx.txid.clone(), new_outputs)); diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 186793581..77ce1bdc4 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -531,8 +531,8 @@ impl OnchainTxHandler { } return None; }, - &InputMaterial::Funding {} => { - let signed_tx = self.get_fully_signed_local_tx().unwrap(); + &InputMaterial::Funding { ref funding_redeemscript } => { + let signed_tx = self.get_fully_signed_local_tx(funding_redeemscript).unwrap(); // Timer set to $NEVER given we can't bump tx without anchor outputs log_trace!(self, "Going to broadcast Local Transaction {} claiming funding output {} from {}...", signed_tx.txid(), outp.vout, outp.txid); return Some((None, self.local_commitment.as_ref().unwrap().feerate_per_kw, signed_tx)); @@ -780,19 +780,25 @@ impl OnchainTxHandler { // have empty local commitment transaction if a ChannelMonitor is asked to force-close just after Channel::get_outbound_funding_created, // before providing a initial commitment transaction. For outbound channel, init ChannelMonitor at Channel::funding_signed, there is nothing // to monitor before. - pub(super) fn get_fully_signed_local_tx(&mut self) -> Option { + pub(super) fn get_fully_signed_local_tx(&mut self, funding_redeemscript: &Script) -> Option { if let Some(ref mut local_commitment) = self.local_commitment { - self.key_storage.sign_local_commitment(local_commitment, &self.secp_ctx); + match self.key_storage.sign_local_commitment(local_commitment, &self.secp_ctx) { + Ok(sig) => local_commitment.add_local_sig(funding_redeemscript, sig), + Err(_) => return None, + } return Some(local_commitment.with_valid_witness().clone()); } None } #[cfg(test)] - pub(super) fn get_fully_signed_copy_local_tx(&mut self) -> Option { + pub(super) fn get_fully_signed_copy_local_tx(&mut self, funding_redeemscript: &Script) -> Option { if let Some(ref mut local_commitment) = self.local_commitment { let mut local_commitment = local_commitment.clone(); - self.key_storage.unsafe_sign_local_commitment(&mut local_commitment, &self.secp_ctx); + match self.key_storage.sign_local_commitment(&local_commitment, &self.secp_ctx) { + Ok(sig) => local_commitment.add_local_sig(funding_redeemscript, sig), + Err(_) => return None, + } return Some(local_commitment.with_valid_witness().clone()); } None diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index 1e5dc707c..989a16c37 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -79,13 +79,13 @@ impl ChannelKeys for EnforcingChannelKeys { Ok(self.inner.sign_remote_commitment(feerate_per_kw, commitment_tx, keys, htlcs, to_self_delay, secp_ctx).unwrap()) } - fn sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1) { + fn sign_local_commitment(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { self.inner.sign_local_commitment(local_commitment_tx, secp_ctx) } #[cfg(test)] - fn unsafe_sign_local_commitment(&self, local_commitment_tx: &mut LocalCommitmentTransaction, secp_ctx: &Secp256k1) { - self.inner.unsafe_sign_local_commitment(local_commitment_tx, secp_ctx); + fn unsafe_sign_local_commitment(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { + self.inner.unsafe_sign_local_commitment(local_commitment_tx, secp_ctx) } fn sign_htlc_transaction(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1) {