feat(epoch-sync): adding EpochSyncInfo validation tool #10146

posvyatokum · 2023-11-09T17:48:53Z

Adding tool for testing epoch sync implementation.

Right now only one subcommand is available validate-epoch-sync-info. It constructs EpochSyncInfo for every finalised epoch in DB and checks that EpochSyncInfo passes validation.
Currently we only have one validation (kinda) implemented -- checking of epoch_sync_data_hash.

As epoch sync is under feature and that feature includes DB upgrade, the tool first creates a snapshot of the DB, and then modifies the config to work with the snapshot.

nagisa · 2023-11-13T09:43:33Z

tools/epoch-sync/Cargo.toml

+near-store.workspace = true
+
+[features]
+new_epoch_sync = ["nearcore/new_epoch_sync"]


Since this crate currently only does anything at all when this feature is enabled, wouldn’t it be better if the dependency on the crate itself was optional over implementing a crate feature like this right now?

I tried my best :) 67aea72
Please, check this commit

Cargo.toml

tools/epoch-sync/src/cli.rs

nagisa · 2023-11-15T09:48:31Z

neard/Cargo.toml

@@ -40,6 +40,7 @@ near-config-utils.workspace = true
 near-crypto.workspace = true
 near-database-tool.workspace = true
 near-dyn-configs.workspace = true
+near-epoch-sync-tool = { path = "../tools/epoch-sync", optional = true }


Ah, I was thinking that the path would remain in the Cargo.toml, just when specyfing an optional dependency there’d be

near-epoch-sync-tool = { workspace = true, optional = true }

There are a couple of examples of this in e.g. nearcore/Cargo.toml around near-jsonrpc dependency. Is there any reason why you have chosen to do it this way instead? Did something not work the way you expected?

nagisa · 2023-11-15T09:53:01Z

tools/epoch-sync/Cargo.toml

+near-store.workspace = true
+
+[features]
+default = ["nearcore/new_epoch_sync"]


If this is a required feature for this crate to work, I believe you want

nearcore = { workspace = true, features = ["new_epoch_sync"] }

The rule of thumb to use in making the choice between these two options is: "would this crate compile if the default features were not enabled for some reason (e.g. somebody built code with the --no-default-features flag)?" If the answer is "yes", then the current suggested approach is fine. Otherwise the feature has to be enabled unconditionally as suggested in this comment.

tools/epoch-sync/src/cli.rs

nikurt · 2023-11-15T10:26:40Z

tools/epoch-sync/src/cli.rs

+    let mut hash_to_next_hash = HashMap::new();
+    let mut cur_head = *chain_head;
+    while let Some(prev_hash) = hash_to_prev_hash.get(&cur_head) {
+        hash_to_next_hash.insert(*prev_hash, cur_head);


You can assert!() that the insertion happened. This would detect forks in the recent epochs.

So, with forks, the mapping from cur_hash to next_hash is not correct, as we may encounter key collision.
To avoid that I don't build this mapping from the whole hash_to_prev_hash mapping, but only from one chain that end with provided chain_head.

Please add this as a comment to the code.

nikurt · 2023-11-15T10:28:17Z

tools/epoch-sync/src/cli.rs

+fn get_hash_to_prev_hash_from_block_header(
+    storage: &NodeStorage,
+) -> anyhow::Result<HashMap<CryptoHash, CryptoHash>> {
+    let mut hash_to_prev_hash = HashMap::new();


As you are iterating over all headers available, I think you may encounter forks.
Are forks garbage collected in the normal garbage collection or sooner (in finalization of blocks?)?

Even with forks mapping from cur_hash to prev_hash is unique, as there will not be two blocks with the same hash, so there will not be key collision. Value collision may happen, but it is irrelevant for HashMap.

Please add this as a comment to the code.

nikurt · 2023-11-15T10:31:53Z

tools/epoch-sync/src/cli.rs

+
+        // Edge case if we exactly at the epoch boundary.
+        if epoch_ids.contains(&cur_hash) {
+            cur_hash = hash_to_prev_hash[&cur_hash];


Why is it ok to skip the header head block?
Don't we need to check that the last block of an epoch exists and has block info?

EpochSyncInfo is only constructed when the next epoch has a finalised block, because we need a header with epoch_sync_data_hash present.

Thanks, please add that as a comment to the code.

nikurt · 2023-11-15T10:39:21Z

tools/epoch-sync/src/cli.rs

+                let next_epoch_first_hash = hash_to_next_hash[last_header.hash()];
+                tracing::debug!("Creating EpochSyncInfo from block {:?}", last_header);
+
+                let epoch_sync_info = chain_update.create_epoch_sync_info(


I'm lost.
The function is called validate, but you're not reading any EpochSyncInfo from storage, but create them and validate the ones created?
A few comments explaining what you're validating will be helpful.

Yeah, I will add the comment. I validate the epoch sync implementation, not the DB.

tools/epoch-sync/src/cli.rs

nikurt · 2023-11-20T13:09:43Z

tools/epoch-sync/src/cli.rs

+
+        // Edge case if we exactly at the epoch boundary.
+        if epoch_ids.contains(&cur_hash) {
+            cur_hash = hash_to_prev_hash[&cur_hash];


Thanks, please add that as a comment to the code.

nikurt · 2023-11-20T13:11:03Z

tools/epoch-sync/src/cli.rs

+fn get_hash_to_prev_hash_from_block_header(
+    storage: &NodeStorage,
+) -> anyhow::Result<HashMap<CryptoHash, CryptoHash>> {
+    let mut hash_to_prev_hash = HashMap::new();


Please add this as a comment to the code.

nikurt · 2023-11-20T13:11:11Z

tools/epoch-sync/src/cli.rs

+    let mut hash_to_next_hash = HashMap::new();
+    let mut cur_head = *chain_head;
+    while let Some(prev_hash) = hash_to_prev_hash.get(&cur_head) {
+        hash_to_next_hash.insert(*prev_hash, cur_head);


Please add this as a comment to the code.

codecov · 2023-11-20T15:15:23Z

Codecov Report

Attention: 244 lines in your changes are missing coverage. Please review.

Comparison is base (8fcd63c) 72.04% compared to head (2e7e29a) 71.89%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
tools/epoch-sync/src/cli.rs	0.00%	219 Missing ⚠️
chain/epoch-manager/src/lib.rs	9.09%	20 Missing ⚠️
chain/chain/src/chain.rs	60.00%	0 Missing and 2 partials ⚠️
chain/epoch-manager/src/adapter.rs	60.00%	2 Missing ⚠️
chain/chain/src/test_utils/kv_runtime.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10146      +/-   ##
==========================================
- Coverage   72.04%   71.89%   -0.16%     
==========================================
  Files         706      707       +1     
  Lines      141305   141788     +483     
  Branches   141305   141788     +483     
==========================================
+ Hits       101804   101937     +133     
- Misses      34807    35131     +324     
- Partials     4694     4720      +26

Flag	Coverage Δ
backward-compatibility	`0.08% <ø> (ø)`
db-migration	`0.08% <ø> (ø)`
genesis-check	`1.23% <ø> (ø)`
integration-tests	`36.24% <0.00%> (-0.15%)`	⬇️
linux	`71.76% <3.17%> (-0.17%)`	⬇️
linux-nightly	`71.64% <3.17%> (-0.17%)`	⬇️
macos	`55.75% <3.17%> (+0.49%)`	⬆️
pytests	`1.46% <ø> (ø)`
sanity-checks	`1.26% <ø> (ø)`
unittests	`68.22% <3.17%> (-0.16%)`	⬇️
upgradability	`0.13% <ø> (ø)`

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.

posvyatokum added 5 commits November 9, 2023 17:38

adding EpochSyncInfo validation tool

5686d72

fmt

f189d06

migrate storage

81dd58d

feature + optimisation

eb06ac5

add get_all_epoch_hashes_from_cache

d294121

nagisa reviewed Nov 13, 2023

View reviewed changes

posvyatokum added 3 commits November 14, 2023 18:27

make near-epoch-sync-tool optional dep

67aea72

naming

779157c

Make tool run on both archival and rpc nodes

5bb5ad4

posvyatokum requested a review from nikurt November 14, 2023 18:48

posvyatokum marked this pull request as ready for review November 14, 2023 18:48

posvyatokum requested a review from a team as a code owner November 14, 2023 18:48

nagisa reviewed Nov 15, 2023

View reviewed changes

nikurt reviewed Nov 15, 2023

View reviewed changes

posvyatokum added 3 commits November 15, 2023 18:45

fix deps

0ca1819

refactor

e6c7281

add comments

cb47975

posvyatokum requested review from nikurt and nagisa November 15, 2023 19:34

nikurt approved these changes Nov 20, 2023

View reviewed changes

add fn descriptions

a0c570a

posvyatokum enabled auto-merge November 20, 2023 15:06

posvyatokum added 2 commits November 20, 2023 15:26

Merge branch 'master' into epoch-sync-tool

4bd1de4

fix themis

2e7e29a

posvyatokum added this pull request to the merge queue Nov 20, 2023

Merged via the queue into master with commit 1102b37 Nov 20, 2023
15 of 17 checks passed

posvyatokum deleted the epoch-sync-tool branch November 20, 2023 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(epoch-sync): adding EpochSyncInfo validation tool #10146

feat(epoch-sync): adding EpochSyncInfo validation tool #10146

posvyatokum commented Nov 9, 2023

nagisa Nov 13, 2023

posvyatokum Nov 14, 2023

nagisa Nov 15, 2023

nagisa Nov 15, 2023 •

edited

nikurt Nov 15, 2023

posvyatokum Nov 15, 2023

nikurt Nov 20, 2023

nikurt Nov 15, 2023

posvyatokum Nov 15, 2023

nikurt Nov 20, 2023

nikurt Nov 15, 2023

posvyatokum Nov 15, 2023

nikurt Nov 20, 2023

nikurt Nov 15, 2023

posvyatokum Nov 15, 2023

nikurt Nov 20, 2023

nikurt Nov 20, 2023

nikurt Nov 20, 2023

codecov bot commented Nov 20, 2023 •

edited

feat(epoch-sync): adding EpochSyncInfo validation tool #10146

feat(epoch-sync): adding EpochSyncInfo validation tool #10146

Conversation

posvyatokum commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa Nov 15, 2023 • edited

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

codecov bot commented Nov 20, 2023 • edited

Codecov Report

nagisa Nov 15, 2023 •

edited

codecov bot commented Nov 20, 2023 •

edited