Skip to content

Commit

Permalink
[authority_store] Schedule certificate after acquiring tx lock (#4365)
Browse files Browse the repository at this point in the history
We currently schedule certificate for execution and then acquire tx lock. This opens a relatively small possibility for race condition where execution driver will pick up and lock transaction before we can do that in `persist_certificate_and_lock_shared_objects`. This would not be end of the world, but it would waste time and produce noisy error in logs since execution driver will fail because `assigned_object_versions` was not yet updated.

Simple one line fix is to move scheduling after lock is acquired so that `assigned_object_versions` will be ready for execution driver when it will be able to acquire a lock.

We can't just move `add_pending_certificates` to the end of this function since we want to make sure transaction is scheduled for execution by the time `consensus_message_processed` is updated.
  • Loading branch information
andll authored Aug 30, 2022
1 parent 9b30d50 commit d427cf2
Showing 1 changed file with 3 additions and 3 deletions.
6 changes: 3 additions & 3 deletions crates/sui-core/src/authority/authority_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1110,9 +1110,6 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
// Make an iterator to update the last consensus index.
let index_to_write = std::iter::once((LAST_CONSENSUS_INDEX_ADDR, consensus_index));

// Schedule the certificate for execution
self.add_pending_certificates(vec![(transaction_digest, Some(certificate.clone()))])?;

// Holding _tx_lock avoids the following race:
// - we check effects_exist, returns false
// - another task (starting from handle_node_sync_certificate) writes effects,
Expand All @@ -1121,6 +1118,9 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
// - now it's possible to run a new tx against old versions of the shared objects.
let _tx_lock = self.acquire_tx_lock(&transaction_digest).await;

// Schedule the certificate for execution
self.add_pending_certificates(vec![(transaction_digest, Some(certificate.clone()))])?;

// Note: if we crash here we are not in an inconsistent state since
// it is ok to just update the pending list without updating the sequence.

Expand Down

0 comments on commit d427cf2

Please sign in to comment.