-
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 all 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 |
---|---|---|
|
@@ -164,17 +164,7 @@ impl ShardLayoutV1 { | |
} | ||
|
||
/// Making the shard ids non-contiguous. | ||
#[derive( | ||
BorshSerialize, | ||
BorshDeserialize, | ||
serde::Serialize, | ||
serde::Deserialize, | ||
Clone, | ||
Debug, | ||
PartialEq, | ||
Eq, | ||
ProtocolSchema, | ||
)] | ||
#[derive(BorshSerialize, BorshDeserialize, Clone, Debug, PartialEq, Eq, ProtocolSchema)] | ||
pub struct ShardLayoutV2 { | ||
/// The boundary accounts are the accounts on boundaries between shards. | ||
/// Each shard contains a range of accounts from one boundary account to | ||
|
@@ -212,6 +202,114 @@ pub struct ShardLayoutV2 { | |
version: ShardVersion, | ||
} | ||
|
||
/// Counterpart to `ShardLayoutV2` composed of maps with string keys to aid | ||
/// serde serialization. | ||
#[derive(serde::Serialize, serde::Deserialize)] | ||
struct SerdeShardLayoutV2 { | ||
boundary_accounts: Vec<AccountId>, | ||
shard_ids: Vec<ShardId>, | ||
id_to_index_map: BTreeMap<String, ShardIndex>, | ||
index_to_id_map: BTreeMap<String, ShardId>, | ||
shards_split_map: Option<BTreeMap<String, Vec<ShardId>>>, | ||
shards_parent_map: Option<BTreeMap<String, ShardId>>, | ||
version: ShardVersion, | ||
} | ||
|
||
impl From<&ShardLayoutV2> for SerdeShardLayoutV2 { | ||
fn from(layout: &ShardLayoutV2) -> Self { | ||
let id_to_index_map = | ||
layout.id_to_index_map.iter().map(|(k, v)| (k.to_string(), *v)).collect(); | ||
Comment on lines
+220
to
+221
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 is completely fine but I would be tempted to write a generic function that converts a Map<ShardId, T> to Map<String, T> and use it for all the maps in the shard layout. We're looking at whooping potential savings of ~2 lines of code so up to you if you think it's worth it :) |
||
|
||
let index_to_id_map = | ||
layout.index_to_id_map.iter().map(|(k, v)| (k.to_string(), *v)).collect(); | ||
|
||
let shards_split_map = layout | ||
.shards_split_map | ||
.as_ref() | ||
.map(|map| map.iter().map(|(k, v)| (k.to_string(), v.clone())).collect()); | ||
|
||
let shards_parent_map = layout | ||
.shards_parent_map | ||
.as_ref() | ||
.map(|map| map.iter().map(|(k, v)| (k.to_string(), *v)).collect()); | ||
|
||
Self { | ||
boundary_accounts: layout.boundary_accounts.clone(), | ||
shard_ids: layout.shard_ids.clone(), | ||
id_to_index_map, | ||
index_to_id_map, | ||
shards_split_map, | ||
shards_parent_map, | ||
version: layout.version, | ||
} | ||
} | ||
} | ||
|
||
impl TryFrom<SerdeShardLayoutV2> for ShardLayoutV2 { | ||
type Error = Box<dyn std::error::Error + Send + Sync>; | ||
|
||
fn try_from(layout: SerdeShardLayoutV2) -> Result<Self, Self::Error> { | ||
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: May unpack the layout as first step and then use the unpacked values directly? It may be a bit prettier and it would be more obvious that there isn't unnecessary cloning. |
||
let id_to_index_map = layout | ||
.id_to_index_map | ||
.into_iter() | ||
.map(|(k, v)| Ok((k.parse::<u64>()?.into(), v))) | ||
.collect::<Result<_, Self::Error>>()?; | ||
Comment on lines
+252
to
+256
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. Ditto about generic function for this but here it may actually make some sense because it's less trivial logic. |
||
|
||
let index_to_id_map = layout | ||
.index_to_id_map | ||
.into_iter() | ||
.map(|(k, v)| Ok((k.parse()?, v))) | ||
.collect::<Result<_, Self::Error>>()?; | ||
|
||
let shards_split_map = layout | ||
.shards_split_map | ||
.map(|map| { | ||
map.into_iter() | ||
.map(|(k, v)| Ok((k.parse::<u64>()?.into(), v))) | ||
.collect::<Result<_, Self::Error>>() | ||
}) | ||
.transpose()?; | ||
|
||
let shards_parent_map = layout | ||
.shards_parent_map | ||
.map(|map| { | ||
map.into_iter() | ||
.map(|(k, v)| Ok((k.parse::<u64>()?.into(), v))) | ||
.collect::<Result<_, Self::Error>>() | ||
}) | ||
.transpose()?; | ||
|
||
Ok(Self { | ||
boundary_accounts: layout.boundary_accounts, | ||
shard_ids: layout.shard_ids, | ||
id_to_index_map, | ||
index_to_id_map, | ||
shards_split_map, | ||
shards_parent_map, | ||
version: layout.version, | ||
}) | ||
} | ||
} | ||
|
||
impl serde::Serialize for ShardLayoutV2 { | ||
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
wacban marked this conversation as resolved.
Show resolved
Hide resolved
|
||
where | ||
S: serde::Serializer, | ||
{ | ||
SerdeShardLayoutV2::from(self).serialize(serializer) | ||
} | ||
} | ||
|
||
impl<'de> serde::Deserialize<'de> for ShardLayoutV2 { | ||
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. nit: I think the convention is to call the lifespan |
||
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
where | ||
D: serde::Deserializer<'de>, | ||
{ | ||
let serde_layout = SerdeShardLayoutV2::deserialize(deserializer)?; | ||
ShardLayoutV2::try_from(serde_layout).map_err(serde::de::Error::custom) | ||
} | ||
} | ||
|
||
impl ShardLayoutV2 { | ||
pub fn account_id_to_shard_id(&self, account_id: &AccountId) -> ShardId { | ||
// TODO(resharding) - This could be optimized. | ||
|
@@ -241,9 +339,34 @@ pub enum ShardLayoutError { | |
} | ||
|
||
impl ShardLayout { | ||
/* Some constructors */ | ||
pub fn v0_single_shard() -> Self { | ||
Self::v0(1, 0) | ||
/// Handy constructor for a single-shard layout, mostly for test purposes | ||
pub fn single_shard() -> Self { | ||
wacban marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Self::multi_shard(1, 0) | ||
} | ||
|
||
/// Can be used to construct a multi-shard layout, mostly for test purposes | ||
pub fn multi_shard(num_shards: NumShards, version: ShardVersion) -> Self { | ||
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. |
||
|
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?