Remove unreachable `Err` cases on `derive_*_revocation_key`

The `derive_{public,private}_revocation_key` methods hash the two
input keys and then multiply the two input keys by hashed values
before adding them together. 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 point-multiplied-by-hash
values must be the inverse of each other, however each point
commits the SHA-256 hash of both keys together. Thus, because
changing either key changes the hashes (and the ultimate points
added together) in an unpredictable way, there should be no way to
construct such points.

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index a93985e094816ed153dcc51eca4e6f971c30eff1..cc88a0119fc95cf972680e7fcb4e5eb63495130c 100644 (file)
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2443,7 +2443,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        let secret = self.get_secret(commitment_number).unwrap();
                        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 revocation_pubkey = chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint);
                        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);
@@ -2556,26 +2556,18 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                        } else { return (claimable_outpoints, to_counterparty_output_info); };
 
                if let Some(transaction) = tx {
-                       let revokeable_p2wsh_opt =
-                               if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key(
-                                       &self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint)
-                               {
-                                       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 revocation pubkey key for a commitment state we accepted");
-                                       None
-                               };
-                       if let Some(revokeable_p2wsh) = revokeable_p2wsh_opt {
-                               for (idx, outp) in transaction.output.iter().enumerate() {
-                                       if outp.script_pubkey == revokeable_p2wsh {
-                                               to_counterparty_output_info =
-                                                       Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value));
-                                       }
+                       let revocation_pubkey = chan_utils::derive_public_revocation_key(
+                               &self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint);
+                       let delayed_key = chan_utils::derive_public_key(&self.secp_ctx,
+                               &per_commitment_point,
+                               &self.counterparty_commitment_params.counterparty_delayed_payment_base_key);
+                       let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(&revocation_pubkey,
+                               self.counterparty_commitment_params.on_counterparty_tx_csv,
+                               &delayed_key).to_v0_p2wsh();
+                       for (idx, outp) in transaction.output.iter().enumerate() {
+                               if outp.script_pubkey == revokeable_p2wsh {
+                                       to_counterparty_output_info =
+                                               Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value));
                                }
                        }
                }

diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 266544d7841508ec8550cae53d87b4daeb12e2a5..db1c6e943716c4a6c0cc2060e48f6646a5ef175c 100644 (file)
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -742,9 +742,9 @@ impl BaseSign for InMemorySigner {
        }
 
        fn sign_justice_revoked_output(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
-               let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key).map_err(|_| ())?;
+               let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key);
                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 revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint);
                let witness_script = {
                        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)
@@ -755,9 +755,9 @@ impl BaseSign for InMemorySigner {
        }
 
        fn sign_justice_revoked_htlc(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
-               let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key).map_err(|_| ())?;
+               let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key);
                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 revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint);
                let witness_script = {
                        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);
@@ -770,11 +770,10 @@ 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, ()> {
                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 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);
+               let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &htlcpubkey, &revocation_pubkey);
                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))

diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index 0dbf186840c32d2ed11ea94211fabfdefb4cfe9c..0b9381d2d4381981c37ce9643901b6fb5e007ce2 100644 (file)
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -347,10 +347,9 @@ pub fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_com
 /// commitment transaction, thus per_commitment_secret always come from cheater
 /// and revocation_base_secret always come from punisher, which is the broadcaster
 /// of the transaction spending with this key knowledge.
-///
-/// 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_revocation_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_secret: &SecretKey, countersignatory_revocation_base_secret: &SecretKey) -> Result<SecretKey, SecpError> {
+pub fn derive_private_revocation_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>,
+       per_commitment_secret: &SecretKey, countersignatory_revocation_base_secret: &SecretKey)
+-> SecretKey {
        let countersignatory_revocation_base_point = PublicKey::from_secret_key(&secp_ctx, &countersignatory_revocation_base_secret);
        let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret);
 
@@ -369,9 +368,12 @@ pub fn derive_private_revocation_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1
                Sha256::from_engine(sha).into_inner()
        };
 
-       let countersignatory_contrib = countersignatory_revocation_base_secret.clone().mul_tweak(&Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap())?;
-       let broadcaster_contrib = per_commitment_secret.clone().mul_tweak(&Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap())?;
+       let countersignatory_contrib = countersignatory_revocation_base_secret.clone().mul_tweak(&Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap())
+               .expect("Multiplying a secret key by a hash is expected to never fail per secp256k1 docs");
+       let broadcaster_contrib = per_commitment_secret.clone().mul_tweak(&Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap())
+               .expect("Multiplying a secret key by a hash is expected to never fail per secp256k1 docs");
        countersignatory_contrib.add_tweak(&Scalar::from_be_bytes(broadcaster_contrib.secret_bytes()).unwrap())
+               .expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak commits to the key.")
 }
 
 /// Derives a per-commitment-transaction revocation public key from its constituent parts. This is
@@ -385,7 +387,9 @@ pub fn derive_private_revocation_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1
 ///
 /// 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_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, countersignatory_revocation_base_point: &PublicKey) -> Result<PublicKey, SecpError> {
+pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp256k1<T>,
+       per_commitment_point: &PublicKey, countersignatory_revocation_base_point: &PublicKey)
+-> PublicKey {
        let rev_append_commit_hash_key = {
                let mut sha = Sha256::engine();
                sha.input(&countersignatory_revocation_base_point.serialize());
@@ -401,9 +405,12 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
                Sha256::from_engine(sha).into_inner()
        };
 
-       let countersignatory_contrib = countersignatory_revocation_base_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap())?;
-       let broadcaster_contrib = per_commitment_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap())?;
+       let countersignatory_contrib = countersignatory_revocation_base_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap())
+               .expect("Multiplying a valid public key by a hash is expected to never fail per secp256k1 docs");
+       let broadcaster_contrib = per_commitment_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap())
+               .expect("Multiplying a valid public key by a hash is expected to never fail per secp256k1 docs");
        countersignatory_contrib.combine(&broadcaster_contrib)
+               .expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak commits to the key.")
 }
 
 /// The set of public keys which are used in the creation of one commitment transaction.
@@ -479,7 +486,7 @@ impl TxCreationKeys {
        pub fn derive_new<T: secp256k1::Signing + secp256k1::Verification>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, broadcaster_delayed_payment_base: &PublicKey, broadcaster_htlc_base: &PublicKey, countersignatory_revocation_base: &PublicKey, countersignatory_htlc_base: &PublicKey) -> Result<TxCreationKeys, SecpError> {
                Ok(TxCreationKeys {
                        per_commitment_point: per_commitment_point.clone(),
-                       revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &countersignatory_revocation_base)?,
+                       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),

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 0c0cbe83feef96c02b32309df7ac4c9089979b29..ef7c77f0b1fb1cd89bcbf3cdf8a4d9df1fcbbf5e 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -7968,10 +7968,10 @@ mod tests {
                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()[..],
+               assert_eq!(chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &base_point).serialize()[..],
                                hex::decode("02916e326636d19c33f13e8c0c3a03dd157f332f3e99c317c141dd865eb01f8ff0").unwrap()[..]);
 
-               assert_eq!(chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_secret, &base_secret).unwrap(),
+               assert_eq!(chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_secret, &base_secret),
                                SecretKey::from_slice(&hex::decode("d09ffff62ddb2297ab000cc85bcb4283fdeb6aa052affbc9dddcf33b61078110").unwrap()[..]).unwrap());
        }