Merge pull request #404 from TheBlueMatt/2019-11-signer-api

Replace keys API with Signer API to support hardware wallets eventually

fuzz/src/chanmon_consistency.rs

@@ -24,7 +24,7 @@ use bitcoin_hashes::sha256d::Hash as Sha256d;
 use lightning::chain::chaininterface;
 use lightning::chain::transaction::OutPoint;
 use lightning::chain::chaininterface::{BroadcasterInterface,ConfirmationTarget,ChainListener,FeeEstimator,ChainWatchInterfaceUtil};
-use lightning::chain::keysinterface::{ChannelKeys, KeysInterface};
+use lightning::chain::keysinterface::{InMemoryChannelKeys, KeysInterface};
 use lightning::ln::channelmonitor;
 use lightning::ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, HTLCUpdate};
 use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, ChannelManagerReadArgs};
@@ -130,6 +130,8 @@ struct KeyProvider {
 	channel_id: atomic::AtomicU8,
 }
 impl KeysInterface for KeyProvider {
+	type ChanKeySigner = InMemoryChannelKeys;
+
 	fn get_node_secret(&self) -> SecretKey {
 		SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, self.node_id]).unwrap()
 	}
@@ -146,8 +148,8 @@ impl KeysInterface for KeyProvider {
 		PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, self.node_id]).unwrap())
 	}
 
-	fn get_channel_keys(&self, _inbound: bool) -> ChannelKeys {
+	fn get_channel_keys(&self, _inbound: bool) -> InMemoryChannelKeys {
-		ChannelKeys {
+		InMemoryChannelKeys {
 			funding_key:               SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, self.node_id]).unwrap(),
 			revocation_base_key:       SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, self.node_id]).unwrap(),
 			payment_base_key:          SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, self.node_id]).unwrap(),
@@ -223,7 +225,7 @@ pub fn do_test(data: &[u8]) {
 				channel_monitors: &monitor_refs,
 			};
 
-			let res = (<(Sha256d, ChannelManager)>::read(&mut Cursor::new(&$ser.0), read_args).expect("Failed to read manager").1, monitor);
+			let res = (<(Sha256d, ChannelManager<InMemoryChannelKeys>)>::read(&mut Cursor::new(&$ser.0), read_args).expect("Failed to read manager").1, monitor);
 			for (_, was_good) in $old_monitors.latest_updates_good_at_last_ser.lock().unwrap().iter() {
 				if !was_good {
 					// If the last time we updated a monitor we didn't successfully update (and we

fuzz/src/full_stack.rs

@@ -20,7 +20,7 @@ use bitcoin_hashes::sha256d::Hash as Sha256dHash;
 
 use lightning::chain::chaininterface::{BroadcasterInterface,ConfirmationTarget,ChainListener,FeeEstimator,ChainWatchInterfaceUtil};
 use lightning::chain::transaction::OutPoint;
-use lightning::chain::keysinterface::{ChannelKeys, KeysInterface};
+use lightning::chain::keysinterface::{InMemoryChannelKeys, KeysInterface};
 use lightning::ln::channelmonitor;
 use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage};
 use lightning::ln::peer_handler::{MessageHandler,PeerManager,SocketDescriptor};
@@ -135,7 +135,7 @@ impl<'a> Hash for Peer<'a> {
 }
 
 struct MoneyLossDetector<'a, 'b> {
-	manager: Arc<ChannelManager<'b>>,
+	manager: Arc<ChannelManager<'b, InMemoryChannelKeys>>,
 	monitor: Arc<channelmonitor::SimpleManyChannelMonitor<OutPoint>>,
 	handler: PeerManager<Peer<'a>>,
 
@@ -148,7 +148,7 @@ struct MoneyLossDetector<'a, 'b> {
 	blocks_connected: u32,
 }
 impl<'a, 'b> MoneyLossDetector<'a, 'b> {
-	pub fn new(peers: &'a RefCell<[bool; 256]>, manager: Arc<ChannelManager<'b>>, monitor: Arc<channelmonitor::SimpleManyChannelMonitor<OutPoint>>, handler: PeerManager<Peer<'a>>) -> Self {
+	pub fn new(peers: &'a RefCell<[bool; 256]>, manager: Arc<ChannelManager<'b, InMemoryChannelKeys>>, monitor: Arc<channelmonitor::SimpleManyChannelMonitor<OutPoint>>, handler: PeerManager<Peer<'a>>) -> Self {
 		MoneyLossDetector {
 			manager,
 			monitor,
@@ -228,6 +228,8 @@ struct KeyProvider {
 	counter: AtomicU64,
 }
 impl KeysInterface for KeyProvider {
+	type ChanKeySigner = InMemoryChannelKeys;
+
 	fn get_node_secret(&self) -> SecretKey {
 		self.node_secret.clone()
 	}
@@ -244,10 +246,10 @@ impl KeysInterface for KeyProvider {
 		PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]).unwrap())
 	}
 
-	fn get_channel_keys(&self, inbound: bool) -> ChannelKeys {
+	fn get_channel_keys(&self, inbound: bool) -> InMemoryChannelKeys {
 		let ctr = self.counter.fetch_add(1, Ordering::Relaxed) as u8;
 		if inbound {
-			ChannelKeys {
+			InMemoryChannelKeys {
 				funding_key:               SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, ctr]).unwrap(),
 				revocation_base_key:       SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, ctr]).unwrap(),
 				payment_base_key:          SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, ctr]).unwrap(),
@@ -256,7 +258,7 @@ impl KeysInterface for KeyProvider {
 				commitment_seed: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, ctr],
 			}
 		} else {
-			ChannelKeys {
+			InMemoryChannelKeys {
 				funding_key:               SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, ctr]).unwrap(),
 				revocation_base_key:       SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 8, ctr]).unwrap(),
 				payment_base_key:          SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 9, ctr]).unwrap(),

lightning/src/chain/keysinterface.rs

@@ -2,11 +2,12 @@
 //! 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;
@@ -14,12 +15,15 @@ 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};
 
@@ -68,6 +72,9 @@ pub enum SpendableOutputDescriptor {
 
 /// A trait to describe an object which can get user secrets and key material.
 pub trait KeysInterface: Send + Sync {
+	/// A type which implements ChannelKeys which will be returned by get_channel_keys.
+	type ChanKeySigner : ChannelKeys;
+
 	/// Get node secret key (aka node_id or network_key)
 	fn get_node_secret(&self) -> SecretKey;
 	/// Get destination redeemScript to encumber static protocol exit points.
@@ -76,7 +83,7 @@ pub trait KeysInterface: Send + Sync {
 	fn get_shutdown_pubkey(&self) -> PublicKey;
 	/// Get a new set of ChannelKeys for per-channel secrets. These MUST be unique even if you
 	/// restarted with some stale data!
-	fn get_channel_keys(&self, inbound: bool) -> ChannelKeys;
+	fn get_channel_keys(&self, inbound: bool) -> Self::ChanKeySigner;
 	/// Get a secret and PRNG seed for construting an onion packet
 	fn get_onion_rand(&self) -> (SecretKey, [u8; 32]);
 	/// Get a unique temporary channel id. Channels will be referred to by this until the funding
@@ -85,9 +92,59 @@ pub trait KeysInterface: Send + Sync {
 	fn get_channel_id(&self) -> [u8; 32];
 }
 
-/// Set of lightning keys needed to operate a channel as described in BOLT 3
+/// Set of lightning keys needed to operate a channel as described in BOLT 3.
+///
+/// Signing services could be implemented on a hardware wallet. In this case,
+/// the current ChannelKeys would be a front-end on top of a communication
+/// channel connected to your secure device and lightning key material wouldn't
+/// reside on a hot server. Nevertheless, a this deployment would still need
+/// to trust the ChannelManager to avoid loss of funds as this latest component
+/// could ask to sign commitment transaction with HTLCs paying to attacker pubkeys.
+///
+/// A more secure iteration would be to use hashlock (or payment points) to pair
+/// invoice/incoming HTLCs with outgoing HTLCs to implement a no-trust-ChannelManager
+/// at the price of more state and computation on the hardware wallet side. In the future,
+/// we are looking forward to design such interface.
+///
+/// In any case, ChannelMonitor or fallback watchtowers are always going to be trusted
+/// to act, as liveness and breach reply correctness are always going to be hard requirements
+/// of LN security model, orthogonal of key management issues.
+///
+/// If you're implementing a custom signer, you almost certainly want to implement
+/// 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).
+pub trait ChannelKeys : Send {
+	/// Gets the private key for the anchor tx
+	fn funding_key<'a>(&'a self) -> &'a SecretKey;
+	/// Gets the local secret key for blinded revocation pubkey
+	fn revocation_base_key<'a>(&'a self) -> &'a SecretKey;
+	/// Gets the local secret key used in to_remote output of remote commitment tx
+	/// (and also as part of obscured commitment number)
+	fn payment_base_key<'a>(&'a self) -> &'a SecretKey;
+	/// Gets the local secret key used in HTLC-Success/HTLC-Timeout txn and to_local output
+	fn delayed_payment_base_key<'a>(&'a self) -> &'a SecretKey;
+	/// Gets the local htlc secret key used in commitment tx htlc outputs
+	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)]
-pub struct ChannelKeys {
+/// A simple implementation of ChannelKeys that just keeps the private keys in memory.
+pub struct InMemoryChannelKeys {
 	/// Private key of anchor tx
 	pub funding_key: SecretKey,
 	/// Local secret key for blinded revocation pubkey
@@ -102,7 +159,41 @@ pub struct ChannelKeys {
 	pub commitment_seed: [u8; 32],
 }
 
-impl_writeable!(ChannelKeys, 0, {
+impl ChannelKeys for InMemoryChannelKeys {
+	fn funding_key(&self) -> &SecretKey { &self.funding_key }
+	fn revocation_base_key(&self) -> &SecretKey { &self.revocation_base_key }
+	fn payment_base_key(&self) -> &SecretKey { &self.payment_base_key }
+	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, {
 	funding_key,
 	revocation_base_key,
 	payment_base_key,
@@ -203,6 +294,8 @@ impl KeysManager {
 }
 
 impl KeysInterface for KeysManager {
+	type ChanKeySigner = InMemoryChannelKeys;
+
 	fn get_node_secret(&self) -> SecretKey {
 		self.node_secret.clone()
 	}
@@ -215,7 +308,7 @@ impl KeysInterface for KeysManager {
 		self.shutdown_pubkey.clone()
 	}
 
-	fn get_channel_keys(&self, _inbound: bool) -> ChannelKeys {
+	fn get_channel_keys(&self, _inbound: bool) -> InMemoryChannelKeys {
 		// We only seriously intend to rely on the channel_master_key for true secure
 		// entropy, everything else just ensures uniqueness. We rely on the unique_start (ie
 		// starting_time provided in the constructor) to be unique.
@@ -248,7 +341,7 @@ impl KeysInterface for KeysManager {
 		let delayed_payment_base_key = key_step!(b"delayed payment base key", payment_base_key);
 		let htlc_base_key = key_step!(b"HTLC base key", delayed_payment_base_key);
 
-		ChannelKeys {
+		InMemoryChannelKeys {
 			funding_key,
 			revocation_base_key,
 			payment_base_key,

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,14 +18,14 @@ use secp256k1::key::{PublicKey,SecretKey};
 use secp256k1::Secp256k1;
 use secp256k1;
 
-pub const HTLC_SUCCESS_TX_WEIGHT: u64 = 703;
+pub(super) const HTLC_SUCCESS_TX_WEIGHT: u64 = 703;
-pub const HTLC_TIMEOUT_TX_WEIGHT: u64 = 663;
+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;
+	let mut res: [u8; 32] = commitment_seed.clone();
 	for i in 0..48 {
 		let bitpos = 47 - i;
 		if idx & (1 << bitpos) == (1 << bitpos) {
@@ -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

@@ -228,7 +228,7 @@ enum UpdateStatus {
 // has been completed, and then turn into a Channel to get compiler-time enforcement of things like
 // calling channel_id() before we're set up or things like get_outbound_funding_signed on an
 // inbound channel.
-pub(super) struct Channel {
+pub(super) struct Channel<ChanSigner: ChannelKeys> {
 	config: ChannelConfig,
 
 	user_id: u64,
@@ -239,7 +239,7 @@ pub(super) struct Channel {
 	secp_ctx: Secp256k1<secp256k1::All>,
 	channel_value_satoshis: u64,
 
-	local_keys: ChannelKeys,
+	local_keys: ChanSigner,
 	shutdown_pubkey: PublicKey,
 
 	// Our commitment numbers start at 2^48-1 and count down, whereas the ones used in transaction
@@ -410,7 +410,7 @@ macro_rules! secp_check {
 	};
 }
 
-impl Channel {
+impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	// Convert constants + channel value to limits:
 	fn get_our_max_htlc_value_in_flight_msat(channel_value_satoshis: u64) -> u64 {
 		channel_value_satoshis * 1000 / 10 //TODO
@@ -433,7 +433,7 @@ impl Channel {
 	}
 
 	// Constructors:
-	pub fn new_outbound(fee_estimator: &FeeEstimator, keys_provider: &Arc<KeysInterface>, their_node_id: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, logger: Arc<Logger>, config: &UserConfig) -> Result<Channel, APIError> {
+	pub fn new_outbound(fee_estimator: &FeeEstimator, keys_provider: &Arc<KeysInterface<ChanKeySigner = ChanSigner>>, their_node_id: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, logger: Arc<Logger>, config: &UserConfig) -> Result<Channel<ChanSigner>, APIError> {
 		let chan_keys = keys_provider.get_channel_keys(false);
 
 		if channel_value_satoshis >= MAX_FUNDING_SATOSHIS {
@@ -449,15 +449,15 @@ impl Channel {
 
 
 		let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-		if Channel::get_our_channel_reserve_satoshis(channel_value_satoshis) < Channel::derive_our_dust_limit_satoshis(background_feerate) {
+		if Channel::<ChanSigner>::get_our_channel_reserve_satoshis(channel_value_satoshis) < Channel::<ChanSigner>::derive_our_dust_limit_satoshis(background_feerate) {
 			return Err(APIError::FeeRateTooHigh{err: format!("Not enough reserve above dust limit can be found at current fee rate({})", background_feerate), feerate: background_feerate});
 		}
 
 		let feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
 
 		let secp_ctx = Secp256k1::new();
-		let channel_monitor = ChannelMonitor::new(&chan_keys.revocation_base_key, &chan_keys.delayed_payment_base_key,
+		let channel_monitor = ChannelMonitor::new(chan_keys.revocation_base_key(), chan_keys.delayed_payment_base_key(),
-		                                          &chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), config.own_channel_config.our_to_self_delay,
+		                                          chan_keys.htlc_base_key(), chan_keys.payment_base_key(), &keys_provider.get_shutdown_pubkey(), config.own_channel_config.our_to_self_delay,
 		                                          keys_provider.get_destination_script(), logger.clone());
 
 		Ok(Channel {
@@ -509,11 +509,11 @@ impl Channel {
 
 			feerate_per_kw: feerate,
 			their_dust_limit_satoshis: 0,
-			our_dust_limit_satoshis: Channel::derive_our_dust_limit_satoshis(background_feerate),
+			our_dust_limit_satoshis: Channel::<ChanSigner>::derive_our_dust_limit_satoshis(background_feerate),
 			their_max_htlc_value_in_flight_msat: 0,
 			their_channel_reserve_satoshis: 0,
 			their_htlc_minimum_msat: 0,
-			our_htlc_minimum_msat: Channel::derive_our_htlc_minimum_msat(feerate),
+			our_htlc_minimum_msat: Channel::<ChanSigner>::derive_our_htlc_minimum_msat(feerate),
 			their_to_self_delay: 0,
 			our_to_self_delay: config.own_channel_config.our_to_self_delay,
 			their_max_accepted_htlcs: 0,
@@ -551,7 +551,7 @@ impl Channel {
 
 	/// Creates a new channel from a remote sides' request for one.
 	/// Assumes chain_hash has already been checked and corresponds with what we expect!
-	pub fn new_from_req(fee_estimator: &FeeEstimator, keys_provider: &Arc<KeysInterface>, their_node_id: PublicKey, their_local_features: LocalFeatures, msg: &msgs::OpenChannel, user_id: u64, logger: Arc<Logger>, config: &UserConfig) -> Result<Channel, ChannelError> {
+	pub fn new_from_req(fee_estimator: &FeeEstimator, keys_provider: &Arc<KeysInterface<ChanKeySigner = ChanSigner>>, their_node_id: PublicKey, their_local_features: LocalFeatures, msg: &msgs::OpenChannel, user_id: u64, logger: Arc<Logger>, config: &UserConfig) -> Result<Channel<ChanSigner>, ChannelError> {
 		let chan_keys = keys_provider.get_channel_keys(true);
 		let mut local_config = (*config).channel_options.clone();
 
@@ -578,7 +578,7 @@ impl Channel {
 		if msg.htlc_minimum_msat >= (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 {
 			return Err(ChannelError::Close("Minimum htlc value is full channel value"));
 		}
-		Channel::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
+		Channel::<ChanSigner>::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
 
 		if msg.to_self_delay > config.peer_channel_config_limits.their_to_self_delay || msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT {
 			return Err(ChannelError::Close("They wanted our payments to be delayed by a needlessly long period"));
@@ -626,8 +626,8 @@ impl Channel {
 
 		let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
 
-		let our_dust_limit_satoshis = Channel::derive_our_dust_limit_satoshis(background_feerate);
+		let our_dust_limit_satoshis = Channel::<ChanSigner>::derive_our_dust_limit_satoshis(background_feerate);
-		let our_channel_reserve_satoshis = Channel::get_our_channel_reserve_satoshis(msg.funding_satoshis);
+		let our_channel_reserve_satoshis = Channel::<ChanSigner>::get_our_channel_reserve_satoshis(msg.funding_satoshis);
 		if our_channel_reserve_satoshis < our_dust_limit_satoshis {
 			return Err(ChannelError::Close("Suitable channel reserve not found. aborting"));
 		}
@@ -652,8 +652,8 @@ impl Channel {
 		}
 
 		let secp_ctx = Secp256k1::new();
-		let mut channel_monitor = ChannelMonitor::new(&chan_keys.revocation_base_key, &chan_keys.delayed_payment_base_key,
+		let mut channel_monitor = ChannelMonitor::new(chan_keys.revocation_base_key(), chan_keys.delayed_payment_base_key(),
-		                                              &chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), config.own_channel_config.our_to_self_delay,
+		                                              chan_keys.htlc_base_key(), chan_keys.payment_base_key(), &keys_provider.get_shutdown_pubkey(), config.own_channel_config.our_to_self_delay,
 		                                              keys_provider.get_destination_script(), logger.clone());
 		channel_monitor.set_their_base_keys(&msg.htlc_basepoint, &msg.delayed_payment_basepoint);
 		channel_monitor.set_their_to_self_delay(msg.to_self_delay);
@@ -732,7 +732,7 @@ impl Channel {
 			their_max_htlc_value_in_flight_msat: cmp::min(msg.max_htlc_value_in_flight_msat, msg.funding_satoshis * 1000),
 			their_channel_reserve_satoshis: msg.channel_reserve_satoshis,
 			their_htlc_minimum_msat: msg.htlc_minimum_msat,
-			our_htlc_minimum_msat: Channel::derive_our_htlc_minimum_msat(msg.feerate_per_kw as u64),
+			our_htlc_minimum_msat: Channel::<ChanSigner>::derive_our_htlc_minimum_msat(msg.feerate_per_kw as u64),
 			their_to_self_delay: msg.to_self_delay,
 			our_to_self_delay: config.own_channel_config.our_to_self_delay,
 			their_max_accepted_htlcs: msg.max_accepted_htlcs,
@@ -766,7 +766,7 @@ impl Channel {
 	// Utilities to derive keys:
 
 	fn build_local_commitment_secret(&self, idx: u64) -> SecretKey {
-		let res = chan_utils::build_commitment_secret(self.local_keys.commitment_seed, idx);
+		let res = chan_utils::build_commitment_secret(self.local_keys.commitment_seed(), idx);
 		SecretKey::from_slice(&res).unwrap()
 	}
 
@@ -774,7 +774,7 @@ impl Channel {
 
 	fn get_commitment_transaction_number_obscure_factor(&self) -> u64 {
 		let mut sha = Sha256::engine();
-		let our_payment_basepoint = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.payment_base_key);
+		let our_payment_basepoint = PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.payment_base_key());
 
 		if self.channel_outbound {
 			sha.input(&our_payment_basepoint.serialize());
@@ -833,7 +833,7 @@ impl Channel {
 		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) => {
@@ -952,7 +952,7 @@ impl Channel {
 			};
 			debug_assert!(max_commitment_tx_output.0 <= value_to_self_msat as u64 || value_to_self_msat / 1000 >= self.their_channel_reserve_satoshis as i64);
 			max_commitment_tx_output.0 = cmp::max(max_commitment_tx_output.0, value_to_self_msat as u64);
-			debug_assert!(max_commitment_tx_output.1 <= value_to_remote_msat as u64 || value_to_remote_msat / 1000 >= Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis) as i64);
+			debug_assert!(max_commitment_tx_output.1 <= value_to_remote_msat as u64 || value_to_remote_msat / 1000 >= Channel::<ChanSigner>::get_our_channel_reserve_satoshis(self.channel_value_satoshis) as i64);
 			max_commitment_tx_output.1 = cmp::max(max_commitment_tx_output.1, value_to_remote_msat as u64);
 		}
 
@@ -1097,8 +1097,8 @@ impl Channel {
 	/// TODO Some magic rust shit to compile-time check this?
 	fn build_local_transaction_keys(&self, commitment_number: u64) -> Result<TxCreationKeys, ChannelError> {
 		let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(commitment_number));
-		let delayed_payment_base = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.delayed_payment_base_key);
+		let delayed_payment_base = PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.delayed_payment_base_key());
-		let htlc_basepoint = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.htlc_base_key);
+		let htlc_basepoint = PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.htlc_base_key());
 
 		Ok(secp_check!(TxCreationKeys::new(&self.secp_ctx, &per_commitment_point, &delayed_payment_base, &htlc_basepoint, &self.their_revocation_basepoint.unwrap(), &self.their_payment_basepoint.unwrap(), &self.their_htlc_basepoint.unwrap()), "Local tx keys generation got bogus keys"))
 	}
@@ -1110,9 +1110,9 @@ impl Channel {
 	fn build_remote_transaction_keys(&self) -> Result<TxCreationKeys, ChannelError> {
 		//TODO: Ensure that the payment_key derived here ends up in the library users' wallet as we
 		//may see payments to it!
-		let payment_basepoint = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.payment_base_key);
+		let payment_basepoint = PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.payment_base_key());
-		let revocation_basepoint = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.revocation_base_key);
+		let revocation_basepoint = PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.revocation_base_key());
-		let htlc_basepoint = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.htlc_base_key);
+		let htlc_basepoint = PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.htlc_base_key());
 
 		Ok(secp_check!(TxCreationKeys::new(&self.secp_ctx, &self.their_cur_commitment_point.unwrap(), &self.their_delayed_payment_basepoint.unwrap(), &self.their_htlc_basepoint.unwrap(), &revocation_basepoint, &payment_basepoint, &htlc_basepoint), "Remote tx keys generation got bogus keys"))
 	}
@@ -1122,7 +1122,7 @@ impl Channel {
 	/// Panics if called before accept_channel/new_from_req
 	pub fn get_funding_redeemscript(&self) -> Script {
 		let builder = Builder::new().push_opcode(opcodes::all::OP_PUSHNUM_2);
-		let our_funding_key = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key).serialize();
+		let our_funding_key = PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()).serialize();
 		let their_funding_key = self.their_funding_pubkey.expect("get_funding_redeemscript only allowed after accept_channel").serialize();
 		if our_funding_key[..] < their_funding_key[..] {
 			builder.push_slice(&our_funding_key)
@@ -1144,11 +1144,11 @@ impl Channel {
 		let funding_redeemscript = self.get_funding_redeemscript();
 
 		let sighash = hash_to_message!(&bip143::SighashComponents::new(&tx).sighash_all(&tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]);
-		let our_sig = self.secp_ctx.sign(&sighash, &self.local_keys.funding_key);
+		let our_sig = self.secp_ctx.sign(&sighash, self.local_keys.funding_key());
 
 		tx.input[0].witness.push(Vec::new()); // First is the multisig dummy
 
-		let our_funding_key = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key).serialize();
+		let our_funding_key = PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()).serialize();
 		let their_funding_key = self.their_funding_pubkey.unwrap().serialize();
 		if our_funding_key[..] < their_funding_key[..] {
 			tx.input[0].witness.push(our_sig.serialize_der().to_vec());
@@ -1179,7 +1179,7 @@ impl Channel {
 
 		let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &keys);
 
-		let our_htlc_key = secp_check!(chan_utils::derive_private_key(&self.secp_ctx, &keys.per_commitment_point, &self.local_keys.htlc_base_key), "Derived invalid key, peer is maliciously selecting parameters");
+		let our_htlc_key = secp_check!(chan_utils::derive_private_key(&self.secp_ctx, &keys.per_commitment_point, self.local_keys.htlc_base_key()), "Derived invalid key, peer is maliciously selecting parameters");
 		let sighash = hash_to_message!(&bip143::SighashComponents::new(&tx).sighash_all(&tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]);
 		let is_local_tx = PublicKey::from_secret_key(&self.secp_ctx, &our_htlc_key) == keys.a_htlc_key;
 		Ok((htlc_redeemscript, self.secp_ctx.sign(&sighash, &our_htlc_key), is_local_tx))
@@ -1417,7 +1417,7 @@ impl Channel {
 		if msg.channel_reserve_satoshis < self.our_dust_limit_satoshis {
 			return Err(ChannelError::Close("Peer never wants payout outputs?"));
 		}
-		if msg.dust_limit_satoshis > Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis) {
+		if msg.dust_limit_satoshis > Channel::<ChanSigner>::get_our_channel_reserve_satoshis(self.channel_value_satoshis) {
 			return Err(ChannelError::Close("Dust limit is bigger than our channel reverse"));
 		}
 		if msg.htlc_minimum_msat >= (self.channel_value_satoshis - msg.channel_reserve_satoshis) * 1000 {
@@ -1518,10 +1518,11 @@ impl Channel {
 
 		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> {
@@ -1695,7 +1696,7 @@ impl Channel {
 			return Err(ChannelError::Close("Remote tried to push more than our max accepted HTLCs"));
 		}
 		// Check our_max_htlc_value_in_flight_msat
-		if htlc_inbound_value_msat + msg.amount_msat > Channel::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis) {
+		if htlc_inbound_value_msat + msg.amount_msat > Channel::<ChanSigner>::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis) {
 			return Err(ChannelError::Close("Remote HTLC add would put them over our max HTLC value"));
 		}
 		// Check our_channel_reserve_satoshis (we're getting paid, so they have to at least meet
@@ -1718,7 +1719,7 @@ impl Channel {
 				removed_outbound_total_msat += htlc.amount_msat;
 			}
 		}
-		if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 + removed_outbound_total_msat {
+		if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::<ChanSigner>::get_our_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 + removed_outbound_total_msat {
 			return Err(ChannelError::Close("Remote HTLC add would put them over their reserve value"));
 		}
 		if self.next_remote_htlc_id != msg.htlc_id {
@@ -1884,7 +1885,7 @@ impl Channel {
 		}
 
 		let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number - 1));
-		let per_commitment_secret = chan_utils::build_commitment_secret(self.local_keys.commitment_seed, self.cur_local_commitment_transaction_number + 1);
+		let per_commitment_secret = chan_utils::build_commitment_secret(self.local_keys.commitment_seed(), self.cur_local_commitment_transaction_number + 1);
 
 		// Update state now that we've passed all the can-fail calls...
 		let mut need_our_commitment = false;
@@ -2444,7 +2445,7 @@ impl Channel {
 		if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
 			return Err(ChannelError::Close("Peer sent update_fee when we needed a channel_reestablish"));
 		}
-		Channel::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
+		Channel::<ChanSigner>::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
 		self.pending_update_fee = Some(msg.feerate_per_kw as u64);
 		self.channel_update_count += 1;
 		Ok(())
@@ -2452,7 +2453,7 @@ impl Channel {
 
 	fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK {
 		let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number));
-		let per_commitment_secret = chan_utils::build_commitment_secret(self.local_keys.commitment_seed, self.cur_local_commitment_transaction_number + 2);
+		let per_commitment_secret = chan_utils::build_commitment_secret(self.local_keys.commitment_seed(), self.cur_local_commitment_transaction_number + 2);
 		msgs::RevokeAndACK {
 			channel_id: self.channel_id,
 			per_commitment_secret,
@@ -2535,7 +2536,7 @@ impl Channel {
 		if msg.next_remote_commitment_number > 0 {
 			match msg.data_loss_protect {
 				OptionalField::Present(ref data_loss) => {
-					if chan_utils::build_commitment_secret(self.local_keys.commitment_seed, INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1) != data_loss.your_last_per_commitment_secret {
+					if chan_utils::build_commitment_secret(self.local_keys.commitment_seed(), INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1) != data_loss.your_last_per_commitment_secret {
 						return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided"));
 					}
 					if msg.next_remote_commitment_number > INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
@@ -2671,7 +2672,7 @@ impl Channel {
 		Some(msgs::ClosingSigned {
 			channel_id: self.channel_id,
 			fee_satoshis: total_fee_satoshis,
-			signature: self.secp_ctx.sign(&sighash, &self.local_keys.funding_key),
+			signature: self.secp_ctx.sign(&sighash, &self.local_keys.funding_key()),
 		})
 	}
 
@@ -2794,7 +2795,7 @@ impl Channel {
 				let closing_tx_max_weight = Self::get_closing_transaction_weight(&self.get_closing_scriptpubkey(), self.their_shutdown_scriptpubkey.as_ref().unwrap());
 				let (closing_tx, used_total_fee) = self.build_closing_transaction($new_feerate * closing_tx_max_weight / 1000, false);
 				sighash = hash_to_message!(&bip143::SighashComponents::new(&closing_tx).sighash_all(&closing_tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]);
-				let our_sig = self.secp_ctx.sign(&sighash, &self.local_keys.funding_key);
+				let our_sig = self.secp_ctx.sign(&sighash, &self.local_keys.funding_key());
 				self.last_sent_closing_fee = Some(($new_feerate, used_total_fee));
 				return Ok((Some(msgs::ClosingSigned {
 					channel_id: self.channel_id,
@@ -2912,7 +2913,7 @@ impl Channel {
 	}
 
 	#[cfg(test)]
-	pub fn get_local_keys(&self) -> &ChannelKeys {
+	pub fn get_local_keys(&self) -> &ChanSigner {
 		&self.local_keys
 	}
 
@@ -3171,17 +3172,17 @@ impl Channel {
 			funding_satoshis: self.channel_value_satoshis,
 			push_msat: self.channel_value_satoshis * 1000 - self.value_to_self_msat,
 			dust_limit_satoshis: self.our_dust_limit_satoshis,
-			max_htlc_value_in_flight_msat: Channel::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis),
+			max_htlc_value_in_flight_msat: Channel::<ChanSigner>::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis),
-			channel_reserve_satoshis: Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis),
+			channel_reserve_satoshis: Channel::<ChanSigner>::get_our_channel_reserve_satoshis(self.channel_value_satoshis),
 			htlc_minimum_msat: self.our_htlc_minimum_msat,
 			feerate_per_kw: fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background) as u32,
 			to_self_delay: self.our_to_self_delay,
 			max_accepted_htlcs: OUR_MAX_HTLCS,
-			funding_pubkey: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key),
+			funding_pubkey: PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()),
-			revocation_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.revocation_base_key),
+			revocation_basepoint: PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.revocation_base_key()),
-			payment_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.payment_base_key),
+			payment_basepoint: PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.payment_base_key()),
-			delayed_payment_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.delayed_payment_base_key),
+			delayed_payment_basepoint: PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.delayed_payment_base_key()),
-			htlc_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.htlc_base_key),
+			htlc_basepoint: PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.htlc_base_key()),
 			first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret),
 			channel_flags: if self.config.announced_channel {1} else {0},
 			shutdown_scriptpubkey: OptionalField::Present(if self.config.commit_upfront_shutdown_pubkey { self.get_closing_scriptpubkey() } else { Builder::new().into_script() })
@@ -3204,17 +3205,17 @@ impl Channel {
 		msgs::AcceptChannel {
 			temporary_channel_id: self.channel_id,
 			dust_limit_satoshis: self.our_dust_limit_satoshis,
-			max_htlc_value_in_flight_msat: Channel::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis),
+			max_htlc_value_in_flight_msat: Channel::<ChanSigner>::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis),
-			channel_reserve_satoshis: Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis),
+			channel_reserve_satoshis: Channel::<ChanSigner>::get_our_channel_reserve_satoshis(self.channel_value_satoshis),
 			htlc_minimum_msat: self.our_htlc_minimum_msat,
 			minimum_depth: self.minimum_depth,
 			to_self_delay: self.our_to_self_delay,
 			max_accepted_htlcs: OUR_MAX_HTLCS,
-			funding_pubkey: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key),
+			funding_pubkey: PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()),
-			revocation_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.revocation_base_key),
+			revocation_basepoint: PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.revocation_base_key()),
-			payment_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.payment_base_key),
+			payment_basepoint: PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.payment_base_key()),
-			delayed_payment_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.delayed_payment_base_key),
+			delayed_payment_basepoint: PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.delayed_payment_base_key()),
-			htlc_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.htlc_base_key),
+			htlc_basepoint: PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.htlc_base_key()),
 			first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret),
 			shutdown_scriptpubkey: OptionalField::Present(if self.config.commit_upfront_shutdown_pubkey { self.get_closing_scriptpubkey() } else { Builder::new().into_script() })
 		}
@@ -3222,14 +3223,10 @@ impl Channel {
 
 	/// 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)[..]);
+		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))
-		// 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))
 	}
 
 	/// Updates channel state with knowledge of the funding transaction's txid/index, and generates
@@ -3300,7 +3297,7 @@ impl Channel {
 		}
 
 		let were_node_one = our_node_id.serialize()[..] < self.their_node_id.serialize()[..];
-		let our_bitcoin_key = PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key);
+		let our_bitcoin_key = PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key());
 
 		let msg = msgs::UnsignedChannelAnnouncement {
 			features: msgs::GlobalFeatures::new(),
@@ -3314,7 +3311,7 @@ impl Channel {
 		};
 
 		let msghash = hash_to_message!(&Sha256dHash::hash(&msg.encode()[..])[..]);
-		let sig = self.secp_ctx.sign(&msghash, &self.local_keys.funding_key);
+		let sig = self.secp_ctx.sign(&msghash, self.local_keys.funding_key());
 
 		Ok((msg, sig))
 	}
@@ -3519,8 +3516,6 @@ impl Channel {
 	/// 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 Channel {
 
 		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 (signature, htlc_signatures);
-		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 mut htlc_sigs = Vec::with_capacity(remote_commitment_tx.1);
+		{
-		for &(ref htlc, _) in remote_commitment_tx.2.iter() {
+			let mut htlcs = Vec::with_capacity(remote_commitment_tx.2.len());
-			if let Some(_) = htlc.transaction_output_index {
+			for &(ref htlc, _) in remote_commitment_tx.2.iter() {
-				let htlc_tx = self.build_htlc_transaction(&remote_commitment_txid, htlc, false, &remote_keys, feerate_per_kw);
+				htlcs.push(htlc);
-				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");
+			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)
-				htlc_sigs.push(self.secp_ctx.sign(&htlc_sighash, &our_htlc_key));
+				.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed"))?;
-				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()[..]));
+			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,
+			signature,
-			htlc_signatures: htlc_sigs,
+			htlc_signatures,
 		}, (remote_commitment_tx.0, remote_commitment_tx.2)))
 	}
 
@@ -3688,7 +3693,7 @@ impl<R: ::std::io::Read> Readable<R> for InboundHTLCRemovalReason {
 	}
 }
 
-impl Writeable for Channel {
+impl<ChanSigner: ChannelKeys + Writeable> Writeable for Channel<ChanSigner> {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
 		// Note that we write out as if remove_uncommitted_htlcs_and_mark_paused had just been
 		// called but include holding cell updates (and obviously we don't modify self).
@@ -3892,7 +3897,7 @@ impl Writeable for Channel {
 	}
 }
 
-impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
+impl<R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArgs<R, Arc<Logger>> for Channel<ChanSigner> {
 	fn read(reader: &mut R, logger: Arc<Logger>) -> Result<Self, DecodeError> {
 		let _ver: u8 = Readable::read(reader)?;
 		let min_ver: u8 = Readable::read(reader)?;
@@ -4152,7 +4157,7 @@ mod tests {
 	use ln::channel::MAX_FUNDING_SATOSHIS;
 	use ln::chan_utils;
 	use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
-	use chain::keysinterface::KeysInterface;
+	use chain::keysinterface::{InMemoryChannelKeys, KeysInterface};
 	use chain::transaction::OutPoint;
 	use util::config::UserConfig;
 	use util::test_utils;
@@ -4181,9 +4186,11 @@ mod tests {
 	}
 
 	struct Keys {
-		chan_keys: ChannelKeys,
+		chan_keys: InMemoryChannelKeys,
 	}
 	impl KeysInterface for Keys {
+		type ChanKeySigner = InMemoryChannelKeys;
+
 		fn get_node_secret(&self) -> SecretKey { panic!(); }
 		fn get_destination_script(&self) -> Script {
 			let secp_ctx = Secp256k1::signing_only();
@@ -4198,7 +4205,7 @@ mod tests {
 			PublicKey::from_secret_key(&secp_ctx, &channel_close_key)
 		}
 
-		fn get_channel_keys(&self, _inbound: bool) -> ChannelKeys { self.chan_keys.clone() }
+		fn get_channel_keys(&self, _inbound: bool) -> InMemoryChannelKeys { self.chan_keys.clone() }
 		fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) { panic!(); }
 		fn get_channel_id(&self) -> [u8; 32] { [0; 32] }
 	}
@@ -4210,7 +4217,7 @@ mod tests {
 		let logger : Arc<Logger> = Arc::new(test_utils::TestLogger::new());
 		let secp_ctx = Secp256k1::new();
 
-		let chan_keys = ChannelKeys {
+		let chan_keys = InMemoryChannelKeys {
 			funding_key: SecretKey::from_slice(&hex::decode("30ff4956bbdd3222d44cc5e8a1261dab1e07957bdac5ae88fe3261ef321f3749").unwrap()[..]).unwrap(),
 			payment_base_key: SecretKey::from_slice(&hex::decode("1111111111111111111111111111111111111111111111111111111111111111").unwrap()[..]).unwrap(),
 			delayed_payment_base_key: SecretKey::from_slice(&hex::decode("3333333333333333333333333333333333333333333333333333333333333333").unwrap()[..]).unwrap(),
@@ -4220,14 +4227,14 @@ mod tests {
 			revocation_base_key: SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(),
 			commitment_seed: [0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff],
 		};
-		assert_eq!(PublicKey::from_secret_key(&secp_ctx, &chan_keys.funding_key).serialize()[..],
+		assert_eq!(PublicKey::from_secret_key(&secp_ctx, chan_keys.funding_key()).serialize()[..],
 				hex::decode("023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb").unwrap()[..]);
-		let keys_provider: Arc<KeysInterface> = Arc::new(Keys { chan_keys });
+		let keys_provider: Arc<KeysInterface<ChanKeySigner = InMemoryChannelKeys>> = Arc::new(Keys { chan_keys });
 
 		let their_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let mut config = UserConfig::default();
 		config.channel_options.announced_channel = false;
-		let mut chan = Channel::new_outbound(&feeest, &keys_provider, their_node_id, 10000000, 100000, 42, Arc::clone(&logger), &config).unwrap(); // Nothing uses their network key in this test
+		let mut chan = Channel::<InMemoryChannelKeys>::new_outbound(&feeest, &keys_provider, their_node_id, 10000000, 100000, 42, Arc::clone(&logger), &config).unwrap(); // Nothing uses their network key in this test
 		chan.their_to_self_delay = 144;
 		chan.our_dust_limit_satoshis = 546;
 
@@ -4251,10 +4258,10 @@ mod tests {
 		// We can't just use build_local_transaction_keys here as the per_commitment_secret is not
 		// derived from a commitment_seed, so instead we copy it here and call
 		// build_commitment_transaction.
-		let delayed_payment_base = PublicKey::from_secret_key(&secp_ctx, &chan.local_keys.delayed_payment_base_key);
+		let delayed_payment_base = PublicKey::from_secret_key(&secp_ctx, chan.local_keys.delayed_payment_base_key());
 		let per_commitment_secret = SecretKey::from_slice(&hex::decode("1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100").unwrap()[..]).unwrap();
 		let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret);
-		let htlc_basepoint = PublicKey::from_secret_key(&secp_ctx, &chan.local_keys.htlc_base_key);
+		let htlc_basepoint = PublicKey::from_secret_key(&secp_ctx, chan.local_keys.htlc_base_key());
 		let keys = TxCreationKeys::new(&secp_ctx, &per_commitment_point, &delayed_payment_base, &htlc_basepoint, &chan.their_revocation_basepoint.unwrap(), &chan.their_payment_basepoint.unwrap(), &chan.their_htlc_basepoint.unwrap()).unwrap();
 
 		let mut unsigned_tx: (Transaction, Vec<HTLCOutputInCommitment>);
@@ -4699,21 +4706,21 @@ mod tests {
 
 		let mut seed = [0; 32];
 		seed[0..32].clone_from_slice(&hex::decode("0000000000000000000000000000000000000000000000000000000000000000").unwrap());
-		assert_eq!(chan_utils::build_commitment_secret(seed, 281474976710655),
+		assert_eq!(chan_utils::build_commitment_secret(&seed, 281474976710655),
 		           hex::decode("02a40c85b6f28da08dfdbe0926c53fab2de6d28c10301f8f7c4073d5e42e3148").unwrap()[..]);
 
 		seed[0..32].clone_from_slice(&hex::decode("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF").unwrap());
-		assert_eq!(chan_utils::build_commitment_secret(seed, 281474976710655),
+		assert_eq!(chan_utils::build_commitment_secret(&seed, 281474976710655),
 		           hex::decode("7cc854b54e3e0dcdb010d7a3fee464a9687be6e8db3be6854c475621e007a5dc").unwrap()[..]);
 
-		assert_eq!(chan_utils::build_commitment_secret(seed, 0xaaaaaaaaaaa),
+		assert_eq!(chan_utils::build_commitment_secret(&seed, 0xaaaaaaaaaaa),
 		           hex::decode("56f4008fb007ca9acf0e15b054d5c9fd12ee06cea347914ddbaed70d1c13a528").unwrap()[..]);
 
-		assert_eq!(chan_utils::build_commitment_secret(seed, 0x555555555555),
+		assert_eq!(chan_utils::build_commitment_secret(&seed, 0x555555555555),
 		           hex::decode("9015daaeb06dba4ccc05b91b2f73bd54405f2be9f217fbacd3c5ac2e62327d31").unwrap()[..]);
 
 		seed[0..32].clone_from_slice(&hex::decode("0101010101010101010101010101010101010101010101010101010101010101").unwrap());
-		assert_eq!(chan_utils::build_commitment_secret(seed, 1),
+		assert_eq!(chan_utils::build_commitment_secret(&seed, 1),
 		           hex::decode("915c75942a26bb3a433a8ce2cb0427c29ec6c1775cfc78328b57f6ba7bfeaa9c").unwrap()[..]);
 	}
 

lightning/src/ln/channelmanager.rs

@@ -34,7 +34,7 @@ use ln::msgs;
 use ln::msgs::LocalFeatures;
 use ln::onion_utils;
 use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError};
-use chain::keysinterface::KeysInterface;
+use chain::keysinterface::{ChannelKeys, KeysInterface};
 use util::config::UserConfig;
 use util::{byte_utils, events};
 use util::ser::{Readable, ReadableArgs, Writeable, Writer};
@@ -49,6 +49,8 @@ use std::sync::{Arc, Mutex, MutexGuard, RwLock};
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::time::Duration;
 
+const SIXTY_FIVE_ZEROS: [u8; 65] = [0; 65];
+
 // We hold various information about HTLC relay in the HTLC objects in Channel itself:
 //
 // Upon receipt of an HTLC from a peer, we'll give it a PendingHTLCStatus indicating if it should
@@ -254,8 +256,8 @@ pub(super) enum RAACommitmentOrder {
 }
 
 // Note this is only exposed in cfg(test):
-pub(super) struct ChannelHolder {
+pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
-	pub(super) by_id: HashMap<[u8; 32], Channel>,
+	pub(super) by_id: HashMap<[u8; 32], Channel<ChanSigner>>,
 	pub(super) short_to_id: HashMap<u64, [u8; 32]>,
 	/// short channel id -> forward infos. Key of 0 means payments received
 	/// Note that while this is held in the same mutex as the channels themselves, no consistency
@@ -272,15 +274,15 @@ pub(super) struct ChannelHolder {
 	/// for broadcast messages, where ordering isn't as strict).
 	pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
 }
-pub(super) struct MutChannelHolder<'a> {
+pub(super) struct MutChannelHolder<'a, ChanSigner: ChannelKeys + 'a> {
-	pub(super) by_id: &'a mut HashMap<[u8; 32], Channel>,
+	pub(super) by_id: &'a mut HashMap<[u8; 32], Channel<ChanSigner>>,
 	pub(super) short_to_id: &'a mut HashMap<u64, [u8; 32]>,
 	pub(super) forward_htlcs: &'a mut HashMap<u64, Vec<HTLCForwardInfo>>,
 	pub(super) claimable_htlcs: &'a mut HashMap<PaymentHash, Vec<(u64, HTLCPreviousHopData)>>,
 	pub(super) pending_msg_events: &'a mut Vec<events::MessageSendEvent>,
 }
-impl ChannelHolder {
+impl<ChanSigner: ChannelKeys> ChannelHolder<ChanSigner> {
-	pub(super) fn borrow_parts(&mut self) -> MutChannelHolder {
+	pub(super) fn borrow_parts(&mut self) -> MutChannelHolder<ChanSigner> {
 		MutChannelHolder {
 			by_id: &mut self.by_id,
 			short_to_id: &mut self.short_to_id,
@@ -324,7 +326,7 @@ const ERR: () = "You need at least 32 bit pointers (well, usize, but we'll assum
 /// spam due to quick disconnection/reconnection, updates are not sent until the channel has been
 /// offline for a full minute. In order to track this, you must call
 /// timer_chan_freshness_every_min roughly once per minute, though it doesn't have to be perfec.
-pub struct ChannelManager<'a> {
+pub struct ChannelManager<'a, ChanSigner: ChannelKeys> {
 	default_configuration: UserConfig,
 	genesis_hash: Sha256dHash,
 	fee_estimator: Arc<FeeEstimator>,
@@ -339,9 +341,9 @@ pub struct ChannelManager<'a> {
 	secp_ctx: Secp256k1<secp256k1::All>,
 
 	#[cfg(test)]
-	pub(super) channel_state: Mutex<ChannelHolder>,
+	pub(super) channel_state: Mutex<ChannelHolder<ChanSigner>>,
 	#[cfg(not(test))]
-	channel_state: Mutex<ChannelHolder>,
+	channel_state: Mutex<ChannelHolder<ChanSigner>>,
 	our_network_key: SecretKey,
 
 	pending_events: Mutex<Vec<events::Event>>,
@@ -350,7 +352,7 @@ pub struct ChannelManager<'a> {
 	/// Taken first everywhere where we are making changes before any other locks.
 	total_consistency_lock: RwLock<()>,
 
-	keys_manager: Arc<KeysInterface>,
+	keys_manager: Arc<KeysInterface<ChanKeySigner = ChanSigner>>,
 
 	logger: Arc<Logger>,
 }
@@ -581,7 +583,7 @@ macro_rules! maybe_break_monitor_err {
 	}
 }
 
-impl<'a> ChannelManager<'a> {
+impl<'a, ChanSigner: ChannelKeys> ChannelManager<'a, ChanSigner> {
 	/// Constructs a new ChannelManager to hold several channels and route between them.
 	///
 	/// This is the main "logic hub" for all channel-related actions, and implements
@@ -600,7 +602,7 @@ impl<'a> ChannelManager<'a> {
 	/// the ChannelManager as a listener to the BlockNotifier and call the BlockNotifier's
 	/// `block_(dis)connected` methods, which will notify all registered listeners in one
 	/// go.
-	pub fn new(network: Network, feeest: Arc<FeeEstimator>, monitor: Arc<ManyChannelMonitor + 'a>, tx_broadcaster: Arc<BroadcasterInterface>, logger: Arc<Logger>,keys_manager: Arc<KeysInterface>, config: UserConfig, current_blockchain_height: usize) -> Result<Arc<ChannelManager<'a>>, secp256k1::Error> {
+	pub fn new(network: Network, feeest: Arc<FeeEstimator>, monitor: Arc<ManyChannelMonitor + 'a>, tx_broadcaster: Arc<BroadcasterInterface>, logger: Arc<Logger>,keys_manager: Arc<KeysInterface<ChanKeySigner = ChanSigner>>, config: UserConfig, current_blockchain_height: usize) -> Result<Arc<ChannelManager<'a, ChanSigner>>, secp256k1::Error> {
 		let secp_ctx = Secp256k1::new();
 
 		let res = Arc::new(ChannelManager {
@@ -818,8 +820,7 @@ impl<'a> ChannelManager<'a> {
 		}
 	}
 
-	const ZERO:[u8; 65] = [0; 65];
+	fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> (PendingHTLCStatus, MutexGuard<ChannelHolder<ChanSigner>>) {
-	fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> (PendingHTLCStatus, MutexGuard<ChannelHolder>) {
 		macro_rules! return_malformed_err {
 			($msg: expr, $err_code: expr) => {
 				{
@@ -941,7 +942,7 @@ impl<'a> ChannelManager<'a> {
 			} else {
 				let mut new_packet_data = [0; 20*65];
 				chacha.process(&msg.onion_routing_packet.hop_data[65..], &mut new_packet_data[0..19*65]);
-				chacha.process(&ChannelManager::ZERO[..], &mut new_packet_data[19*65..]);
+				chacha.process(&SIXTY_FIVE_ZEROS[..], &mut new_packet_data[19*65..]);
 
 				let mut new_pubkey = msg.onion_routing_packet.public_key.unwrap();
 
@@ -1038,7 +1039,7 @@ impl<'a> ChannelManager<'a> {
 
 	/// only fails if the channel does not yet have an assigned short_id
 	/// May be called with channel_state already locked!
-	fn get_channel_update(&self, chan: &Channel) -> Result<msgs::ChannelUpdate, LightningError> {
+	fn get_channel_update(&self, chan: &Channel<ChanSigner>) -> Result<msgs::ChannelUpdate, LightningError> {
 		let short_channel_id = match chan.get_short_channel_id() {
 			None => return Err(LightningError{err: "Channel not yet established", action: msgs::ErrorAction::IgnoreError}),
 			Some(id) => id,
@@ -1267,7 +1268,7 @@ impl<'a> ChannelManager<'a> {
 		}
 	}
 
-	fn get_announcement_sigs(&self, chan: &Channel) -> Option<msgs::AnnouncementSignatures> {
+	fn get_announcement_sigs(&self, chan: &Channel<ChanSigner>) -> Option<msgs::AnnouncementSignatures> {
 		if !chan.should_announce() { return None }
 
 		let (announcement, our_bitcoin_sig) = match chan.get_channel_announcement(self.get_our_node_id(), self.genesis_hash.clone()) {
@@ -1561,7 +1562,7 @@ impl<'a> ChannelManager<'a> {
 	/// to fail and take the channel_state lock for each iteration (as we take ownership and may
 	/// drop it). In other words, no assumptions are made that entries in claimable_htlcs point to
 	/// still-available channels.
-	fn fail_htlc_backwards_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder>, source: HTLCSource, payment_hash: &PaymentHash, onion_error: HTLCFailReason) {
+	fn fail_htlc_backwards_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<ChanSigner>>, source: HTLCSource, payment_hash: &PaymentHash, onion_error: HTLCFailReason) {
 		//TODO: There is a timing attack here where if a node fails an HTLC back to us they can
 		//identify whether we sent it or not based on the (I presume) very different runtime
 		//between the branches here. We should make this async and move it into the forward HTLCs
@@ -1689,7 +1690,7 @@ impl<'a> ChannelManager<'a> {
 			true
 		} else { false }
 	}
-	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder>, source: HTLCSource, payment_preimage: PaymentPreimage) {
+	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<ChanSigner>>, source: HTLCSource, payment_preimage: PaymentPreimage) {
 		let (their_node_id, err) = loop {
 			match source {
 				HTLCSource::OutboundRoute { .. } => {
@@ -2566,7 +2567,7 @@ impl<'a> ChannelManager<'a> {
 	}
 }
 
-impl<'a> events::MessageSendEventsProvider for ChannelManager<'a> {
+impl<'a, ChanSigner: ChannelKeys> events::MessageSendEventsProvider for ChannelManager<'a, ChanSigner> {
 	fn get_and_clear_pending_msg_events(&self) -> Vec<events::MessageSendEvent> {
 		// TODO: Event release to users and serialization is currently race-y: it's very easy for a
 		// user to serialize a ChannelManager with pending events in it and lose those events on
@@ -2591,7 +2592,7 @@ impl<'a> events::MessageSendEventsProvider for ChannelManager<'a> {
 	}
 }
 
-impl<'a> events::EventsProvider for ChannelManager<'a> {
+impl<'a, ChanSigner: ChannelKeys> events::EventsProvider for ChannelManager<'a, ChanSigner> {
 	fn get_and_clear_pending_events(&self) -> Vec<events::Event> {
 		// TODO: Event release to users and serialization is currently race-y: it's very easy for a
 		// user to serialize a ChannelManager with pending events in it and lose those events on
@@ -2616,7 +2617,7 @@ impl<'a> events::EventsProvider for ChannelManager<'a> {
 	}
 }
 
-impl<'a> ChainListener for ChannelManager<'a> {
+impl<'a, ChanSigner: ChannelKeys> ChainListener for ChannelManager<'a, ChanSigner> {
 	fn block_connected(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[u32]) {
 		let header_hash = header.bitcoin_hash();
 		log_trace!(self, "Block {} at height {} connected with {} txn matched", header_hash, height, txn_matched.len());
@@ -2730,7 +2731,7 @@ impl<'a> ChainListener for ChannelManager<'a> {
 	}
 }
 
-impl<'a> ChannelMessageHandler for ChannelManager<'a> {
+impl<'a, ChanSigner: ChannelKeys> ChannelMessageHandler for ChannelManager<'a, ChanSigner> {
 	//TODO: Handle errors and close channel (or so)
 	fn handle_open_channel(&self, their_node_id: &PublicKey, their_local_features: LocalFeatures, msg: &msgs::OpenChannel) -> Result<(), LightningError> {
 		let _ = self.total_consistency_lock.read().unwrap();
@@ -3115,7 +3116,7 @@ impl<R: ::std::io::Read> Readable<R> for HTLCForwardInfo {
 	}
 }
 
-impl<'a> Writeable for ChannelManager<'a> {
+impl<'a, ChanSigner: ChannelKeys + Writeable> Writeable for ChannelManager<'a, ChanSigner> {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
 		let _ = self.total_consistency_lock.write().unwrap();
 
@@ -3178,10 +3179,10 @@ impl<'a> Writeable for ChannelManager<'a> {
 /// 5) Move the ChannelMonitors into your local ManyChannelMonitor.
 /// 6) Disconnect/connect blocks on the ChannelManager.
 /// 7) Register the new ChannelManager with your ChainWatchInterface.
-pub struct ChannelManagerReadArgs<'a, 'b> {
+pub struct ChannelManagerReadArgs<'a, 'b, ChanSigner: ChannelKeys> {
 	/// The keys provider which will give us relevant keys. Some keys will be loaded during
 	/// deserialization.
-	pub keys_manager: Arc<KeysInterface>,
+	pub keys_manager: Arc<KeysInterface<ChanKeySigner = ChanSigner>>,
 
 	/// The fee_estimator for use in the ChannelManager in the future.
 	///
@@ -3218,8 +3219,8 @@ pub struct ChannelManagerReadArgs<'a, 'b> {
 	pub channel_monitors: &'a HashMap<OutPoint, &'a ChannelMonitor>,
 }
 
-impl<'a, 'b, R : ::std::io::Read> ReadableArgs<R, ChannelManagerReadArgs<'a, 'b>> for (Sha256dHash, ChannelManager<'b>) {
+impl<'a, 'b, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>> ReadableArgs<R, ChannelManagerReadArgs<'a, 'b, ChanSigner>> for (Sha256dHash, ChannelManager<'b, ChanSigner>) {
-	fn read(reader: &mut R, args: ChannelManagerReadArgs<'a, 'b>) -> Result<Self, DecodeError> {
+	fn read(reader: &mut R, args: ChannelManagerReadArgs<'a, 'b, ChanSigner>) -> Result<Self, DecodeError> {
 		let _ver: u8 = Readable::read(reader)?;
 		let min_ver: u8 = Readable::read(reader)?;
 		if min_ver > SERIALIZATION_VERSION {
@@ -3237,7 +3238,7 @@ impl<'a, 'b, R : ::std::io::Read> ReadableArgs<R, ChannelManagerReadArgs<'a, 'b>
 		let mut by_id = HashMap::with_capacity(cmp::min(channel_count as usize, 128));
 		let mut short_to_id = HashMap::with_capacity(cmp::min(channel_count as usize, 128));
 		for _ in 0..channel_count {
-			let mut channel: Channel = ReadableArgs::read(reader, args.logger.clone())?;
+			let mut channel: Channel<ChanSigner> = ReadableArgs::read(reader, args.logger.clone())?;
 			if channel.last_block_connected != last_block_hash {
 				return Err(DecodeError::InvalidValue);
 			}

lightning/src/ln/functional_test_utils.rs

@@ -8,6 +8,7 @@ use ln::channelmanager::{ChannelManager,RAACommitmentOrder, PaymentPreimage, Pay
 use ln::router::{Route, Router};
 use ln::msgs;
 use ln::msgs::{ChannelMessageHandler,RoutingMessageHandler, LocalFeatures};
+use util::enforcing_trait_impls::EnforcingChannelKeys;
 use util::test_utils;
 use util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsProvider};
 use util::errors::APIError;
@@ -60,7 +61,7 @@ pub struct Node<'a, 'b: 'a> {
 	pub tx_broadcaster: Arc<test_utils::TestBroadcaster>,
 	pub chan_monitor: Arc<test_utils::TestChannelMonitor>,
 	pub keys_manager: Arc<test_utils::TestKeysInterface>,
-	pub node: Arc<ChannelManager<'b>>,
+	pub node: Arc<ChannelManager<'b, EnforcingChannelKeys>>,
 	pub router: Router,
 	pub node_seed: [u8; 32],
 	pub network_payment_count: Rc<RefCell<u8>>,

lightning/src/ln/functional_tests.rs

@@ -4,8 +4,7 @@
 
 use chain::transaction::OutPoint;
 use chain::chaininterface::{ChainListener, ChainWatchInterfaceUtil};
-use chain::keysinterface::{KeysInterface, SpendableOutputDescriptor, KeysManager};
+use chain::keysinterface::{KeysInterface, SpendableOutputDescriptor};
-use chain::keysinterface;
 use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
 use ln::channelmanager::{ChannelManager,ChannelManagerReadArgs,HTLCForwardInfo,RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT};
 use ln::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ManyChannelMonitor, ANTI_REORG_DELAY};
@@ -14,6 +13,7 @@ use ln::onion_utils;
 use ln::router::{Route, RouteHop};
 use ln::msgs;
 use ln::msgs::{ChannelMessageHandler,RoutingMessageHandler,HTLCFailChannelUpdate, LocalFeatures, ErrorAction};
+use util::enforcing_trait_impls::EnforcingChannelKeys;
 use util::test_utils;
 use util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsProvider};
 use util::errors::APIError;
@@ -57,7 +57,7 @@ fn test_insane_channel_opens() {
 	// Instantiate channel parameters where we push the maximum msats given our
 	// funding satoshis
 	let channel_value_sat = 31337; // same as funding satoshis
-	let channel_reserve_satoshis = Channel::get_our_channel_reserve_satoshis(channel_value_sat);
+	let channel_reserve_satoshis = Channel::<EnforcingChannelKeys>::get_our_channel_reserve_satoshis(channel_value_sat);
 	let push_msat = (channel_value_sat - channel_reserve_satoshis) * 1000;
 
 	// Have node0 initiate a channel to node1 with aforementioned parameters
@@ -3324,8 +3324,8 @@ fn test_invalid_channel_announcement() {
 
 	let _ = nodes[0].router.handle_htlc_fail_channel_update(&msgs::HTLCFailChannelUpdate::ChannelClosed { short_channel_id : as_chan.get_short_channel_id().unwrap(), is_permanent: false } );
 
-	let as_bitcoin_key = PublicKey::from_secret_key(&secp_ctx, &as_chan.get_local_keys().funding_key);
+	let as_bitcoin_key = PublicKey::from_secret_key(&secp_ctx, &as_chan.get_local_keys().inner.funding_key);
-	let bs_bitcoin_key = PublicKey::from_secret_key(&secp_ctx, &bs_chan.get_local_keys().funding_key);
+	let bs_bitcoin_key = PublicKey::from_secret_key(&secp_ctx, &bs_chan.get_local_keys().inner.funding_key);
 
 	let as_network_key = nodes[0].node.get_our_node_id();
 	let bs_network_key = nodes[1].node.get_our_node_id();
@@ -3352,8 +3352,8 @@ fn test_invalid_channel_announcement() {
 	macro_rules! sign_msg {
 		($unsigned_msg: expr) => {
 			let msghash = Message::from_slice(&Sha256dHash::hash(&$unsigned_msg.encode()[..])[..]).unwrap();
-			let as_bitcoin_sig = secp_ctx.sign(&msghash, &as_chan.get_local_keys().funding_key);
+			let as_bitcoin_sig = secp_ctx.sign(&msghash, &as_chan.get_local_keys().inner.funding_key);
-			let bs_bitcoin_sig = secp_ctx.sign(&msghash, &bs_chan.get_local_keys().funding_key);
+			let bs_bitcoin_sig = secp_ctx.sign(&msghash, &bs_chan.get_local_keys().inner.funding_key);
 			let as_node_sig = secp_ctx.sign(&msghash, &nodes[0].keys_manager.get_node_secret());
 			let bs_node_sig = secp_ctx.sign(&msghash, &nodes[1].keys_manager.get_node_secret());
 			chan_announcement = msgs::ChannelAnnouncement {
@@ -3406,7 +3406,7 @@ fn test_no_txn_manager_serialize_deserialize() {
 	let (_, nodes_0_deserialized) = {
 		let mut channel_monitors = HashMap::new();
 		channel_monitors.insert(chan_0_monitor.get_funding_txo().unwrap(), &chan_0_monitor);
-		<(Sha256dHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
+		<(Sha256dHash, ChannelManager<EnforcingChannelKeys>)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
 			default_config: config,
 			keys_manager,
 			fee_estimator: Arc::new(test_utils::TestFeeEstimator { sat_per_kw: 253 }),
@@ -3470,7 +3470,7 @@ fn test_simple_manager_serialize_deserialize() {
 	let (_, nodes_0_deserialized) = {
 		let mut channel_monitors = HashMap::new();
 		channel_monitors.insert(chan_0_monitor.get_funding_txo().unwrap(), &chan_0_monitor);
-		<(Sha256dHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
+		<(Sha256dHash, ChannelManager<EnforcingChannelKeys>)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
 			default_config: UserConfig::default(),
 			keys_manager,
 			fee_estimator: Arc::new(test_utils::TestFeeEstimator { sat_per_kw: 253 }),
@@ -3530,7 +3530,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() {
 
 	let mut nodes_0_read = &nodes_0_serialized[..];
 	let keys_manager = Arc::new(test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet, Arc::new(test_utils::TestLogger::new())));
-	let (_, nodes_0_deserialized) = <(Sha256dHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
+	let (_, nodes_0_deserialized) = <(Sha256dHash, ChannelManager<EnforcingChannelKeys>)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
 		default_config: UserConfig::default(),
 		keys_manager,
 		fee_estimator: Arc::new(test_utils::TestFeeEstimator { sat_per_kw: 253 }),
@@ -6067,7 +6067,7 @@ fn test_user_configurable_csv_delay() {
 	let nodes = create_network(2, &cfgs);
 
 	// We test config.our_to_self > BREAKDOWN_TIMEOUT is enforced in Channel::new_outbound()
-	let keys_manager: Arc<KeysInterface> = Arc::new(KeysManager::new(&nodes[0].node_seed, Network::Testnet, Arc::new(test_utils::TestLogger::new()), 10, 20));
+	let keys_manager: Arc<KeysInterface<ChanKeySigner = EnforcingChannelKeys>> = Arc::new(test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet, Arc::new(test_utils::TestLogger::new())));
 	if let Err(error) = Channel::new_outbound(&test_utils::TestFeeEstimator { sat_per_kw: 253 }, &keys_manager, nodes[1].node.get_our_node_id(), 1000000, 1000000, 0, Arc::new(test_utils::TestLogger::new()), &low_our_to_self_config) {
 		match error {
 			APIError::APIMisuseError { err } => { assert_eq!(err, "Configured with an unreasonable our_to_self_delay putting user funds at risks"); },
@@ -6142,8 +6142,8 @@ fn test_data_loss_protect() {
 	let monitor = Arc::new(test_utils::TestChannelMonitor::new(chain_monitor.clone(), tx_broadcaster.clone(), logger.clone(), feeest.clone()));
 	let mut channel_monitors = HashMap::new();
 	channel_monitors.insert(OutPoint { txid: chan.3.txid(), index: 0 }, &chan_monitor);
-	let node_state_0 = <(Sha256dHash, ChannelManager)>::read(&mut ::std::io::Cursor::new(previous_node_state), ChannelManagerReadArgs {
+	let node_state_0 = <(Sha256dHash, ChannelManager<EnforcingChannelKeys>)>::read(&mut ::std::io::Cursor::new(previous_node_state), ChannelManagerReadArgs {
-		keys_manager: Arc::new(keysinterface::KeysManager::new(&nodes[0].node_seed, Network::Testnet, Arc::clone(&logger), 42, 21)),
+		keys_manager: Arc::new(test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet, Arc::clone(&logger))),
 		fee_estimator: feeest.clone(),
 		monitor: monitor.clone(),
 		logger: Arc::clone(&logger),

lightning/src/ln/mod.rs

@@ -14,6 +14,7 @@ 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;
@@ -21,7 +22,6 @@ pub mod peer_channel_encryptor;
 pub(crate) mod peer_channel_encryptor;
 
 mod channel;
-mod chan_utils;
 mod onion_utils;
 
 #[cfg(test)]

lightning/src/util/enforcing_trait_impls.rs

@@ -0,0 +1,58 @@
+use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys};
+use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys};
+
+use std::cmp;
+use std::sync::Mutex;
+
+use bitcoin::blockdata::transaction::Transaction;
+use bitcoin::blockdata::script::Script;
+
+use secp256k1;
+use secp256k1::key::SecretKey;
+use secp256k1::{Secp256k1, Signature};
+
+/// Enforces some rules on ChannelKeys calls. Eventually we will probably want to expose a variant
+/// of this which would essentially be what you'd want to run on a hardware wallet.
+pub struct EnforcingChannelKeys {
+	pub inner: InMemoryChannelKeys,
+	commitment_number_obscure_and_last: Mutex<(Option<u64>, u64)>,
+}
+
+impl EnforcingChannelKeys {
+	pub fn new(inner: InMemoryChannelKeys) -> Self {
+		Self {
+			inner,
+			commitment_number_obscure_and_last: Mutex::new((None, 0)),
+		}
+	}
+}
+impl ChannelKeys for EnforcingChannelKeys {
+	fn funding_key(&self) -> &SecretKey { self.inner.funding_key() }
+	fn revocation_base_key(&self) -> &SecretKey { self.inner.revocation_base_key() }
+	fn payment_base_key(&self) -> &SecretKey { self.inner.payment_base_key() }
+	fn delayed_payment_base_key(&self) -> &SecretKey { self.inner.delayed_payment_base_key() }
+	fn htlc_base_key(&self) -> &SecretKey { self.inner.htlc_base_key() }
+	fn commitment_seed(&self) -> &[u8; 32] { self.inner.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 { panic!(); }
+		let obscured_commitment_transaction_number = (commitment_tx.lock_time & 0xffffff) as u64 | ((commitment_tx.input[0].sequence as u64 & 0xffffff) << 3*8);
+
+		{
+			let mut commitment_data = self.commitment_number_obscure_and_last.lock().unwrap();
+			if commitment_data.0.is_none() {
+				commitment_data.0 = Some(obscured_commitment_transaction_number ^ commitment_data.1);
+			}
+			let commitment_number = obscured_commitment_transaction_number ^ commitment_data.0.unwrap();
+			assert!(commitment_number == commitment_data.1 || commitment_number == commitment_data.1 + 1);
+			commitment_data.1 = cmp::max(commitment_number, commitment_data.1)
+		}
+
+		Ok(self.inner.sign_remote_commitment(channel_value_satoshis, channel_funding_script, feerate_per_kw, commitment_tx, keys, htlcs, to_self_delay, secp_ctx).unwrap())
+	}
+}
+
+impl_writeable!(EnforcingChannelKeys, 0, {
+	inner,
+	commitment_number_obscure_and_last
+});

lightning/src/util/mod.rs

@@ -23,5 +23,10 @@ pub mod config;
 #[cfg(test)]
 pub(crate) mod test_utils;
 
+/// impls of traits that add exra enforcement on the way they're called. Useful for detecting state
+/// 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;

lightning/src/util/ser.rs

@@ -5,6 +5,7 @@ use std::result::Result;
 use std::io::{Read, Write};
 use std::collections::HashMap;
 use std::hash::Hash;
+use std::sync::Mutex;
 
 use secp256k1::Signature;
 use secp256k1::key::{PublicKey, SecretKey};
@@ -442,3 +443,29 @@ impl<R: Read> Readable<R> for OutPoint {
 		})
 	}
 }
+
+impl<R: Read, T: Readable<R>> Readable<R> for Mutex<T> {
+	fn read(r: &mut R) -> Result<Self, DecodeError> {
+		let t: T = Readable::read(r)?;
+		Ok(Mutex::new(t))
+	}
+}
+impl<T: Writeable> Writeable for Mutex<T> {
+	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
+		self.lock().unwrap().write(w)
+	}
+}
+
+impl<R: Read, A: Readable<R>, B: Readable<R>> Readable<R> for (A, B) {
+	fn read(r: &mut R) -> Result<Self, DecodeError> {
+		let a: A = Readable::read(r)?;
+		let b: B = Readable::read(r)?;
+		Ok((a, b))
+	}
+}
+impl<A: Writeable, B: Writeable> Writeable for (A, B) {
+	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
+		self.0.write(w)?;
+		self.1.write(w)
+	}
+}

lightning/src/util/test_utils.rs

@@ -7,6 +7,7 @@ use ln::msgs;
 use ln::msgs::LocalFeatures;
 use ln::msgs::{LightningError};
 use ln::channelmonitor::HTLCUpdate;
+use util::enforcing_trait_impls::EnforcingChannelKeys;
 use util::events;
 use util::logger::{Logger, Level, Record};
 use util::ser::{ReadableArgs, Writer};
@@ -221,10 +222,12 @@ pub struct TestKeysInterface {
 }
 
 impl keysinterface::KeysInterface for TestKeysInterface {
+	type ChanKeySigner = EnforcingChannelKeys;
+
 	fn get_node_secret(&self) -> SecretKey { self.backing.get_node_secret() }
 	fn get_destination_script(&self) -> Script { self.backing.get_destination_script() }
 	fn get_shutdown_pubkey(&self) -> PublicKey { self.backing.get_shutdown_pubkey() }
-	fn get_channel_keys(&self, inbound: bool) -> keysinterface::ChannelKeys { self.backing.get_channel_keys(inbound) }
+	fn get_channel_keys(&self, inbound: bool) -> EnforcingChannelKeys { EnforcingChannelKeys::new(self.backing.get_channel_keys(inbound)) }
 
 	fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) {
 		match *self.override_session_priv.lock().unwrap() {