Various crypto cleanups (MystenLabs#2523)

* Make VerificationObligation::messages private

* Move quorum sig verification to crypto

* Rename add_tx_sig_to_verification_obligation

* Use obligation in SignedTransaction and make expanded_keys private

* Always use verify flow

* Move Transaction verify to the right struct

* Make obligation functions private

* Add add_to_verification_obligation to AuthoritySignInfo

* comments

crates/sui-core/src/authority.rs

@@ -332,7 +332,7 @@ impl AuthorityState {
     ) -> Result<TransactionInfoResponse, SuiError> {
         self.metrics.tx_orders.inc();
         // Check the sender's signature.
-        transaction.verify_signature().map_err(|e| {
+        transaction.verify().map_err(|e| {
             self.metrics.signature_errors.inc();
             e
         })?;

crates/sui-core/src/authority/authority_store.rs

@@ -1243,11 +1243,10 @@ pub async fn generate_genesis_system_object<
     let genesis_digest = genesis_ctx.digest();
     let mut temporary_store =
         AuthorityTemporaryStore::new(store.clone(), InputObjects::new(vec![]), genesis_digest);
-    let pubkeys: Vec<Vec<u8>> = committee
-        .expanded_keys
-        .values()
-        .map(|pk| pk.to_bytes().to_vec())
-        .collect();
+    let mut pubkeys = Vec::new();
+    for name in committee.voting_rights.keys() {
+        pubkeys.push(committee.public_key(name)?.to_bytes().to_vec());
+    }
     // TODO: May use separate sui address than derived from pubkey.
     let sui_addresses: Vec<AccountAddress> = committee
         .voting_rights

crates/sui-core/src/authority_server.rs

@@ -21,7 +21,7 @@ use sui_network::{
     tonic,
 };
 
-use sui_types::{crypto::VerificationObligation, error::*, messages::*};
+use sui_types::{error::*, messages::*};
 use tokio::{
     sync::mpsc::{channel, Sender},
     task::JoinHandle,
@@ -244,12 +244,8 @@ impl Validator for ValidatorService {
     ) -> Result<tonic::Response<TransactionInfoResponse>, tonic::Status> {
         let mut transaction = request.into_inner();
 
-        let mut obligation = VerificationObligation::default();
         transaction
-            .add_tx_sig_to_verification_obligation(&mut obligation)
-            .map_err(|e| tonic::Status::invalid_argument(e.to_string()))?;
-        obligation
-            .verify_all()
+            .verify()
             .map_err(|e| tonic::Status::invalid_argument(e.to_string()))?;
         //TODO This is really really bad, we should have different types for signature-verified transactions
         transaction.is_verified = true;
@@ -279,12 +275,8 @@ impl Validator for ValidatorService {
     ) -> Result<tonic::Response<TransactionInfoResponse>, tonic::Status> {
         let mut transaction = request.into_inner();
 
-        let mut obligation = VerificationObligation::default();
         transaction
-            .add_to_verification_obligation(&self.state.committee.load(), &mut obligation)
-            .map_err(|e| tonic::Status::invalid_argument(e.to_string()))?;
-        obligation
-            .verify_all()
+            .verify(&self.state.committee.load())
             .map_err(|e| tonic::Status::invalid_argument(e.to_string()))?;
         //TODO This is really really bad, we should have different types for signature verified transactions
         transaction.is_verified = true;

crates/sui-core/src/gateway_state.rs

@@ -521,7 +521,7 @@ where
         transaction: Transaction,
         is_last_retry: bool,
     ) -> Result<(CertifiedTransaction, CertifiedTransactionEffects), anyhow::Error> {
-        transaction.verify_signature()?;
+        transaction.verify()?;
 
         self.sync_input_objects_with_authorities(&transaction)
             .await?;

crates/sui-types/src/committee.rs

@@ -3,6 +3,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use super::base_types::*;
+use crate::error::SuiResult;
 use ed25519_dalek::PublicKey;
 use itertools::Itertools;
 use rand::distributions::{Distribution, Uniform};
@@ -22,14 +23,16 @@ pub struct Committee {
     pub total_votes: StakeUnit,
     // Note: this is a derived structure, no need to store.
     #[serde(skip)]
-    pub expanded_keys: HashMap<AuthorityName, PublicKey>,
+    expanded_keys: HashMap<AuthorityName, PublicKey>,
 }
 
 impl Committee {
     pub fn new(epoch: EpochId, voting_rights: BTreeMap<AuthorityName, StakeUnit>) -> Self {
         let total_votes = voting_rights.iter().map(|(_, votes)| votes).sum();
         let expanded_keys: HashMap<_, _> = voting_rights
             .iter()
+            // TODO: Verify all code path to make sure we always have valid public keys.
+            // e.g. when a new validator is registering themself on-chain.
             .map(|(addr, _)| (*addr, (*addr).try_into().expect("Invalid Authority Key")))
             .collect();
         Committee {
@@ -44,6 +47,13 @@ impl Committee {
         self.epoch
     }
 
+    pub fn public_key(&self, authority: &AuthorityName) -> SuiResult<PublicKey> {
+        match self.expanded_keys.get(authority) {
+            Some(v) => Ok(*v),
+            None => (*authority).try_into(),
+        }
+    }
+
     /// Samples authorities by weight
     pub fn sample(&self) -> &AuthorityName {
         // Uniform number [0, total_votes) non-inclusive of the upper bound

crates/sui-types/src/crypto.rs

@@ -1,7 +1,7 @@
 // Copyright (c) 2022, Mysten Labs, Inc.
 // SPDX-License-Identifier: Apache-2.0
 use crate::base_types::{AuthorityName, SuiAddress};
-use crate::committee::EpochId;
+use crate::committee::{Committee, EpochId};
 use crate::error::{SuiError, SuiResult};
 use crate::sui_serde::Base64;
 use crate::sui_serde::Readable;
@@ -20,7 +20,7 @@ use serde_with::serde_as;
 use serde_with::Bytes;
 use sha3::Sha3_256;
 use std::borrow::Borrow;
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::hash::{Hash, Hasher};
 use std::str::FromStr;
 
@@ -481,6 +481,22 @@ impl PartialEq for AuthoritySignInfo {
     }
 }
 
+impl AuthoritySignInfo {
+    pub fn add_to_verification_obligation(
+        &self,
+        committee: &Committee,
+        obligation: &mut VerificationObligation,
+        message_index: usize,
+    ) -> SuiResult<()> {
+        obligation
+            .public_keys
+            .push(committee.public_key(&self.authority)?);
+        obligation.signatures.push(self.signature.0);
+        obligation.message_index.push(message_index);
+        Ok(())
+    }
+}
+
 /// Represents at least a quorum (could be more) of authority signatures.
 #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
 pub struct AuthorityQuorumSignInfo {
@@ -501,6 +517,53 @@ pub struct AuthorityQuorumSignInfo {
 static_assertions::assert_not_impl_any!(AuthorityQuorumSignInfo: Hash, Eq, PartialEq);
 impl AuthoritySignInfoTrait for AuthorityQuorumSignInfo {}
 
+impl AuthorityQuorumSignInfo {
+    pub fn add_to_verification_obligation(
+        &self,
+        committee: &Committee,
+        obligation: &mut VerificationObligation,
+        message_index: usize,
+    ) -> SuiResult<()> {
+        // Check epoch
+        fp_ensure!(
+            self.epoch == committee.epoch(),
+            SuiError::WrongEpoch {
+                expected_epoch: committee.epoch()
+            }
+        );
+
+        let mut weight = 0;
+        let mut used_authorities = HashSet::new();
+
+        // Create obligations for the committee signatures
+        for (authority, signature) in self.signatures.iter() {
+            // Check that each authority only appears once.
+            fp_ensure!(
+                !used_authorities.contains(authority),
+                SuiError::CertificateAuthorityReuse
+            );
+            used_authorities.insert(*authority);
+            // Update weight.
+            let voting_rights = committee.weight(authority);
+            fp_ensure!(voting_rights > 0, SuiError::UnknownSigner);
+            weight += voting_rights;
+
+            obligation
+                .public_keys
+                .push(committee.public_key(authority)?);
+            obligation.signatures.push(signature.0);
+            obligation.message_index.push(message_index);
+        }
+
+        fp_ensure!(
+            weight >= committee.quorum_threshold(),
+            SuiError::CertificateRequiresQuorum
+        );
+
+        Ok(())
+    }
+}
+
 mod private {
     pub trait SealedAuthoritySignInfoTrait {}
     impl SealedAuthoritySignInfoTrait for super::EmptySignInfo {}
@@ -560,7 +623,7 @@ pub fn sha3_hash<S: Signable<Sha3_256>>(signable: &S) -> [u8; 32] {
 #[derive(Default)]
 pub struct VerificationObligation {
     lookup: PubKeyLookup,
-    pub messages: Vec<Vec<u8>>,
+    messages: Vec<Vec<u8>>,
     pub message_index: Vec<usize>,
     pub signatures: Vec<dalek::Signature>,
     pub public_keys: Vec<dalek::PublicKey>,
@@ -588,6 +651,14 @@ impl VerificationObligation {
         }
     }
 
+    /// Add a new message to the list of messages to be verified.
+    /// Returns the index of the message.
+    pub fn add_message(&mut self, message: Vec<u8>) -> usize {
+        let idx = self.messages.len();
+        self.messages.push(message);
+        idx
+    }
+
     pub fn verify_all(self) -> SuiResult<PubKeyLookup> {
         let messages_inner: Vec<_> = self
             .message_index

crates/sui-types/src/messages.rs

@@ -470,6 +470,8 @@ pub struct TransactionEnvelope<S> {
     #[serde(skip)]
     transaction_digest: OnceCell<TransactionDigest>,
     // Deserialization sets this to "false"
+    // TODO: is_verified is only set to true in some callsites after verification.
+    // Hence it's not optimal.
     #[serde(skip)]
     pub is_verified: bool,
 
@@ -484,13 +486,7 @@ pub struct TransactionEnvelope<S> {
 }
 
 impl<S> TransactionEnvelope<S> {
-    pub fn verify_signature(&self) -> Result<(), SuiError> {
-        let mut obligation = VerificationObligation::default();
-        self.add_tx_sig_to_verification_obligation(&mut obligation)?;
-        obligation.verify_all().map(|_| ())
-    }
-
-    pub fn add_tx_sig_to_verification_obligation(
+    fn add_sender_sig_to_verification_obligation(
         &self,
         obligation: &mut VerificationObligation,
     ) -> SuiResult<()> {
@@ -505,8 +501,7 @@ impl<S> TransactionEnvelope<S> {
         let (message, signature, public_key) = self
             .tx_signature
             .get_verification_inputs(&self.data, self.data.sender)?;
-        let idx = obligation.messages.len();
-        obligation.messages.push(message);
+        let idx = obligation.add_message(message);
         let key = obligation.lookup_public_key(&public_key)?;
         obligation.public_keys.push(key);
         obligation.signatures.push(signature);
@@ -620,6 +615,12 @@ impl Transaction {
             auth_sign_info: EmptySignInfo {},
         }
     }
+
+    pub fn verify(&self) -> Result<(), SuiError> {
+        let mut obligation = VerificationObligation::default();
+        self.add_sender_sig_to_verification_obligation(&mut obligation)?;
+        obligation.verify_all().map(|_| ())
+    }
 }
 
 impl Hash for Transaction {
@@ -695,12 +696,17 @@ impl SignedTransaction {
 
     /// Verify the signature and return the non-zero voting right of the authority.
     pub fn verify(&self, committee: &Committee) -> Result<u64, SuiError> {
-        self.verify_signature()?;
+        let mut obligation = VerificationObligation::default();
+        self.add_sender_sig_to_verification_obligation(&mut obligation)?;
         let weight = committee.weight(&self.auth_sign_info.authority);
         fp_ensure!(weight > 0, SuiError::UnknownSigner);
+        let mut message = Vec::new();
+        self.data.write(&mut message);
+        let idx = obligation.add_message(message);
         self.auth_sign_info
-            .signature
-            .verify(&self.data, self.auth_sign_info.authority)?;
+            .add_to_verification_obligation(committee, &mut obligation, idx)?;
+
+        obligation.verify_all()?;
         Ok(weight)
     }
 
@@ -1174,7 +1180,7 @@ pub struct SignatureAggregator<'a> {
 impl<'a> SignatureAggregator<'a> {
     /// Start aggregating signatures for the given value into a certificate.
     pub fn try_new(transaction: Transaction, committee: &'a Committee) -> Result<Self, SuiError> {
-        transaction.verify_signature()?;
+        transaction.verify()?;
         Ok(Self::new_unsafe(transaction, committee))
     }
 
@@ -1259,70 +1265,21 @@ impl CertifiedTransaction {
         obligation.verify_all().map(|_| ())
     }
 
-    pub fn add_to_verification_obligation(
+    fn add_to_verification_obligation(
         &self,
         committee: &Committee,
         obligation: &mut VerificationObligation,
     ) -> SuiResult<()> {
-        // Check epoch
-        fp_ensure!(
-            self.auth_sign_info.epoch == committee.epoch(),
-            SuiError::WrongEpoch {
-                expected_epoch: committee.epoch()
-            }
-        );
-
-        // First check the quorum is sufficient
-
-        let mut weight = 0;
-        let mut used_authorities = HashSet::new();
-        for (authority, _) in self.auth_sign_info.signatures.iter() {
-            // Check that each authority only appears once.
-            fp_ensure!(
-                !used_authorities.contains(authority),
-                SuiError::CertificateAuthorityReuse
-            );
-            used_authorities.insert(*authority);
-            // Update weight.
-            let voting_rights = committee.weight(authority);
-            fp_ensure!(voting_rights > 0, SuiError::UnknownSigner);
-            weight += voting_rights;
-        }
-        fp_ensure!(
-            weight >= committee.quorum_threshold(),
-            SuiError::CertificateRequiresQuorum
-        );
-
-        // Add the obligation of the transaction
-        self.add_tx_sig_to_verification_obligation(obligation)?;
-
-        // Create obligations for the committee signatures
+        // Add the obligation of the sender signature verification.
+        self.add_sender_sig_to_verification_obligation(obligation)?;
 
+        // Add the obligation of the authority signature verifications.
         let mut message = Vec::new();
         self.data.write(&mut message);
+        let idx = obligation.add_message(message);
 
-        let idx = obligation.messages.len();
-        obligation.messages.push(message);
-
-        for tuple in self.auth_sign_info.signatures.iter() {
-            let (authority, signature) = tuple;
-            // do we know, or can we build a valid public key?
-            match committee.expanded_keys.get(authority) {
-                Some(v) => obligation.public_keys.push(*v),
-                None => {
-                    let public_key = (*authority).try_into()?;
-                    obligation.public_keys.push(public_key);
-                }
-            }
-
-            // build a signature
-            obligation.signatures.push(signature.0);
-
-            // collect the message
-            obligation.message_index.push(idx);
-        }
-
-        Ok(())
+        self.auth_sign_info
+            .add_to_verification_obligation(committee, obligation, idx)
     }
 }