-
Notifications
You must be signed in to change notification settings - Fork 622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try nuking ShardLayout::V0 #12313
base: master
Are you sure you want to change the base?
Try nuking ShardLayout::V0 #12313
Changes from 4 commits
d709cf9
7ba337a
4133145
4067758
5b7d17e
af3296a
2a2710d
28b24b0
753019f
af9a154
76f0c78
99bb9ca
67522cf
52f1812
0bf8d43
1c502af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2294,7 +2294,7 @@ fn test_protocol_version_switch_with_shard_layout_change() { | |
epoch_manager.get_epoch_info(&epochs[1]).unwrap().protocol_version(), | ||
new_protocol_version - 1 | ||
); | ||
assert_eq!(epoch_manager.get_shard_layout(&epochs[1]).unwrap(), ShardLayout::v0_single_shard(),); | ||
assert_eq!(epoch_manager.get_shard_layout(&epochs[1]).unwrap(), ShardLayout::single_shard(),); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mini nit: remove the trailing comma |
||
assert_eq!( | ||
epoch_manager.get_epoch_info(&epochs[2]).unwrap().protocol_version(), | ||
new_protocol_version | ||
|
@@ -2331,7 +2331,7 @@ fn test_protocol_version_switch_with_many_seats() { | |
online_max_threshold: Ratio::new(99, 100), | ||
protocol_upgrade_stake_threshold: Ratio::new(80, 100), | ||
minimum_stake_divisor: 1, | ||
shard_layout: ShardLayout::v0_single_shard(), | ||
shard_layout: ShardLayout::single_shard(), | ||
validator_selection_config: Default::default(), | ||
validator_max_kickout_stake_perc: 100, | ||
}; | ||
|
@@ -3237,7 +3237,7 @@ fn test_chunk_validator_exempted() { | |
|
||
#[test] | ||
/// Tests the case where a chunk validator has low endorsement stats and is kicked out (not exempted). | ||
/// In this test, first 3 accounts are block and chunk producers and next 2 are chunk validator only. | ||
/// In this test, first 3 accounts are block and chunk producers and next 2 are chunk validator only. | ||
fn test_chunk_validator_kicked_out_for_low_endorsement() { | ||
test_chunk_validator_kickout( | ||
HashMap::from([( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,7 +65,7 @@ fn default_protocol_upgrade_stake_threshold() -> Rational32 { | |
} | ||
|
||
fn default_shard_layout() -> ShardLayout { | ||
ShardLayout::v0_single_shard() | ||
ShardLayout::v0(1, 0) | ||
} | ||
|
||
fn default_minimum_stake_ratio() -> Rational32 { | ||
|
@@ -194,7 +194,8 @@ pub struct GenesisConfig { | |
pub minimum_stake_divisor: u64, | ||
/// Layout information regarding how to split accounts to shards | ||
#[serde(default = "default_shard_layout")] | ||
#[default(ShardLayout::v0_single_shard())] | ||
// FIXME eagr what should be the default? | ||
#[default(ShardLayout::v0(1, 0))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally it should use the nit: The convention here seems to be to use the |
||
pub shard_layout: ShardLayout, | ||
#[serde(default = "default_num_chunk_only_producer_seats")] | ||
#[default(300)] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -241,9 +241,34 @@ pub enum ShardLayoutError { | |
} | ||
|
||
impl ShardLayout { | ||
/* Some constructors */ | ||
pub fn v0_single_shard() -> Self { | ||
Self::v0(1, 0) | ||
/// Construct a layout with a single shard | ||
pub fn single_shard() -> Self { | ||
wacban marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Self::of_num_shards(1, 0) | ||
} | ||
|
||
/// Construct a layout with given number of shards | ||
pub fn of_num_shards(num_shards: NumShards, version: ShardVersion) -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got a better idea for the fn name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was my first thought but it could also be used to create a single-shard layout, so I changed my mind. But if you like that name I'm also down with it. :) |
||
assert!(num_shards > 0, "at least 1 shard is required"); | ||
|
||
let boundary_accounts = (0..num_shards - 1) | ||
.map(|i| format!("shard{}.test.near", i).parse().unwrap()) | ||
.collect::<Vec<AccountId>>(); | ||
let shard_ids = (0..num_shards).map(ShardId::new).collect::<Vec<ShardId>>(); | ||
let (id_to_index_map, index_to_id_map) = shard_ids | ||
.iter() | ||
.enumerate() | ||
.map(|(i, &shard_id)| ((shard_id, i), (i, shard_id))) | ||
.unzip(); | ||
wacban marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Self::V2(ShardLayoutV2 { | ||
boundary_accounts, | ||
shard_ids, | ||
id_to_index_map, | ||
index_to_id_map, | ||
shards_split_map: None, | ||
shards_parent_map: None, | ||
version, | ||
}) | ||
} | ||
|
||
/// Return a V0 Shardlayout | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you mark it as deprecated? I don't know how to do this properly in rust, if it's not straight forward then a comment should do. |
||
|
@@ -811,10 +836,7 @@ mod tests { | |
use near_primitives_core::types::ProtocolVersion; | ||
use near_primitives_core::types::{AccountId, ShardId}; | ||
use near_primitives_core::version::{ProtocolFeature, PROTOCOL_VERSION}; | ||
use rand::distributions::Alphanumeric; | ||
use rand::rngs::StdRng; | ||
use rand::{Rng, SeedableRng}; | ||
use std::collections::{BTreeMap, HashMap}; | ||
use std::collections::BTreeMap; | ||
|
||
use super::{new_shards_split_map_v2, ShardVersion, ShardsSplitMap}; | ||
|
||
|
@@ -873,32 +895,6 @@ mod tests { | |
} | ||
} | ||
|
||
#[test] | ||
fn test_shard_layout_v0() { | ||
let num_shards = 4; | ||
let shard_layout = ShardLayout::v0(num_shards, 0); | ||
let mut shard_id_distribution: HashMap<ShardId, _> = | ||
shard_layout.shard_ids().map(|shard_id| (shard_id.into(), 0)).collect(); | ||
let mut rng = StdRng::from_seed([0; 32]); | ||
for _i in 0..1000 { | ||
let s: Vec<u8> = (&mut rng).sample_iter(&Alphanumeric).take(10).collect(); | ||
let s = String::from_utf8(s).unwrap(); | ||
let account_id = s.to_lowercase().parse().unwrap(); | ||
let shard_id = account_id_to_shard_id(&account_id, &shard_layout); | ||
assert!(shard_id < num_shards); | ||
*shard_id_distribution.get_mut(&shard_id).unwrap() += 1; | ||
} | ||
let expected_distribution: HashMap<ShardId, _> = [ | ||
(ShardId::new(0), 247), | ||
(ShardId::new(1), 268), | ||
(ShardId::new(2), 233), | ||
(ShardId::new(3), 252), | ||
] | ||
.into_iter() | ||
.collect(); | ||
assert_eq!(shard_id_distribution, expected_distribution); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please keep this one, the V0 may still be used when replaying some very old blocks. |
||
|
||
#[test] | ||
fn test_shard_layout_v1() { | ||
let aid = |s: &str| s.parse().unwrap(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -355,7 +355,9 @@ fn test_protocol_upgrade_81() { | |
} | ||
|
||
/// Test that Client rejects ChunkStateWitnesses with invalid shard_id | ||
// FIXME eagr unsure if this is expected behavior | ||
#[test] | ||
#[should_panic(expected = "no entry found for key")] | ||
fn test_chunk_state_witness_bad_shard_id() { | ||
init_integration_logger(); | ||
|
||
|
@@ -383,11 +385,7 @@ fn test_chunk_state_witness_bad_shard_id() { | |
// Client should reject this ChunkStateWitness and the error message should mention "shard" | ||
tracing::info!(target: "test", "Processing invalid ChunkStateWitness"); | ||
let signer = env.clients[0].validator_signer.get(); | ||
let res = env.clients[0].process_chunk_state_witness(witness, witness_size, None, signer); | ||
let error = res.unwrap_err(); | ||
let error_message = format!("{}", error).to_lowercase(); | ||
tracing::info!(target: "test", "error message: {}", error_message); | ||
assert!(error_message.contains("shard")); | ||
let _res = env.clients[0].process_chunk_state_witness(witness, witness_size, None, signer); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a panic from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah that's pretty bad. Feel free to either:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll try 3 (should probably work from the look of the code) which seems like a nice middle ground before finishing transition to V2 |
||
} | ||
|
||
/// Test that processing chunks with invalid transactions does not lead to panics | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,6 @@ use std::fs; | |
use std::fs::File; | ||
use std::io::{Read, Write}; | ||
use std::path::{Path, PathBuf}; | ||
use std::str::FromStr; | ||
use std::sync::Arc; | ||
use tracing::{info, warn}; | ||
|
||
|
@@ -964,19 +963,8 @@ pub fn init_configs( | |
CryptoHash::default(), | ||
); | ||
add_protocol_account(&mut records); | ||
let shards = if num_shards > 1 { | ||
ShardLayout::v1( | ||
(1..num_shards) | ||
.map(|f| { | ||
AccountId::from_str(format!("shard{f}.test.near").as_str()).unwrap() | ||
}) | ||
.collect(), | ||
None, | ||
1, | ||
) | ||
} else { | ||
ShardLayout::v0_single_shard() | ||
}; | ||
// FIXME eagr version? | ||
let shards = ShardLayout::of_num_shards(num_shards, 3); | ||
wacban marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
let genesis_config = GenesisConfig { | ||
protocol_version: PROTOCOL_VERSION, | ||
|
@@ -1087,7 +1075,7 @@ pub fn create_localnet_configs_from_seeds( | |
.map(|seed| InMemorySigner::from_seed("node".parse().unwrap(), KeyType::ED25519, seed)) | ||
.collect::<Vec<_>>(); | ||
|
||
let shard_layout = ShardLayout::v0(num_shards, 0); | ||
let shard_layout = ShardLayout::of_num_shards(num_shards, 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would cause some sanity check to fail as you could see from the CI logs. It seems like some json parsing issue. Not sure whether if you'd like to keep it as it was or to update the config somewhere else to make it work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's try updating the config and if it doesn't work leave as is. |
||
let accounts_to_add_to_genesis: Vec<AccountId> = | ||
seeds.iter().map(|s| s.parse().unwrap()).collect(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,6 @@ impl CorruptStateSnapshotCommand { | |
let mut store_update = store.store_update(); | ||
// TODO(resharding) automatically detect the shard version | ||
let shard_layout = match self.shard_layout_version { | ||
0 => ShardLayout::v0(1, 0), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you keep this one? |
||
1 => ShardLayout::get_simple_nightshade_layout(), | ||
2 => ShardLayout::get_simple_nightshade_layout_v2(), | ||
_ => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep this?