Remove unreachable `Err` cases on `derive_{public,private}_key`

The `derive_{public,private}_key` methods hash the two input keys
and then add them to the input public key. Because addition can
fail if the tweak is the inverse of the secret key this method
currently returns a `Result`.

However, it is not cryptographically possible to reach the error
case - in order to create an issue, the SHA-256 hash of the
`base_point` (and other data) must be the inverse of the
`base_point`('s secret key). Because changing the `base_point`
changes the hash in an unpredictable way, there should be no way to
construct such a `base_point`.

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 92f1a8df45d8e88bb75e9db80c7974fa89a430ec..a93985e094816ed153dcc51eca4e6f971c30eff1 100644 (file)
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2444,7 +2444,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
                        let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
                        let revocation_pubkey = ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint));
-                       let delayed_key = ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &self.counterparty_commitment_params.counterparty_delayed_payment_base_key));
+                       let delayed_key = chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &self.counterparty_commitment_params.counterparty_delayed_payment_base_key);
 
                        let revokeable_redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.counterparty_commitment_params.on_counterparty_tx_csv, &delayed_key);
                        let revokeable_p2wsh = revokeable_redeemscript.to_v0_p2wsh();
@@ -2560,17 +2560,12 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key(
                                        &self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint)
                                {
-                                       if let Ok(delayed_key) = chan_utils::derive_public_key(&self.secp_ctx,
+                                       let delayed_key = chan_utils::derive_public_key(&self.secp_ctx,
                                                &per_commitment_point,
-                                               &self.counterparty_commitment_params.counterparty_delayed_payment_base_key)
-                                       {
-                                               Some(chan_utils::get_revokeable_redeemscript(&revocation_pubkey,
-                                                       self.counterparty_commitment_params.on_counterparty_tx_csv,
-                                                       &delayed_key).to_v0_p2wsh())
-                                       } else {
-                                               debug_assert!(false, "Failed to derive a delayed payment key for a commitment state we accepted");
-                                               None
-                                       }
+                                               &self.counterparty_commitment_params.counterparty_delayed_payment_base_key);
+                                       Some(chan_utils::get_revokeable_redeemscript(&revocation_pubkey,
+                                               self.counterparty_commitment_params.on_counterparty_tx_csv,
+                                               &delayed_key).to_v0_p2wsh())
                                } else {
                                        debug_assert!(false, "Failed to derive a revocation pubkey key for a commitment state we accepted");
                                        None

diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 9f81976906cf7fa3b31961869ed4132435513d18..266544d7841508ec8550cae53d87b4daeb12e2a5 100644 (file)
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -655,8 +655,7 @@ impl InMemorySigner {
                if spend_tx.input[input_idx].previous_output != descriptor.outpoint.into_bitcoin_outpoint() { return Err(()); }
                if spend_tx.input[input_idx].sequence.0 != descriptor.to_self_delay as u32 { return Err(()); }
 
-               let delayed_payment_key = chan_utils::derive_private_key(&secp_ctx, &descriptor.per_commitment_point, &self.delayed_payment_base_key)
-                       .expect("We constructed the payment_base_key, so we can only fail here if the RNG is busted.");
+               let delayed_payment_key = chan_utils::derive_private_key(&secp_ctx, &descriptor.per_commitment_point, &self.delayed_payment_base_key);
                let delayed_payment_pubkey = PublicKey::from_secret_key(&secp_ctx, &delayed_payment_key);
                let witness_script = chan_utils::get_revokeable_redeemscript(&descriptor.revocation_pubkey, descriptor.to_self_delay, &delayed_payment_pubkey);
                let sighash = hash_to_message!(&sighash::SighashCache::new(spend_tx).segwit_signature_hash(input_idx, &witness_script, descriptor.output.value, EcdsaSighashType::All).unwrap()[..]);
@@ -710,7 +709,7 @@ impl BaseSign for InMemorySigner {
                        let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, self.opt_anchors(), &keys);
                        let htlc_sighashtype = if self.opt_anchors() { EcdsaSighashType::SinglePlusAnyoneCanPay } else { EcdsaSighashType::All };
                        let htlc_sighash = hash_to_message!(&sighash::SighashCache::new(&htlc_tx).segwit_signature_hash(0, &htlc_redeemscript, htlc.amount_msat / 1000, htlc_sighashtype).unwrap()[..]);
-                       let holder_htlc_key = chan_utils::derive_private_key(&secp_ctx, &keys.per_commitment_point, &self.htlc_base_key).map_err(|_| ())?;
+                       let holder_htlc_key = chan_utils::derive_private_key(&secp_ctx, &keys.per_commitment_point, &self.htlc_base_key);
                        htlc_sigs.push(sign(secp_ctx, &htlc_sighash, &holder_htlc_key));
                }
 
@@ -747,7 +746,7 @@ impl BaseSign for InMemorySigner {
                let per_commitment_point = PublicKey::from_secret_key(secp_ctx, &per_commitment_key);
                let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint).map_err(|_| ())?;
                let witness_script = {
-                       let counterparty_delayedpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().delayed_payment_basepoint).map_err(|_| ())?;
+                       let counterparty_delayedpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().delayed_payment_basepoint);
                        chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.holder_selected_contest_delay(), &counterparty_delayedpubkey)
                };
                let mut sighash_parts = sighash::SighashCache::new(justice_tx);
@@ -760,8 +759,8 @@ impl BaseSign for InMemorySigner {
                let per_commitment_point = PublicKey::from_secret_key(secp_ctx, &per_commitment_key);
                let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint).map_err(|_| ())?;
                let witness_script = {
-                       let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint).map_err(|_| ())?;
-                       let holder_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint).map_err(|_| ())?;
+                       let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint);
+                       let holder_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint);
                        chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &holder_htlcpubkey, &revocation_pubkey)
                };
                let mut sighash_parts = sighash::SighashCache::new(justice_tx);
@@ -770,19 +769,15 @@ impl BaseSign for InMemorySigner {
        }
 
        fn sign_counterparty_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
-               if let Ok(htlc_key) = chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &self.htlc_base_key) {
-                       let witness_script = if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint) {
-                               if let Ok(counterparty_htlcpubkey) = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint) {
-                                       if let Ok(htlcpubkey) = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint) {
-                                               chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &htlcpubkey, &revocation_pubkey)
-                                       } else { return Err(()) }
-                               } else { return Err(()) }
-                       } else { return Err(()) };
-                       let mut sighash_parts = sighash::SighashCache::new(htlc_tx);
-                       let sighash = hash_to_message!(&sighash_parts.segwit_signature_hash(input, &witness_script, amount, EcdsaSighashType::All).unwrap()[..]);
-                       return Ok(sign(secp_ctx, &sighash, &htlc_key))
-               }
-               Err(())
+               let htlc_key = chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &self.htlc_base_key);
+               let witness_script = if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint) {
+                       let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint);
+                       let htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint);
+                       chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &htlcpubkey, &revocation_pubkey)
+               } else { return Err(()) };
+               let mut sighash_parts = sighash::SighashCache::new(htlc_tx);
+               let sighash = hash_to_message!(&sighash_parts.segwit_signature_hash(input, &witness_script, amount, EcdsaSighashType::All).unwrap()[..]);
+               Ok(sign(secp_ctx, &sighash, &htlc_key))
        }
 
        fn sign_closing_transaction(&self, closing_tx: &ClosingTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {

diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index 3a6e81e6399c804bed7fac94a8526ddef809e7df..0dbf186840c32d2ed11ea94211fabfdefb4cfe9c 100644 (file)
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -316,32 +316,29 @@ impl Readable for CounterpartyCommitmentSecrets {
 
 /// Derives a per-commitment-transaction private key (eg an htlc key or delayed_payment key)
 /// from the base secret and the per_commitment_point.
-///
-/// Note that this is infallible iff we trust that at least one of the two input keys are randomly
-/// generated (ie our own).
-pub fn derive_private_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, base_secret: &SecretKey) -> Result<SecretKey, SecpError> {
+pub fn derive_private_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, base_secret: &SecretKey) -> SecretKey {
        let mut sha = Sha256::engine();
        sha.input(&per_commitment_point.serialize());
        sha.input(&PublicKey::from_secret_key(&secp_ctx, &base_secret).serialize());
        let res = Sha256::from_engine(sha).into_inner();
 
        base_secret.clone().add_tweak(&Scalar::from_be_bytes(res).unwrap())
+               .expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak contains the hash of the key.")
 }
 
 /// Derives a per-commitment-transaction public key (eg an htlc key or a delayed_payment key)
 /// from the base point and the per_commitment_key. This is the public equivalent of
 /// derive_private_key - using only public keys to derive a public key instead of private keys.
-///
-/// Note that this is infallible iff we trust that at least one of the two input keys are randomly
-/// generated (ie our own).
-pub fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, base_point: &PublicKey) -> Result<PublicKey, SecpError> {
+pub fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, base_point: &PublicKey) -> PublicKey {
        let mut sha = Sha256::engine();
        sha.input(&per_commitment_point.serialize());
        sha.input(&base_point.serialize());
        let res = Sha256::from_engine(sha).into_inner();
 
-       let hashkey = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&res)?);
+       let hashkey = PublicKey::from_secret_key(&secp_ctx,
+               &SecretKey::from_slice(&res).expect("Hashes should always be valid keys unless SHA-256 is broken"));
        base_point.combine(&hashkey)
+               .expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak contains the hash of the key.")
 }
 
 /// Derives a per-commitment-transaction revocation key from its constituent parts.
@@ -483,9 +480,9 @@ impl TxCreationKeys {
                Ok(TxCreationKeys {
                        per_commitment_point: per_commitment_point.clone(),
                        revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &countersignatory_revocation_base)?,
-                       broadcaster_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_htlc_base)?,
-                       countersignatory_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &countersignatory_htlc_base)?,
-                       broadcaster_delayed_payment_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_delayed_payment_base)?,
+                       broadcaster_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_htlc_base),
+                       countersignatory_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &countersignatory_htlc_base),
+                       broadcaster_delayed_payment_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_delayed_payment_base),
                })
        }
 
@@ -1506,7 +1503,7 @@ impl<'a> TrustedCommitmentTransaction<'a> {
                let keys = &inner.keys;
                let txid = inner.built.txid;
                let mut ret = Vec::with_capacity(inner.htlcs.len());
-               let holder_htlc_key = derive_private_key(secp_ctx, &inner.keys.per_commitment_point, htlc_base_key).map_err(|_| ())?;
+               let holder_htlc_key = derive_private_key(secp_ctx, &inner.keys.per_commitment_point, htlc_base_key);
 
                for this_htlc in inner.htlcs.iter() {
                        assert!(this_htlc.transaction_output_index.is_some());

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 5cc4f4a364bf3fe80ab15b896b51557a58acc5d1..0c0cbe83feef96c02b32309df7ac4c9089979b29 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -7962,10 +7962,10 @@ mod tests {
                let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret);
                assert_eq!(per_commitment_point.serialize()[..], hex::decode("025f7117a78150fe2ef97db7cfc83bd57b2e2c0d0dd25eaf467a4a1c2a45ce1486").unwrap()[..]);
 
-               assert_eq!(chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &base_point).unwrap().serialize()[..],
+               assert_eq!(chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &base_point).serialize()[..],
                                hex::decode("0235f2dbfaa89b57ec7b055afe29849ef7ddfeb1cefdb9ebdc43f5494984db29e5").unwrap()[..]);
 
-               assert_eq!(chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &base_secret).unwrap(),
+               assert_eq!(chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &base_secret),
                                SecretKey::from_slice(&hex::decode("cbced912d3b21bf196a766651e436aff192362621ce317704ea2f75d87e7be0f").unwrap()[..]).unwrap());
 
                assert_eq!(chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &base_point).unwrap().serialize()[..],