Skip to content
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

Implement ShardUId #4659

Merged
merged 24 commits into from
Aug 23, 2021
Merged

Implement ShardUId #4659

merged 24 commits into from
Aug 23, 2021

Conversation

mzhangmzz
Copy link
Contributor

@mzhangmzz mzhangmzz commented Aug 9, 2021

This PR implements ShardUId as specified in near/NEPs#241 so that trie states are indexed by 'shard_uidinstead ofshard_id`.

The main change is to change the interface around trie storage to use shard_uid instead of shard_id. The database columns ColState and ColTrieChanges are now indexed by shard_uid instead of shard_id.

Changes that this PR does not include:

  • change account_id_to_shard_id to account for changing shard layout
  • gc

Test Plan:
The main source for errors is where shard_id and shard_uid is mixed up or the wrong shard_version is used for shard_uid, which may result in DB not found error. To test for the first type, we will change a wide range of existing unit and integration tests to use a shard layout with shard_version = 1 so shard_id and shard_uid cannot be mixed up.

self.runtime_adapter.get_state_root_node(shard_id, &chunk_header.prev_state_root())?;
let state_root_node = self.runtime_adapter.get_state_root_node(
shard_id,
&sync_hash,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid that sync_hash is not what you want here because sync_hash is the first block of the new block and I believe here the chunk could be included in a block in the previous epoch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but we will disable state sync for epoch changes where resharding happens. I will add a comment here noting that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue #4711 noting this

chain/chain/src/types.rs Outdated Show resolved Hide resolved
core/primitives/src/shard_layout.rs Outdated Show resolved Hide resolved
core/primitives/src/shard_layout.rs Outdated Show resolved Hide resolved
core/primitives/src/shard_layout.rs Show resolved Hide resolved
core/store/src/trie/shard_tries.rs Outdated Show resolved Hide resolved
@matklad matklad removed their request for review August 11, 2021 16:08
Base automatically changed from shardlayout to master August 14, 2021 18:25
@mzhangmzz mzhangmzz marked this pull request as ready for review August 15, 2021 22:51
@bowenwang1996
Copy link
Collaborator

looks like CI is still failing

chain/chain/src/chain.rs Outdated Show resolved Hide resolved
chain/chain/src/chain.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Show resolved Hide resolved
chain/chain/src/store.rs Show resolved Hide resolved
core/primitives/src/shard_layout.rs Show resolved Hide resolved
@bowenwang1996
Copy link
Collaborator

Given how invasive this change is, could you run nayduck to see that no test regresses?

@mzhangmzz

This comment has been minimized.

@frol
Copy link
Collaborator

frol commented Aug 17, 2021

@mzhangmzz @bowenwang1996

That is fine if the config file does specify num_shards to be 1 through num_block_producer_seats_per_shard.len(), but the backward compatible test is actually testing for 4 shards.

#4569

@mzhangmzz

This comment has been minimized.

@bowenwang1996

This comment has been minimized.

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Estimator changes looks good to me, leaving approval to unblock the PR. I haven't looked at the change as a whole.

@frol

This comment has been minimized.

@mzhangmzz

This comment has been minimized.

@mzhangmzz
Copy link
Contributor Author

mzhangmzz commented Aug 18, 2021

@frol @bowenwang1996 hmm I realized that #4709 will not fix the backward_compatible test until that commit is included the in the latest release branch, i.e., the branch the test is using to make comparison.

I'm not sure when is the next time we will make the next release. Is there any way to bypass that?

@bowenwang1996
Copy link
Collaborator

@frol @bowenwang1996 hmm I realized that #4709 will not fix the backward_compatible test until that commit is included the in the latest release branch, i.e., the branch the test is using to make comparison.

I'm not sure when is the next time we will make the next release. Is there any way to bypass that?

Do you need just the branch or do you need a specific commit? If you just need it to be included in the branch, feel free to cherry-pick the commit on top of 1.21.0 branch.

Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly checked runtime part, and looks good!

@mzhangmzz mzhangmzz merged commit baafc66 into master Aug 23, 2021
@mzhangmzz mzhangmzz deleted the shardid-new branch August 23, 2021 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants