Merge pull request #3185 from ProvableHQ/mainnet-hotfix-backport

Avoid current_block RwLock reentry during block checks

Author: vicsn

ledger/src/advance.rs

@@ -289,8 +289,11 @@ impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> {
                         let prover_address = solution.address();
                         let num_accepted_solutions = accepted_solutions.get(&prover_address).copied().unwrap_or(0);
                         // Check if the prover has reached their solution limit.
-                        if self.is_solution_limit_reached_inner(previous_block, &prover_address, num_accepted_solutions)
-                        {
+                        if self.is_solution_limit_reached_at_timestamp(
+                            &prover_address,
+                            num_accepted_solutions,
+                            previous_block.timestamp(),
+                        ) {
                             return false;
                         }
                         // Check if the solution is valid and update the number of accepted solutions.

ledger/src/check_next_block.rs

@@ -203,20 +203,17 @@ impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> {
         block: &Block<N>,
         rng: &mut R,
     ) -> Result<(), CheckBlockError<N>> {
-        // Grab lock to the previous block here, to ensure it does not change mid-check.
-        let previous_block = self.current_block.read();
+        let latest_block = self.current_block.read();
+        let latest_block_timestamp = latest_block.timestamp();
 
         // Ensure, again, that the ledger has not advanced yet. This prevents cryptic errors form appearing during the block check.
-        if block.height() != previous_block.height() + 1 {
-            return Err(CheckBlockError::InvalidHeight {
-                expected: previous_block.height() + 1,
-                actual: block.height(),
-            });
+        if block.height() != latest_block.height() + 1 {
+            return Err(CheckBlockError::InvalidHeight { expected: latest_block.height() + 1, actual: block.height() });
         }
 
         // Also ensure the round is valid, otherwise speculation on transactions will fail with a cryptic error.
-        if block.round() <= previous_block.round() {
-            return Err(CheckBlockError::InvalidRound { new: block.round(), previous: previous_block.round() });
+        if block.round() <= latest_block.round() {
+            return Err(CheckBlockError::InvalidRound { new: block.round(), previous: latest_block.round() });
         }
 
         // Ensure the solutions do not already exist.
@@ -240,7 +237,7 @@ impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> {
         )?;
 
         // Ensure speculation over the unconfirmed transactions is correct and ensure each transaction is well-formed and unique.
-        let time_since_last_block = block.timestamp().saturating_sub(previous_block.timestamp());
+        let time_since_last_block = block.timestamp().saturating_sub(latest_block_timestamp);
         let ratified_finalize_operations = self.vm.check_speculate(
             state,
             time_since_last_block,
@@ -267,13 +264,13 @@ impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> {
         // Get the latest epoch hash.
         let latest_epoch_hash = match self.current_epoch_hash.read().as_ref() {
             Some(epoch_hash) => *epoch_hash,
-            None => self.get_epoch_hash(previous_block.height())?,
+            None => self.get_epoch_hash(latest_block.height())?,
         };
 
         // Ensure the block is correct.
         let (expected_existing_solution_ids, expected_existing_transaction_ids) = block
             .verify(
-                &previous_block,
+                &latest_block,
                 self.latest_state_root(),
                 &previous_committee_lookback,
                 &committee_lookback,
@@ -291,7 +288,11 @@ impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> {
                 let prover_address = solution.address();
                 let num_accepted_solutions = *accepted_solutions.get(&prover_address).unwrap_or(&0);
                 // Check if the prover has reached their solution limit.
-                if self.is_solution_limit_reached_inner(&previous_block, &prover_address, num_accepted_solutions) {
+                if self.is_solution_limit_reached_at_timestamp(
+                    &prover_address,
+                    num_accepted_solutions,
+                    latest_block_timestamp,
+                ) {
                     return Err(CheckBlockError::SolutionLimitReached { prover_address });
                 }
                 // Track the already accepted solutions.

ledger/src/is_solution_limit_reached.rs

@@ -82,27 +82,19 @@ pub fn maximum_allowed_solutions_per_epoch<N: Network>(prover_stake: u64, curren
 }
 
 impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> {
-    /// Returns the number of remaining solutions a prover can submit in the current epoch.
-    ///
-    /// # Locking
-    /// This function may deadlock if called while holding a write lock to the current block.
-    pub fn num_remaining_solutions(&self, prover_address: &Address<N>, additional_solutions_in_block: u64) -> u64 {
-        self.num_remaining_solutions_inner(&self.current_block.read(), prover_address, additional_solutions_in_block)
-    }
-
-    /// Internal version of [`Self::num_remaining_solutions`] to be used when already holding a lock to the current block.
-    pub(super) fn num_remaining_solutions_inner(
+    /// Returns the number of remaining solutions a prover can submit in the current epoch,
+    /// using the provided ledger timestamp instead of re-reading the current block.
+    pub(crate) fn num_remaining_solutions_at_timestamp(
         &self,
-        latest_block: &Block<N>,
         prover_address: &Address<N>,
         additional_solutions_in_block: u64,
+        current_time: i64,
     ) -> u64 {
         // Fetch the prover's stake.
         let prover_stake = self.get_bonded_amount(prover_address).unwrap_or(0);
 
         // Determine the maximum number of solutions allowed based on this prover's stake.
-        let maximum_allowed_solutions =
-            maximum_allowed_solutions_per_epoch::<N>(prover_stake, latest_block.timestamp());
+        let maximum_allowed_solutions = maximum_allowed_solutions_per_epoch::<N>(prover_stake, current_time);
 
         // Fetch the number of solutions the prover has earned rewards for in the current epoch.
         let prover_num_solutions_in_epoch = *self.epoch_provers_cache.read().get(prover_address).unwrap_or(&0);
@@ -114,33 +106,44 @@ impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> {
         maximum_allowed_solutions.saturating_sub(num_solutions)
     }
 
-    /// Returns `true` if the given prover address has reached their solution limit for the current epoch.
-    ///
-    /// # Locking
-    /// This function may deadlock if called while holding a write lock to the current block.
-    pub fn is_solution_limit_reached(&self, prover_address: &Address<N>, additional_solutions_in_block: u64) -> bool {
-        self.is_solution_limit_reached_inner(&self.current_block.read(), prover_address, additional_solutions_in_block)
+    /// Returns the number of remaining solutions a prover can submit in the current epoch.
+    pub fn num_remaining_solutions(&self, prover_address: &Address<N>, additional_solutions_in_block: u64) -> u64 {
+        self.num_remaining_solutions_at_timestamp(
+            prover_address,
+            additional_solutions_in_block,
+            self.latest_timestamp(),
+        )
     }
 
-    /// Internal version of [`Self::is_solution_limit_reached`] to be used when already holding a lock to the current block.
-    pub(super) fn is_solution_limit_reached_inner(
+    /// Returns `true` if the given prover address has reached their solution limit for the current epoch,
+    /// using the provided ledger timestamp instead of re-reading the current block.
+    pub(crate) fn is_solution_limit_reached_at_timestamp(
         &self,
-        latest_block: &Block<N>,
         prover_address: &Address<N>,
         additional_solutions_in_block: u64,
+        current_time: i64,
     ) -> bool {
-        // Calculate the number of remaining solutions for the prover.
-        let num_remaining_solutions =
-            self.num_remaining_solutions_inner(latest_block, prover_address, additional_solutions_in_block);
+        self.num_remaining_solutions_at_timestamp(prover_address, additional_solutions_in_block, current_time) == 0
+    }
 
-        // If the number of remaining solutions is zero, the limit is reached.
-        num_remaining_solutions == 0
+    /// Returns `true` if the given prover address has reached their solution limit for the current epoch.
+    ///
+    /// # Locking
+    /// This function may deadlock if called while holding a write lock to the current block.
+    pub fn is_solution_limit_reached(&self, prover_address: &Address<N>, additional_solutions_in_block: u64) -> bool {
+        self.is_solution_limit_reached_at_timestamp(
+            prover_address,
+            additional_solutions_in_block,
+            self.latest_timestamp(),
+        )
     }
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::test_helpers::{CurrentLedger, LedgerType};
+    use std::{thread, time::Duration};
 
     type CurrentNetwork = console::network::MainnetV0;
 
@@ -226,4 +229,37 @@ mod tests {
             );
         }
     }
+
+    #[test]
+    fn test_solution_limit_helper_avoids_current_block_reentry_with_pending_writer() {
+        let rng = &mut TestRng::default();
+
+        let private_key = PrivateKey::<CurrentNetwork>::new(rng).unwrap();
+        let validator_address = Address::try_from(&private_key).unwrap();
+        let store = ConsensusStore::<CurrentNetwork, LedgerType>::open(StorageMode::new_test(None)).unwrap();
+        let genesis = VM::from(store).unwrap().genesis_beacon(&private_key, rng).unwrap();
+        let ledger = CurrentLedger::load(genesis, StorageMode::new_test(None)).unwrap();
+
+        let next_block =
+            ledger.prepare_advance_to_next_beacon_block(&private_key, vec![], vec![], vec![], rng).unwrap();
+        let expected = ledger.num_remaining_solutions(&validator_address, 0);
+
+        // Mimic check_next_block() holding a read guard while a writer queues behind it.
+        let latest_block = ledger.current_block.read();
+        let latest_timestamp = latest_block.timestamp();
+
+        let ledger_clone = ledger.clone();
+        let next_block_clone = next_block.clone();
+        let writer = thread::spawn(move || ledger_clone.advance_to_next_block(&next_block_clone));
+
+        thread::sleep(Duration::from_millis(50));
+        assert!(!writer.is_finished(), "advance_to_next_block should be waiting on current_block.write()");
+
+        let actual = ledger.num_remaining_solutions_at_timestamp(&validator_address, 0, latest_timestamp);
+        assert_eq!(actual, expected);
+        assert_eq!(ledger.is_solution_limit_reached_at_timestamp(&validator_address, 0, latest_timestamp), actual == 0);
+
+        drop(latest_block);
+        writer.join().unwrap().unwrap();
+    }
 }