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.

Sign up for GitHub

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

Jump to bottom

feat(storage): Store Validator State Test + Binary #2680

Merged
merged 6 commits into from
May 22, 2020
Merged

feat(storage): Store Validator State Test + Binary #2680

merged 6 commits into from
May 22, 2020

Conversation

Kouprin
Copy link
Member

@Kouprin Kouprin commented May 19, 2020 •
edited
Loading

Related to #2597

  • Refactoring
  • Changing output format
  • chunks_state_roots_in_trie test is added
  • Turning off Store Validation in python tests
  • Adv test start_from_genesis.py fix
  • Minor changes

@gitpod-io
Copy link

gitpod-io bot commented May 19, 2020 •
edited
Loading

@Kouprin Kouprin mentioned this pull request May 19, 2020
Closed
@Kouprin Kouprin mentioned this pull request May 20, 2020
Open
6 tasks
@Kouprin Kouprin force-pushed the store_validator_state branch 2 times, most recently from b9b02b7 to b7f9d62 Compare May 20, 2020 11:18
@Kouprin Kouprin marked this pull request as ready for review May 20, 2020 11:20
@Kouprin Kouprin requested a review from mikhailOK May 20, 2020 11:21
mikhailOK
mikhailOK reviewed May 20, 2020
View reviewed changes
chain/chain/src/types.rs Outdated Show resolved Hide resolved
test-utils/store-validator-bin/src/main.rs

let mut store_validator = StoreValidator::new(
near_config.genesis.config.clone(),
runtime_adapter.get_tries(),
Copy link
Contributor

Choose a reason for hiding this comment

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

could do ShardTries::new(store, num_shards) unless we need RuntimeAdapter for something else

Copy link
Member Author

Choose a reason for hiding this comment

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

I need RuntimeAdapter to call num_shards() then.

Copy link
Contributor

Choose a reason for hiding this comment

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

RuntimeAdapter gets it from genesis:
genesis.config.num_block_producer_seats_per_shard.len() as NumShards

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think that's a good idea to bypass standard and transparent way of getting the data and use some weird unclear way to use length of the array of seats per shard as num_shards just to save a couple of lines of code in the separate util?

bowenwang1996
bowenwang1996 reviewed May 20, 2020
View reviewed changes
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

Just to make sure that I didn't miss anything, does this PR actually add any tests?

core/store/src/trie/shard_tries.rs Outdated Show resolved Hide resolved
test-utils/store-validator/src/lib.rs Outdated
for shard_id in 0..self.shard_tries.len() {
let _trie = self.shard_tries.get_trie_for_shard(shard_id).unwrap();
// TODO #2597
// Collect somehow (ask Misha) all State Roots
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can get head of the chain from store and then go from there (getting latest block and get state roots similar to how it's done in state-viewer).

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not correct because it's not guaranteed we will receive all the data that stored in the Trie.
This will be resolved in #2696.

@Kouprin Kouprin changed the title Store Validator State Test feat: Store Validator State Test May 20, 2020
Kouprin and others added 3 commits May 21, 2020 15:16
@Kouprin
WIP
284a021
@mikhailOK @Kouprin
refactor: extract StoreValidator from near-store
b06dff1 
store-validator: all logic for validation,
depends on store and chain-configs
store-validator-bin: standalone binary,
depends on neard and store-validator-lib

Test plan
---------
N/A
@Kouprin
Updates
005dcb8
@Kouprin
Copy link
Member Author

Kouprin commented May 21, 2020

Just to make sure that I didn't miss anything, does this PR actually add any tests?

chunks_state_roots_in_trie

@Kouprin Kouprin changed the title feat: Store Validator State Test feat(storage): Store Validator State Test + Binary May 21, 2020
bowenwang1996
bowenwang1996 reviewed May 22, 2020
View reviewed changes
core/primitives/src/lib.rs
@@ -5,6 +5,8 @@ extern crate jemallocator;
#[global_allocator]
static ALLOC: jemallocator::Jemalloc = jemallocator::Jemalloc;

pub use borsh;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this for? Doesn't make sense to me as borsh is an external library.

Copy link
Member Author

Choose a reason for hiding this comment

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

It needs by validator to deserialize Storage data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't you import borsh in that crate?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't understand what's wrong with adding borsh for public usage here. Could you please decide it with @mikhailOK? I'm okay with any solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Re-exporting solves inconsistencies, so I think it is a good practice: near/near-sdk-rs#142

@Kouprin Kouprin merged commit c034673 into master May 22, 2020
@Kouprin Kouprin deleted the store_validator_state branch May 22, 2020 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowenwang1996 bowenwang1996 bowenwang1996 left review comments

@mikhailOK mikhailOK mikhailOK left review comments

@SkidanovAlex SkidanovAlex SkidanovAlex approved these changes

@evgenykuzyakov evgenykuzyakov Awaiting requested review from evgenykuzyakov

@lexfrl lexfrl Awaiting requested review from lexfrl

@frol frol Awaiting requested review from frol

@MaksymZavershynskyi MaksymZavershynskyi Awaiting requested review from MaksymZavershynskyi

@olonho olonho Awaiting requested review from olonho

Assignees

@mikhailOK mikhailOK

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Kouprin @frol @SkidanovAlex @mikhailOK @bowenwang1996