ChainSafe / forest

🌲 Rust Filecoin Node Implementation
https://forest.chainsafe.io
Apache License 2.0
632 stars 153 forks source link

snapshot validate does not validate snapshots independently #3403

Closed LesnyRumcajs closed 1 month ago

LesnyRumcajs commented 1 year ago

Describe the bug

The forest-tool snapshot validate [FILES]... does not seem to validate the snapshots independently. It may be by design, but it's a pretty unexpected behaviour and should be either mitigated functionally (by validating each snapshot independently and failing the command if one of them is corrupted) or by having a separate subcommand for validating a union of snapshots.

To Reproduce Steps to reproduce the behavior:

  1. Download a snapshot that is missing a genesis block. You can find one here
  2. Download any valid snapshot.
  3. Run forest-tool snapshot validate --check-network calibnet forest_snapshot_calibnet_2023-08-21_height_842289.forest.car.zst. You should see:
    Checking IPLD integrity:       ✅ verified!
    Identifying genesis block:     ❌ No valid genesis block!                                                                                                                                                                                                                                                                     Error: Snapshot does not contain a genesis block
  4. Run `forest-tool snapshot validate --check-network calibnet
    ❯ forest-tool snapshot validate --check-network calibnet forest_snapshot_calibnet_2023-08-21_height_842529.forest.car.zst
    Checking IPLD integrity:       ✅ verified!
    Identifying genesis block:     ✅ found!                                                                                                                                                                                                                                                                                        Verifying network identity:    ✅ verified!
    Running tipset transactions:   ✅ verified!                                                                                                                                                                                                                                                                                   Snapshot is valid
  5. Run the same with both snapshots as positional arguments:
    ❯ forest-tool snapshot validate --check-network calibnet forest_snapshot_calibnet_2023-08-21_height_842289.forest.car.zst forest_snapshot_calibnet_2023-08-21_height_842529.forest.car.zst
    Checking IPLD integrity:       ✅ verified!
    Identifying genesis block:     ✅ found!                                                                                                                                                                                                                                                                                        Verifying network identity:    ✅ verified!
    Running tipset transactions:   ✅ verified!                                                                                                                                                                                                                                                                                   Snapshot is valid

Expected behaviour

This snapshot validate <multiple files>... should fail if any file is corrupt. If we want to validate a union, there should be a separate subcommand, e.g., snapshot validate-union.

Screenshots

Environment (please complete the following information):

Other information and links This was first encountered in https://github.com/ChainSafe/forest-iac/pull/224.

ruseinov commented 1 year ago

The reason for the above is that we are using ManyCar::open for the validate command, which is loading all the files as a union.

Perhaps it's best to convert this command to something that either:

  1. Takes exactly one snapshot as an argument.
  2. Allows for validating each snapshot independently in some sort of loop.
  3. Does 2 and has a union argument for union validation.

Having a separate command for union would probably be more obvious, even if it points to the same code under the hood, just a different branch of it.

lemmih commented 2 months ago

Solution: Rename the validate command to validate-diffs and add a new validate command that checks each file separately.