From ce93bd320075d5cb7a494f0a6d61fe2fb8ab8763 Mon Sep 17 00:00:00 2001 From: George Danezis Date: Fri, 18 Mar 2022 10:06:56 +0000 Subject: [PATCH] [authority] Small code fixes on the back of profiling (#839) * Integrate OneCell digest * Removed &mut and copy less * Changed variable name, and simplified logging Co-authored-by: George Danezis --- sui/src/bench.rs | 5 +-- sui_core/src/authority.rs | 41 +++++++++------------ sui_core/src/authority/authority_store.rs | 29 +++++++-------- sui_core/src/authority_aggregator.rs | 9 +++-- sui_core/src/authority_server.rs | 12 +++--- sui_core/src/consensus_handler.rs | 3 +- sui_core/src/unit_tests/authority_tests.rs | 10 ++--- sui_core/src/unit_tests/client_tests.rs | 5 +-- sui_programmability/adapter/src/genesis.rs | 2 +- sui_types/src/base_types.rs | 17 +++------ sui_types/src/messages.rs | 36 ++++++++++++++++-- sui_types/src/move_package.rs | 26 ++++++------- sui_types/src/object.rs | 2 +- sui_types/src/unit_tests/serialize_tests.rs | 15 ++------ 14 files changed, 108 insertions(+), 104 deletions(-) diff --git a/sui/src/bench.rs b/sui/src/bench.rs index 520b282f91d44..e013db47fef05 100644 --- a/sui/src/bench.rs +++ b/sui/src/bench.rs @@ -289,10 +289,7 @@ impl ClientServerBenchmark { if self.benchmark_type != BenchmarkType::TransactionsOnly { // Make certificate - let mut certificate = CertifiedTransaction { - transaction, - signatures: Vec::with_capacity(committee.quorum_threshold()), - }; + let mut certificate = CertifiedTransaction::new(transaction); for i in 0..committee.quorum_threshold() { let (pubx, secx) = keys.get(i).unwrap(); let sig = AuthoritySignature::new(&certificate.transaction.data, secx); diff --git a/sui_core/src/authority.rs b/sui_core/src/authority.rs index da1b1d9f08cd2..13397ac57a900 100644 --- a/sui_core/src/authority.rs +++ b/sui_core/src/authority.rs @@ -110,7 +110,7 @@ impl AuthorityState { self.batch_channels .as_ref() .map(|(_, tx)| tx.subscribe()) - .ok_or(SuiError::GenericAuthorityError { + .ok_or_else(|| SuiError::GenericAuthorityError { error: "No broadcast subscriptions allowed for this authority.".to_string(), }) } @@ -341,9 +341,8 @@ impl AuthorityState { &self, confirmation_transaction: ConfirmationTransaction, ) -> SuiResult { + let transaction_digest = *confirmation_transaction.certificate.digest(); let certificate = &confirmation_transaction.certificate; - let transaction = &certificate.transaction; - let transaction_digest = transaction.digest(); // Ensure an idempotent answer. if self._database.signed_effects_exists(&transaction_digest)? { @@ -359,7 +358,7 @@ impl AuthorityState { async fn check_shared_locks( &self, - transaction_digest: TransactionDigest, + transaction_digest: &TransactionDigest, transaction: &Transaction, inputs: &[(InputObjectKind, Object)], ) -> Result<(), SuiError> { @@ -428,15 +427,15 @@ impl AuthorityState { confirmation_transaction: ConfirmationTransaction, ) -> Result { let certificate = confirmation_transaction.certificate; - let transaction = certificate.transaction.clone(); - let transaction_digest = transaction.digest(); + let transaction_digest = *certificate.digest(); + let transaction = &certificate.transaction; - let objects_by_kind: Vec<_> = self.check_locks(&transaction).await?; + let objects_by_kind: Vec<_> = self.check_locks(transaction).await?; // At this point we need to check if any shared objects need locks, // and whether they have them. let _shared_objects = self - .check_shared_locks(transaction_digest, &transaction, &objects_by_kind) + .check_shared_locks(&transaction_digest, transaction, &objects_by_kind) .await?; // inputs.extend(shared_objects); @@ -456,14 +455,14 @@ impl AuthorityState { .collect(); // Insert into the certificates map - let mut tx_ctx = TxContext::new(&transaction.sender_address(), transaction_digest); + let mut tx_ctx = TxContext::new(&transaction.sender_address(), &transaction_digest); let gas_object_id = transaction.gas_payment_object_ref().0; let mut temporary_store = AuthorityTemporaryStore::new(self._database.clone(), &inputs, tx_ctx.digest()); let status = execution_engine::execute_transaction( &mut temporary_store, - transaction, + transaction.clone(), inputs, &mut tx_ctx, &self.move_vm, @@ -503,22 +502,21 @@ impl AuthorityState { /// called by a single task (ie. the task handling consensus outputs). pub async fn handle_consensus_certificate( &self, - certificate: &CertifiedTransaction, + certificate: CertifiedTransaction, ) -> SuiResult<()> { - let transaction = &certificate.transaction; - // Ensure it is a shared object certificate - if !transaction.contains_shared_object() { + if !certificate.transaction.contains_shared_object() { // TODO: Maybe add a warning here, no respectable authority should // have sequenced this. return Ok(()); } // Ensure it is the first time we see this certificate. - let transaction_digest = transaction.digest(); - if self - ._database - .sequenced(transaction_digest, transaction.shared_input_objects())?[0] + let transaction_digest = *certificate.digest(); + if self._database.sequenced( + &transaction_digest, + certificate.transaction.shared_input_objects(), + )?[0] .is_some() { return Ok(()); @@ -530,11 +528,8 @@ impl AuthorityState { // Persist the certificate. We are about to lock one or more shared object. // We thus need to make sure someone (if not the client) can continue the protocol. // Also atomically lock the shared objects for this particular transaction. - self._database.persist_certificate_and_lock_shared_objects( - transaction_digest, - transaction, - certificate.clone(), - ) + self._database + .persist_certificate_and_lock_shared_objects(&transaction_digest, certificate) } pub async fn handle_transaction_info_request( diff --git a/sui_core/src/authority/authority_store.rs b/sui_core/src/authority/authority_store.rs index 81db831b69c01..338ba1d13fcbc 100644 --- a/sui_core/src/authority/authority_store.rs +++ b/sui_core/src/authority/authority_store.rs @@ -381,12 +381,12 @@ impl SuiDataStore { /// Read a lock for a specific (transaction, shared object) pair. pub fn sequenced( &self, - transaction_digest: TransactionDigest, + transaction_digest: &TransactionDigest, object_ids: &[ObjectID], ) -> Result>, SuiError> { let keys: Vec<_> = object_ids .iter() - .map(|objid| (transaction_digest, *objid)) + .map(|objid| (*transaction_digest, *objid)) .collect(); self.sequenced.multi_get(&keys[..]).map_err(SuiError::from) @@ -395,13 +395,13 @@ impl SuiDataStore { /// Read a lock for a specific (transaction, shared object) pair. pub fn all_shared_locks( &self, - transaction_digest: TransactionDigest, + transaction_digest: &TransactionDigest, ) -> Result, SuiError> { Ok(self .sequenced .iter() - .skip_to(&(transaction_digest, ObjectID::ZERO))? - .take_while(|((tx, _objid), _ver)| *tx == transaction_digest) + .skip_to(&(*transaction_digest, ObjectID::ZERO))? + .take_while(|((tx, _objid), _ver)| tx == transaction_digest) .map(|((_tx, objid), ver)| (objid, ver)) .collect()) } @@ -551,7 +551,7 @@ impl SuiDataStore { let mut write_batch = self.transaction_lock.batch(); // Store the certificate indexed by transaction digest - let transaction_digest: TransactionDigest = certificate.transaction.digest(); + let transaction_digest: &TransactionDigest = certificate.digest(); write_batch = write_batch.insert_batch( &self.certificates, std::iter::once((transaction_digest, &certificate)), @@ -572,13 +572,13 @@ impl SuiDataStore { // Safe to unwrap since the "true" flag ensures we get a sequence value back. let seq: TxSequenceNumber = self - .batch_update_objects(write_batch, temporary_store, transaction_digest, true)? + .batch_update_objects(write_batch, temporary_store, *transaction_digest, true)? .unwrap(); Ok(( seq, TransactionInfoResponse { - signed_transaction: self.signed_transactions.get(&transaction_digest)?, + signed_transaction: self.signed_transactions.get(transaction_digest)?, certified_transaction: Some(certificate), signed_effects: Some(signed_effects), }, @@ -785,13 +785,13 @@ impl SuiDataStore { pub fn remove_shared_objects_locks( &self, mut write_batch: DBBatch, - transaction_digest: TransactionDigest, + transaction_digest: &TransactionDigest, transaction: &Transaction, ) -> SuiResult { let mut sequenced_to_delete = Vec::new(); let mut schedule_to_delete = Vec::new(); for object_id in transaction.shared_input_objects() { - sequenced_to_delete.push((transaction_digest, *object_id)); + sequenced_to_delete.push((*transaction_digest, *object_id)); if self.get_object(object_id)?.is_none() { schedule_to_delete.push(object_id); } @@ -804,17 +804,16 @@ impl SuiDataStore { /// Lock a sequence number for the shared objects of the input transaction. pub fn persist_certificate_and_lock_shared_objects( &self, - transaction_digest: TransactionDigest, - transaction: &Transaction, + transaction_digest: &TransactionDigest, certificate: CertifiedTransaction, ) -> Result<(), SuiError> { - let certificate_to_write = std::iter::once((transaction_digest, certificate)); + let certificate_to_write = std::iter::once((transaction_digest, &certificate)); let mut sequenced_to_write = Vec::new(); let mut schedule_to_write = Vec::new(); - for id in transaction.shared_input_objects() { + for id in certificate.transaction.shared_input_objects() { let version = self.schedule.get(id)?.unwrap_or_default(); - sequenced_to_write.push(((transaction_digest, *id), version)); + sequenced_to_write.push(((*transaction_digest, *id), version)); let next_version = version.increment(); schedule_to_write.push((id, next_version)); } diff --git a/sui_core/src/authority_aggregator.rs b/sui_core/src/authority_aggregator.rs index 956d712ca14dd..a6d1f75c0afe9 100644 --- a/sui_core/src/authority_aggregator.rs +++ b/sui_core/src/authority_aggregator.rs @@ -805,10 +805,11 @@ where .push((name, inner_signed_transaction.signature)); state.good_stake += weight; if state.good_stake >= threshold { - state.certificate = Some(CertifiedTransaction { - transaction: transaction_ref.clone(), - signatures: state.signatures.clone(), - }); + state.certificate = + Some(CertifiedTransaction::new_with_signatures( + transaction_ref.clone(), + state.signatures.clone(), + )); } } // If we get back an error, then we aggregate and check diff --git a/sui_core/src/authority_server.rs b/sui_core/src/authority_server.rs index 95bf32e62c8fe..db4cbace3b5fb 100644 --- a/sui_core/src/authority_server.rs +++ b/sui_core/src/authority_server.rs @@ -181,16 +181,18 @@ impl AuthorityServer { .map(|info| Some(serialize_transaction_info(&info))) } SerializedMessage::Cert(message) => { + // Cache the transaction digest + let tx_digest = *message.digest(); + // Get the kind + let tx_kind = message.transaction.data.kind_as_str(); + let confirmation_transaction = ConfirmationTransaction { - certificate: message.as_ref().clone(), + certificate: *message, }; - let tx_kind = message.transaction.data.kind_as_str(); match self .state .handle_confirmation_transaction(confirmation_transaction) - .instrument(tracing::debug_span!("process_cert", - tx_digest =? message.transaction.digest(), - tx_kind)) + .instrument(tracing::debug_span!("process_cert", ?tx_digest, tx_kind)) .await { Ok(info) => { diff --git a/sui_core/src/consensus_handler.rs b/sui_core/src/consensus_handler.rs index 9036d5917ce4c..0495db4b1f184 100644 --- a/sui_core/src/consensus_handler.rs +++ b/sui_core/src/consensus_handler.rs @@ -56,11 +56,10 @@ impl ConsensusHandler { }; // Process the certificate to set the locks on the shared objects. - let certificate = &confirmation.certificate; let result = self .server .state - .handle_consensus_certificate(certificate) + .handle_consensus_certificate(confirmation.certificate) .await; match &result { // Log the errors that are our faults (not the client's). diff --git a/sui_core/src/unit_tests/authority_tests.rs b/sui_core/src/unit_tests/authority_tests.rs index c07a99b18b46e..f493d8e993a32 100644 --- a/sui_core/src/unit_tests/authority_tests.rs +++ b/sui_core/src/unit_tests/authority_tests.rs @@ -419,7 +419,7 @@ async fn test_publish_dependent_module_ok() { let signature = Signature::new(&data, &sender_key); let transaction = Transaction::new(data, signature); - let dependent_module_id = TxContext::new(&sender, transaction.digest()).fresh_id(); + let dependent_module_id = TxContext::new(&sender, &transaction.digest()).fresh_id(); // Object does not exist assert!(authority @@ -456,7 +456,7 @@ async fn test_publish_module_no_dependencies_ok() { let data = TransactionData::new_module(sender, gas_payment_object_ref, module_bytes, MAX_GAS); let signature = Signature::new(&data, &sender_key); let transaction = Transaction::new(data, signature); - let _module_object_id = TxContext::new(&sender, transaction.digest()).fresh_id(); + let _module_object_id = TxContext::new(&sender, &transaction.digest()).fresh_id(); let response = send_and_confirm_transaction(&authority, transaction) .await .unwrap(); @@ -1653,13 +1653,13 @@ async fn shared_object() { // Sequence the certificate to assign a sequence number to the shared object. authority - .handle_consensus_certificate(&certificate) + .handle_consensus_certificate(certificate) .await .unwrap(); let shared_object_version = authority .db() - .sequenced(transaction_digest, &[shared_object_id]) + .sequenced(&transaction_digest, &[shared_object_id]) .unwrap()[0] .unwrap(); assert_eq!(shared_object_version, SequenceNumber::new()); @@ -1673,7 +1673,7 @@ async fn shared_object() { let shared_object_lock = authority .db() - .sequenced(transaction_digest, &[shared_object_id]) + .sequenced(&transaction_digest, &[shared_object_id]) .unwrap()[0]; assert!(shared_object_lock.is_none()); diff --git a/sui_core/src/unit_tests/client_tests.rs b/sui_core/src/unit_tests/client_tests.rs index b0eb4e3944c10..831d512be135f 100644 --- a/sui_core/src/unit_tests/client_tests.rs +++ b/sui_core/src/unit_tests/client_tests.rs @@ -146,10 +146,7 @@ async fn extract_cert( let stake: usize = votes.iter().map(|(name, _)| committee.weight(name)).sum(); assert!(stake >= committee.quorum_threshold()); - CertifiedTransaction { - transaction: transaction.unwrap(), - signatures: votes, - } + CertifiedTransaction::new_with_signatures(transaction.unwrap(), votes) } #[cfg(test)] diff --git a/sui_programmability/adapter/src/genesis.rs b/sui_programmability/adapter/src/genesis.rs index ed6d30bd4597b..60635c6b02808 100644 --- a/sui_programmability/adapter/src/genesis.rs +++ b/sui_programmability/adapter/src/genesis.rs @@ -34,7 +34,7 @@ pub fn get_genesis_context() -> TxContext { } pub fn get_genesis_context_with_custom_address(address: &SuiAddress) -> TxContext { - TxContext::new(address, TransactionDigest::genesis()) + TxContext::new(address, &TransactionDigest::genesis()) } /// Create and return objects wrapping the genesis modules for sui diff --git a/sui_types/src/base_types.rs b/sui_types/src/base_types.rs index b86d5fdeeca3c..4bbb81080e522 100644 --- a/sui_types/src/base_types.rs +++ b/sui_types/src/base_types.rs @@ -153,7 +153,7 @@ pub struct TxContext { } impl TxContext { - pub fn new(sender: &SuiAddress, digest: TransactionDigest) -> Self { + pub fn new(sender: &SuiAddress, digest: &TransactionDigest) -> Self { Self { sender: AccountAddress::new(sender.0), digest: digest.0.to_vec(), @@ -197,7 +197,7 @@ impl TxContext { pub fn random_for_testing_only() -> Self { Self::new( &SuiAddress::random_for_testing_only(), - TransactionDigest::random(), + &TransactionDigest::random(), ) } @@ -617,21 +617,16 @@ impl TryFrom for ObjectID { type Error = ObjectIDParseError; fn try_from(s: String) -> Result { - match Self::from_hex(s.clone()) { - Ok(q) => Ok(q), - Err(_) => Self::from_hex_literal(&s), - } + Self::from_hex(s.clone()).or_else(|_| Self::from_hex_literal(&s)) } } impl std::str::FromStr for ObjectID { type Err = ObjectIDParseError; - // Try to match both the literal (0xABC..) and the normal (ABC) + fn from_str(s: &str) -> Result { - match Self::from_hex(s) { - Ok(q) => Ok(q), - Err(_) => Self::from_hex_literal(s), - } + // Try to match both the literal (0xABC..) and the normal (ABC) + Self::from_hex(s).or_else(|_| Self::from_hex_literal(s)) } } diff --git a/sui_types/src/messages.rs b/sui_types/src/messages.rs index 4434842db555b..57df6e21abef3 100644 --- a/sui_types/src/messages.rs +++ b/sui_types/src/messages.rs @@ -17,6 +17,7 @@ use move_core_types::{ value::MoveStructLayout, }; use name_variant::NamedVariant; +use once_cell::sync::OnceCell; use serde::{Deserialize, Serialize}; use static_assertions::const_assert_eq; use std::fmt::Write; @@ -170,6 +171,11 @@ pub struct SignedTransaction { /// #[derive(Clone, Debug, Serialize, Deserialize)] pub struct CertifiedTransaction { + // This is a cache of an otherwise expensive to compute value. + // DO NOT serialize or deserialize from the network or disk. + #[serde(skip)] + transaction_digest: OnceCell, + pub transaction: Transaction, pub signatures: Vec<(AuthorityName, AuthoritySignature)>, } @@ -731,10 +737,7 @@ impl<'a> SignatureAggregator<'a> { committee, weight: 0, used_authorities: HashSet::new(), - partial: CertifiedTransaction { - transaction, - signatures: Vec::new(), - }, + partial: CertifiedTransaction::new(transaction), } } @@ -769,6 +772,31 @@ impl<'a> SignatureAggregator<'a> { } impl CertifiedTransaction { + pub fn new(transaction: Transaction) -> CertifiedTransaction { + CertifiedTransaction { + transaction_digest: OnceCell::new(), + transaction, + signatures: Vec::new(), + } + } + + pub fn new_with_signatures( + transaction: Transaction, + signatures: Vec<(AuthorityName, AuthoritySignature)>, + ) -> CertifiedTransaction { + CertifiedTransaction { + transaction_digest: OnceCell::new(), + transaction, + signatures, + } + } + + /// Get the transaction digest and write it to the cache + pub fn digest(&self) -> &TransactionDigest { + self.transaction_digest + .get_or_init(|| self.transaction.digest()) + } + /// Verify the certificate. pub fn check(&self, committee: &Committee) -> Result<(), SuiError> { // Check the quorum. diff --git a/sui_types/src/move_package.rs b/sui_types/src/move_package.rs index a7c7c2a365e27..3c8b0e5d7f7c0 100644 --- a/sui_types/src/move_package.rs +++ b/sui_types/src/move_package.rs @@ -60,12 +60,12 @@ impl MovePackage { } pub fn module_id(&self, module: &Identifier) -> Result { - let ser = - self.serialized_module_map() - .get(module.as_str()) - .ok_or(SuiError::ModuleNotFound { - module_name: module.to_string(), - })?; + let ser = self + .serialized_module_map() + .get(module.as_str()) + .ok_or_else(|| SuiError::ModuleNotFound { + module_name: module.to_string(), + })?; Ok(CompiledModule::deserialize(ser)?.self_id()) } @@ -75,16 +75,16 @@ impl MovePackage { module: &Identifier, function: &Identifier, ) -> Result { - let bytes = - self.serialized_module_map() - .get(module.as_str()) - .ok_or(SuiError::ModuleNotFound { - module_name: module.to_string(), - })?; + let bytes = self + .serialized_module_map() + .get(module.as_str()) + .ok_or_else(|| SuiError::ModuleNotFound { + module_name: module.to_string(), + })?; let m = CompiledModule::deserialize(bytes) .expect("Unwrap safe because Sui serializes/verifies modules before publishing them"); - Function::new_from_name(&m, function).ok_or(SuiError::FunctionNotFound { + Function::new_from_name(&m, function).ok_or_else(|| SuiError::FunctionNotFound { error: format!( "Could not resolve function '{}' in module {}::{}", function, diff --git a/sui_types/src/object.rs b/sui_types/src/object.rs index 57ef56ef5d65f..eaf72744825a0 100644 --- a/sui_types/src/object.rs +++ b/sui_types/src/object.rs @@ -468,7 +468,7 @@ impl Object { /// like this: `S`. /// Returns the inner parameter type `T`. pub fn get_move_template_type(&self) -> SuiResult { - let move_struct = self.data.type_().ok_or(SuiError::TypeError { + let move_struct = self.data.type_().ok_or_else(|| SuiError::TypeError { error: "Object must be a Move object".to_owned(), })?; fp_ensure!( diff --git a/sui_types/src/unit_tests/serialize_tests.rs b/sui_types/src/unit_tests/serialize_tests.rs index fb805fc8ce5fb..818eb2f4df331 100644 --- a/sui_types/src/unit_tests/serialize_tests.rs +++ b/sui_types/src/unit_tests/serialize_tests.rs @@ -171,10 +171,7 @@ fn test_cert() { ), &sender_key, ); - let mut cert = CertifiedTransaction { - transaction, - signatures: Vec::new(), - }; + let mut cert = CertifiedTransaction::new(transaction); for _ in 0..3 { let (_, authority_key) = get_key_pair(); @@ -210,10 +207,7 @@ fn test_info_response() { let (_, auth_key) = get_key_pair(); let vote = SignedTransaction::new(transaction.clone(), *auth_key.public_key_bytes(), &auth_key); - let mut cert = CertifiedTransaction { - transaction, - signatures: Vec::new(), - }; + let mut cert = CertifiedTransaction::new(transaction); for _ in 0..3 { let (_, authority_key) = get_key_pair(); @@ -343,10 +337,7 @@ fn test_time_cert() { ), &sender_key, ); - let mut cert = CertifiedTransaction { - transaction, - signatures: Vec::new(), - }; + let mut cert = CertifiedTransaction::new(transaction); use std::collections::HashMap; let mut cache = HashMap::new();