Skip to content

Commit

Permalink
[sui-core] avoid certificate cloning
Browse files Browse the repository at this point in the history
Reading this code for this first time in awhile and noticed that some cert clones are not needed. Certs can be big, so this seems worth fixing.
  • Loading branch information
sblackshear committed Oct 19, 2022
1 parent 5d79905 commit 127e092
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 42 deletions.
10 changes: 5 additions & 5 deletions crates/sui-core/src/authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ impl AuthorityState {
#[instrument(level = "trace", skip_all)]
pub async fn handle_certificate(
&self,
certificate: CertifiedTransaction,
certificate: &CertifiedTransaction,
) -> SuiResult<TransactionInfoResponse> {
let _metrics_guard = start_timer(self.metrics.handle_certificate_latency.clone());
self.handle_certificate_impl(certificate, false).await
Expand All @@ -662,15 +662,15 @@ impl AuthorityState {
#[instrument(level = "trace", skip_all)]
pub async fn handle_certificate_bypass_validator_halt(
&self,
certificate: CertifiedTransaction,
certificate: &CertifiedTransaction,
) -> SuiResult<TransactionInfoResponse> {
self.handle_certificate_impl(certificate, true).await
}

#[instrument(level = "trace", skip_all)]
async fn handle_certificate_impl(
&self,
certificate: CertifiedTransaction,
certificate: &CertifiedTransaction,
bypass_validator_halt: bool,
) -> SuiResult<TransactionInfoResponse> {
self.metrics.total_cert_attempts.inc();
Expand Down Expand Up @@ -700,11 +700,11 @@ impl AuthorityState {
);
let tx_guard = self
.database
.acquire_tx_guard(&certificate)
.acquire_tx_guard(certificate)
.instrument(span)
.await?;

self.process_certificate(tx_guard, &certificate, bypass_validator_halt)
self.process_certificate(tx_guard, certificate, bypass_validator_halt)
.await
.tap_err(|e| debug!(?tx_digest, "process_certificate failed: {e}"))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl AuthorityAPI for ConfigurableBatchActionClient {
certificate: CertifiedTransaction,
) -> Result<TransactionInfoResponse, SuiError> {
let state = self.state.clone();
state.handle_certificate(certificate).await
state.handle_certificate(&certificate).await
}

async fn handle_account_info_request(
Expand Down
11 changes: 6 additions & 5 deletions crates/sui-core/src/authority_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -410,11 +410,12 @@ impl AuthorityAPI for LocalAuthorityClient {
certificate: CertifiedTransaction,
) -> Result<TransactionInfoResponse, SuiError> {
let state = self.state.clone();
let cert = certificate.clone();
let fault_config = self.fault_config;
tokio::spawn(async move { Self::handle_certificate(state, cert, fault_config).await })
.await
.unwrap()
tokio::spawn(
async move { Self::handle_certificate(state, &certificate, fault_config).await },
)
.await
.unwrap()
}

async fn handle_account_info_request(
Expand Down Expand Up @@ -524,7 +525,7 @@ impl LocalAuthorityClient {

async fn handle_certificate(
state: Arc<AuthorityState>,
certificate: CertifiedTransaction,
certificate: &CertifiedTransaction,
fault_config: LocalAuthorityClientFaultConfig,
) -> Result<TransactionInfoResponse, SuiError> {
if fault_config.fail_before_handle_confirmation {
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-core/src/authority_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ impl ValidatorService {
);

match state
.handle_certificate(certificate.clone())
.handle_certificate(&certificate)
.instrument(span)
.await
.map_err(|e| tonic::Status::internal(e.to_string()))
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-core/src/epoch/reconfiguration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ where
.process_transaction(advance_epoch_tx.clone().to_transaction())
.await
{
Ok(certificate) => match self.state.handle_certificate(certificate).await {
Ok(certificate) => match self.state.handle_certificate(&certificate).await {
Ok(_) => {
break;
}
Expand Down
5 changes: 1 addition & 4 deletions crates/sui-core/src/epoch/tests/reconfiguration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,7 @@ async fn test_start_epoch_change() {
}
let certificate = cert.unwrap();
assert_eq!(
state
.handle_certificate(certificate.clone())
.await
.unwrap_err(),
state.handle_certificate(&certificate).await.unwrap_err(),
SuiError::ValidatorHaltedAtEpochEnd
);

Expand Down
8 changes: 4 additions & 4 deletions crates/sui-core/src/node_sync/node_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -577,10 +577,10 @@ where

let result = if bypass_validator_halt {
self.state()
.handle_certificate_bypass_validator_halt(cert.clone())
.handle_certificate_bypass_validator_halt(&cert)
.await
} else {
self.state().handle_certificate(cert.clone()).await
self.state().handle_certificate(&cert).await
};
match result {
Ok(_) => Ok(SyncStatus::CertExecuted),
Expand All @@ -602,10 +602,10 @@ where
debug!(?digest, "parents executed, re-attempting cert");
if bypass_validator_halt {
self.state()
.handle_certificate_bypass_validator_halt(cert.clone())
.handle_certificate_bypass_validator_halt(&cert)
.await
} else {
self.state().handle_certificate(cert.clone()).await
self.state().handle_certificate(&cert).await
}?;
Ok(SyncStatus::CertExecuted)
}
Expand Down
39 changes: 18 additions & 21 deletions crates/sui-core/src/unit_tests/authority_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ pub async fn send_and_confirm_transaction_with_shared(

// Submit the confirmation. *Now* execution actually happens, and it should fail when we try to look up our dummy module.
// we unfortunately don't get a very descriptive error message, but we can at least see that something went wrong inside the VM
authority.handle_certificate(certificate).await
authority.handle_certificate(&certificate).await
}

/// Create a `CompiledModule` that depends on `m`
Expand Down Expand Up @@ -867,7 +867,7 @@ async fn test_handle_confirmation_transaction_unknown_sender() {
);

assert!(authority_state
.handle_certificate(certified_transfer_transaction)
.handle_certificate(&certified_transfer_transaction)
.await
.is_err());
}
Expand Down Expand Up @@ -934,7 +934,7 @@ async fn test_handle_confirmation_transaction_bad_sequence_number() {
// Explanation: providing an old cert that has already need applied
// returns a Ok(_) with info about the new object states.
let response = authority_state
.handle_certificate(certified_transfer_transaction)
.handle_certificate(&certified_transfer_transaction)
.await
.unwrap();
assert!(response.signed_effects.is_none());
Expand Down Expand Up @@ -978,7 +978,7 @@ async fn test_handle_confirmation_transaction_receiver_equal_sender() {
&authority_state,
);
let response = authority_state
.handle_certificate(certified_transfer_transaction)
.handle_certificate(&certified_transfer_transaction)
.await
.unwrap();
response.signed_effects.unwrap().effects.status.unwrap();
Expand Down Expand Up @@ -1032,7 +1032,7 @@ async fn test_handle_confirmation_transaction_ok() {
next_sequence_number = next_sequence_number.increment();

let info = authority_state
.handle_certificate(certified_transfer_transaction.clone())
.handle_certificate(&certified_transfer_transaction.clone())
.await
.unwrap();
info.signed_effects.unwrap().effects.status.unwrap();
Expand Down Expand Up @@ -1173,7 +1173,7 @@ async fn test_handle_certificate_interrupted_retry() {
let state1 = authority_state.clone();

let limited_fut = Box::pin(LimitedPoll::new(*limit, async move {
state1.handle_certificate(clone1).await.unwrap();
state1.handle_certificate(&clone1).await.unwrap();
}));

let res = limited_fut.await;
Expand All @@ -1195,7 +1195,7 @@ async fn test_handle_certificate_interrupted_retry() {

// Now run the tx to completion
let info = authority_state
.handle_certificate(shared_object_cert.clone())
.handle_certificate(&shared_object_cert)
.await
.unwrap();

Expand Down Expand Up @@ -1235,13 +1235,13 @@ async fn test_handle_confirmation_transaction_idempotent() {
);

let info = authority_state
.handle_certificate(certified_transfer_transaction.clone())
.handle_certificate(&certified_transfer_transaction)
.await
.unwrap();
assert!(info.signed_effects.as_ref().unwrap().effects.status.is_ok());

let info2 = authority_state
.handle_certificate(certified_transfer_transaction.clone())
.handle_certificate(&certified_transfer_transaction)
.await
.unwrap();
assert!(info2
Expand Down Expand Up @@ -1377,7 +1377,7 @@ async fn test_move_call_insufficient_gas() {
&authority_state,
);
let effects = authority_state
.handle_certificate(certified_transfer_transaction)
.handle_certificate(&certified_transfer_transaction)
.await
.unwrap()
.signed_effects
Expand Down Expand Up @@ -1703,7 +1703,7 @@ async fn test_idempotent_reversed_confirmation() {
&authority_state,
);
let result1 = authority_state
.handle_certificate(certified_transfer_transaction.clone())
.handle_certificate(&certified_transfer_transaction)
.await;
assert!(result1.is_ok());
let result2 = authority_state
Expand Down Expand Up @@ -1762,7 +1762,7 @@ async fn test_change_epoch_transaction() {
.unwrap()
.unwrap();
let result = authority_state
.handle_certificate(certificate)
.handle_certificate(&certificate)
.await
.unwrap();
assert!(result.signed_effects.unwrap().effects.status.is_ok());
Expand Down Expand Up @@ -1796,7 +1796,7 @@ async fn test_transfer_sui_no_amount() {

let certificate = init_certified_transaction(transaction, &authority_state);
let response = authority_state
.handle_certificate(certificate)
.handle_certificate(&certificate)
.await
.unwrap();
let effects = response.signed_effects.unwrap().effects;
Expand Down Expand Up @@ -1839,7 +1839,7 @@ async fn test_transfer_sui_with_amount() {
let transaction = to_sender_signed_transaction(tx_data, &sender_key);
let certificate = init_certified_transaction(transaction, &authority_state);
let response = authority_state
.handle_certificate(certificate)
.handle_certificate(&certificate)
.await
.unwrap();
let effects = response.signed_effects.unwrap().effects;
Expand Down Expand Up @@ -1893,7 +1893,7 @@ async fn test_store_revert_state_update() {
let certificate = init_certified_transaction(transaction, &authority_state);
let tx_digest = *certificate.digest();
authority_state
.handle_certificate(certificate)
.handle_certificate(&certificate)
.await
.unwrap();

Expand Down Expand Up @@ -2286,7 +2286,7 @@ async fn shared_object() {
let transaction_digest = certificate.digest();

// Sending the certificate now fails since it was not sequenced.
let result = authority.handle_certificate(certificate.clone()).await;
let result = authority.handle_certificate(&certificate).await;
assert!(
matches!(
result,
Expand All @@ -2309,10 +2309,7 @@ async fn shared_object() {

// Finally process the certificate and execute the contract. Ensure that the
// shared object lock is cleaned up and that its sequence number increased.
authority
.handle_certificate(certificate.clone())
.await
.unwrap();
authority.handle_certificate(&certificate).await.unwrap();

let shared_object_lock = authority
.db()
Expand Down Expand Up @@ -2375,7 +2372,7 @@ async fn test_consensus_message_processed() {
if let TransactionInfoResponse {
signed_effects: Some(effects),
..
} = authority.handle_certificate(cert.clone()).await.unwrap()
} = authority.handle_certificate(cert).await.unwrap()
{
effects
} else {
Expand Down

0 comments on commit 127e092

Please sign in to comment.