refactor: Make validator_signer mutable (#11400)

Issue: #11264

This is follow-up to #11372.
The actual changes (+test) for
#11264 will be done in a third,
final PR: #11536.

### Summary
This PR should mostly be no-op. It focuses on propagating
`MutableConfigValue` for `validator_signer` everywhere.
All instances of mutable `validator_signer` are synchronized.
In case validator_id only is needed, we propagate `validator_signer`
anyway as it contains the current validator info.

### Extra changes
- Remove signer as a field and pass to methods instead: `Doomslug`,
`InfoHelper`, `ChunkValidator`.
- Make some public methods internal where they do not need to be public.
- Split `process_ready_orphan_witnesses_and_clean_old` into two
functions.
- Removed `block_production_started` from `ClientActorInner`.
- Add `FrozenValidatorConfig` to make it possible to return a snapshot
of `ValidatorConfig`.

---------

Co-authored-by: Your Name <you@example.com>

chain/chain-primitives/src/error.rs

@@ -198,8 +198,8 @@ pub enum Error {
     #[error("Invalid Split Shard Ids when resharding. shard_id: {0}, parent_shard_id: {1}")]
     InvalidSplitShardsIds(u64, u64),
     /// Someone is not a validator. Usually happens in signature verification
-    #[error("Not A Validator")]
-    NotAValidator,
+    #[error("Not A Validator: {0}")]
+    NotAValidator(String),
     /// Someone is not a chunk validator. Happens if we're asked to validate a chunk we're not
     /// supposed to validate, or to verify a chunk approval signed by a validator that isn't
     /// supposed to validate the chunk.
@@ -308,7 +308,7 @@ impl Error {
             | Error::InvalidRandomnessBeaconOutput
             | Error::InvalidBlockMerkleRoot
             | Error::InvalidProtocolVersion
-            | Error::NotAValidator
+            | Error::NotAValidator(_)
             | Error::NotAChunkValidator
             | Error::InvalidChallengeRoot => true,
         }
@@ -384,7 +384,7 @@ impl Error {
             Error::InvalidRandomnessBeaconOutput => "invalid_randomness_beacon_output",
             Error::InvalidBlockMerkleRoot => "invalid_block_merkele_root",
             Error::InvalidProtocolVersion => "invalid_protocol_version",
-            Error::NotAValidator => "not_a_validator",
+            Error::NotAValidator(_) => "not_a_validator",
             Error::NotAChunkValidator => "not_a_chunk_validator",
             Error::InvalidChallengeRoot => "invalid_challenge_root",
         }
@@ -396,7 +396,9 @@ impl From<EpochError> for Error {
         match error {
             EpochError::EpochOutOfBounds(epoch_id) => Error::EpochOutOfBounds(epoch_id),
             EpochError::MissingBlock(h) => Error::DBNotFoundErr(format!("epoch block: {h}")),
-            EpochError::NotAValidator(_account_id, _epoch_id) => Error::NotAValidator,
+            EpochError::NotAValidator(account_id, epoch_id) => {
+                Error::NotAValidator(format!("account_id: {account_id}, epoch_id: {epoch_id:?}"))
+            }
             err => Error::ValidatorError(err.to_string()),
         }
     }

chain/chain/src/chain.rs

@@ -87,6 +87,7 @@ use near_primitives::unwrap_or_return;
 #[cfg(feature = "new_epoch_sync")]
 use near_primitives::utils::index_to_bytes;
 use near_primitives::utils::MaybeValidated;
+use near_primitives::validator_signer::ValidatorSigner;
 use near_primitives::version::{ProtocolFeature, ProtocolVersion, PROTOCOL_VERSION};
 use near_primitives::views::{
     BlockStatusView, DroppedReason, ExecutionOutcomeWithIdView, ExecutionStatusView,
@@ -400,7 +401,7 @@ impl Chain {
         chain_config: ChainConfig,
         snapshot_callbacks: Option<SnapshotCallbacks>,
         apply_chunks_spawner: Arc<dyn AsyncComputationSpawner>,
-        validator_account_id: Option<&AccountId>,
+        validator: MutableConfigValue<Option<Arc<ValidatorSigner>>>,
     ) -> Result<Chain, Error> {
         // Get runtime initial state and create genesis block out of it.
         let state_roots = get_genesis_state_roots(runtime_adapter.store())?
@@ -539,7 +540,7 @@ impl Chain {
             .iter()
             .filter(|shard_uid| {
                 shard_tracker.care_about_shard(
-                    validator_account_id,
+                    validator.get().map(|v| v.validator_id().clone()).as_ref(),
                     &tip.prev_block_hash,
                     shard_uid.shard_id(),
                     true,

chain/chain/src/doomslug.rs

@@ -138,7 +138,6 @@ pub struct Doomslug {
     endorsement_pending: bool,
     /// Information to track the timer (see `start_timer` routine in the paper)
     timer: DoomslugTimer,
-    signer: Option<Arc<ValidatorSigner>>,
     /// How many approvals to have before producing a block. In production should be always `HalfStake`,
     ///    but for many tests we use `NoApprovals` to invoke more forkfulness
     threshold_mode: DoomslugThresholdMode,
@@ -362,7 +361,6 @@ impl Doomslug {
         min_delay: Duration,
         delay_step: Duration,
         max_delay: Duration,
-        signer: Option<Arc<ValidatorSigner>>,
         threshold_mode: DoomslugThresholdMode,
     ) -> Self {
         Doomslug {
@@ -392,7 +390,6 @@ impl Doomslug {
                 delay_step,
                 max_delay,
             },
-            signer,
             threshold_mode,
             history: VecDeque::new(),
         }
@@ -465,7 +462,7 @@ impl Doomslug {
     /// A vector of approvals that need to be sent to other block producers as a result of processing
     /// the timers
     #[must_use]
-    pub fn process_timer(&mut self) -> Vec<Approval> {
+    pub fn process_timer(&mut self, signer: &Option<Arc<ValidatorSigner>>) -> Vec<Approval> {
         let now = self.clock.now();
         let mut ret = vec![];
         for _ in 0..MAX_TIMER_ITERS {
@@ -486,7 +483,7 @@ impl Doomslug {
                 if tip_height >= self.largest_target_height.get() {
                     self.largest_target_height.set(tip_height + 1);
 
-                    if let Some(approval) = self.create_approval(tip_height + 1) {
+                    if let Some(approval) = self.create_approval(tip_height + 1, signer) {
                         ret.push(approval);
                     }
                     self.update_history(ApprovalHistoryEntry {
@@ -514,7 +511,7 @@ impl Doomslug {
                 self.largest_target_height
                     .set(std::cmp::max(self.timer.height + 1, self.largest_target_height.get()));
 
-                if let Some(approval) = self.create_approval(self.timer.height + 1) {
+                if let Some(approval) = self.create_approval(self.timer.height + 1, signer) {
                     ret.push(approval);
                 }
                 self.update_history(ApprovalHistoryEntry {
@@ -541,9 +538,13 @@ impl Doomslug {
         ret
     }
 
-    fn create_approval(&self, target_height: BlockHeight) -> Option<Approval> {
-        self.signer.as_ref().map(|signer| {
-            Approval::new(self.tip.block_hash, self.tip.height, target_height, &**signer)
+    fn create_approval(
+        &self,
+        target_height: BlockHeight,
+        signer: &Option<Arc<ValidatorSigner>>,
+    ) -> Option<Approval> {
+        signer.as_ref().map(|signer| {
+            Approval::new(self.tip.block_hash, self.tip.height, target_height, &*signer)
         })
     }
 
@@ -787,36 +788,36 @@ mod tests {
     #[test]
     fn test_endorsements_and_skips_basic() {
         let clock = FakeClock::new(Utc::UNIX_EPOCH);
+        let signer = Some(Arc::new(create_test_signer("test").into()));
         let mut ds = Doomslug::new(
             clock.clock(),
             0,
             Duration::milliseconds(400),
             Duration::milliseconds(1000),
             Duration::milliseconds(100),
             Duration::milliseconds(3000),
-            Some(Arc::new(create_test_signer("test"))),
             DoomslugThresholdMode::TwoThirds,
         );
 
         // Set a new tip, must produce an endorsement
         ds.set_tip(hash(&[1]), 1, 1);
         clock.advance(Duration::milliseconds(399));
-        assert_eq!(ds.process_timer().len(), 0);
+        assert_eq!(ds.process_timer(&signer).len(), 0);
         clock.advance(Duration::milliseconds(1));
-        let approval = ds.process_timer().into_iter().nth(0).unwrap();
+        let approval = ds.process_timer(&signer).into_iter().nth(0).unwrap();
         assert_eq!(approval.inner, ApprovalInner::Endorsement(hash(&[1])));
         assert_eq!(approval.target_height, 2);
 
         // Same tip => no approval
-        assert_eq!(ds.process_timer(), vec![]);
+        assert_eq!(ds.process_timer(&signer), vec![]);
 
         // The block was `ds_final` and therefore started the timer. Try checking before one second expires
         clock.advance(Duration::milliseconds(599));
-        assert_eq!(ds.process_timer(), vec![]);
+        assert_eq!(ds.process_timer(&signer), vec![]);
 
         // But one second should trigger the skip
         clock.advance(Duration::milliseconds(1));
-        match ds.process_timer() {
+        match ds.process_timer(&signer) {
             approvals if approvals.is_empty() => assert!(false),
             approvals => {
                 assert_eq!(approvals[0].inner, ApprovalInner::Skip(1));
@@ -827,26 +828,26 @@ mod tests {
         // Not processing a block at height 2 should not produce an appoval
         ds.set_tip(hash(&[2]), 2, 0);
         clock.advance(Duration::milliseconds(400));
-        assert_eq!(ds.process_timer(), vec![]);
+        assert_eq!(ds.process_timer(&signer), vec![]);
 
         // Go forward more so we have 1 second
         clock.advance(Duration::milliseconds(600));
 
         // But at height 3 should (also neither block has finality set, keep last final at 0 for now)
         ds.set_tip(hash(&[3]), 3, 0);
         clock.advance(Duration::milliseconds(400));
-        let approval = ds.process_timer().into_iter().nth(0).unwrap();
+        let approval = ds.process_timer(&signer).into_iter().nth(0).unwrap();
         assert_eq!(approval.inner, ApprovalInner::Endorsement(hash(&[3])));
         assert_eq!(approval.target_height, 4);
 
         // Go forward more so we have another second
         clock.advance(Duration::milliseconds(600));
 
         clock.advance(Duration::milliseconds(199));
-        assert_eq!(ds.process_timer(), vec![]);
+        assert_eq!(ds.process_timer(&signer), vec![]);
 
         clock.advance(Duration::milliseconds(1));
-        match ds.process_timer() {
+        match ds.process_timer(&signer) {
             approvals if approvals.is_empty() => assert!(false),
             approvals if approvals.len() == 1 => {
                 assert_eq!(approvals[0].inner, ApprovalInner::Skip(3));
@@ -860,10 +861,10 @@ mod tests {
 
         // Now skip 5 (the extra delay is 200+300 = 500)
         clock.advance(Duration::milliseconds(499));
-        assert_eq!(ds.process_timer(), vec![]);
+        assert_eq!(ds.process_timer(&signer), vec![]);
 
         clock.advance(Duration::milliseconds(1));
-        match ds.process_timer() {
+        match ds.process_timer(&signer) {
             approvals if approvals.is_empty() => assert!(false),
             approvals => {
                 assert_eq!(approvals[0].inner, ApprovalInner::Skip(3));
@@ -876,10 +877,10 @@ mod tests {
 
         // Skip 6 (the extra delay is 0+200+300+400 = 900)
         clock.advance(Duration::milliseconds(899));
-        assert_eq!(ds.process_timer(), vec![]);
+        assert_eq!(ds.process_timer(&signer), vec![]);
 
         clock.advance(Duration::milliseconds(1));
-        match ds.process_timer() {
+        match ds.process_timer(&signer) {
             approvals if approvals.is_empty() => assert!(false),
             approvals => {
                 assert_eq!(approvals[0].inner, ApprovalInner::Skip(3));
@@ -893,11 +894,11 @@ mod tests {
         // Accept block at 5 with finality on the prev block, expect it to not produce an approval
         ds.set_tip(hash(&[5]), 5, 4);
         clock.advance(Duration::milliseconds(400));
-        assert_eq!(ds.process_timer(), vec![]);
+        assert_eq!(ds.process_timer(&signer), vec![]);
 
         // Skip a whole bunch of heights by moving 100 seconds ahead
         clock.advance(Duration::seconds(100));
-        assert!(ds.process_timer().len() > 10);
+        assert!(ds.process_timer(&signer).len() > 10);
 
         // Add some random small number of milliseconds to test that when the next block is added, the
         // timer is reset
@@ -906,15 +907,15 @@ mod tests {
         // No approval, since we skipped 6
         ds.set_tip(hash(&[6]), 6, 4);
         clock.advance(Duration::milliseconds(400));
-        assert_eq!(ds.process_timer(), vec![]);
+        assert_eq!(ds.process_timer(&signer), vec![]);
 
         // The block height was less than the timer height, and thus the timer was reset.
         // The wait time for height 7 with last ds final block at 5 is 1100
         clock.advance(Duration::milliseconds(699));
-        assert_eq!(ds.process_timer(), vec![]);
+        assert_eq!(ds.process_timer(&signer), vec![]);
 
         clock.advance(Duration::milliseconds(1));
-        match ds.process_timer() {
+        match ds.process_timer(&signer) {
             approvals if approvals.is_empty() => assert!(false),
             approvals => {
                 assert_eq!(approvals[0].inner, ApprovalInner::Skip(6));
@@ -942,7 +943,6 @@ mod tests {
             .map(|(account_id, _, _)| create_test_signer(account_id))
             .collect::<Vec<_>>();
 
-        let signer = Arc::new(create_test_signer("test"));
         let clock = FakeClock::new(Utc::UNIX_EPOCH);
         let mut ds = Doomslug::new(
             clock.clock(),
@@ -951,7 +951,6 @@ mod tests {
             Duration::milliseconds(1000),
             Duration::milliseconds(100),
             Duration::milliseconds(3000),
-            Some(signer),
             DoomslugThresholdMode::TwoThirds,
         );
 

chain/chain/src/runtime/tests.rs

@@ -21,8 +21,8 @@ use num_rational::Ratio;
 use rand::{rngs::StdRng, seq::SliceRandom, SeedableRng};
 
 use near_chain_configs::{
-    default_produce_chunk_add_transactions_time_limit, Genesis, DEFAULT_GC_NUM_EPOCHS_TO_KEEP,
-    NEAR_BASE,
+    default_produce_chunk_add_transactions_time_limit, Genesis, MutableConfigValue,
+    DEFAULT_GC_NUM_EPOCHS_TO_KEEP, NEAR_BASE,
 };
 use near_crypto::{InMemorySigner, KeyType, Signer};
 use near_o11y::testonly::init_test_logger;
@@ -1619,7 +1619,7 @@ fn get_test_env_with_chain_and_pool() -> (TestEnv, Chain, TransactionPool) {
         ChainConfig::test(),
         None,
         Arc::new(RayonAsyncComputationSpawner),
-        None,
+        MutableConfigValue::new(None, "validator_signer"),
     )
     .unwrap();
 

chain/chain/src/store_validator.rs

@@ -382,7 +382,7 @@ impl StoreValidator {
 #[cfg(test)]
 mod tests {
     use near_async::time::Clock;
-    use near_chain_configs::Genesis;
+    use near_chain_configs::{Genesis, MutableConfigValue};
     use near_epoch_manager::EpochManager;
     use near_store::genesis::initialize_genesis_state;
     use near_store::test_utils::create_test_store;
@@ -418,7 +418,7 @@ mod tests {
             ChainConfig::test(),
             None,
             Arc::new(RayonAsyncComputationSpawner),
-            None,
+            MutableConfigValue::new(None, "validator_signer"),
         )
         .unwrap();
         (

chain/chain/src/test_utils.rs

@@ -13,7 +13,7 @@ use crate::types::{AcceptedBlock, ChainConfig, ChainGenesis};
 use crate::DoomslugThresholdMode;
 use crate::{BlockProcessingArtifact, Provenance};
 use near_async::time::Clock;
-use near_chain_configs::Genesis;
+use near_chain_configs::{Genesis, MutableConfigValue};
 use near_chain_primitives::Error;
 use near_epoch_manager::shard_tracker::ShardTracker;
 use near_epoch_manager::{EpochManager, EpochManagerHandle};
@@ -75,7 +75,7 @@ pub fn get_chain_with_epoch_length_and_num_shards(
         ChainConfig::test(),
         None,
         Arc::new(RayonAsyncComputationSpawner),
-        None,
+        MutableConfigValue::new(None, "validator_signer"),
     )
     .unwrap()
 }
@@ -159,7 +159,7 @@ pub fn setup_with_tx_validity_period(
         ChainConfig::test(),
         None,
         Arc::new(RayonAsyncComputationSpawner),
-        None,
+        MutableConfigValue::new(None, "validator_signer"),
     )
     .unwrap();