feat: fix chunk producer staking threshold (#12660)

In case of **empty seats**, the seat price of cp used to depend on the
number of existing shards (approximately `block_producer_threshold /
num_shard`), but it is no longer the case. Hence the fix.

cc @Longarithm @wacban

chain/epoch-manager/src/tests/mod.rs

@@ -2514,13 +2514,13 @@ fn test_epoch_validators_cache() {
 #[test]
 fn test_chunk_producers() {
     let amount_staked = 1_000_000;
-    // Make sure that last validator has at least 160/1'000'000  / num_shards of stake.
+    // Make sure that last validator has at least 160/1'000'000 of stake.
     // We're running with 2 shards and test1 + test2 has 2'000'000 tokens - so chunk_only should have over 160.
     let validators = vec![
         ("test1".parse().unwrap(), amount_staked),
         ("test2".parse().unwrap(), amount_staked),
-        ("chunk_only".parse().unwrap(), 200),
-        ("not_enough_producer".parse().unwrap(), 100),
+        ("chunk_only".parse().unwrap(), 321),
+        ("not_enough_producer".parse().unwrap(), 320),
     ];
 
     // There are 2 shards, and 2 block producers seats.

chain/epoch-manager/src/validator_selection.rs

@@ -373,12 +373,13 @@ fn select_chunk_producers(
     num_shards: u64,
     protocol_version: ProtocolVersion,
 ) -> (Vec<ValidatorStake>, BinaryHeap<OrderedValidatorStake>, Balance) {
-    select_validators(
-        all_proposals,
-        max_num_selected,
-        min_stake_ratio * Ratio::new(1, num_shards as u128),
-        protocol_version,
-    )
+    let min_stake_ratio =
+        if checked_feature!("stable", FixChunkProducerStakingThreshold, protocol_version) {
+            min_stake_ratio
+        } else {
+            min_stake_ratio * Ratio::new(1, num_shards as u128)
+        };
+    select_validators(all_proposals, max_num_selected, min_stake_ratio, protocol_version)
 }
 
 // Takes the top N proposals (by stake), or fewer if there are not enough or the
@@ -1037,10 +1038,10 @@ mod tests {
         // Given `epoch_info` and `proposals` above, the sample at a given height is deterministic.
         let height = 42;
         let expected_assignments = vec![
-            vec![(4, 56), (1, 168), (2, 300), (3, 84), (0, 364)],
-            vec![(3, 70), (1, 300), (4, 42), (2, 266), (0, 308)],
-            vec![(4, 42), (1, 238), (3, 42), (0, 450), (2, 196)],
-            vec![(2, 238), (1, 294), (3, 64), (0, 378)],
+            vec![(2, 192), (0, 396), (1, 280)],
+            vec![(1, 216), (2, 264), (0, 396)],
+            vec![(0, 396), (2, 288), (1, 192)],
+            vec![(2, 256), (1, 312), (0, 312)],
         ];
         assert_eq!(epoch_info.sample_chunk_validators(height), expected_assignments);
     }

core/chain-configs/src/test_genesis.rs

@@ -139,7 +139,7 @@ impl Default for TestEpochConfigBuilder {
             fishermen_threshold: FISHERMEN_THRESHOLD,
             protocol_upgrade_stake_threshold: PROTOCOL_UPGRADE_STAKE_THRESHOLD,
             minimum_stake_divisor: 10,
-            minimum_stake_ratio: Rational32::new(160i32, 1_000_000i32),
+            minimum_stake_ratio: Rational32::new(16i32, 1_000_000i32),
             chunk_producer_assignment_changes_limit: 5,
             shuffle_shard_assignment_for_chunk_producers: false,
             // consider them ineffective
@@ -628,6 +628,42 @@ pub struct GenesisAndEpochConfigParams<'a> {
 
 /// Handy factory for building test genesis and epoch config store. Use it if it is enough to have
 /// one epoch config for your test. Otherwise, just use builders directly.
+///
+/// ```
+/// use near_chain_configs::test_genesis::build_genesis_and_epoch_config_store;
+/// use near_chain_configs::test_genesis::GenesisAndEpochConfigParams;
+/// use near_chain_configs::test_genesis::ValidatorsSpec;
+/// use near_primitives::shard_layout::ShardLayout;
+/// use near_primitives::test_utils::create_test_signer;
+/// use near_primitives::types::AccountId;
+/// use near_primitives::types::AccountInfo;
+///
+/// const ONE_NEAR: u128 = 1_000_000_000_000_000_000_000_000;
+///
+/// let protocol_version = 73;
+/// let epoch_length = 10;
+/// let accounts = (0..6).map(|i| format!("test{}", i).parse().unwrap()).collect::<Vec<AccountId>>();
+/// let shard_layout = ShardLayout::multi_shard(6, 1);
+/// let validators = vec![
+///     AccountInfo {
+///         account_id: accounts[0].clone(),
+///         public_key: create_test_signer(accounts[0].as_str()).public_key(),
+///         amount: 62500 * ONE_NEAR,
+///     },
+/// ];
+/// let validators_spec = ValidatorsSpec::raw(validators, 3, 3, 3);
+/// let (genesis, epoch_config_store) = build_genesis_and_epoch_config_store(
+///     GenesisAndEpochConfigParams {
+///         protocol_version,
+///         epoch_length,
+///         accounts: &accounts,
+///         shard_layout,
+///         validators_spec,
+///     },
+///     |genesis_builder| genesis_builder.genesis_height(10000).transaction_validity_period(1000),
+///     |epoch_config_builder| epoch_config_builder.shuffle_shard_assignment_for_chunk_producers(true),
+/// );
+/// ```
 pub fn build_genesis_and_epoch_config_store<'a>(
     params: GenesisAndEpochConfigParams<'a>,
     customize_genesis_builder: impl FnOnce(TestGenesisBuilder) -> TestGenesisBuilder,

core/primitives-core/src/version.rs

@@ -121,6 +121,9 @@ pub enum ProtocolFeature {
     /// price - it reports as alpha * sum_stake instead of alpha * sum_stake / (1 - alpha), where
     /// alpha is min stake ratio
     FixStakingThreshold,
+    /// In case not all validator seats are occupied, the minimum seat price of a chunk producer
+    /// used to depend on the number of existing shards, which is no longer the case.
+    FixChunkProducerStakingThreshold,
     /// Charge for contract loading before it happens.
     #[cfg(feature = "protocol_feature_fix_contract_loading_cost")]
     FixContractLoadingCost,
@@ -253,7 +256,8 @@ impl ProtocolFeature {
             | ProtocolFeature::StateStoredReceipt => 72,
             ProtocolFeature::ExcludeContractCodeFromStateWitness => 73,
             ProtocolFeature::FixStakingThreshold
-            | ProtocolFeature::RejectBlocksWithOutdatedProtocolVersions => 74,
+            | ProtocolFeature::RejectBlocksWithOutdatedProtocolVersions
+            | ProtocolFeature::FixChunkProducerStakingThreshold => 74,
 
             // This protocol version is reserved for use in resharding tests. An extra resharding
             // is simulated on top of the latest shard layout in production. Note that later

integration-tests/src/test_loop/tests/fix_chunk_producer_stake_threshold.rs

@@ -0,0 +1,144 @@
+use crate::test_loop::builder::TestLoopBuilder;
+use crate::test_loop::env::TestLoopEnv;
+use crate::test_loop::utils::validators::get_epoch_all_validators;
+use crate::test_loop::utils::ONE_NEAR;
+use near_async::test_loop::data::TestLoopData;
+use near_async::time::Duration;
+use near_chain_configs::test_genesis::build_genesis_and_epoch_config_store;
+use near_chain_configs::test_genesis::GenesisAndEpochConfigParams;
+use near_chain_configs::test_genesis::ValidatorsSpec;
+use near_o11y::testonly::init_test_logger;
+use near_primitives::shard_layout::ShardLayout;
+use near_primitives::test_utils::create_test_signer;
+use near_primitives::types::AccountId;
+use near_primitives::types::AccountInfo;
+use near_primitives_core::version::ProtocolFeature;
+
+#[test]
+fn slow_test_fix_cp_stake_threshold() {
+    init_test_logger();
+
+    let protocol_version = ProtocolFeature::FixChunkProducerStakingThreshold.protocol_version() - 1;
+    let epoch_length = 10;
+    let accounts =
+        (0..6).map(|i| format!("test{}", i).parse().unwrap()).collect::<Vec<AccountId>>();
+    let clients = accounts.iter().cloned().collect::<Vec<_>>();
+    let num_shards = 6;
+    let shard_layout = ShardLayout::multi_shard(num_shards, 1);
+    let validators = vec![
+        AccountInfo {
+            account_id: accounts[0].clone(),
+            public_key: create_test_signer(accounts[0].as_str()).public_key(),
+            amount: 30 * 62500 * ONE_NEAR,
+        },
+        AccountInfo {
+            account_id: accounts[1].clone(),
+            public_key: create_test_signer(accounts[1].as_str()).public_key(),
+            amount: 30 * 62500 * ONE_NEAR,
+        },
+        AccountInfo {
+            account_id: accounts[2].clone(),
+            public_key: create_test_signer(accounts[2].as_str()).public_key(),
+            // cp min stake ratio was `(1 / 62500) / num_shards` before the fix
+            // stake is right at threshold, and proposal should not be approved
+            amount: ((30 + 30) * 62500 / 62500 / num_shards) as u128 * ONE_NEAR,
+        },
+        AccountInfo {
+            account_id: accounts[3].clone(),
+            public_key: create_test_signer(accounts[3].as_str()).public_key(),
+            // stake is above threshold, so proposal should be approved
+            amount: ((30 + 30) * 62500 / 62500 / num_shards + 1) as u128 * ONE_NEAR,
+        },
+    ];
+    let validators_spec = ValidatorsSpec::raw(validators, 5, 5, 5);
+    let (genesis, epoch_config_store) = build_genesis_and_epoch_config_store(
+        GenesisAndEpochConfigParams {
+            protocol_version,
+            epoch_length,
+            accounts: &accounts,
+            shard_layout,
+            validators_spec,
+        },
+        |genesis_builder| genesis_builder,
+        |epoch_config_builder| epoch_config_builder,
+    );
+
+    let TestLoopEnv { mut test_loop, datas: node_data, tempdir } = TestLoopBuilder::new()
+        .genesis(genesis)
+        .epoch_config_store(epoch_config_store.clone())
+        .clients(clients)
+        .build();
+
+    let sender = node_data[0].client_sender.clone();
+    let handle = sender.actor_handle();
+    let client = &test_loop.data.get(&handle).client;
+
+    // premise checks
+    let epoch_id = client
+        .epoch_manager
+        .get_epoch_id_from_prev_block(&client.chain.head().unwrap().last_block_hash)
+        .unwrap();
+    let protocol_version = client.epoch_manager.get_epoch_protocol_version(&epoch_id).unwrap();
+    let validators = get_epoch_all_validators(client);
+    assert!(
+        protocol_version < ProtocolFeature::FixChunkProducerStakingThreshold.protocol_version()
+    );
+    assert_eq!(
+        epoch_config_store.get_config(protocol_version).shard_layout.num_shards(),
+        num_shards
+    );
+    assert_eq!(
+        validators,
+        vec![String::from("test0"), String::from("test1"), String::from("test3")]
+    );
+
+    test_loop.run_until(
+        |test_loop_data: &mut TestLoopData| {
+            let client = &test_loop_data.get(&handle).client;
+            let head = client.chain.head().unwrap();
+            let epoch_height = client
+                .epoch_manager
+                .get_epoch_height_from_prev_block(&head.prev_block_hash)
+                .unwrap();
+            // ensure loop is exited because of condition is met
+            assert!(epoch_height < 3);
+
+            let epoch_id = client
+                .epoch_manager
+                .get_epoch_id_from_prev_block(&client.chain.head().unwrap().last_block_hash)
+                .unwrap();
+            let protocol_version =
+                client.epoch_manager.get_epoch_protocol_version(&epoch_id).unwrap();
+            // exits when protocol version catches up with the fix
+            protocol_version >= ProtocolFeature::FixChunkProducerStakingThreshold.protocol_version()
+        },
+        Duration::seconds(4 * epoch_length as i64),
+    );
+
+    test_loop.run_until(
+        |test_loop_data: &mut TestLoopData| {
+            let client = &test_loop_data.get(&handle).client;
+            let head = client.chain.head().unwrap();
+            let epoch_height = client
+                .epoch_manager
+                .get_epoch_height_from_prev_block(&head.prev_block_hash)
+                .unwrap();
+            // ensure loop is exited because of condition is met
+            assert!(epoch_height < 5);
+
+            // threshold is raised to approximately 6x of the previous in this case as threshold is
+            // no longer divided by num_shards (6), so test3's proposal won't be approved
+            let validators = get_epoch_all_validators(client);
+            if validators.len() == 2 {
+                assert_eq!(validators, vec![String::from("test0"), String::from("test1")]);
+                true
+            } else {
+                false
+            }
+        },
+        Duration::seconds(3 * epoch_length as i64),
+    );
+
+    TestLoopEnv { test_loop, datas: node_data, tempdir }
+        .shutdown_and_drain_remaining_events(Duration::seconds(20));
+}

integration-tests/src/test_loop/tests/fix_stake_threshold.rs

@@ -88,9 +88,8 @@ fn slow_test_fix_validator_stake_threshold() {
     // prior to threshold fix
     // threshold = min_stake_ratio * total_stake
     //           = (1 / 62_500) * total_stake
-    // TODO Chunk producer stake threshold is dependent on the number of shards, which should no
-    // longer be the case.  Get rid of num_shards once protocol is updated to correct the ratio.
-    assert_eq!(epoch_info.seat_price() * num_shards as u128 / ONE_NEAR, 600_000);
+    // Chunk producer stake threshold used to be dependent on the number of shards.
+    assert_eq!(epoch_info.seat_price() / ONE_NEAR, 600_000 / num_shards as u128);
 
     test_loop.run_until(
         |test_loop_data: &mut TestLoopData| {
@@ -112,12 +111,11 @@ fn slow_test_fix_validator_stake_threshold() {
                 client.epoch_manager.get_epoch_protocol_version(&epoch_id).unwrap();
             if protocol_version >= ProtocolFeature::FixStakingThreshold.protocol_version() {
                 let epoch_info = client.epoch_manager.get_epoch_info(&epoch_id).unwrap();
-                let num_shards =
-                    epoch_config_store.get_config(protocol_version).shard_layout.num_shards();
                 // after threshold fix
                 // threshold = min_stake_ratio * total_stake / (1 - min_stake_ratio)
                 //           = (1 / 62_500) * total_stake / (62_499 / 62_500)
-                assert_eq!(epoch_info.seat_price() * num_shards as u128 / ONE_NEAR, 600_001);
+                //           = total_stake / 62_499
+                assert_eq!(epoch_info.seat_price() / ONE_NEAR, total_stake / 62_499 / ONE_NEAR);
                 true
             } else {
                 false

integration-tests/src/test_loop/tests/mod.rs

@@ -7,6 +7,7 @@ mod contract_distribution_cross_shard;
 mod contract_distribution_simple;
 mod create_delete_account;
 mod epoch_sync;
+mod fix_chunk_producer_stake_threshold;
 mod fix_min_stake_ratio;
 mod fix_stake_threshold;
 mod in_memory_tries;