This repository was archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add JSON format to import blocks and set it as default #5816
Merged
Merged
Changes from all commits
Commits
Show all changes
67 commits
Select commit
Hold shift + click to select a range
0bb1eb7
Add BlockStream Enum and utility fn
b291ff1
WIP: Modify import closure to work with BlockStream
49b0144
Fix trait bounds
cb6aded
Working prototype
155695d
Revamp block importing
e59e1ad
Add export_import_flow tests
7b729fe
Merge branch 'master' of github.com:paritytech/substrate into scott_d…
69a8bff
Add comments and clean code
ab173a5
Add more comments in the import fn
e22ed9b
Add link code to import function
00fbd6c
Add condition when returning Ready(Ok(()) to make sure we've imported…
e7a3c4d
Add check for imported blocks in JSON case
56d49fa
Use rest pattern
ff75523
Fix compilation error for undeclared variable
7c66af2
Add polling and waker before pending
419b0ab
Print read_block_count instead of count
be1d626
Simplify binary cli option with structopt
a67a75d
Update test to reflect changes in CLI api
e1ce1aa
Change Stream to take SignedBlock<B> instead of B
30f60de
Merge branch 'master' of github.com:paritytech/substrate into scott_d…
ecf8729
Add comments to BlockStream
7b8f34c
Move out logic to smaller functions for clearer code
1a9e910
Merge branch 'master' of github.com:paritytech/substrate into scott_d…
61260b5
Remove result over import_blocks return type
4ca8fe6
Check for error in command output rather than simply checking command…
c986b44
Revamp export/import/revert testing
8b3a17a
Fix minor typos and formatting errors
pscott c57415b
Remove unnecessary if condition in terminating condition
pscott 32eb4af
Explicit error instead of returning it as a string
pscott 412e942
Pass BlockStream to log_importing_status_updates and simplify matchin…
2266d67
Use .contains() instead of regex match
f62d5e0
Line break in match block; return future::ready instead of poll_fn
ad44445
Update Cargo.lock
17f4c74
Add check so that queue doesn't grow too big
994157c
Use Iterator instead of Stream
4d1234e
Remove allow dead_code
37ae67b
Merge branch 'master' of github.com:paritytech/substrate into scott_d…
757693d
Remove outdated comments
pscott 9865fae
Return Errors instead of logging them
1150f12
Simplify match arms
pscott 377823c
Remove check before terminating block import
576606c
Apply suggestions from code review
bkchr 9b4658a
Check that queue is not full BEFORE calling
76a5954
Revert "Remove check before terminating block import"
f848a05
Improve unit tests to make sure we actually import blocks
d924935
Remove Unpin implementation for BlockIter
3d00933
Add prototype of enum for ImportStates
fb040fc
Add working prototype for StateMachine
4f9bcf8
Add comments for clearer code
9a134eb
Add sleep before calling Waker when waiting for import queue
fdd3683
Merge branch 'master' of github.com:paritytech/substrate into scott_d…
d98c92d
Add Speedometer
344286e
Merge branch 'master' of github.com:paritytech/substrate into scott_d…
5bdc4fd
add dbg!(&log) for test debugging
5d1f1e3
Fix lines with more than 100 cols
768c309
Merge branch 'master' of github.com:paritytech/substrate into scott_d…
a17cc1e
Fix regex capture for test
9d7d7a1
Update regexes to take to capture the whole number
03a5cc8
Merge branch 'master' of github.com:paritytech/substrate into scott_d…
ef02ec3
Rename Cmd to Command
pscott fb91c53
Actually rename Cmd to Command
4f067ef
Apply suggestions from code review
gnunicorn 0c47a45
Merge branch 'master' of github.com:paritytech/substrate into scott_d…
196de60
Fix compilation errors for tests
731f1b7
Merge branch 'scott_default_json_import_blocks' of github.com:parityt…
f43f3de
Fix compilation errors from code review suggestion
cdc1290
Update bin/node/cli/tests/export_import_flow.rs
gavofyork File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,212 @@ | ||
| // This file is part of Substrate. | ||
|
|
||
| // Copyright (C) 2020 Parity Technologies (UK) Ltd. | ||
| // SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 | ||
|
|
||
| // This program is free software: you can redistribute it and/or modify | ||
| // it under the terms of the GNU General Public License as published by | ||
| // the Free Software Foundation, either version 3 of the License, or | ||
| // (at your option) any later version. | ||
|
|
||
| // This program is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| // GNU General Public License for more details. | ||
|
|
||
| // You should have received a copy of the GNU General Public License | ||
| // along with this program. If not, see <https://www.gnu.org/licenses/>. | ||
|
|
||
| #![cfg(unix)] | ||
|
|
||
| use assert_cmd::cargo::cargo_bin; | ||
| use std::{process::Command, fs, path::PathBuf}; | ||
| use tempfile::{tempdir, TempDir}; | ||
| use regex::Regex; | ||
|
|
||
| pub mod common; | ||
|
|
||
| fn contains_error(logged_output: &str) -> bool { | ||
| logged_output.contains("Error") | ||
| } | ||
|
|
||
| /// Helper struct to execute the export/import/revert tests. | ||
| /// The fields are paths to a temporary directory | ||
| struct ExportImportRevertExecutor<'a> { | ||
| base_path: &'a TempDir, | ||
| exported_blocks_file: &'a PathBuf, | ||
| db_path: &'a PathBuf, | ||
| num_exported_blocks: Option<u64>, | ||
| } | ||
|
|
||
| /// Format options for export / import commands. | ||
| enum FormatOpt { | ||
| Json, | ||
| Binary, | ||
| } | ||
|
|
||
| /// Command corresponding to the different commands we would like to run. | ||
| enum SubCommand { | ||
| ExportBlocks, | ||
| ImportBlocks, | ||
| } | ||
|
|
||
| impl ToString for SubCommand { | ||
| fn to_string(&self) -> String { | ||
| match self { | ||
| SubCommand::ExportBlocks => String::from("export-blocks"), | ||
| SubCommand::ImportBlocks => String::from("import-blocks"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<'a> ExportImportRevertExecutor<'a> { | ||
| fn new( | ||
| base_path: &'a TempDir, | ||
| exported_blocks_file: &'a PathBuf, | ||
| db_path: &'a PathBuf | ||
| ) -> Self { | ||
| Self { | ||
| base_path, | ||
| exported_blocks_file, | ||
| db_path, | ||
| num_exported_blocks: None, | ||
| } | ||
| } | ||
|
|
||
| /// Helper method to run a command. Returns a string corresponding to what has been logged. | ||
| fn run_block_command(&self, | ||
| sub_command: SubCommand, | ||
| format_opt: FormatOpt, | ||
| expected_to_fail: bool | ||
| ) -> String { | ||
| let sub_command_str = sub_command.to_string(); | ||
| // Adding "--binary" if need be. | ||
| let arguments: Vec<&str> = match format_opt { | ||
| FormatOpt::Binary => vec![&sub_command_str, "--dev", "--pruning", "archive", "--binary", "-d"], | ||
| FormatOpt::Json => vec![&sub_command_str, "--dev", "--pruning", "archive", "-d"], | ||
| }; | ||
|
|
||
| let tmp: TempDir; | ||
| // Setting base_path to be a temporary folder if we are importing blocks. | ||
| // This allows us to make sure we are importing from scratch. | ||
| let base_path = match sub_command { | ||
| SubCommand::ExportBlocks => &self.base_path.path(), | ||
| SubCommand::ImportBlocks => { | ||
| tmp = tempdir().unwrap(); | ||
| tmp.path() | ||
| } | ||
| }; | ||
|
|
||
| // Running the command and capturing the output. | ||
| let output = Command::new(cargo_bin("substrate")) | ||
| .args(&arguments) | ||
| .arg(&base_path) | ||
| .arg(&self.exported_blocks_file) | ||
| .output() | ||
| .unwrap(); | ||
|
|
||
| let logged_output = String::from_utf8_lossy(&output.stderr).to_string(); | ||
|
|
||
| if expected_to_fail { | ||
| // Checking that we did indeed find an error. | ||
| assert!(contains_error(&logged_output), "expected to error but did not error!"); | ||
| assert!(!output.status.success()); | ||
| } else { | ||
| // Making sure no error were logged. | ||
| assert!(!contains_error(&logged_output), "expected not to error but error'd!"); | ||
| assert!(output.status.success()); | ||
| } | ||
|
|
||
| logged_output | ||
| } | ||
|
|
||
| /// Runs the `export-blocks` command. | ||
| fn run_export(&mut self, fmt_opt: FormatOpt) { | ||
| let log = self.run_block_command(SubCommand::ExportBlocks, fmt_opt, false); | ||
|
|
||
| // Using regex to find out how many block we exported. | ||
| let re = Regex::new(r"Exporting blocks from #\d* to #(?P<exported_blocks>\d*)").unwrap(); | ||
| let caps = re.captures(&log).unwrap(); | ||
| // Saving the number of blocks we've exported for further use. | ||
| self.num_exported_blocks = Some(caps["exported_blocks"].parse::<u64>().unwrap()); | ||
|
|
||
| let metadata = fs::metadata(&self.exported_blocks_file).unwrap(); | ||
| assert!(metadata.len() > 0, "file exported_blocks should not be empty"); | ||
|
|
||
| let _ = fs::remove_dir_all(&self.db_path); | ||
| } | ||
|
|
||
| /// Runs the `import-blocks` command, asserting that an error was found or | ||
| /// not depending on `expected_to_fail`. | ||
| fn run_import(&mut self, fmt_opt: FormatOpt, expected_to_fail: bool) { | ||
| let log = self.run_block_command(SubCommand::ImportBlocks, fmt_opt, expected_to_fail); | ||
|
|
||
| if !expected_to_fail { | ||
| // Using regex to find out how much block we imported, | ||
| // and what's the best current block. | ||
| let re = Regex::new(r"Imported (?P<imported>\d*) blocks. Best: #(?P<best>\d*)").unwrap(); | ||
| let caps = re.captures(&log).expect("capture should have succeeded"); | ||
| let imported = caps["imported"].parse::<u64>().unwrap(); | ||
| let best = caps["best"].parse::<u64>().unwrap(); | ||
|
|
||
| assert_eq!( | ||
| imported, | ||
| best, | ||
| "numbers of blocks imported and best number differs" | ||
| ); | ||
| assert_eq!( | ||
| best, | ||
| self.num_exported_blocks.expect("number of exported blocks cannot be None; qed"), | ||
| "best block number and number of expected blocks should not differ" | ||
| ); | ||
| } | ||
| self.num_exported_blocks = None; | ||
| } | ||
|
|
||
| /// Runs the `revert` command. | ||
| fn run_revert(&self) { | ||
| let output = Command::new(cargo_bin("substrate")) | ||
| .args(&["revert", "--dev", "--pruning", "archive", "-d"]) | ||
| .arg(&self.base_path.path()) | ||
| .output() | ||
| .unwrap(); | ||
|
|
||
| let logged_output = String::from_utf8_lossy(&output.stderr).to_string(); | ||
|
|
||
| // Reverting should not log any error. | ||
| assert!(!contains_error(&logged_output)); | ||
| // Command should never fail. | ||
| assert!(output.status.success()); | ||
| } | ||
|
|
||
| /// Helper function that runs the whole export / import / revert flow and checks for errors. | ||
| fn run(&mut self, export_fmt: FormatOpt, import_fmt: FormatOpt, expected_to_fail: bool) { | ||
| self.run_export(export_fmt); | ||
| self.run_import(import_fmt, expected_to_fail); | ||
| self.run_revert(); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn export_import_revert() { | ||
| let base_path = tempdir().expect("could not create a temp dir"); | ||
| let exported_blocks_file = base_path.path().join("exported_blocks"); | ||
| let db_path = base_path.path().join("db"); | ||
|
|
||
| common::run_dev_node_for_a_while(base_path.path()); | ||
|
|
||
| let mut executor = ExportImportRevertExecutor::new( | ||
| &base_path, | ||
| &exported_blocks_file, | ||
| &db_path, | ||
| ); | ||
|
|
||
| // Binary and binary should work. | ||
| executor.run(FormatOpt::Binary, FormatOpt::Binary, false); | ||
| // Binary and JSON should fail. | ||
| executor.run(FormatOpt::Binary, FormatOpt::Json, true); | ||
| // JSON and JSON should work. | ||
| executor.run(FormatOpt::Json, FormatOpt::Json, false); | ||
| // JSON and binary should fail. | ||
| executor.run(FormatOpt::Json, FormatOpt::Binary, true); | ||
| } | ||
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why delete files manually if
TempDiris supposed to handle that?Uh oh!
There was an error while loading. Please reload this page.
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.
So this was part of the original test and I believe the main idea is to run the node, export the blocks, and then delete the DB to make sure that
import-blocksis actually importing blocks, and not "doing nothing".Maybe what we should add is a test to see that import blocks actually imported blocks? Not sure how to do that, I wouldn't really know what we could query...
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.
@arkpar Yes that was my code actually. First create the temp directory, export, delete the directory, import and then only let the destructor clean-up. It was done on purpose but you could also create another tempdir instead of reusing one, it might make the code more understandable.
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.
So keep in mind that now that we've changed the test flow, we need to keep the same directory active for the whole testing period, in order to keep the db :)
I believe the
remove_dir_allonly happens between theexport-blockscommand and theimport-blockswhich makes sense, so that we make sure that we are indeed importing stuff! :)