From 359b3d5702b585a67e4e6d4044c35caf86bd787d Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Mon, 20 Apr 2020 21:19:00 -0400 Subject: [PATCH 1/8] Dry-up InputMaterial::Funding As channel_value last usage was for computing feerate but as this one is static per-commitment and will always-be following specification, we remove it. --- lightning/src/ln/channelmonitor.rs | 14 ++++---------- lightning/src/ln/onchaintx.rs | 9 ++------- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index c8e6b8260ad..da8ba03a727 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -442,9 +442,7 @@ pub(crate) enum InputMaterial { preimage: Option, amount: u64, }, - Funding { - channel_value: u64, - } + Funding {} } impl Writeable for InputMaterial { @@ -471,9 +469,8 @@ impl Writeable for InputMaterial { preimage.write(writer)?; writer.write_all(&byte_utils::be64_to_array(*amount))?; }, - &InputMaterial::Funding { ref channel_value } => { + &InputMaterial::Funding {} => { writer.write_all(&[3; 1])?; - channel_value.write(writer)?; } } Ok(()) @@ -520,10 +517,7 @@ impl Readable for InputMaterial { } }, 3 => { - let channel_value = Readable::read(reader)?; - InputMaterial::Funding { - channel_value - } + InputMaterial::Funding {} } _ => return Err(DecodeError::InvalidValue), }; @@ -1864,7 +1858,7 @@ 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 { channel_value: self.channel_value_satoshis }}); + 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 {}}); } if should_broadcast { if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx() { diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index cb76dcb2a55..186793581ce 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -531,16 +531,11 @@ impl OnchainTxHandler { } return None; }, - &InputMaterial::Funding { ref channel_value } => { + &InputMaterial::Funding {} => { let signed_tx = self.get_fully_signed_local_tx().unwrap(); - let mut amt_outputs = 0; - for outp in signed_tx.output.iter() { - amt_outputs += outp.value; - } - let feerate = (channel_value - amt_outputs) * 1000 / signed_tx.get_weight() as u64; // 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, feerate, signed_tx)); + return Some((None, self.local_commitment.as_ref().unwrap().feerate_per_kw, signed_tx)); } _ => unreachable!() } From ddd85fb55023ac7fea1c1a7ae4748b2b795dff61 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 19 Apr 2020 14:15:56 -0400 Subject: [PATCH 2/8] Track signing of local txn in channelmonitor and refuse updates In e46e183084ed858f41aa304acd78503aea1a96ed we began tracking whether a local commitment transaction had been signed and broadcast in OnchainTxHandler, refusing to update the local commitment transaction state in the ChannelMonitor on that basis. This is fine, except that it doesn't make a lot of sense to store the full local transaction state in OnchainTxHandler - we should be providing it the unsigned local transaction at the time we wish to broadcast and no more (just like we do all other transaction data). --- lightning/src/ln/channelmonitor.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index da8ba03a727..5241d97d4fe 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -784,9 +784,16 @@ pub struct ChannelMonitor { #[cfg(not(test))] onchain_tx_handler: OnchainTxHandler, - // Used to detect programming bug due to unsafe monitor update sequence { ChannelForceClosed, LatestLocalCommitmentTXInfo } + // This is set when the Channel[Manager] generated a ChannelMonitorUpdate which indicated the + // channel has been force-closed. After this is set, no further local commitment transaction + // updates may occur, and we panic!() if one is provided. lockdown_from_offchain: bool, + // Set once we've signed a local commitment transaction and handed it over to our + // OnchainTxHandler. After this is set, no future updates to our local commitment transactions + // may occur, and we fail any such monitor updates. + local_tx_signed: bool, + // We simply modify last_block_hash in Channel's block_connected so that serialization is // consistent but hopefully the users' copy handles block_connected in a consistent way. // (we do *not*, however, update them in update_monitor to ensure any local user copies keep @@ -830,7 +837,9 @@ impl PartialEq for ChannelMonitor { self.pending_htlcs_updated != other.pending_htlcs_updated || self.pending_events.len() != other.pending_events.len() || // We trust events to round-trip properly self.onchain_events_waiting_threshold_conf != other.onchain_events_waiting_threshold_conf || - self.outputs_to_watch != other.outputs_to_watch + self.outputs_to_watch != other.outputs_to_watch || + self.lockdown_from_offchain != other.lockdown_from_offchain || + self.local_tx_signed != other.local_tx_signed { false } else { @@ -1031,6 +1040,7 @@ impl ChannelMonitor { self.onchain_tx_handler.write(writer)?; self.lockdown_from_offchain.write(writer)?; + self.local_tx_signed.write(writer)?; Ok(()) } @@ -1113,6 +1123,7 @@ impl ChannelMonitor { onchain_tx_handler, lockdown_from_offchain: false, + local_tx_signed: false, last_block_hash: Default::default(), secp_ctx: Secp256k1::new(), @@ -1229,6 +1240,9 @@ impl ChannelMonitor { /// up-to-date as our local commitment transaction is updated. /// Panics if set_their_to_self_delay has never been called. pub(super) fn provide_latest_local_commitment_tx_info(&mut self, commitment_tx: LocalCommitmentTransaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>) -> Result<(), MonitorUpdateError> { + if self.local_tx_signed { + return Err(MonitorUpdateError("A local commitment tx has already been signed, no new local commitment txn can be sent to our counterparty")); + } let txid = commitment_tx.txid(); let sequence = commitment_tx.without_valid_witness().input[0].sequence as u64; let locktime = commitment_tx.without_valid_witness().lock_time as u64; @@ -1756,6 +1770,7 @@ impl ChannelMonitor { /// In any-case, choice is up to the user. 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() { let txid = commitment_tx.txid(); let mut res = vec![commitment_tx]; @@ -2415,6 +2430,7 @@ impl ReadableArgs> for (Sha256dH let onchain_tx_handler = ReadableArgs::read(reader, logger.clone())?; let lockdown_from_offchain = Readable::read(reader)?; + let local_tx_signed = Readable::read(reader)?; Ok((last_block_hash.clone(), ChannelMonitor { latest_update_id, @@ -2459,6 +2475,7 @@ impl ReadableArgs> for (Sha256dH onchain_tx_handler, lockdown_from_offchain, + local_tx_signed, last_block_hash, secp_ctx: Secp256k1::new(), From bf74bb625fb92f3a0345bee31fca97487e3aa6e7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 19 Apr 2020 17:26:41 -0400 Subject: [PATCH 3/8] 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. --- lightning/src/chain/keysinterface.rs | 27 ++++++++++----------- lightning/src/ln/chan_utils.rs | 17 +++++++------ lightning/src/ln/channel.rs | 3 ++- lightning/src/ln/channelmonitor.rs | 19 +++++++++------ lightning/src/ln/onchaintx.rs | 18 +++++++++----- lightning/src/util/enforcing_trait_impls.rs | 6 ++--- 6 files changed, 52 insertions(+), 38 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index ec76262b15a..03774884400 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 43f567933fd..b899fe49407 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 35f10ef2a91..9ea859ecbba 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 5241d97d4fe..4987f8f3b38 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 186793581ce..77ce1bdc4be 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 1e5dc707c44..989a16c3753 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) { From 7159d1546ae92281a7e0533813a6e7558f16354a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 19 Apr 2020 22:59:53 -0400 Subject: [PATCH 4/8] Batch-sign local HTLC txn with a well-doc'd API, returning sigs 1107ab06c33bd360bdee7ee64f4b690e753003f6 introduced an API to have a ChannelKeys implementer sign HTLC transactions by calling into the LocalCommitmentTransaction object, which would then store the tx. This API was incredibly awkward, both because it required an external signer trust our own internal interfaces, but also because it didn't allow for any inspection of what was about to be signed. Further, it signed the HTLC transactions one-by-one in a somewhat inefficient way, and there isn't a clear way to resolve this (as the which-HTLC parameter has to refer to something in between the HTLC's arbitrary index, and its index in the commitment tx, which has "holes" for the non-HTLC outputs and skips some HTLCs). We replace it with a new function in ChannelKeys which allows us to sign all HTLCs in a given commitment transaction (which allows for a bit more effeciency on the signers' part, as well as sidesteps the which-HTLC issue). This may also simplify the signer implementation as we will always want to sign all HTLCs spending a given commitment transaction at once anyway. We also de-mut the LocalCommitmentTransaction passed to the ChanKeys, instead opting to make LocalCommitmentTransaction const and avoid storing any new HTLC-related data in it. --- lightning/src/chain/keysinterface.rs | 24 ++- lightning/src/ln/chan_utils.rs | 112 +++++++------- lightning/src/ln/channel.rs | 18 ++- lightning/src/ln/channelmonitor.rs | 38 ++++- lightning/src/ln/onchaintx.rs | 154 +++++++++++++++++--- lightning/src/util/enforcing_trait_impls.rs | 25 +++- lightning/src/util/mod.rs | 6 +- 7 files changed, 278 insertions(+), 99 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 03774884400..44f347abc12 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -25,7 +25,6 @@ use util::ser::{Writeable, Writer, Readable}; use ln::chan_utils; use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, LocalCommitmentTransaction}; -use ln::channelmanager::PaymentPreimage; use ln::msgs; use std::sync::Arc; @@ -231,10 +230,21 @@ pub trait ChannelKeys : Send+Clone { #[cfg(test)] 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! - /// TODO: should be merged with sign_local_commitment as a slice of HTLC transactions to sign - fn sign_htlc_transaction(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1); + /// Create a signature for each HTLC transaction spending a local commitment transaction. + /// + /// Unlike sign_local_commitment, this may be called multiple times with *different* + /// local_commitment_tx values. While this will never be called with a revoked + /// local_commitment_tx, it is possible that it is called with the second-latest + /// local_commitment_tx (only if we haven't yet revoked it) if some watchtower/secondary + /// ChannelMonitor decided to broadcast before it had been updated to the latest. + /// + /// Either an Err should be returned, or a Vec with one entry for each HTLC which exists in + /// local_commitment_tx. For those HTLCs which have transaction_output_index set to None + /// (implying they were considered dust at the time the commitment transaction was negotiated), + /// a corresponding None should be included in the return value. All other positions in the + /// return value must contain a signature. + fn sign_local_commitment_htlc_transactions(&self, local_commitment_tx: &LocalCommitmentTransaction, local_csv: u16, secp_ctx: &Secp256k1) -> Result>, ()>; + /// Create a signature for a (proposed) closing transaction. /// /// Note that, due to rounding, there may be one "missing" satoshi, and either party may have @@ -379,8 +389,8 @@ impl ChannelKeys for InMemoryChannelKeys { 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) { - local_commitment_tx.add_htlc_sig(&self.htlc_base_key, htlc_index, preimage, local_csv, secp_ctx); + fn sign_local_commitment_htlc_transactions(&self, local_commitment_tx: &LocalCommitmentTransaction, local_csv: u16, secp_ctx: &Secp256k1) -> Result>, ()> { + local_commitment_tx.get_htlc_sigs(&self.htlc_base_key, local_csv, secp_ctx) } fn sign_closing_transaction(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1) -> Result { diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index b899fe49407..f6cbdc23b54 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -487,7 +487,7 @@ pub struct LocalCommitmentTransaction { tx: Transaction, pub(crate) local_keys: TxCreationKeys, pub(crate) feerate_per_kw: u64, - per_htlc: Vec<(HTLCOutputInCommitment, Option, Option)> + pub(crate) per_htlc: Vec<(HTLCOutputInCommitment, Option)>, } impl LocalCommitmentTransaction { #[cfg(test)] @@ -524,7 +524,7 @@ impl LocalCommitmentTransaction { /// Generate a new LocalCommitmentTransaction based on a raw commitment transaction, /// remote signature and both parties keys - pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, mut htlc_data: Vec<(HTLCOutputInCommitment, Option)>) -> LocalCommitmentTransaction { + pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_data: Vec<(HTLCOutputInCommitment, Option)>) -> LocalCommitmentTransaction { if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); } if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); } @@ -543,8 +543,7 @@ impl LocalCommitmentTransaction { Self { tx, local_keys, feerate_per_kw, - // TODO: Avoid the conversion of a Vec created likely just for this: - per_htlc: htlc_data.drain(..).map(|(a, b)| (a, b, None)).collect(), + per_htlc: htlc_data, } } @@ -606,55 +605,70 @@ impl LocalCommitmentTransaction { &self.tx } - /// Add local signature for a htlc transaction, do nothing if a cached signed transaction is - /// already present - pub fn add_htlc_sig(&mut self, htlc_base_key: &SecretKey, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1) { + /// Get a signature for each HTLC which was included in the commitment transaction (ie for + /// which HTLCOutputInCommitment::transaction_output_index.is_some()). + /// + /// The returned Vec has one entry for each HTLC, and in the same order. For HTLCs which were + /// considered dust and not included, a None entry exists, for all others a signature is + /// included. + pub fn get_htlc_sigs(&self, htlc_base_key: &SecretKey, local_csv: u16, secp_ctx: &Secp256k1) -> Result>, ()> { let txid = self.txid(); - for this_htlc in self.per_htlc.iter_mut() { - if this_htlc.0.transaction_output_index == Some(htlc_index) { - if this_htlc.2.is_some() { return; } // we already have a cached htlc transaction at provided index - let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key); - if !this_htlc.0.offered && preimage.is_none() { return; } // if we don't have preimage for HTLC-Success, don't try to generate - let htlc_secret = if !this_htlc.0.offered { preimage } else { None }; // if we have a preimage for HTLC-Timeout, don't use it that's likely a duplicate HTLC hash - if this_htlc.1.is_none() { return; } // we don't have any remote signature for this htlc - if htlc_tx.input.len() != 1 { return; } - if htlc_tx.input[0].witness.len() != 0 { return; } - - let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key); - - if let Ok(our_htlc_key) = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key) { - let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]); - let our_sig = secp_ctx.sign(&sighash, &our_htlc_key); + let mut ret = Vec::with_capacity(self.per_htlc.len()); + let our_htlc_key = derive_private_key(secp_ctx, &self.local_keys.per_commitment_point, htlc_base_key).map_err(|_| ())?; - htlc_tx.input[0].witness.push(Vec::new()); // First is the multisig dummy - - htlc_tx.input[0].witness.push(this_htlc.1.unwrap().serialize_der().to_vec()); - htlc_tx.input[0].witness.push(our_sig.serialize_der().to_vec()); - htlc_tx.input[0].witness[1].push(SigHashType::All as u8); - htlc_tx.input[0].witness[2].push(SigHashType::All as u8); - - if this_htlc.0.offered { - htlc_tx.input[0].witness.push(Vec::new()); - assert!(htlc_secret.is_none()); - } else { - htlc_tx.input[0].witness.push(htlc_secret.unwrap().0.to_vec()); - } + for this_htlc in self.per_htlc.iter() { + if this_htlc.0.transaction_output_index.is_some() { + let htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key); + assert_eq!(htlc_tx.input.len(), 1); + assert_eq!(htlc_tx.input[0].witness.len(), 0); - htlc_tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec()); + let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key); - this_htlc.2 = Some(htlc_tx); - } else { return; } + let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]); + ret.push(Some(secp_ctx.sign(&sighash, &our_htlc_key))); + } else { + ret.push(None); } } + Ok(ret) } - /// Expose raw htlc transaction, guarante witness is complete if non-empty - pub fn htlc_with_valid_witness(&self, htlc_index: u32) -> &Option { - for this_htlc in self.per_htlc.iter() { - if this_htlc.0.transaction_output_index.unwrap() == htlc_index { - return &this_htlc.2; - } + + /// Gets a signed HTLC transaction given a preimage (for !htlc.offered) and the local HTLC transaction signature. + pub(crate) fn get_signed_htlc_tx(&self, htlc_index: usize, signature: &Signature, preimage: &Option, local_csv: u16) -> Transaction { + let txid = self.txid(); + let this_htlc = &self.per_htlc[htlc_index]; + assert!(this_htlc.0.transaction_output_index.is_some()); + // if we don't have preimage for an HTLC-Success, we can't generate an HTLC transaction. + if !this_htlc.0.offered && preimage.is_none() { unreachable!(); } + // Further, we should never be provided the preimage for an HTLC-Timeout transaction. + if this_htlc.0.offered && preimage.is_some() { unreachable!(); } + + let mut htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key); + // Channel should have checked that we have a remote signature for this HTLC at + // creation, and we should have a sensible htlc transaction: + assert!(this_htlc.1.is_some()); + assert_eq!(htlc_tx.input.len(), 1); + assert_eq!(htlc_tx.input[0].witness.len(), 0); + + let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key); + + // First push the multisig dummy, note that due to BIP147 (NULLDUMMY) it must be a zero-length element. + htlc_tx.input[0].witness.push(Vec::new()); + + htlc_tx.input[0].witness.push(this_htlc.1.unwrap().serialize_der().to_vec()); + htlc_tx.input[0].witness.push(signature.serialize_der().to_vec()); + htlc_tx.input[0].witness[1].push(SigHashType::All as u8); + htlc_tx.input[0].witness[2].push(SigHashType::All as u8); + + if this_htlc.0.offered { + // Due to BIP146 (MINIMALIF) this must be a zero-length element to relay. + htlc_tx.input[0].witness.push(Vec::new()); + } else { + htlc_tx.input[0].witness.push(preimage.unwrap().0.to_vec()); } - &None + + htlc_tx.input[0].witness.push(htlc_redeemscript.as_bytes().to_vec()); + htlc_tx } } impl PartialEq for LocalCommitmentTransaction { @@ -674,10 +688,9 @@ impl Writeable for LocalCommitmentTransaction { self.local_keys.write(writer)?; self.feerate_per_kw.write(writer)?; writer.write_all(&byte_utils::be64_to_array(self.per_htlc.len() as u64))?; - for &(ref htlc, ref sig, ref htlc_tx) in self.per_htlc.iter() { + for &(ref htlc, ref sig) in self.per_htlc.iter() { htlc.write(writer)?; sig.write(writer)?; - htlc_tx.write(writer)?; } Ok(()) } @@ -694,12 +707,11 @@ impl Readable for LocalCommitmentTransaction { let local_keys = Readable::read(reader)?; let feerate_per_kw = Readable::read(reader)?; let htlcs_count: u64 = Readable::read(reader)?; - let mut per_htlc = Vec::with_capacity(cmp::min(htlcs_count as usize, MAX_ALLOC_SIZE / mem::size_of::<(HTLCOutputInCommitment, Option, Option)>())); + let mut per_htlc = Vec::with_capacity(cmp::min(htlcs_count as usize, MAX_ALLOC_SIZE / mem::size_of::<(HTLCOutputInCommitment, Option)>())); for _ in 0..htlcs_count { let htlc: HTLCOutputInCommitment = Readable::read(reader)?; let sigs = Readable::read(reader)?; - let htlc_tx = Readable::read(reader)?; - per_htlc.push((htlc, sigs, htlc_tx)); + per_htlc.push((htlc, sigs)); } if tx.input.len() != 1 { diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 9ea859ecbba..c5908906a07 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4495,7 +4495,7 @@ mod tests { macro_rules! test_commitment { ( $their_sig_hex: expr, $our_sig_hex: expr, $tx_hex: expr, { $( { $htlc_idx: expr, $their_htlc_sig_hex: expr, $our_htlc_sig_hex: expr, $htlc_tx_hex: expr } ), * - } ) => { + } ) => { { unsigned_tx = { let mut res = chan.build_commitment_transaction(0xffffffffffff - 42, &keys, true, false, chan.feerate_per_kw); let htlcs = res.2.drain(..) @@ -4523,6 +4523,9 @@ mod tests { assert_eq!(serialize(localtx.with_valid_witness())[..], hex::decode($tx_hex).unwrap()[..]); + let htlc_sigs = chan_keys.sign_local_commitment_htlc_transactions(&localtx, chan.their_to_self_delay, &chan.secp_ctx).unwrap(); + let mut htlc_sig_iter = localtx.per_htlc.iter().zip(htlc_sigs.iter().enumerate()); + $({ let remote_signature = Signature::from_der(&hex::decode($their_htlc_sig_hex).unwrap()[..]).unwrap(); @@ -4544,12 +4547,19 @@ mod tests { assert!(preimage.is_some()); } - chan_keys.sign_htlc_transaction(&mut localtx, $htlc_idx, preimage, chan.their_to_self_delay, &chan.secp_ctx); + let mut htlc_sig = htlc_sig_iter.next().unwrap(); + while (htlc_sig.1).1.is_none() { htlc_sig = htlc_sig_iter.next().unwrap(); } + assert_eq!((htlc_sig.0).0.transaction_output_index, Some($htlc_idx)); - assert_eq!(serialize(localtx.htlc_with_valid_witness($htlc_idx).as_ref().unwrap())[..], + assert_eq!(serialize(&localtx.get_signed_htlc_tx((htlc_sig.1).0, &(htlc_sig.1).1.unwrap(), &preimage, chan.their_to_self_delay))[..], hex::decode($htlc_tx_hex).unwrap()[..]); })* - } + loop { + let htlc_sig = htlc_sig_iter.next(); + if htlc_sig.is_none() { break; } + assert!((htlc_sig.unwrap().1).1.is_none()); + } + } } } { diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 4987f8f3b38..0dbf98c72df 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1677,8 +1677,18 @@ impl ChannelMonitor { for &(ref htlc, _, _) in local_tx.htlc_outputs.iter() { if let Some(transaction_output_index) = htlc.transaction_output_index { - let preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) { Some(*preimage) } else { None }; - claim_requests.push(ClaimRequest { absolute_timelock: ::std::u32::MAX, aggregable: false, outpoint: BitcoinOutPoint { txid: local_tx.txid, vout: transaction_output_index as u32 }, witness_data: InputMaterial::LocalHTLC { preimage, amount: htlc.amount_msat / 1000 }}); + claim_requests.push(ClaimRequest { absolute_timelock: ::std::u32::MAX, aggregable: false, outpoint: BitcoinOutPoint { txid: local_tx.txid, vout: transaction_output_index as u32 }, + witness_data: InputMaterial::LocalHTLC { + preimage: if !htlc.offered { + if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) { + Some(preimage.clone()) + } else { + // We can't build an HTLC-Success transaction without the preimage + continue; + } + } else { None }, + amount: htlc.amount_msat, + }}); watch_outputs.push(commitment_tx.output[transaction_output_index as usize].clone()); } } @@ -1780,9 +1790,15 @@ impl ChannelMonitor { let txid = commitment_tx.txid(); let mut res = vec![commitment_tx]; for htlc in self.current_local_commitment_tx.htlc_outputs.iter() { - if let Some(htlc_index) = htlc.0.transaction_output_index { - let preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(*preimage) } else { None }; - if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx(txid, htlc_index, preimage) { + if let Some(vout) = htlc.0.transaction_output_index { + let preimage = if !htlc.0.offered { + if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(preimage.clone()) } else { + // We can't build an HTLC-Success transaction without the preimage + continue; + } + } else { None }; + if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx( + &::bitcoin::OutPoint { txid, vout }, &preimage) { res.push(htlc_tx); } } @@ -1804,9 +1820,15 @@ impl ChannelMonitor { let txid = commitment_tx.txid(); let mut res = vec![commitment_tx]; for htlc in self.current_local_commitment_tx.htlc_outputs.iter() { - if let Some(htlc_index) = htlc.0.transaction_output_index { - let preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(*preimage) } else { None }; - if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx(txid, htlc_index, preimage) { + if let Some(vout) = htlc.0.transaction_output_index { + let preimage = if !htlc.0.offered { + if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(preimage.clone()) } else { + // We can't build an HTLC-Success transaction without the preimage + continue; + } + } else { None }; + if let Some(htlc_tx) = self.onchain_tx_handler.unsafe_get_fully_signed_htlc_tx( + &::bitcoin::OutPoint { txid, vout }, &preimage) { res.push(htlc_tx); } } diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 77ce1bdc4be..6b5da67c1a5 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -10,7 +10,7 @@ use bitcoin::util::bip143; use bitcoin_hashes::sha256d::Hash as Sha256dHash; -use secp256k1::Secp256k1; +use secp256k1::{Secp256k1, Signature}; use secp256k1; use ln::msgs::DecodeError; @@ -137,13 +137,63 @@ macro_rules! subtract_high_prio_fee { } } +impl Readable for Option>> { + fn read(reader: &mut R) -> Result { + match Readable::read(reader)? { + 0u8 => Ok(None), + 1u8 => { + let vlen: u64 = Readable::read(reader)?; + let mut ret = Vec::with_capacity(cmp::min(vlen as usize, MAX_ALLOC_SIZE / ::std::mem::size_of::>())); + for _ in 0..vlen { + ret.push(match Readable::read(reader)? { + 0u8 => None, + 1u8 => Some((::read(reader)? as usize, Readable::read(reader)?)), + _ => return Err(DecodeError::InvalidValue) + }); + } + Ok(Some(ret)) + }, + _ => Err(DecodeError::InvalidValue), + } + } +} + +impl Writeable for Option>> { + fn write(&self, writer: &mut W) -> Result<(), ::std::io::Error> { + match self { + &Some(ref vec) => { + 1u8.write(writer)?; + (vec.len() as u64).write(writer)?; + for opt in vec.iter() { + match opt { + &Some((ref idx, ref sig)) => { + 1u8.write(writer)?; + (*idx as u64).write(writer)?; + sig.write(writer)?; + }, + &None => 0u8.write(writer)?, + } + } + }, + &None => 0u8.write(writer)?, + } + Ok(()) + } +} + /// OnchainTxHandler receives claiming requests, aggregates them if it's sound, broadcast and /// do RBF bumping if possible. pub struct OnchainTxHandler { destination_script: Script, local_commitment: Option, + // local_htlc_sigs and prev_local_htlc_sigs are in the order as they appear in the commitment + // transaction outputs (hence the Option<>s inside the Vec). The first usize is the index in + // the set of HTLCs in the LocalCommitmentTransaction (including those which do not appear in + // the commitment transaction). + local_htlc_sigs: Option>>, prev_local_commitment: Option, + prev_local_htlc_sigs: Option>>, local_csv: u16, key_storage: ChanSigner, @@ -185,7 +235,9 @@ impl OnchainTxHandler { pub(crate) fn write(&self, writer: &mut W) -> Result<(), ::std::io::Error> { self.destination_script.write(writer)?; self.local_commitment.write(writer)?; + self.local_htlc_sigs.write(writer)?; self.prev_local_commitment.write(writer)?; + self.prev_local_htlc_sigs.write(writer)?; self.local_csv.write(writer)?; @@ -231,7 +283,9 @@ impl ReadableArgs> for OnchainTx let destination_script = Readable::read(reader)?; let local_commitment = Readable::read(reader)?; + let local_htlc_sigs = Readable::read(reader)?; let prev_local_commitment = Readable::read(reader)?; + let prev_local_htlc_sigs = Readable::read(reader)?; let local_csv = Readable::read(reader)?; @@ -283,7 +337,9 @@ impl ReadableArgs> for OnchainTx Ok(OnchainTxHandler { destination_script, local_commitment, + local_htlc_sigs, prev_local_commitment, + prev_local_htlc_sigs, local_csv, key_storage, claimable_outpoints, @@ -303,7 +359,9 @@ impl OnchainTxHandler { OnchainTxHandler { destination_script, local_commitment: None, + local_htlc_sigs: None, prev_local_commitment: None, + prev_local_htlc_sigs: None, local_csv, key_storage, pending_claim_requests: HashMap::new(), @@ -510,19 +568,7 @@ impl OnchainTxHandler { for (_, (outp, per_outp_material)) in cached_claim_datas.per_input_material.iter().enumerate() { match per_outp_material { &InputMaterial::LocalHTLC { ref preimage, ref amount } => { - let mut htlc_tx = None; - if let Some(ref mut local_commitment) = self.local_commitment { - if local_commitment.txid() == outp.txid { - self.key_storage.sign_htlc_transaction(local_commitment, outp.vout, *preimage, self.local_csv, &self.secp_ctx); - htlc_tx = local_commitment.htlc_with_valid_witness(outp.vout).clone(); - } - } - if let Some(ref mut prev_local_commitment) = self.prev_local_commitment { - if prev_local_commitment.txid() == outp.txid { - self.key_storage.sign_htlc_transaction(prev_local_commitment, outp.vout, *preimage, self.local_csv, &self.secp_ctx); - htlc_tx = prev_local_commitment.htlc_with_valid_witness(outp.vout).clone(); - } - } + let htlc_tx = self.get_fully_signed_htlc_tx(outp, preimage); if let Some(htlc_tx) = htlc_tx { let feerate = (amount - htlc_tx.output[0].value) * 1000 / htlc_tx.get_weight() as u64; // Timer set to $NEVER given we can't bump tx without anchor outputs @@ -771,11 +817,47 @@ impl OnchainTxHandler { if let Some(ref local_commitment) = self.local_commitment { if local_commitment.has_local_sig() { return Err(()) } } + if self.local_htlc_sigs.is_some() || self.prev_local_htlc_sigs.is_some() { + return Err(()); + } self.prev_local_commitment = self.local_commitment.take(); self.local_commitment = Some(tx); Ok(()) } + fn sign_latest_local_htlcs(&mut self) { + if let Some(ref local_commitment) = self.local_commitment { + if let Ok(sigs) = self.key_storage.sign_local_commitment_htlc_transactions(local_commitment, self.local_csv, &self.secp_ctx) { + self.local_htlc_sigs = Some(Vec::new()); + let ret = self.local_htlc_sigs.as_mut().unwrap(); + for (htlc_idx, (local_sig, &(ref htlc, _))) in sigs.iter().zip(local_commitment.per_htlc.iter()).enumerate() { + if let Some(tx_idx) = htlc.transaction_output_index { + if ret.len() <= tx_idx as usize { ret.resize(tx_idx as usize + 1, None); } + ret[tx_idx as usize] = Some((htlc_idx, local_sig.expect("Did not receive a signature for a non-dust HTLC"))); + } else { + assert!(local_sig.is_none(), "Received a signature for a dust HTLC"); + } + } + } + } + } + fn sign_prev_local_htlcs(&mut self) { + if let Some(ref local_commitment) = self.prev_local_commitment { + if let Ok(sigs) = self.key_storage.sign_local_commitment_htlc_transactions(local_commitment, self.local_csv, &self.secp_ctx) { + self.prev_local_htlc_sigs = Some(Vec::new()); + let ret = self.prev_local_htlc_sigs.as_mut().unwrap(); + for (htlc_idx, (local_sig, &(ref htlc, _))) in sigs.iter().zip(local_commitment.per_htlc.iter()).enumerate() { + if let Some(tx_idx) = htlc.transaction_output_index { + if ret.len() <= tx_idx as usize { ret.resize(tx_idx as usize + 1, None); } + ret[tx_idx as usize] = Some((htlc_idx, local_sig.expect("Did not receive a signature for a non-dust HTLC"))); + } else { + assert!(local_sig.is_none(), "Received a signature for a dust HTLC"); + } + } + } + } + } + //TODO: getting lastest local transactions should be infaillible and result in us "force-closing the channel", but we may // 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 @@ -804,14 +886,44 @@ impl OnchainTxHandler { None } - pub(super) fn get_fully_signed_htlc_tx(&mut self, txid: Sha256dHash, htlc_index: u32, preimage: Option) -> Option { - //TODO: store preimage in OnchainTxHandler - if let Some(ref mut local_commitment) = self.local_commitment { - if local_commitment.txid() == txid { - self.key_storage.sign_htlc_transaction(local_commitment, htlc_index, preimage, self.local_csv, &self.secp_ctx); - return local_commitment.htlc_with_valid_witness(htlc_index).clone(); + pub(super) fn get_fully_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option) -> Option { + let mut htlc_tx = None; + if self.local_commitment.is_some() { + let commitment_txid = self.local_commitment.as_ref().unwrap().txid(); + if commitment_txid == outp.txid { + self.sign_latest_local_htlcs(); + if let &Some(ref htlc_sigs) = &self.local_htlc_sigs { + let &(ref htlc_idx, ref htlc_sig) = htlc_sigs[outp.vout as usize].as_ref().unwrap(); + htlc_tx = Some(self.local_commitment.as_ref().unwrap() + .get_signed_htlc_tx(*htlc_idx, htlc_sig, preimage, self.local_csv)); + } } } - None + if self.prev_local_commitment.is_some() { + let commitment_txid = self.prev_local_commitment.as_ref().unwrap().txid(); + if commitment_txid == outp.txid { + self.sign_prev_local_htlcs(); + if let &Some(ref htlc_sigs) = &self.prev_local_htlc_sigs { + let &(ref htlc_idx, ref htlc_sig) = htlc_sigs[outp.vout as usize].as_ref().unwrap(); + htlc_tx = Some(self.prev_local_commitment.as_ref().unwrap() + .get_signed_htlc_tx(*htlc_idx, htlc_sig, preimage, self.local_csv)); + } + } + } + htlc_tx + } + + #[cfg(test)] + pub(super) fn unsafe_get_fully_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option) -> Option { + let latest_had_sigs = self.local_htlc_sigs.is_some(); + let prev_had_sigs = self.prev_local_htlc_sigs.is_some(); + let ret = self.get_fully_signed_htlc_tx(outp, preimage); + if !latest_had_sigs { + self.local_htlc_sigs = None; + } + if !prev_had_sigs { + self.prev_local_htlc_sigs = None; + } + ret } } diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index 989a16c3753..3e1ffd04446 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -1,12 +1,12 @@ use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys, LocalCommitmentTransaction}; -use ln::channelmanager::PaymentPreimage; -use ln::msgs; +use ln::{chan_utils, msgs}; use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys}; use std::cmp; use std::sync::{Mutex, Arc}; use bitcoin::blockdata::transaction::Transaction; +use bitcoin::util::bip143; use secp256k1; use secp256k1::key::{SecretKey, PublicKey}; @@ -80,16 +80,29 @@ impl ChannelKeys for EnforcingChannelKeys { } fn sign_local_commitment(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { - self.inner.sign_local_commitment(local_commitment_tx, secp_ctx) + Ok(self.inner.sign_local_commitment(local_commitment_tx, secp_ctx).unwrap()) } #[cfg(test)] 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) + Ok(self.inner.unsafe_sign_local_commitment(local_commitment_tx, secp_ctx).unwrap()) } - fn sign_htlc_transaction(&self, local_commitment_tx: &mut LocalCommitmentTransaction, htlc_index: u32, preimage: Option, local_csv: u16, secp_ctx: &Secp256k1) { - self.inner.sign_htlc_transaction(local_commitment_tx, htlc_index, preimage, local_csv, secp_ctx); + fn sign_local_commitment_htlc_transactions(&self, local_commitment_tx: &LocalCommitmentTransaction, local_csv: u16, secp_ctx: &Secp256k1) -> Result>, ()> { + let commitment_txid = local_commitment_tx.txid(); + + for this_htlc in local_commitment_tx.per_htlc.iter() { + if this_htlc.0.transaction_output_index.is_some() { + let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, local_commitment_tx.feerate_per_kw, local_csv, &this_htlc.0, &local_commitment_tx.local_keys.a_delayed_payment_key, &local_commitment_tx.local_keys.revocation_key); + + let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&this_htlc.0, &local_commitment_tx.local_keys); + + let sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, this_htlc.0.amount_msat / 1000)[..]); + secp_ctx.verify(&sighash, this_htlc.1.as_ref().unwrap(), &local_commitment_tx.local_keys.b_htlc_key).unwrap(); + } + } + + Ok(self.inner.sign_local_commitment_htlc_transactions(local_commitment_tx, local_csv, secp_ctx).unwrap()) } fn sign_closing_transaction(&self, closing_tx: &Transaction, secp_ctx: &Secp256k1) -> Result { diff --git a/lightning/src/util/mod.rs b/lightning/src/util/mod.rs index 8c94ac5ca22..42c59b4536b 100644 --- a/lightning/src/util/mod.rs +++ b/lightning/src/util/mod.rs @@ -1,5 +1,8 @@ //! Some utility modules live here. See individual sub-modules for more info. +#[macro_use] +pub(crate) mod fuzz_wrappers; + pub mod events; pub mod errors; pub mod ser; @@ -27,6 +30,3 @@ pub(crate) mod test_utils; /// machine errors and used in fuzz targets and tests. #[cfg(any(test, feature = "fuzztarget"))] pub mod enforcing_trait_impls; - -#[macro_use] -pub(crate) mod fuzz_wrappers; From 29199fae4634425eb4307651b3b21d93580c8f42 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 23 Apr 2020 15:43:21 -0400 Subject: [PATCH 5/8] Don't modify LocalCommitmemntTransaction after construction Instead of adding signatures to LocalCommitmentTransactions, we instead leave them unsigned and use them to construct signed Transactions when we want them. This cleans up the guts of LocalCommitmentTransaction enough that we can, and do, expose its state to the world, allowing external signers to have a basic awareness of what they're signing. --- lightning/src/ln/chan_utils.rs | 123 +++++++++++++++-------------- lightning/src/ln/channel.rs | 11 ++- lightning/src/ln/channelmonitor.rs | 8 +- lightning/src/ln/onchaintx.rs | 17 ++-- 4 files changed, 78 insertions(+), 81 deletions(-) diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index f6cbdc23b54..d576fd338cf 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -484,10 +484,30 @@ pub fn build_htlc_transaction(prev_hash: &Sha256dHash, feerate_per_kw: u64, to_s /// to broadcast. Eventually this will require a signer which is possibly external, but for now we /// just pass in the SecretKeys required. pub struct LocalCommitmentTransaction { - tx: Transaction, - pub(crate) local_keys: TxCreationKeys, - pub(crate) feerate_per_kw: u64, - pub(crate) per_htlc: Vec<(HTLCOutputInCommitment, Option)>, + // TODO: We should migrate away from providing the transaction, instead providing enough to + // allow the ChannelKeys to construct it from scratch. Luckily we already have HTLC data here, + // so we're probably most of the way there. + /// The commitment transaction itself, in unsigned form. + pub unsigned_tx: Transaction, + /// Our counterparty's signature for the transaction, above. + pub their_sig: Signature, + // Which order the signatures should go in when constructing the final commitment tx witness. + // The user should be able to reconstruc this themselves, so we don't bother to expose it. + our_sig_first: bool, + /// The key derivation parameters for this commitment transaction + pub local_keys: TxCreationKeys, + /// The feerate paid per 1000-weight-unit in this commitment transaction. This value is + /// controlled by the channel initiator. + pub feerate_per_kw: u64, + /// The HTLCs and remote htlc signatures which were included in this commitment transaction. + /// + /// Note that this includes all HTLCs, including ones which were considered dust and not + /// actually included in the transaction as it appears on-chain, but who's value is burned as + /// fees and not included in the to_local or to_remote outputs. + /// + /// The remote HTLC signatures in the second element will always be set for non-dust HTLCs, ie + /// those for which transaction_output_index.is_some(). + pub per_htlc: Vec<(HTLCOutputInCommitment, Option)>, } impl LocalCommitmentTransaction { #[cfg(test)] @@ -499,16 +519,19 @@ impl LocalCommitmentTransaction { }, script_sig: Default::default(), sequence: 0, - witness: vec![vec![], vec![], vec![]] + witness: vec![] }; let dummy_key = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&[42; 32]).unwrap()); + let dummy_sig = Secp256k1::new().sign(&secp256k1::Message::from_slice(&[42; 32]).unwrap(), &SecretKey::from_slice(&[42; 32]).unwrap()); Self { - tx: Transaction { + unsigned_tx: Transaction { version: 2, input: vec![dummy_input], output: Vec::new(), lock_time: 0, }, + their_sig: dummy_sig, + our_sig_first: false, local_keys: TxCreationKeys { per_commitment_point: dummy_key.clone(), revocation_key: dummy_key.clone(), @@ -524,23 +547,14 @@ impl LocalCommitmentTransaction { /// Generate a new LocalCommitmentTransaction based on a raw commitment transaction, /// remote signature and both parties keys - pub(crate) fn new_missing_local_sig(mut tx: Transaction, their_sig: &Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_data: Vec<(HTLCOutputInCommitment, Option)>) -> LocalCommitmentTransaction { - if tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); } - if tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); } - - tx.input[0].witness.push(Vec::new()); // First is the multisig dummy - - if our_funding_key.serialize()[..] < their_funding_key.serialize()[..] { - tx.input[0].witness.push(Vec::new()); - tx.input[0].witness.push(their_sig.serialize_der().to_vec()); - tx.input[0].witness[2].push(SigHashType::All as u8); - } else { - tx.input[0].witness.push(their_sig.serialize_der().to_vec()); - tx.input[0].witness[1].push(SigHashType::All as u8); - tx.input[0].witness.push(Vec::new()); - } + pub(crate) fn new_missing_local_sig(unsigned_tx: Transaction, their_sig: Signature, our_funding_key: &PublicKey, their_funding_key: &PublicKey, local_keys: TxCreationKeys, feerate_per_kw: u64, htlc_data: Vec<(HTLCOutputInCommitment, Option)>) -> LocalCommitmentTransaction { + if unsigned_tx.input.len() != 1 { panic!("Tried to store a commitment transaction that had input count != 1!"); } + if unsigned_tx.input[0].witness.len() != 0 { panic!("Tried to store a signed commitment transaction?"); } - Self { tx, + Self { + unsigned_tx, + their_sig, + our_sig_first: our_funding_key.serialize()[..] < their_funding_key.serialize()[..], local_keys, feerate_per_kw, per_htlc: htlc_data, @@ -550,22 +564,7 @@ impl LocalCommitmentTransaction { /// Get the txid of the local commitment transaction contained in this /// LocalCommitmentTransaction pub fn txid(&self) -> Sha256dHash { - self.tx.txid() - } - - /// Check if LocalCommitmentTransaction has already been signed by us - 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()); - assert!(!self.tx.input[0].witness[2].is_empty()); - true - } else { - assert_eq!(self.tx.input[0].witness.len(), 3); - assert!(self.tx.input[0].witness[0].is_empty()); - assert!(self.tx.input[0].witness[1].is_empty() || self.tx.input[0].witness[2].is_empty()); - false - } + self.unsigned_tx.txid() } /// Gets our signature for the contained commitment transaction given our funding private key. @@ -577,32 +576,28 @@ impl LocalCommitmentTransaction { /// ChannelKeys::sign_local_commitment() calls directly. /// Channel value is amount locked in funding_outpoint. 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 sighash = hash_to_message!(&bip143::SighashComponents::new(&self.unsigned_tx) + .sighash_all(&self.unsigned_tx.input[0], funding_redeemscript, channel_value_satoshis)[..]); secp_ctx.sign(&sighash, funding_key) } + pub(crate) fn add_local_sig(&self, funding_redeemscript: &Script, our_sig: Signature) -> Transaction { + let mut tx = self.unsigned_tx.clone(); + // First push the multisig dummy, note that due to BIP147 (NULLDUMMY) it must be a zero-length element. + tx.input[0].witness.push(Vec::new()); - 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(); - self.tx.input[0].witness[1].push(SigHashType::All as u8); + if self.our_sig_first { + tx.input[0].witness.push(our_sig.serialize_der().to_vec()); + tx.input[0].witness.push(self.their_sig.serialize_der().to_vec()); } else { - self.tx.input[0].witness[2] = our_sig.serialize_der().to_vec(); - self.tx.input[0].witness[2].push(SigHashType::All as u8); + tx.input[0].witness.push(self.their_sig.serialize_der().to_vec()); + tx.input[0].witness.push(our_sig.serialize_der().to_vec()); } + tx.input[0].witness[1].push(SigHashType::All as u8); + tx.input[0].witness[2].push(SigHashType::All as u8); - self.tx.input[0].witness.push(funding_redeemscript.as_bytes().to_vec()); - } - - /// 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(crate) fn with_valid_witness(&self) -> &Transaction { - assert!(self.has_local_sig()); - &self.tx + tx.input[0].witness.push(funding_redeemscript.as_bytes().to_vec()); + tx } /// Get a signature for each HTLC which was included in the commitment transaction (ie for @@ -679,12 +674,14 @@ impl PartialEq for LocalCommitmentTransaction { } impl Writeable for LocalCommitmentTransaction { fn write(&self, writer: &mut W) -> Result<(), ::std::io::Error> { - if let Err(e) = self.tx.consensus_encode(&mut WriterWriteAdaptor(writer)) { + if let Err(e) = self.unsigned_tx.consensus_encode(&mut WriterWriteAdaptor(writer)) { match e { encode::Error::Io(e) => return Err(e), _ => panic!("local tx must have been well-formed!"), } } + self.their_sig.write(writer)?; + self.our_sig_first.write(writer)?; self.local_keys.write(writer)?; self.feerate_per_kw.write(writer)?; writer.write_all(&byte_utils::be64_to_array(self.per_htlc.len() as u64))?; @@ -697,13 +694,15 @@ impl Writeable for LocalCommitmentTransaction { } impl Readable for LocalCommitmentTransaction { fn read(reader: &mut R) -> Result { - let tx = match Transaction::consensus_decode(reader.by_ref()) { + let unsigned_tx = match Transaction::consensus_decode(reader.by_ref()) { Ok(tx) => tx, Err(e) => match e { encode::Error::Io(ioe) => return Err(DecodeError::Io(ioe)), _ => return Err(DecodeError::InvalidValue), }, }; + let their_sig = Readable::read(reader)?; + let our_sig_first = Readable::read(reader)?; let local_keys = Readable::read(reader)?; let feerate_per_kw = Readable::read(reader)?; let htlcs_count: u64 = Readable::read(reader)?; @@ -714,12 +713,14 @@ impl Readable for LocalCommitmentTransaction { per_htlc.push((htlc, sigs)); } - if tx.input.len() != 1 { + if unsigned_tx.input.len() != 1 { // Ensure tx didn't hit the 0-input ambiguity case. return Err(DecodeError::InvalidValue); } Ok(Self { - tx, + unsigned_tx, + their_sig, + our_sig_first, local_keys, feerate_per_kw, per_htlc, diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index c5908906a07..35e688a0d76 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1460,7 +1460,7 @@ impl Channel { // They sign the "local" commitment transaction... secp_check!(self.secp_ctx.verify(&local_sighash, &sig, self.their_funding_pubkey()), "Invalid funding_created signature from peer"); - let localtx = LocalCommitmentTransaction::new_missing_local_sig(local_initial_commitment_tx, sig, &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), self.their_funding_pubkey(), local_keys, self.feerate_per_kw, Vec::new()); + let localtx = LocalCommitmentTransaction::new_missing_local_sig(local_initial_commitment_tx, sig.clone(), &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), self.their_funding_pubkey(), local_keys, self.feerate_per_kw, Vec::new()); let remote_keys = self.build_remote_transaction_keys()?; let remote_initial_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw).0; @@ -1574,7 +1574,7 @@ impl Channel { let funding_txo_script = funding_redeemscript.to_v0_p2wsh(); macro_rules! create_monitor { () => { { - let local_commitment_tx = LocalCommitmentTransaction::new_missing_local_sig(local_initial_commitment_tx.clone(), &msg.signature, &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), their_funding_pubkey, local_keys.clone(), self.feerate_per_kw, Vec::new()); + let local_commitment_tx = LocalCommitmentTransaction::new_missing_local_sig(local_initial_commitment_tx.clone(), msg.signature.clone(), &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), their_funding_pubkey, local_keys.clone(), self.feerate_per_kw, Vec::new()); let mut channel_monitor = ChannelMonitor::new(self.local_keys.clone(), &self.shutdown_pubkey, self.our_to_self_delay, &self.destination_script, (funding_txo.clone(), funding_txo_script.clone()), @@ -1902,7 +1902,7 @@ impl Channel { let mut monitor_update = ChannelMonitorUpdate { update_id: self.latest_monitor_update_id, updates: vec![ChannelMonitorUpdateStep::LatestLocalCommitmentTXInfo { - commitment_tx: LocalCommitmentTransaction::new_missing_local_sig(local_commitment_tx.0, &msg.signature, &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), &their_funding_pubkey, local_keys, self.feerate_per_kw, htlcs_without_source), + commitment_tx: LocalCommitmentTransaction::new_missing_local_sig(local_commitment_tx.0, msg.signature.clone(), &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), &their_funding_pubkey, local_keys, self.feerate_per_kw, htlcs_without_source), htlc_outputs: htlcs_and_sigs }] }; @@ -4516,11 +4516,10 @@ 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); + localtx = LocalCommitmentTransaction::new_missing_local_sig(unsigned_tx.0.clone(), their_signature.clone(), &PublicKey::from_secret_key(&secp_ctx, chan.local_keys.funding_key()), chan.their_funding_pubkey(), keys.clone(), chan.feerate_per_kw, per_htlc); 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())[..], + assert_eq!(serialize(&localtx.add_local_sig(&redeemscript, local_sig))[..], hex::decode($tx_hex).unwrap()[..]); let htlc_sigs = chan_keys.sign_local_commitment_htlc_transactions(&localtx, chan.their_to_self_delay, &chan.secp_ctx).unwrap(); diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 0dbf98c72df..c417671eca0 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1066,8 +1066,8 @@ impl ChannelMonitor { let mut onchain_tx_handler = OnchainTxHandler::new(destination_script.clone(), keys.clone(), their_to_self_delay, logger.clone()); - let local_tx_sequence = initial_local_commitment_tx.without_valid_witness().input[0].sequence as u64; - let local_tx_locktime = initial_local_commitment_tx.without_valid_witness().lock_time as u64; + let local_tx_sequence = initial_local_commitment_tx.unsigned_tx.input[0].sequence as u64; + let local_tx_locktime = initial_local_commitment_tx.unsigned_tx.lock_time as u64; let local_commitment_tx = LocalSignedTx { txid: initial_local_commitment_tx.txid(), revocation_key: initial_local_commitment_tx.local_keys.revocation_key, @@ -1249,8 +1249,8 @@ impl ChannelMonitor { return Err(MonitorUpdateError("A local commitment tx has already been signed, no new local commitment txn can be sent to our counterparty")); } let txid = commitment_tx.txid(); - let sequence = commitment_tx.without_valid_witness().input[0].sequence as u64; - let locktime = commitment_tx.without_valid_witness().lock_time as u64; + let sequence = commitment_tx.unsigned_tx.input[0].sequence as u64; + let locktime = commitment_tx.unsigned_tx.lock_time as u64; let mut new_local_commitment_tx = LocalSignedTx { txid, revocation_key: commitment_tx.local_keys.revocation_key, diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 6b5da67c1a5..49572f2cb8b 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -814,9 +814,6 @@ impl OnchainTxHandler { // HTLC-timeout or channel force-closure), don't allow any further update of local // commitment transaction view to avoid delivery of revocation secret to counterparty // for the aformentionned signed transaction. - if let Some(ref local_commitment) = self.local_commitment { - if local_commitment.has_local_sig() { return Err(()) } - } if self.local_htlc_sigs.is_some() || self.prev_local_htlc_sigs.is_some() { return Err(()); } @@ -865,25 +862,25 @@ impl OnchainTxHandler { pub(super) fn get_fully_signed_local_tx(&mut self, funding_redeemscript: &Script) -> Option { if let Some(ref mut local_commitment) = self.local_commitment { match self.key_storage.sign_local_commitment(local_commitment, &self.secp_ctx) { - Ok(sig) => local_commitment.add_local_sig(funding_redeemscript, sig), + Ok(sig) => Some(local_commitment.add_local_sig(funding_redeemscript, sig)), Err(_) => return None, } - return Some(local_commitment.with_valid_witness().clone()); + } else { + None } - None } #[cfg(test)] 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(); + let local_commitment = local_commitment.clone(); match self.key_storage.sign_local_commitment(&local_commitment, &self.secp_ctx) { - Ok(sig) => local_commitment.add_local_sig(funding_redeemscript, sig), + Ok(sig) => Some(local_commitment.add_local_sig(funding_redeemscript, sig)), Err(_) => return None, } - return Some(local_commitment.with_valid_witness().clone()); + } else { + None } - None } pub(super) fn get_fully_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option) -> Option { From 92c0698865ffdfa2c603288ef1d7f23130243aae Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 20 Apr 2020 01:07:18 -0400 Subject: [PATCH 6/8] Remove TODOs from documentation in keysinterface We should never be exposing our own TODOs to the world. --- lightning/src/chain/keysinterface.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 44f347abc12..73c22684c54 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -184,11 +184,11 @@ pub trait KeysInterface: Send + Sync { /// Readable/Writable to serialize out a unique reference to this set of keys so /// that you can serialize the full ChannelManager object. /// -/// (TODO: We shouldn't require that, and should have an API to get them at deser time, due mostly -/// to the possibility of reentrancy issues by calling the user's code during our deserialization -/// routine). -/// TODO: We should remove Clone by instead requesting a new ChannelKeys copy when we create -/// ChannelMonitors instead of expecting to clone the one out of the Channel into the monitors. +// (TODO: We shouldn't require that, and should have an API to get them at deser time, due mostly +// to the possibility of reentrancy issues by calling the user's code during our deserialization +// routine). +// TODO: We should remove Clone by instead requesting a new ChannelKeys copy when we create +// ChannelMonitors instead of expecting to clone the one out of the Channel into the monitors. pub trait ChannelKeys : Send+Clone { /// Gets the private key for the anchor tx fn funding_key<'a>(&'a self) -> &'a SecretKey; @@ -209,18 +209,18 @@ pub trait ChannelKeys : Send+Clone { /// Create a signature for a remote commitment transaction and associated HTLC transactions. /// /// Note that if signing fails or is rejected, the channel will be force-closed. - /// - /// 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 - /// making the callee generate it via some util function we expose)! + // + // 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 + // 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. 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: 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 fn sign_local_commitment(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1) -> Result; /// Same as sign_local_commitment, but exists only for tests to get access to local commitment From 03316cd14178c3109174e53d892bab6275b510a1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 24 Apr 2020 15:31:33 -0400 Subject: [PATCH 7/8] Remove trivial assertions in LocalCommitmentTransaction fns We don't need to assert that transaction structure is what we expect when the transaction is created by a function twenty lines up in the same file. --- lightning/src/ln/chan_utils.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index d576fd338cf..62d3103337d 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -614,8 +614,6 @@ impl LocalCommitmentTransaction { for this_htlc in self.per_htlc.iter() { if this_htlc.0.transaction_output_index.is_some() { let htlc_tx = build_htlc_transaction(&txid, self.feerate_per_kw, local_csv, &this_htlc.0, &self.local_keys.a_delayed_payment_key, &self.local_keys.revocation_key); - assert_eq!(htlc_tx.input.len(), 1); - assert_eq!(htlc_tx.input[0].witness.len(), 0); let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key); @@ -642,8 +640,6 @@ impl LocalCommitmentTransaction { // Channel should have checked that we have a remote signature for this HTLC at // creation, and we should have a sensible htlc transaction: assert!(this_htlc.1.is_some()); - assert_eq!(htlc_tx.input.len(), 1); - assert_eq!(htlc_tx.input[0].witness.len(), 0); let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key); From 8b18d906bb1861fc2ac8258b2cd11955b346df5f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 24 Apr 2020 22:10:00 -0400 Subject: [PATCH 8/8] Bump versions to 0.0.11/net-tokio 0.0.3 --- lightning-net-tokio/Cargo.toml | 4 ++-- lightning/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lightning-net-tokio/Cargo.toml b/lightning-net-tokio/Cargo.toml index 9c84b1353ae..adfedea7bcc 100644 --- a/lightning-net-tokio/Cargo.toml +++ b/lightning-net-tokio/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning-net-tokio" -version = "0.0.3" +version = "0.0.4" authors = ["Matt Corallo"] license = "Apache-2.0" edition = "2018" @@ -12,7 +12,7 @@ For Rust-Lightning clients which wish to make direct connections to Lightning P2 [dependencies] bitcoin = "0.21" bitcoin_hashes = "0.7" -lightning = { version = "0.0.10", path = "../lightning" } +lightning = { version = "0.0.11", path = "../lightning" } secp256k1 = "0.15" tokio = { version = ">=0.2.12", features = [ "io-util", "macros", "rt-core", "sync", "tcp", "time" ] } diff --git a/lightning/Cargo.toml b/lightning/Cargo.toml index 341084fc393..96680b56e47 100644 --- a/lightning/Cargo.toml +++ b/lightning/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning" -version = "0.0.10" +version = "0.0.11" authors = ["Matt Corallo"] license = "Apache-2.0" repository = "https://github.com/rust-bitcoin/rust-lightning/"