feat(resharding) - Introducing ShardLayoutV2 and nighshade layout v4 #12066

wacban · 2024-09-09T17:26:50Z

Adding a new shard layout structure. It replaces the boundary accounts with a mapping from shard id to the account range of that shard. This is necessary in order to implement the account id to shard id mapping. Previously it relied on shard ids being contiguous and ordered by the account ranges, neither of which will be the case in this shard layout.

codecov · 2024-09-09T17:48:01Z

Codecov Report

Attention: Patch coverage is 93.75000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 71.60%. Comparing base (e5aa208) to head (797ce1f).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
core/primitives/src/shard_layout.rs	93.68%	11 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12066      +/-   ##
==========================================
+ Coverage   71.59%   71.60%   +0.01%     
==========================================
  Files         823      824       +1     
  Lines      165137   165348     +211     
  Branches   165137   165348     +211     
==========================================
+ Hits       118223   118400     +177     
- Misses      41789    41819      +30     
- Partials     5125     5129       +4

Flag	Coverage Δ
backward-compatibility	`0.17% <0.00%> (-0.01%)`	⬇️
db-migration	`0.17% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.26% <0.00%> (-0.01%)`	⬇️
integration-tests	`38.71% <9.37%> (-0.05%)`	⬇️
linux	`71.38% <93.75%> (+0.01%)`	⬆️
linux-nightly	`71.18% <93.75%> (+0.02%)`	⬆️
macos	`54.09% <93.22%> (+0.03%)`	⬆️
pytests	`1.52% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.32% <0.00%> (-0.01%)`	⬇️
unittests	`65.33% <93.22%> (+0.03%)`	⬆️
upgradability	`0.21% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Longarithm · 2024-09-09T21:12:36Z

core/primitives/src/shard_layout.rs

+    /// A mapping from the shard id to the account range of this shard.
+    shards_account_range: ShardsAccountRange,


Why not to leave boundary_accounts: Vec<AccountId> and just add new field shard_ids: Vec<ShardId>? Then we would have to validate only that boundary_accounts.len() + 1 == shard_ids.len() and that boundary_accounts is in ascending order. After that, we can construct immediately valid ShardsAccountRange for local usage. This would simplify validation in general.

Yeah, this can be a fine and more simplistic strategy, but I don't feel too strongly about it vs the current implementation.

I think either would work but I feel like shard id => range mapping is easier for humans to parse. I'm also considering changing it to range => shard id as that would make the account_id_to_shard_id query faster.

I agree with human readability argument. But I think readability should be handled by some tool over the shard layout format, like we already do for the latest snapshot of runtime config. I prefer the base primitive to be as concise as possible. For me it's like getting rpc response in json or text - both make sense, but json must be the base format.

If we go with ranges, start and end are valid accounts so we need some placeholders which are definitely invalid ($start?)

All right, I'm fine with that, will do tomorrow.

If we go with ranges, start and end are valid accounts so we need some placeholders which are definitely invalid ($start?)

I wouldn't want to use magic values but perhaps I can use Unbounded for those.

Done and it actually simplified a lot of things, thanks!

The account ranges are no longer needed so I didn't need to use the std ranges.

shreyan-gupta · 2024-09-09T23:50:28Z

core/primitives/src/shard_layout.rs

+        let prev = values[i - 1];
+        let curr = values[i];
+        if prev.end != curr.start {
+            return Err(err("account ranges should be contiguous"));


nit: can probably also include the offending account value in the error message?

shreyan-gupta · 2024-09-10T00:06:35Z

core/primitives/src/shard_layout.rs

+    /// A mapping from the shard id to the account range of this shard.
+    shards_account_range: ShardsAccountRange,
+
+    /// A mapping from the parent shard to child shards. Maps shards from the


I'm wondering if it makes sense to have this struct contain all historic mappings instead of just the latest split?

That would be quite useful for store layer where we may recursively need to find parent shard_ids

Maybe but let's keep it simple for now and we can enhance it later as needed.

shreyan-gupta · 2024-09-10T00:06:55Z

core/primitives/src/shard_layout.rs

+    shards_parent_map: Option<ShardsParentMapV2>,
+
+    /// Version of the shard layout, this is useful for uniquely identify the shard layout
+    version: ShardVersion,


I hope with LayoutV2, we would finally increment version by 1?

There are a few options here:

bump the version and do a migration (since state and flat state are keyed by shard uid)

bump the version and add an override on storage layer access, along the lines of if version >= 3 { version = 3 }

do not bump the version

The third one doesn't require any extra changes and for that reason it's my favourite. We can just stick to version 3 until we need to make a real change and then we can figure it out. WDYT?

no bump ➕

shreyan-gupta · 2024-09-10T00:08:20Z

core/primitives/src/shard_layout.rs

+    /// A mapping from the shard id to the account range of this shard.
+    shards_account_range: ShardsAccountRange,


Yeah, this can be a fine and more simplistic strategy, but I don't feel too strongly about it vs the current implementation.

shreyan-gupta · 2024-09-10T00:09:36Z

core/primitives/src/shard_layout.rs

+    fn test_shard_layout_v4() {
+        // Test ShardsAccountRange validation
+
+        // Test account id to shard id


Is this intentionally left empty?

No, I forgot to add this test, thanks!

core/primitives/src/shard_layout.rs

staffik · 2024-09-12T09:38:53Z

We might want to do sth with ShardUId::next_shard_prefix().
Also, there is remove_range_by_shard_uid():

fn remove_range_by_shard_uid(store_update: &mut StoreUpdate, shard_uid: ShardUId, col: DBCol) {
    let key_from = shard_uid.to_bytes();
    let key_to = ShardUId::next_shard_prefix(&key_from);
    store_update.delete_range(col, &key_from, &key_to);
}

Trisfald · 2024-09-19T08:04:06Z

core/primitives/src/shard_layout.rs

+        |reason: &str| ShardLayoutError::InvalidShardsAccountRange { reason: reason.to_string() };
+
+    let values = shards_account_range.values().sorted().collect_vec();
+    println!("{:?}", values);


is this meant to be a log message?

Trisfald · 2024-09-19T08:10:58Z

core/primitives/src/shard_layout.rs

+        Self { start: AccountBoundary::Middle(start.parse().unwrap()), end: AccountBoundary::End }
+    }
+
+    pub fn new_mid(start: &str, end: &str) -> Self {


why is &str preferred to &AccountId?

Technically it should be AccountId but it's just so much more conveninent to
pass &str because then I just need to do .parse().unwrap() in one place. I'm
happy to do it proper way and add some convenience functions just for the tests,
please let me now if you think it's worth it.

I'm inclined to think it's worth it, just because all these methods are public, but I don't have a strong preference.

wacban · 2024-09-25T15:24:30Z

Reviewers, can you have another look please? The most notable change is that the mapping is now from account range to shard id (used to be the other way around). It will allows us to optimize the account id to shard mapping in the future. Also addressed the comments and did some cleanups.

Trisfald

🦀 🚀

core/primitives/src/shard_layout.rs

Trisfald · 2024-09-25T15:29:50Z

core/primitives/src/shard_layout.rs

+    // In this shard layout the accounts are divided into ranges, each range is
+    // mapped to a shard. The shards are contiguous and start from 0.
+    fn account_id_to_shard_id(&self, account_id: &AccountId) -> ShardId {
+        // Note: As we scale up the number of shards we can consider


nit: note is outdated, in a certain sense
I think no one will add more shards to V1 layout

core/primitives/src/shard_layout.rs

Trisfald · 2024-09-25T15:40:32Z

core/primitives/src/shard_layout.rs

+    /// This layout is provisional, the actual shard layout should be determined
+    /// based on the fresh data before the resharding.
+    pub fn get_simple_nightshade_layout_v4() -> ShardLayout {
+        // the boundary accounts in lexicographical order


nit: is it worth to add todo!() or similar compiler help?

Add todo where?

I'm hoping we can get v4 into nightly soon enough and start testing it there. I wouldn't want compiler panicking there.

Just to prevent using the provisional layout by mistake, but anyway we are going to check this again before releasing

Trisfald · 2024-09-25T15:44:35Z

core/primitives/src/shard_layout.rs

+        };
+
+        let mut shards_parent_map = ShardsParentMapV2::new();
+        for (&parent_shard_id, shard_ids) in shards_split_map.iter() {


I'm wondering if at some point we would need to implement a check such as: at most one shard is being split in a given ShardLayout. Maybe another day :)

I'm wondering if we can perform a few splits at the same time :)

Maybe another day :)

Co-authored-by: Andrea <[email protected]>

core/primitives/src/shard_layout.rs

Longarithm

My main concern is about where readability should be handled.

Longarithm · 2024-09-25T21:32:50Z

core/primitives/src/shard_layout.rs

+    /// TODO(resharding) Determine the shard layout for v4.
+    /// This layout is provisional, the actual shard layout should be determined
+    /// based on the fresh data before the resharding.
+    pub fn get_simple_nightshade_layout_v4() -> ShardLayout {


Marketing nit: what about dropping simple and saying get_nightshade_layout_v4 since now?
The ultimate version may be called get_dynamic_nightshade_layout

Technically speaking this particular layout will not be dynamic so I wouldn't want to call it that quite yet. I called it simple for consistency and greppability. I'm not sure the codebase is the best place for marketing but if you like it more I'm happy to rename it to get_nightshade_layout_v4.

Longarithm · 2024-09-25T21:42:15Z

core/primitives/src/shard_layout.rs

+    fn contains(&self, account_id: &AccountId) -> bool {
+        match (&self.start, &self.end) {
+            (AccountBoundary::Start, AccountBoundary::End) => true,
+            (AccountBoundary::Start, AccountBoundary::Middle(end)) => account_id < end,
+            (AccountBoundary::Middle(start), AccountBoundary::End) => account_id >= start,
+            (AccountBoundary::Middle(start), AccountBoundary::Middle(end)) => {
+                account_id >= start && account_id < end
+            }
+            (AccountBoundary::End, _) => panic!("invalid account range"),
+            (_, AccountBoundary::Start) => panic!("invalid account range"),
+        }
+    }
+}


Alternative:

I remembered there are primitives std::ops::RangeBounds and std::ops::Bound. We can represent shard ranges using them. RangeBounds::contains looks exactly like this function, with exception that there is Bound::Unbounded which by definition means both start and end, depending on which side of range it represents. So panic is not needed there.

There are also RangeFull, RangeTo, RangeFrom.

Oh nice, thanks! I should have guessed there is some ready structure for this. I'll check it out tomorrow!

Would be nice to use primitives in std if it's simple enough for our use case

Longarithm · 2024-09-25T21:46:50Z

core/primitives/src/shard_layout.rs

+    /// A mapping from the shard id to the account range of this shard.
+    shards_account_range: ShardsAccountRange,


I agree with human readability argument. But I think readability should be handled by some tool over the shard layout format, like we already do for the latest snapshot of runtime config. I prefer the base primitive to be as concise as possible. For me it's like getting rpc response in json or text - both make sense, but json must be the base format.

If we go with ranges, start and end are valid accounts so we need some placeholders which are definitely invalid ($start?)

shreyan-gupta · 2024-09-27T17:20:48Z

core/primitives/src/shard_layout.rs

+    shards_split_map: Option<ShardsSplitMapV2>,
+    /// A mapping from the child shard to the parent shard. Maps shards in this
+    /// shard layout to their parent shards.
+    shards_parent_map: Option<ShardsParentMapV2>,


Why are the shards_parent_map and shards_split_map optional?

It's for the genesis case. It's used in tests mostly.

Longarithm · 2024-09-30T10:14:37Z

core/primitives/src/shard_layout.rs

@@ -135,6 +213,51 @@ impl ShardLayout {
        })
    }

+    /// Return a V2 Shardlayout
+    pub fn v2(
+        boundary_accounts: Vec<AccountId>,


We can also add debug assert that boundary_accounts is strictly sorted.

feat(resharding) - Introducing ShardLayoutV2 and nighshade layout v4

5f93632

wacban requested review from Longarithm and shreyan-gupta September 9, 2024 17:26

wacban requested a review from a team as a code owner September 9, 2024 17:26

Longarithm reviewed Sep 9, 2024

View reviewed changes

shreyan-gupta approved these changes Sep 10, 2024

View reviewed changes

tayfunelmas reviewed Sep 10, 2024

View reviewed changes

core/primitives/src/shard_layout.rs Show resolved Hide resolved

tayfunelmas reviewed Sep 10, 2024

View reviewed changes

core/primitives/src/shard_layout.rs Outdated Show resolved Hide resolved

Trisfald reviewed Sep 19, 2024

View reviewed changes

wacban and others added 9 commits September 24, 2024 12:48

Merge branch 'master' into waclaw-resharding

a93db6a

cleanup

af1f653

nits

193529e

AccountRangeShardMap

a1a0c67

clippy

7e80a25

auto derive ord

cfdce1d

AccountId

15cba86

more tests

4a65bff

rename next_shard_prefix to get_db_prefix_upper_bound

1c476a0

wacban requested review from tayfunelmas, Longarithm, Trisfald and shreyan-gupta September 25, 2024 15:23

Trisfald approved these changes Sep 25, 2024

View reviewed changes

wacban and others added 2 commits September 25, 2024 17:08

fix build

d9aac8d

Update core/primitives/src/shard_layout.rs

d6b0199

Co-authored-by: Andrea <[email protected]>

wacban commented Sep 25, 2024

View reviewed changes

core/primitives/src/shard_layout.rs Outdated Show resolved Hide resolved

Update core/primitives/src/shard_layout.rs

41a8dd9

Longarithm requested changes Sep 25, 2024

View reviewed changes

wacban and others added 5 commits September 26, 2024 21:20

changed to boundary accounts + shard ids

a467a26

cleanup

9ef1623

more cleanup

5a74a54

Merge branch 'master' into waclaw-resharding

b5fb21f

remove InvalidShardLayout error

27c6828

wacban requested a review from Longarithm September 27, 2024 08:00

shreyan-gupta reviewed Sep 27, 2024

View reviewed changes

Longarithm approved these changes Sep 30, 2024

View reviewed changes

check boundary accounts sorted

797ce1f

wacban enabled auto-merge September 30, 2024 11:36

wacban added this pull request to the merge queue Sep 30, 2024

Merged via the queue into master with commit eb7ceec Sep 30, 2024
30 checks passed

wacban deleted the waclaw-resharding branch September 30, 2024 12:14

		/// A mapping from the shard id to the account range of this shard.
		shards_account_range: ShardsAccountRange,

feat(resharding) - Introducing ShardLayoutV2 and nighshade layout v4 #12066

feat(resharding) - Introducing ShardLayoutV2 and nighshade layout v4 #12066

Conversation

wacban commented Sep 9, 2024

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

Longarithm Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staffik commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wacban commented Sep 25, 2024

Trisfald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Longarithm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Longarithm Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 9, 2024 •

edited

Loading

Longarithm Sep 9, 2024 •

edited

Loading

Longarithm Sep 25, 2024 •

edited

Loading