Make TxCreationKeys public and wrap it in PreCalculatedTxCreationKeys
authorDevrandom <c1.devrandom@niftybox.net>
Sun, 9 Aug 2020 13:45:23 +0000 (15:45 +0200)
committerDevrandom <c1.devrandom@niftybox.net>
Mon, 10 Aug 2020 18:21:07 +0000 (20:21 +0200)
Allows calling of InMemoryChannelKeys methods.

The wrapping makes it obvious to signer implementers that the pre-derived keys are a local cache and should not be trusted in a validating signer.

lightning/src/chain/keysinterface.rs
lightning/src/ln/chan_utils.rs
lightning/src/ln/channel.rs
lightning/src/ln/functional_tests.rs
lightning/src/util/enforcing_trait_impls.rs

index 26d1f07c7c43c38fc378d42431ffa2dde190ab92..63c87ad21f467cda0a0a528f6e56a7b1b5eaab4b 100644 (file)
@@ -23,7 +23,7 @@ use util::byte_utils;
 use util::ser::{Writeable, Writer, Readable};
 
 use ln::chan_utils;
-use ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, LocalCommitmentTransaction};
+use ln::chan_utils::{HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, LocalCommitmentTransaction, PreCalculatedTxCreationKeys};
 use ln::msgs;
 
 use std::sync::atomic::{AtomicUsize, Ordering};
@@ -223,7 +223,7 @@ pub trait ChannelKeys : Send+Clone {
        // 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 + secp256k1::Verification>(&self, feerate_per_kw: u32, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()>;
+       fn sign_remote_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, feerate_per_kw: u32, commitment_tx: &Transaction, keys: &PreCalculatedTxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()>;
 
        /// Create a signature for a local commitment transaction. This will only ever be called with
        /// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees
@@ -461,8 +461,9 @@ impl ChannelKeys for InMemoryChannelKeys {
        fn pubkeys(&self) -> &ChannelPublicKeys { &self.local_channel_pubkeys }
        fn key_derivation_params(&self) -> (u64, u64) { self.key_derivation_params }
 
-       fn sign_remote_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, feerate_per_kw: u32, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
+       fn sign_remote_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, feerate_per_kw: u32, commitment_tx: &Transaction, pre_keys: &PreCalculatedTxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
                if commitment_tx.input.len() != 1 { return Err(()); }
+               let keys = pre_keys.trust_key_derivation();
 
                let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key);
                let accepted_data = self.accepted_channel_data.as_ref().expect("must accept before signing");
index ba175733abeb632065d0cf16fe44ca13b6560b66..1e49782b85e30f5f0c30f4d67d93580c77d5bb7c 100644 (file)
@@ -267,23 +267,52 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
 
 /// 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.
+///
+/// These keys are assumed to be good, either because the code derived them from
+/// channel basepoints via the new function, or they were obtained via
+/// PreCalculatedTxCreationKeys.trust_key_derivation because we trusted the source of the
+/// pre-calculated keys.
 #[derive(PartialEq, Clone)]
 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(crate) revocation_key: PublicKey,
+       pub revocation_key: PublicKey,
        /// A's HTLC Key
-       pub(crate) a_htlc_key: PublicKey,
+       pub a_htlc_key: PublicKey,
        /// B's HTLC Key
-       pub(crate) b_htlc_key: PublicKey,
+       pub b_htlc_key: PublicKey,
        /// A's Payment Key (which isn't allowed to be spent from for some delay)
-       pub(crate) a_delayed_payment_key: PublicKey,
+       pub a_delayed_payment_key: PublicKey,
 }
 impl_writeable!(TxCreationKeys, 33*6,
        { per_commitment_point, revocation_key, a_htlc_key, b_htlc_key, a_delayed_payment_key });
 
+/// The per-commitment point and a set of pre-calculated public keys used for transaction creation
+/// in the signer.
+/// The pre-calculated keys are an optimization, because ChannelKeys has enough
+/// information to re-derive them.
+pub struct PreCalculatedTxCreationKeys(TxCreationKeys);
+
+impl PreCalculatedTxCreationKeys {
+       /// Create a new PreCalculatedTxCreationKeys from TxCreationKeys
+       pub fn new(keys: TxCreationKeys) -> Self {
+               PreCalculatedTxCreationKeys(keys)
+       }
+
+       /// The pre-calculated transaction creation public keys.
+       /// An external validating signer should not trust these keys.
+       pub fn trust_key_derivation(&self) -> &TxCreationKeys {
+               &self.0
+       }
+
+       /// The transaction per-commitment point
+       pub fn per_comitment_point(&self) -> &PublicKey {
+               &self.0.per_commitment_point
+       }
+}
+
 /// One counterparty's public keys which do not change over the life of a channel.
 #[derive(Clone, PartialEq)]
 pub struct ChannelPublicKeys {
@@ -318,7 +347,8 @@ impl_writeable!(ChannelPublicKeys, 33*5, {
 
 
 impl TxCreationKeys {
-       pub(crate) 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_htlc_base: &PublicKey) -> Result<TxCreationKeys, secp256k1::Error> {
+       /// Create a new TxCreationKeys from channel base points and the per-commitment point
+       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_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)?,
index 719d03c6e37dfa3816a5099057126b7e44560d32..1ae894f7fac86a036600b341ae92522830a7b8af 100644 (file)
@@ -19,7 +19,7 @@ use ln::msgs;
 use ln::msgs::{DecodeError, OptionalField, DataLossProtect};
 use ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER};
 use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT, MAX_LOCAL_BREAKDOWN_TIMEOUT};
-use ln::chan_utils::{CounterpartyCommitmentSecrets, LocalCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys};
+use ln::chan_utils::{CounterpartyCommitmentSecrets, LocalCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys, PreCalculatedTxCreationKeys};
 use ln::chan_utils;
 use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
 use chain::transaction::OutPoint;
@@ -1484,7 +1484,8 @@ 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, logger).0;
-               let remote_signature = self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), &self.secp_ctx)
+               let pre_remote_keys = PreCalculatedTxCreationKeys::new(remote_keys);
+               let remote_signature = self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &pre_remote_keys, &Vec::new(), &self.secp_ctx)
                                .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0;
 
                // We sign the "remote" commitment transaction, allowing them to broadcast the tx if they wish.
@@ -3525,7 +3526,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
        fn get_outbound_funding_created_signature<L: Deref>(&mut self, logger: &L) -> Result<Signature, ChannelError> where L::Target: Logger {
                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, logger).0;
-               Ok(self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), &self.secp_ctx)
+               let pre_remote_keys = PreCalculatedTxCreationKeys::new(remote_keys);
+               Ok(self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &pre_remote_keys, &Vec::new(), &self.secp_ctx)
                                .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0)
        }
 
@@ -3878,7 +3880,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                                htlcs.push(htlc);
                        }
 
-                       let res = self.local_keys.sign_remote_commitment(feerate_per_kw, &remote_commitment_tx.0, &remote_keys, &htlcs, &self.secp_ctx)
+                       let pre_remote_keys = PreCalculatedTxCreationKeys::new(remote_keys.clone());
+                       let res = self.local_keys.sign_remote_commitment(feerate_per_kw, &remote_commitment_tx.0, &pre_remote_keys, &htlcs, &self.secp_ctx)
                                .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?;
                        signature = res.0;
                        htlc_signatures = res.1;
index 240a5741b91c72fcc017254eac17038ee035909d..e1254ee771e9236b8ef94d2f4036c8dbfcd748f2 100644 (file)
@@ -52,6 +52,7 @@ use std::sync::atomic::Ordering;
 use std::{mem, io};
 
 use ln::functional_test_utils::*;
+use ln::chan_utils::PreCalculatedTxCreationKeys;
 
 #[test]
 fn test_insane_channel_opens() {
@@ -1694,7 +1695,8 @@ fn test_fee_spike_violation_fails_htlc() {
                let local_chan_lock = nodes[0].node.channel_state.lock().unwrap();
                let local_chan = local_chan_lock.by_id.get(&chan.2).unwrap();
                let local_chan_keys = local_chan.get_local_keys();
-               local_chan_keys.sign_remote_commitment(feerate_per_kw, &commit_tx, &commit_tx_keys, &[&accepted_htlc_info], &secp_ctx).unwrap()
+               let pre_commit_tx_keys = PreCalculatedTxCreationKeys::new(commit_tx_keys);
+               local_chan_keys.sign_remote_commitment(feerate_per_kw, &commit_tx, &pre_commit_tx_keys, &[&accepted_htlc_info], &secp_ctx).unwrap()
        };
 
        let commit_signed_msg = msgs::CommitmentSigned {
index 1864fffbbc2b008eb00279d9cb694b5de3fab814..9f297b4810318862b450f20c572e3da080575d34 100644 (file)
@@ -1,4 +1,4 @@
-use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys, LocalCommitmentTransaction};
+use ln::chan_utils::{HTLCOutputInCommitment, TxCreationKeys, ChannelPublicKeys, LocalCommitmentTransaction, PreCalculatedTxCreationKeys};
 use ln::{chan_utils, msgs};
 use chain::keysinterface::{ChannelKeys, InMemoryChannelKeys};
 
@@ -60,9 +60,9 @@ impl ChannelKeys for EnforcingChannelKeys {
        fn pubkeys(&self) -> &ChannelPublicKeys { self.inner.pubkeys() }
        fn key_derivation_params(&self) -> (u64, u64) { self.inner.key_derivation_params() }
 
-       fn sign_remote_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, feerate_per_kw: u32, commitment_tx: &Transaction, keys: &TxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
+       fn sign_remote_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, feerate_per_kw: u32, commitment_tx: &Transaction, pre_keys: &PreCalculatedTxCreationKeys, htlcs: &[&HTLCOutputInCommitment], secp_ctx: &Secp256k1<T>) -> Result<(Signature, Vec<Signature>), ()> {
                if commitment_tx.input.len() != 1 { panic!("lightning commitment transactions have a single input"); }
-               self.check_keys(secp_ctx, keys);
+               self.check_keys(secp_ctx, pre_keys.trust_key_derivation());
                let obscured_commitment_transaction_number = (commitment_tx.lock_time & 0xffffff) as u64 | ((commitment_tx.input[0].sequence as u64 & 0xffffff) << 3*8);
 
                {
@@ -75,7 +75,7 @@ impl ChannelKeys for EnforcingChannelKeys {
                        commitment_data.1 = cmp::max(commitment_number, commitment_data.1)
                }
 
-               Ok(self.inner.sign_remote_commitment(feerate_per_kw, commitment_tx, keys, htlcs, secp_ctx).unwrap())
+               Ok(self.inner.sign_remote_commitment(feerate_per_kw, commitment_tx, pre_keys, htlcs, secp_ctx).unwrap())
        }
 
        fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {