Added a new option --validate-tipsets in forest-cli validate command #3167

sudo-shashank · 2023-07-11T08:13:00Z

Summary of changes

Changes introduced in this pull request:

Added a new option --validate-tipsets in forest-cli snapshot validate to validate already computed tipsets.

Validates the snapshot

Usage: forest-cli snapshot validate [OPTIONS] <SNAPSHOT>

Arguments:
  <SNAPSHOT>  Path to an uncompressed snapshot (CAR)

Options:
      --recent-stateroots <RECENT_STATEROOTS>
          Number of block headers to validate from the tip [default: 2000]
      --validate-tipsets <VALIDATE_TIPSETS>
          Validate already computed tipsets at given EPOCH, use a negative value -N to validate the last N EPOCH(s) starting at HEAD
  -h, --help
          Print help

Reference issue to close (if applicable)

Closes #3071

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

lemmih

I would prefer it if it were part of the validate command.

lemmih · 2023-07-13T13:42:18Z

src/state_manager/mod.rs

+        epochs: RangeInclusive<i64>,
+        tipsets: itertools::Unfold<Option<Arc<Tipset>>, F>,


The range is not needed if we take a stream. Also, tipsets should be anything that implements stream.

src/state_manager/mod.rs

lemmih · 2023-07-13T13:43:21Z

src/state_manager/mod.rs

+        let pb = crate::utils::io::ProgressBar::new((epochs.end() - epochs.start()) as u64);
+        pb.message("Compute parent state: ");
+        pb.set_max_refresh_rate(Some(std::time::Duration::from_millis(500)));
+
        tipsets
            .take_while(|tipset| tipset.epoch() >= *epochs.start())


No need to trim the stream. The caller can do that if they want it.

sudo-shashank · 2023-07-13T13:52:18Z

@lemmih I am also thinking of using --validate-tipsets=-1999 as default for snapshot validate
or should we leave it upto the user to decide always?

aatifsyed · 2023-07-13T13:49:50Z

src/cli/subcommands/snapshot_cmd.rs

+        let tipsets = itertools::unfold(Some(end_tipset), |tipset| {
+            let child = tipset.take()?;
+            *tipset = state_manager
+                .chain_store()
+                .tipset_from_keys(child.parents())
+                .ok();
+            Some(child)


this is copy-pasted - can you refactor to a common get_all_ancestors function please

aatifsyed · 2023-07-13T13:52:44Z

src/state_manager/mod.rs

+        self.validate_stream(epochs, tipsets)
+    }
+
+    pub fn validate_stream<F>(


I don't understand why this new function is necessary - could you help me understand?

It's to avoid heaviest_tipset, which is undefined outside of the daemon.

lemmih · 2023-07-13T16:58:48Z

src/cli/subcommands/snapshot_cmd.rs

+        ensure_params_downloaded().await?;
+        // Prepare tipset stream to validate
+        let heaviest_epoch = ts.epoch();
+        let end_tipset = state_manager


Aren't ts and end_tipset the same tipset?

lemmih · 2023-07-14T12:10:22Z

src/cli/subcommands/snapshot_cmd.rs

+        ensure_params_downloaded().await?;
+        // Prepare tipset stream to validate
+        let tipsets = ts
+            .chain(Arc::clone(&store))


Suggested change

.chain(Arc::clone(&store))

.chain(&store)

lemmih · 2023-07-14T12:10:47Z

src/state_manager/mod.rs

@@ -1251,8 +1251,6 @@ where
    /// This is suspected to be due something in the VM or its `WASM` runtime.
    #[tracing::instrument(skip(self))]
    pub fn validate(self: &Arc<Self>, epochs: RangeInclusive<i64>) -> anyhow::Result<()> {


Suggested change

pub fn validate(self: &Arc<Self>, epochs: RangeInclusive<i64>) -> anyhow::Result<()> {

pub fn validate_range(self: &Arc<Self>, epochs: RangeInclusive<i64>) -> anyhow::Result<()> {

lemmih · 2023-07-14T12:11:01Z

src/state_manager/mod.rs

+        self.validate_stream(tipsets)
+    }
+
+    pub fn validate_stream<T>(self: &Arc<Self>, tipsets: T) -> anyhow::Result<()>


Suggested change

pub fn validate_stream<T>(self: &Arc<Self>, tipsets: T) -> anyhow::Result<()>

pub fn validate_tipsets<T>(self: &Arc<Self>, tipsets: T) -> anyhow::Result<()>

sudo-shashank · 2023-07-14T13:04:15Z

@aatifsyed @lemmih PR ready for review again

lemmih · 2023-07-14T13:02:30Z

src/cli/subcommands/snapshot_cmd.rs

@@ -240,42 +249,71 @@ async fn validate_with_blockstore<BlockstoreT>(
    roots: Vec<Cid>,
    store: Arc<BlockstoreT>,
    recent_stateroots: &i64,
-) -> Result<(), anyhow::Error>
+    validate_tipsets: &Option<i64>,


Not a blocker:

Suggested change

validate_tipsets: &Option<i64>,

validate_tipsets: Option<i64>,

aatifsyed · 2023-07-14T13:15:21Z

src/blocks/tipset.rs

@@ -282,6 +282,15 @@ impl Tipset {
        }
        broken
    }
+    /// Returns an iterator of all tipsets


Of all ancestor tipsets

since it includes itself also, @lemmih suggested we keep it to Returns an iterator of all tipsets

sudo-shashank added 5 commits July 11, 2023 13:41

Added snapshot validate in forest-cli

3f7dd03

fix chain store init

9cbded2

handle validate options

0126282

make snapshot an option not arg to validate cmd

5173f47

fmt

3c6ed92

sudo-shashank changed the title ~~Added the tipset re-validator into forest-cli snapshot validate~~ Added the tipset re-validator into forest-cli snapshot re-validate Jul 11, 2023

sudo-shashank changed the title ~~Added the tipset re-validator into forest-cli snapshot re-validate~~ Added the tipset re-validator into a new sub-command forest-cli snapshot re-validate Jul 11, 2023

sudo-shashank changed the title ~~Added the tipset re-validator into a new sub-command forest-cli snapshot re-validate~~ Added the tipset re-validator into a new sub-command forest-cli snapshot re-validate Jul 11, 2023

sudo-shashank changed the title ~~Added the tipset re-validator into a new sub-command forest-cli snapshot re-validate~~ Added a new sub-command forest-cli snapshot re-validate Jul 11, 2023

sudo-shashank requested review from lemmih and aatifsyed July 11, 2023 11:28

sudo-shashank marked this pull request as ready for review July 11, 2023 11:29

sudo-shashank requested review from elmattic, LesnyRumcajs, jdjaustin, hanabi1224, creativcoder and ruseinov as code owners July 11, 2023 11:29

lemmih requested changes Jul 11, 2023

View reviewed changes

sudo-shashank marked this pull request as draft July 12, 2023 06:34

fmt

8fa4d7f

sudo-shashank force-pushed the shashank/cli-validate branch from 19e0e80 to 8fa4d7f Compare July 12, 2023 06:56

sudo-shashank and others added 4 commits July 12, 2023 12:45

Update changelog.md

4e925ee

update changelog

d904676

fmt

87c9695

Merge branch 'main' into shashank/cli-validate

08ac01b

sudo-shashank marked this pull request as ready for review July 12, 2023 07:42

sudo-shashank requested a review from lemmih July 12, 2023 07:42

sudo-shashank marked this pull request as draft July 12, 2023 08:05

refactor to validate_tipset_range

0b18cff

lemmih requested changes Jul 13, 2023

View reviewed changes

aatifsyed requested changes Jul 13, 2023

View reviewed changes

lemmih reviewed Jul 13, 2023

View reviewed changes

sudo-shashank and others added 13 commits July 14, 2023 10:52

remove progressbar and other cleanup

44c511f

Add chain method in Tipset

7bd3d22

Merge branch 'main' into shashank/cli-validate

6971ab7

chain to consume self

51700e2

Remove Arc from chain method

27bd174

remove more Arc

1419e83

Cleanup

20719e3

minor improvements

65c3c1e

fmt

40da2ec

fix error handling

26bd1a2

fix clippy error

1b3d6dc

Merge branch 'main' into shashank/cli-validate

5510b61

cleanup

133789d

lemmih requested changes Jul 14, 2023

View reviewed changes

sudo-shashank added 2 commits July 14, 2023 17:44

fmt

950b99d

minor corrections

5ead43a

sudo-shashank requested review from lemmih and aatifsyed July 14, 2023 12:55

lemmih approved these changes Jul 14, 2023

View reviewed changes

Remove reference

721da03

aatifsyed approved these changes Jul 14, 2023

View reviewed changes

sudo-shashank added this pull request to the merge queue Jul 14, 2023

Merged via the queue into main with commit c00871a Jul 14, 2023
20 checks passed

sudo-shashank deleted the shashank/cli-validate branch July 14, 2023 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a new option --validate-tipsets in forest-cli validate command #3167

Added a new option --validate-tipsets in forest-cli validate command #3167

sudo-shashank commented Jul 11, 2023 •

edited

lemmih left a comment

lemmih Jul 13, 2023

sudo-shashank Jul 14, 2023

lemmih Jul 13, 2023

sudo-shashank Jul 14, 2023

sudo-shashank commented Jul 13, 2023

aatifsyed Jul 13, 2023

aatifsyed Jul 13, 2023

lemmih Jul 13, 2023

lemmih Jul 13, 2023 •

edited

sudo-shashank Jul 14, 2023

lemmih Jul 14, 2023

sudo-shashank Jul 14, 2023

lemmih Jul 14, 2023

sudo-shashank Jul 14, 2023

lemmih Jul 14, 2023

sudo-shashank Jul 14, 2023

sudo-shashank commented Jul 14, 2023

lemmih Jul 14, 2023

sudo-shashank Jul 14, 2023

aatifsyed Jul 14, 2023

sudo-shashank Jul 14, 2023

		epochs: RangeInclusive<i64>,
		tipsets: itertools::Unfold<Option<Arc<Tipset>>, F>,

	pub fn validate(self: &Arc<Self>, epochs: RangeInclusive<i64>) -> anyhow::Result<()> {
	pub fn validate_range(self: &Arc<Self>, epochs: RangeInclusive<i64>) -> anyhow::Result<()> {

	pub fn validate_stream<T>(self: &Arc<Self>, tipsets: T) -> anyhow::Result<()>
	pub fn validate_tipsets<T>(self: &Arc<Self>, tipsets: T) -> anyhow::Result<()>

	validate_tipsets: &Option<i64>,
	validate_tipsets: Option<i64>,

Added a new option --validate-tipsets in forest-cli validate command #3167

Added a new option --validate-tipsets in forest-cli validate command #3167

Conversation

sudo-shashank commented Jul 11, 2023 • edited

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

lemmih 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

sudo-shashank commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemmih Jul 13, 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

sudo-shashank commented Jul 14, 2023

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

sudo-shashank commented Jul 11, 2023 •

edited

lemmih Jul 13, 2023 •

edited