Make commitment transaction signing a part of ChannelKeys.

This adds a new fn to ChannelKeys which is called when we generte
a new remote commitment transaction for signing. While it may be
theoretically possible to unwind state updates by disconnecting and
reconnecting as well as making appropriate state machine changes,
the effort required to get it correct likely outweighs the UX cost
of "preflighting" the requests to hardwre wallets.

Author: TheBlueMatt

lightning/src/chain/keysinterface.rs

@@ -2,24 +2,28 @@
 //! spendable on-chain outputs which the user owns and is responsible for using just as any other
 //! on-chain output which is theirs.
 
-use bitcoin::blockdata::transaction::{OutPoint, TxOut};
+use bitcoin::blockdata::transaction::{Transaction, OutPoint, TxOut};
 use bitcoin::blockdata::script::{Script, Builder};
 use bitcoin::blockdata::opcodes;
 use bitcoin::network::constants::Network;
 use bitcoin::util::bip32::{ExtendedPrivKey, ExtendedPubKey, ChildNumber};
+use bitcoin::util::bip143;
 
 use bitcoin_hashes::{Hash, HashEngine};
 use bitcoin_hashes::sha256::HashEngine as Sha256State;
 use bitcoin_hashes::sha256::Hash as Sha256;
 use bitcoin_hashes::hash160::Hash as Hash160;
 
 use secp256k1::key::{SecretKey, PublicKey};
-use secp256k1::Secp256k1;
+use secp256k1::{Secp256k1, Signature};
 use secp256k1;
 
 use util::byte_utils;
 use util::logger::Logger;
 
+use ln::chan_utils;
+use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment};
+
 use std::sync::Arc;
 use std::sync::atomic::{AtomicUsize, Ordering};
 
@@ -110,6 +114,15 @@ pub trait ChannelKeys : Send {
 	fn htlc_base_key<'a>(&'a self) -> &'a SecretKey;
 	/// Gets the commitment seed
 	fn commitment_seed<'a>(&'a self) -> &'a [u8; 32];
+
+	/// 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)!
+	fn sign_remote_commitment<T: secp256k1::Signing>(&self, channel_value_satoshis: u64, channel_funding_script: &Script, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()>;
 }
 
 #[derive(Clone)]
@@ -136,6 +149,31 @@ impl ChannelKeys for InMemoryChannelKeys {
 	fn delayed_payment_base_key(&self) -> &SecretKey { &self.delayed_payment_base_key }
 	fn htlc_base_key(&self) -> &SecretKey { &self.htlc_base_key }
 	fn commitment_seed(&self) -> &[u8; 32] { &self.commitment_seed }
+
+
+	fn sign_remote_commitment<T: secp256k1::Signing>(&self, channel_value_satoshis: u64, channel_funding_script: &Script, feerate_per_kw: u64, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], to_self_delay: u16, secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
+		if commitment_tx.input.len() != 1 { return Err(()); }
+		let commitment_sighash = hash_to_message!(&bip143::SighashComponents::new(&commitment_tx).sighash_all(&commitment_tx.input[0], &channel_funding_script, channel_value_satoshis)[..]);
+		let commitment_sig = secp_ctx.sign(&commitment_sighash, &self.funding_key);
+
+		let commitment_txid = commitment_tx.txid();
+
+		let mut htlc_sigs = Vec::with_capacity(htlcs.len());
+		for ref htlc in htlcs {
+			if let Some(_) = htlc.transaction_output_index {
+				let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, feerate_per_kw, to_self_delay, htlc, &keys.a_delayed_payment_key, &keys.revocation_key);
+				let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &keys);
+				let htlc_sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]);
+				let our_htlc_key = match chan_utils::derive_private_key(&secp_ctx, &keys.per_commitment_point, &self.htlc_base_key) {
+					Ok(s) => s,
+					Err(_) => return Err(()),
+				};
+				htlc_sigs.push(secp_ctx.sign(&htlc_sighash, &our_htlc_key));
+			}
+		}
+
+		Ok((commitment_sig, htlc_sigs))
+	}
 }
 
 impl_writeable!(InMemoryChannelKeys, 0, {

lightning/src/ln/chan_utils.rs

@@ -1,3 +1,7 @@
+//! Various utilities for building scripts and deriving keys related to channels. These are
+//! largely of interest for those implementing chain::keysinterface::ChannelKeys message signing
+//! by hand.
+
 use bitcoin::blockdata::script::{Script,Builder};
 use bitcoin::blockdata::opcodes;
 use bitcoin::blockdata::transaction::{TxIn,TxOut,OutPoint,Transaction};
@@ -14,13 +18,13 @@ use secp256k1::key::{PublicKey,SecretKey};
 use secp256k1::Secp256k1;
 use secp256k1;
 
-pub const HTLC_SUCCESS_TX_WEIGHT: u64 = 703;
-pub const HTLC_TIMEOUT_TX_WEIGHT: u64 = 663;
+pub(super) const HTLC_SUCCESS_TX_WEIGHT: u64 = 703;
+pub(super) const HTLC_TIMEOUT_TX_WEIGHT: u64 = 663;
 
 // Various functions for key derivation and transaction creation for use within channels. Primarily
 // used in Channel and ChannelMonitor.
 
-pub fn build_commitment_secret(commitment_seed: &[u8; 32], idx: u64) -> [u8; 32] {
+pub(super) fn build_commitment_secret(commitment_seed: &[u8; 32], idx: u64) -> [u8; 32] {
 	let mut res: [u8; 32] = commitment_seed.clone();
 	for i in 0..48 {
 		let bitpos = 47 - i;
@@ -32,6 +36,8 @@ pub fn build_commitment_secret(commitment_seed: &[u8; 32], idx: u64) -> [u8; 32]
 	res
 }
 
+/// Derives a per-commitment-transaction private key (eg an htlc key or payment key) from the base
+/// private key for that type of key and the per_commitment_point (available in TxCreationKeys)
 pub fn derive_private_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, base_secret: &SecretKey) -> Result<SecretKey, secp256k1::Error> {
 	let mut sha = Sha256::engine();
 	sha.input(&per_commitment_point.serialize());
@@ -43,7 +49,7 @@ pub fn derive_private_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_co
 	Ok(key)
 }
 
-pub fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, base_point: &PublicKey) -> Result<PublicKey, secp256k1::Error> {
+pub(super) fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, base_point: &PublicKey) -> Result<PublicKey, secp256k1::Error> {
 	let mut sha = Sha256::engine();
 	sha.input(&per_commitment_point.serialize());
 	sha.input(&base_point.serialize());
@@ -54,7 +60,7 @@ pub fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_com
 }
 
 /// Derives a revocation key from its constituent parts
-pub fn derive_private_revocation_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_secret: &SecretKey, revocation_base_secret: &SecretKey) -> Result<SecretKey, secp256k1::Error> {
+pub(super) fn derive_private_revocation_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_secret: &SecretKey, revocation_base_secret: &SecretKey) -> Result<SecretKey, secp256k1::Error> {
 	let revocation_base_point = PublicKey::from_secret_key(&secp_ctx, &revocation_base_secret);
 	let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret);
 
@@ -81,7 +87,7 @@ pub fn derive_private_revocation_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1
 	Ok(part_a)
 }
 
-pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, revocation_base_point: &PublicKey) -> Result<PublicKey, secp256k1::Error> {
+pub(super) fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, revocation_base_point: &PublicKey) -> Result<PublicKey, secp256k1::Error> {
 	let rev_append_commit_hash_key = {
 		let mut sha = Sha256::engine();
 		sha.input(&revocation_base_point.serialize());
@@ -104,17 +110,26 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
 	part_a.combine(&part_b)
 }
 
+/// The set of public keys which are used in the creation of one commitment transaction.
+/// These are derived from the channel base keys and per-commitment data.
 pub struct TxCreationKeys {
+	/// The per-commitment public key which was used to derive the other keys.
 	pub per_commitment_point: PublicKey,
+	/// The revocation key which is used to allow the owner of the commitment transaction to
+	/// provide their counterparty the ability to punish them if they broadcast an old state.
 	pub revocation_key: PublicKey,
+	/// A's HTLC Key
 	pub a_htlc_key: PublicKey,
+	/// B's HTLC Key
 	pub b_htlc_key: PublicKey,
+	/// A's Payment Key (which isn't allowed to be spent from for some delay)
 	pub a_delayed_payment_key: PublicKey,
+	/// B's Payment Key
 	pub b_payment_key: PublicKey,
 }
 
 impl TxCreationKeys {
-	pub fn new<T: secp256k1::Signing + secp256k1::Verification>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, a_delayed_payment_base: &PublicKey, a_htlc_base: &PublicKey, b_revocation_base: &PublicKey, b_payment_base: &PublicKey, b_htlc_base: &PublicKey) -> Result<TxCreationKeys, secp256k1::Error> {
+	pub(super) fn new<T: secp256k1::Signing + secp256k1::Verification>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, a_delayed_payment_base: &PublicKey, a_htlc_base: &PublicKey, b_revocation_base: &PublicKey, b_payment_base: &PublicKey, b_htlc_base: &PublicKey) -> Result<TxCreationKeys, secp256k1::Error> {
 		Ok(TxCreationKeys {
 			per_commitment_point: per_commitment_point.clone(),
 			revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &b_revocation_base)?,
@@ -128,7 +143,7 @@ impl TxCreationKeys {
 
 /// Gets the "to_local" output redeemscript, ie the script which is time-locked or spendable by
 /// the revocation key
-pub fn get_revokeable_redeemscript(revocation_key: &PublicKey, to_self_delay: u16, delayed_payment_key: &PublicKey) -> Script {
+pub(super) fn get_revokeable_redeemscript(revocation_key: &PublicKey, to_self_delay: u16, delayed_payment_key: &PublicKey) -> Script {
 	Builder::new().push_opcode(opcodes::all::OP_IF)
 	              .push_slice(&revocation_key.serialize())
 	              .push_opcode(opcodes::all::OP_ELSE)
@@ -142,16 +157,28 @@ pub fn get_revokeable_redeemscript(revocation_key: &PublicKey, to_self_delay: u1
 }
 
 #[derive(Clone, PartialEq)]
+/// Information about an HTLC as it appears in a commitment transaction
 pub struct HTLCOutputInCommitment {
+	/// Whether the HTLC was "offered" (ie outbound in relation to this commitment transaction).
+	/// Note that this is not the same as whether it is ountbound *from us*. To determine that you
+	/// need to compare this value to whether the commitment transaction in question is that of
+	/// the remote party or our own.
 	pub offered: bool,
+	/// The value, in msat, of the HTLC. The value as it appears in the commitment transaction is
+	/// this divided by 1000.
 	pub amount_msat: u64,
+	/// The CLTV lock-time at which this HTLC expires.
 	pub cltv_expiry: u32,
+	/// The hash of the preimage which unlocks this HTLC.
 	pub payment_hash: PaymentHash,
+	/// The position within the commitment transactions' outputs. This may be None if the value is
+	/// below the dust limit (in which case no output appears in the commitment transaction and the
+	/// value is spent to additional transaction fees).
 	pub transaction_output_index: Option<u32>,
 }
 
 #[inline]
-pub fn get_htlc_redeemscript_with_explicit_keys(htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey) -> Script {
+pub(super) fn get_htlc_redeemscript_with_explicit_keys(htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey) -> Script {
 	let payment_hash160 = Ripemd160::hash(&htlc.payment_hash.0[..]).into_inner();
 	if htlc.offered {
 		Builder::new().push_opcode(opcodes::all::OP_DUP)

lightning/src/ln/channel.rs

@@ -833,7 +833,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		let mut local_htlc_total_msat = 0;
 		let mut value_to_self_msat_offset = 0;
 
-		log_trace!(self, "Building commitment transaction number {} for {}, generated by {} with fee {}...", commitment_number, if local { "us" } else { "remote" }, if generated_by_local { "us" } else { "remote" }, feerate_per_kw);
+		log_trace!(self, "Building commitment transaction number {} (really {} xor {}) for {}, generated by {} with fee {}...", commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number), self.get_commitment_transaction_number_obscure_factor(), if local { "us" } else { "remote" }, if generated_by_local { "us" } else { "remote" }, feerate_per_kw);
 
 		macro_rules! get_htlc_in_commitment {
 			($htlc: expr, $offered: expr) => {
@@ -1518,10 +1518,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 		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;
-		let remote_sighash = hash_to_message!(&bip143::SighashComponents::new(&remote_initial_commitment_tx).sighash_all(&remote_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]);
+		let remote_signature = self.local_keys.sign_remote_commitment(self.channel_value_satoshis, &self.get_funding_redeemscript(), self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), self.our_to_self_delay, &self.secp_ctx)
+				.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed"))?.0;
 
 		// We sign the "remote" commitment transaction, allowing them to broadcast the tx if they wish.
-		Ok((remote_initial_commitment_tx, local_initial_commitment_tx, self.secp_ctx.sign(&remote_sighash, self.local_keys.funding_key()), local_keys))
+		Ok((remote_initial_commitment_tx, local_initial_commitment_tx, remote_signature, local_keys))
 	}
 
 	pub fn funding_created(&mut self, msg: &msgs::FundingCreated) -> Result<(msgs::FundingSigned, ChannelMonitor), ChannelError> {
@@ -3222,14 +3223,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 	/// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created)
 	fn get_outbound_funding_created_signature(&mut self) -> Result<(Signature, Transaction), ChannelError> {
-		let funding_script = self.get_funding_redeemscript();
-
 		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;
-		let remote_sighash = hash_to_message!(&bip143::SighashComponents::new(&remote_initial_commitment_tx).sighash_all(&remote_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]);
-
-		// We sign the "remote" commitment transaction, allowing them to broadcast the tx if they wish.
-		Ok((self.secp_ctx.sign(&remote_sighash, self.local_keys.funding_key()), remote_initial_commitment_tx))
+		Ok((self.local_keys.sign_remote_commitment(self.channel_value_satoshis, &self.get_funding_redeemscript(), self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), self.our_to_self_delay, &self.secp_ctx)
+				.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed"))?.0, remote_initial_commitment_tx))
 	}
 
 	/// Updates channel state with knowledge of the funding transaction's txid/index, and generates
@@ -3519,8 +3516,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	/// Only fails in case of bad keys. Used for channel_reestablish commitment_signed generation
 	/// when we shouldn't change HTLC/channel state.
 	fn send_commitment_no_state_update(&self) -> Result<(msgs::CommitmentSigned, (Transaction, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>)), ChannelError> {
-		let funding_script = self.get_funding_redeemscript();
-
 		let mut feerate_per_kw = self.feerate_per_kw;
 		if let Some(feerate) = self.pending_update_fee {
 			if self.channel_outbound {
@@ -3530,27 +3525,37 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 		let remote_keys = self.build_remote_transaction_keys()?;
 		let remote_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, true, feerate_per_kw);
-		let remote_commitment_txid = remote_commitment_tx.0.txid();
-		let remote_sighash = hash_to_message!(&bip143::SighashComponents::new(&remote_commitment_tx.0).sighash_all(&remote_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]);
-		let our_sig = self.secp_ctx.sign(&remote_sighash, self.local_keys.funding_key());
-		log_trace!(self, "Signing remote commitment tx {} with redeemscript {} with pubkey {} -> {}", encode::serialize_hex(&remote_commitment_tx.0), encode::serialize_hex(&funding_script), log_bytes!(PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()).serialize()), log_bytes!(our_sig.serialize_compact()[..]));
+		let (signature, htlc_signatures);
 
-		let mut htlc_sigs = Vec::with_capacity(remote_commitment_tx.1);
-		for &(ref htlc, _) in remote_commitment_tx.2.iter() {
-			if let Some(_) = htlc.transaction_output_index {
-				let htlc_tx = self.build_htlc_transaction(&remote_commitment_txid, htlc, false, &remote_keys, feerate_per_kw);
-				let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &remote_keys);
-				let htlc_sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]);
-				let our_htlc_key = secp_check!(chan_utils::derive_private_key(&self.secp_ctx, &remote_keys.per_commitment_point, self.local_keys.htlc_base_key()), "Derived invalid key, peer is maliciously selecting parameters");
-				htlc_sigs.push(self.secp_ctx.sign(&htlc_sighash, &our_htlc_key));
-				log_trace!(self, "Signing remote HTLC tx {} with redeemscript {} with pubkey {} -> {}", encode::serialize_hex(&htlc_tx), encode::serialize_hex(&htlc_redeemscript), log_bytes!(PublicKey::from_secret_key(&self.secp_ctx, &our_htlc_key).serialize()), log_bytes!(htlc_sigs.last().unwrap().serialize_compact()[..]));
+		{
+			let mut htlcs = Vec::with_capacity(remote_commitment_tx.2.len());
+			for &(ref htlc, _) in remote_commitment_tx.2.iter() {
+				htlcs.push(htlc);
+			}
+
+			let res = self.local_keys.sign_remote_commitment(self.channel_value_satoshis, &self.get_funding_redeemscript(), feerate_per_kw, &remote_commitment_tx.0, &remote_keys, &htlcs, self.our_to_self_delay, &self.secp_ctx)
+				.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed"))?;
+			signature = res.0;
+			htlc_signatures = res.1;
+
+			log_trace!(self, "Signed remote commitment tx {} with redeemscript {} -> {}",
+				encode::serialize_hex(&remote_commitment_tx.0),
+				encode::serialize_hex(&self.get_funding_redeemscript()),
+				log_bytes!(signature.serialize_compact()[..]));
+
+			for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(htlcs) {
+				log_trace!(self, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {}",
+					encode::serialize_hex(&chan_utils::build_htlc_transaction(&remote_commitment_tx.0.txid(), feerate_per_kw, self.our_to_self_delay, htlc, &remote_keys.a_delayed_payment_key, &remote_keys.revocation_key)),
+					encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &remote_keys)),
+					log_bytes!(remote_keys.a_htlc_key.serialize()),
+					log_bytes!(htlc_sig.serialize_compact()[..]));
 			}
 		}
 
 		Ok((msgs::CommitmentSigned {
 			channel_id: self.channel_id,
-			signature: our_sig,
-			htlc_signatures: htlc_sigs,
+			signature,
+			htlc_signatures,
 		}, (remote_commitment_tx.0, remote_commitment_tx.2)))
 	}
 

lightning/src/ln/mod.rs

@@ -14,14 +14,14 @@ pub mod channelmonitor;
 pub mod msgs;
 pub mod router;
 pub mod peer_handler;
+pub mod chan_utils;
 
 #[cfg(feature = "fuzztarget")]
 pub mod peer_channel_encryptor;
 #[cfg(not(feature = "fuzztarget"))]
 pub(crate) mod peer_channel_encryptor;
 
 mod channel;
-mod chan_utils;
 mod onion_utils;
 
 #[cfg(test)]