fix: don't GC manifests #4959

ruseinov · 2024-10-31T18:17:19Z

Summary of changes

Changes introduced in this pull request:

blessed column for persistent storage

Reference issue to close (if applicable)

Closes #4926

Other information and links

The actors are found fine after several GC rounds, so this is confirmed. Also, the migration now works as intended.

Manual testing:

Several GC runs prior to state migration:

2024-11-12T14:19:05.975826Z  INFO forest_filecoin::db::gc: filter keys for GC
2024-11-12T14:19:06.544906Z  INFO forest_filecoin::db::gc: GC sweep
2024-11-12T14:19:06.793863Z  INFO forest_filecoin::db::gc: GC finished sweep: 4189 deleted records

There are of course other errors that have to do with modifying the state migration epoch, but no bundle retrieval issues, I have also tested this before the persistent fix and it failed miserably trying to retrieve those from the DB.

2024-11-12T15:18:29.939559Z  INFO compute_tipset_state_blocking: forest_filecoin::state_migration: Running TukTuk migration at epoch 2136520
2024-11-12T15:18:29.985712Z  INFO forest_filecoin::chain_sync::tipset_syncer: Validating tipset: EPOCH = 2136611, N blocks = 1
2024-11-12T15:18:29.985734Z  WARN forest_filecoin::chain_sync::tipset_syncer: Got block from the future, but within clock drift threshold, 1731424710 > 1731424709
2024-11-12T15:18:29.988647Z  WARN forest_filecoin::chain_sync::tipset_syncer: Validating block [CID = bafy2bzacea3gbxtureg5u7msimtsnns2j7edqmsetgghs7uy4cxlqdnejgzgo] in EPOCH = 2136611 failed: Validation error: Processing error: Error calculating weight: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaxzciqzroykafdbt2fcizvzphu2hex4dceseo3lzrbl6qtuy3d7u, state_root_version=unknown, Processing error: Failed to calculate state: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaxzciqzroykafdbt2fcizvzphu2hex4dceseo3lzrbl6qtuy3d7u, state_root_version=unknown, Processing error: Could not update state: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaxzciqzroykafdbt2fcizvzphu2hex4dceseo3lzrbl6qtuy3d7u, state_root_version=unknown, Validation error: Consensus error: Power actor not found, Consensus error: StateManager error: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaxzciqzroykafdbt2fcizvzphu2hex4dceseo3lzrbl6qtuy3d7u, state_root_version=unknown
2024-11-12T15:18:29.988693Z  WARN forest_filecoin::chain_sync::tipset_syncer: Sync messages check state failed for single tipset
2024-11-12T15:18:29.988701Z ERROR forest_filecoin::chain_sync::tipset_syncer: Syncing tipset range [2136521, 2136611] failed: Validation error: Processing error: Error calculating weight: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaxzciqzroykafdbt2fcizvzphu2hex4dceseo3lzrbl6qtuy3d7u, state_root_version=unknown, Processing error: Failed to calculate state: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaxzciqzroykafdbt2fcizvzphu2hex4dceseo3lzrbl6qtuy3d7u, state_root_version=unknown, Processing error: Could not update state: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaxzciqzroykafdbt2fcizvzphu2hex4dceseo3lzrbl6qtuy3d7u, state_root_version=unknown, Validation error: Consensus error: Power actor not found, Consensus error: StateManager error: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaxzciqzroykafdbt2fcizvzphu2hex4dceseo3lzrbl6qtuy3d7u, state_root_version=unknown
thread 'state migration thread: 0' panicked at /Users/romanuseinov/projects/chainsafe/forest/src/state_migration/common/state_migration.rs:134:29:
failed executing job for address: t04, Reason: state migration failed for bafk2bzaceclefusmffhuuvtggrmadr3cwpwsgphtlj2wb222ztwwv5mssu5ea actor, addr t04:Serialization error for Cbor protocol: RequireLength { name: "tuple", expect: 15, value: 17 }
2024-11-12T15:18:30.090031Z  INFO forest_filecoin::state_migration::common::state_migration: Processed 100000 actors

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
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.

ruseinov · 2024-11-06T00:08:23Z

Follow-up issue - create common helpers to use for migrations and also for DB operations.
We have to duplicate the way that the keys are being constructed in several different manners, for e.g. EthMappings and the Graph. It would be nice to share the same code that is used by the application itself to avoid human error.

LesnyRumcajs · 2024-11-15T06:56:53Z

Let's hold off merging and releasing until the network upgrade on the mainnet (Nov 20th) to reduce chance of potential issues.

hanabi1224 · 2024-11-26T15:14:14Z

src/db/car/any.rs

@@ -124,6 +125,19 @@ where
    }
 }

+impl<ReaderT> PersistentStore for AnyCar<ReaderT>


I feel it would be less error-prone to make Blockstore trait persistent by default and have a GarbageCollectableStore trait for storing blockchain data. Currently, ppl have to interpret Blockstore as GarbageCollectableStore to avoid mistakes which is counter-intuitive. What do you think?

We can create an issue for that and we’ll address it as prioritized. It’s out of scope for this PR, I have already created an issue to generalize different stores a bit more to be able to rely on tests for different database types better.

I’m also not sure that making Blockstore persistent by default is a good idea as the use-cases for persistent storage are pretty limited at the moment.
It seems our default use-case is garbage-collected, so having persistent storage as an add-on makes more sense. I do agree that in general case this would be counter-intuitive.

hanabi1224 · 2024-11-26T15:20:13Z

src/db/gc/mod.rs

+        let depth = 5 as ChainEpochDelta;
+        let current_epoch = 0 as ChainEpochDelta;
+
+        let tester = GCTester::new();


Shall we test both MemoryDB and ParityDB?

ParityDB is tricky to test, there is an issue for that. Therefore it has been tested manually.

The main issue there is that deletes are not propagating immediately, so unit testing it reliably is not currently possible.

actually, no issue. creating one

hanabi1224 · 2024-11-26T15:23:14Z

src/daemon/mod.rs

-    // garbage collected. Until this is fixed, the GC has to be disabled.
-    // Tracking issue: https://github.com/ChainSafe/forest/issues/4926
-    // if !opts.no_gc {
-    if false {


Would love to see GC being manually triggered in at least one of the CI tests. Could you create a tracking issue if it cannot land in this PR?

there is an issue for manual gc.

ruseinov · 2024-11-26T18:15:41Z

@hanabi1224 I've created some issues and linked others to address the comments to this PR:

Redesigning the DB trait structure is subject to discussion, however there is an issue here: Share more logic between different DB implementations. #5005, feel free to add to it. I'm also looking into introducing a wrapper for MemoryDB/ParityDB in rust-f3 to have a unified interface for all kinds of storages, perhaps we could adopt that approach in forest later. When it comes to making persistent storage a default - I have my doubts about the viability of this approach just to support what is currently one niche use-case (manifest storage).
Manual GC trigger issue is here: [GC] Add manual trigger. #4461, it's already prioritised as low as there isn't much use for it, except the CI integration testing. It will require a bit of a redesign to be able to support channels and override the automatic GC.
ParityDB unit testing issue here: Figure out a way to manually trigger parityDB sync. #5008, it's not trivial, I remember spending time investigating a way to manually sync all the pending transactions and it was not reliably possible at the moment. Would be great to revisit this again.

hanabi1224

LGTM. It'd be great to have @lemmih making a final review as well.

ruseinov · 2024-11-27T07:39:27Z

LGTM. It'd be great to have @lemmih making a final review as well.

we can do this retroactively I guess, if anything pops-up - can be fixed in the next iteration.
I’d love to tackle some of the issues linked from this PR.

ruseinov added 10 commits October 31, 2024 16:36

remove redundant match block

e75471b

wip

b291b78

fix compilation

51d3e4d

fix warns

9835839

add parity_db tests

d58f9f1

fix

83e3757

re-enable GC

1969edc

changelog

3525185

lint fix

a406b8f

fmt again

b48527e

ruseinov marked this pull request as ready for review November 4, 2024 20:32

ruseinov requested a review from a team as a code owner November 4, 2024 20:32

ruseinov requested review from hanabi1224 and LesnyRumcajs and removed request for a team November 4, 2024 20:32

ruseinov added 7 commits November 5, 2024 12:21

add BlessedGraph to the end

88c86ec

Merge branch 'main' into ru/fix/gc-manifest

aaaa984

add a db migration

612e41b

fmt

480cfd6

fix 0.19.0 migration

f8d964d

introduce a flag to fix older migrations

fa0dd54

ethMappings migration

9f7cc5e

ruseinov mentioned this pull request Nov 6, 2024

Follow-up issue - create common helpers to use for migrations and also for DB operations. #4965

Open

hanabi1224 and others added 6 commits November 8, 2024 11:01

Merge branch 'main' into ru/fix/gc-manifest

b096d1b

Merge branch 'main' into ru/fix/gc-manifest

6110bac

temporarily ignore an audit error

da0ade0

fix versions

ab874a2

fix migration

c692f25

fmt

4284d18

Merge branch 'main' into ru/fix/gc-manifest

64f3d3f

LesnyRumcajs requested review from elmattic and removed request for LesnyRumcajs November 15, 2024 06:57

ruseinov added 3 commits November 25, 2024 20:06

Merge branch 'main' into ru/fix/gc-manifest

2be3331

add a gc test

6b42faf

Merge branch 'main' into ru/fix/gc-manifest

9a61222

ruseinov mentioned this pull request Nov 26, 2024

Share more logic between different DB implementations. #5005

Open

elmattic approved these changes Nov 26, 2024

View reviewed changes

Merge branch 'main' into ru/fix/gc-manifest

2927a83

hanabi1224 reviewed Nov 26, 2024

View reviewed changes

ruseinov mentioned this pull request Nov 26, 2024

Figure out a way to manually trigger parityDB sync. #5008

Open

hanabi1224 approved these changes Nov 27, 2024

View reviewed changes

hanabi1224 requested a review from lemmih November 27, 2024 01:57

ruseinov added this pull request to the merge queue Nov 27, 2024

Merged via the queue into main with commit 1cf6eec Nov 27, 2024
35 checks passed

ruseinov deleted the ru/fix/gc-manifest branch November 27, 2024 07:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: don't GC manifests #4959

fix: don't GC manifests #4959

ruseinov commented Oct 31, 2024 •

edited

Loading

ruseinov commented Nov 6, 2024

LesnyRumcajs commented Nov 15, 2024

hanabi1224 Nov 26, 2024 •

edited

Loading

ruseinov Nov 26, 2024

ruseinov Nov 26, 2024

hanabi1224 Nov 26, 2024

ruseinov Nov 26, 2024

ruseinov Nov 26, 2024

ruseinov Nov 26, 2024

hanabi1224 Nov 26, 2024

ruseinov Nov 26, 2024

ruseinov Nov 26, 2024

ruseinov commented Nov 26, 2024

hanabi1224 left a comment

ruseinov commented Nov 27, 2024

fix: don't GC manifests #4959

fix: don't GC manifests #4959

Conversation

ruseinov commented Oct 31, 2024 • edited Loading

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

ruseinov commented Nov 6, 2024

LesnyRumcajs commented Nov 15, 2024

hanabi1224 Nov 26, 2024 • edited Loading

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

ruseinov commented Nov 26, 2024

hanabi1224 left a comment

Choose a reason for hiding this comment

ruseinov commented Nov 27, 2024

ruseinov commented Oct 31, 2024 •

edited

Loading

hanabi1224 Nov 26, 2024 •

edited

Loading