feat: serve forest CAR file as Blockstore #3365

hanabi1224 · 2023-08-14T12:54:58Z

Summary of changes

As part of #3334

Changes introduced in this pull request:

Serve multiple forest CAR files as Blockstore. Design discussion on slack

Running on mainnet on a DO droplet for ~1 week, everything including GC seems to work fine.
(With the Mmap change in #3414 GC time reduces by ~50%)

Reference issue to close (if applicable)

Closes

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.

…as-db

src/db/car/forest.rs

LesnyRumcajs · 2023-08-16T11:10:06Z

src/daemon/mod.rs

@@ -709,14 +708,168 @@ fn create_password(prompt: &str) -> std::io::Result<String> {
        .interact_on(&term)
 }

+pub async fn open_forest_car_union_db(


I'd try to not bloat this file any further, it's already quite big. Maybe let's have a dedicated one?

Moved to db_util.rs

LesnyRumcajs · 2023-08-16T11:11:37Z

src/daemon/mod.rs

@@ -709,14 +708,168 @@ fn create_password(prompt: &str) -> std::io::Result<String> {
        .interact_on(&term)
 }

+pub async fn open_forest_car_union_db(


Is this method unit-tested?

test_prepare_and_open_forest_car_union_db added

And then removed, as this function no longer exists

…as-db

src/daemon/mod.rs

lemmih · 2023-08-19T16:56:23Z

src/daemon/mod.rs

@@ -695,14 +694,170 @@ fn create_password(prompt: &str) -> std::io::Result<String> {
        .interact_on(&term)
 }

+pub async fn open_forest_car_union_db(


I don't understand what this function is supposed to do. It appears that it does several unrelated things (like opening the current database AND handling snapshot importing). Could you write some documentation for it?

In the current implementation, to avoid creating a wrapper type of Mutex<ManyCar> that does not require &mut self for APIs like fn read_only, snapshot import is moved to before DB initialization, thus any functions that require a mutable ManyCar is moved into this function(snapshot import, auto download snapshot).

lemmih

This PR contains a lot of changes that I need help to understand. Let's hold off merging this until the code is well-documented and intuitive.

src/daemon/mod.rs

Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>

…as-db

src/daemon/db_util.rs

src/daemon/mod.rs

src/utils/io/mmap.rs

src/daemon/mod.rs

lemmih · 2023-08-30T09:33:52Z

src/daemon/mod.rs

-        .context("Failed miserably while importing chain from snapshot")?;
-        info!("Imported snapshot in: {}s", stopwatch.elapsed().as_secs());
+        .await?;
+        consume_snapshot_file = true;


No need to set this flag as true. All downloaded files are always consumed.

lemmih

db_util.rs can be refactored away, but let's leave that for another PR.

LesnyRumcajs

Do we have good coverage for this feature? E.g., scenarios with multiple CAR files?

LesnyRumcajs · 2023-08-31T08:23:15Z

src/cli_shared/snapshot.rs

+        RetryArgs {
+            timeout: None,
+            max_retries: Some(3),
+            delay: Some(Duration::from_secs(60)),


Do we want to have it somehow parametrized?

I feel it's better to not ask the function caller to decide these parameters, for simplicity. How about changing it to

RetryArgs { timeout: None, ..Default::default() }

Sounds good

LesnyRumcajs · 2023-08-31T08:23:40Z

src/cli_shared/snapshot.rs

+}
+
+pub async fn download_file_with_retry(
+    url: Url,


Does it need to be owned?

Not here, fixed

LesnyRumcajs · 2023-08-31T08:39:47Z

src/daemon/mod.rs

+    // Import chain if needed
+    if !opts.skip_load.unwrap_or_default() {
+        if let Some(path) = &config.client.snapshot_path {
+            // TODO: respect `--consume-snapshot` CLI option once it's implemented


do we have a tracking issue for this?

Yes it's part of #3334

Add flag --consume-snapshot. It should do the same as --import-snapshot, but it'll move or delete the snapshot.

let's have the link in the comment

hanabi1224 · 2023-08-31T09:42:04Z

Do we have good coverage for this feature? E.g., scenarios with multiple CAR files?

@LesnyRumcajs I think CI tests should have covered most of the important scenarios. As for multiple CAR, it should have been covered by ManyCar tests but I can extend the CI check to load multiple CARs from both vendors we support if that's preferrable, what do you think?

LesnyRumcajs · 2023-08-31T10:44:19Z

Do we have good coverage for this feature? E.g., scenarios with multiple CAR files?

@LesnyRumcajs I think CI tests should have covered most of the important scenarios. As for multiple CAR, it should have been covered by ManyCar tests but I can extend the CI check to load multiple CARs from both vendors we support if that's preferrable, what do you think?

It wouldn't hurt to have such integration test.

Another thing that may be interesting to test (if I understand the properties of this feature correctly) is to add an artificially crafted CAR with a single entry (that would normally not exist in the current blockchain) and see if we can retrieve it via e.g., forest-cli chain read-obj.

hanabi1224 · 2023-08-31T10:52:42Z

Another thing that may be interesting to test (if I understand the properties of this feature correctly) is to add an artificially crafted CAR with a single entry (that would normally not exist in the current blockchain) and see if we can retrieve it via e.g., forest-cli chain read-obj.

@LesnyRumcajs How about loading test-snapshots/chain4.car? Its root CID does not exist in a calibnet DB and that would test both multi-CAR and read-obj

hanabi1224 · 2023-08-31T11:12:55Z

@LesnyRumcajs forest-cli chain read-obj test added

hanabi1224 added 14 commits August 14, 2023 20:53

feat: serve forest CAR file as Blockstore

fb8b660

flush temp_snapshot_writer

17a7bd3

log tipset epoch

ab55388

fix auto-download logic

cb40894

Merge branch 'main' into hm/use-snapshot-file-as-db

1243dfb

Merge remote-tracking branch 'origin/main' into hm/use-snapshot-file-…

2c72852

…as-db

fix ci

f23a45c

Fix tests

229dda9

fix clippy

d32961a

unblock parent process early before db init

8477da6

support multiple car files

7a9d6b1

log

a33c566

cleanup

8b8a443

Merge branch 'main' into hm/use-snapshot-file-as-db

b3a2b49

hanabi1224 marked this pull request as ready for review August 16, 2023 07:36

hanabi1224 requested a review from a team as a code owner August 16, 2023 07:36

hanabi1224 requested review from creativcoder and LesnyRumcajs and removed request for a team August 16, 2023 07:36

hanabi1224 added 2 commits August 16, 2023 16:41

dettach right before DB setup

168505b

fix forest_headless_encrypt_keystore_with_passphrase_should_succeed

f434632

hanabi1224 force-pushed the hm/use-snapshot-file-as-db branch from 990732f to f434632 Compare August 16, 2023 08:59

log level

a68bf43

LesnyRumcajs reviewed Aug 16, 2023

View reviewed changes

hanabi1224 added 2 commits August 18, 2023 23:01

Merge remote-tracking branch 'origin/main' into hm/use-snapshot-file-…

ee8046a

…as-db

read_only_len

027b8ee

lemmih reviewed Aug 19, 2023

View reviewed changes

src/daemon/mod.rs Outdated Show resolved Hide resolved

lemmih reviewed Aug 19, 2023

View reviewed changes

src/daemon/mod.rs Outdated Show resolved Hide resolved

lemmih reviewed Aug 19, 2023

View reviewed changes

src/daemon/mod.rs Outdated Show resolved Hide resolved

src/daemon/mod.rs Outdated Show resolved Hide resolved

src/daemon/mod.rs Outdated Show resolved Hide resolved

src/daemon/mod.rs Outdated Show resolved Hide resolved

src/daemon/mod.rs Outdated Show resolved Hide resolved

hanabi1224 and others added 7 commits August 29, 2023 22:00

Update src/daemon/db_util.rs

7ab94b7

Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>

mmap governed by the environment variable

cdabf95

Merge remote-tracking branch 'origin/main' into hm/use-snapshot-file-…

8af856a

…as-db

fix lint

03d650c

doc

9c2311b

use RwLock for Vec<AnyCar> revert epoch param

b80a502

Merge remote-tracking branch 'origin/main' into hm/use-snapshot-file-…

d4a1647

…as-db

lemmih requested changes Aug 30, 2023

View reviewed changes

resolve comments

8e5b137

hanabi1224 force-pushed the hm/use-snapshot-file-as-db branch from 8657b05 to 8e5b137 Compare August 30, 2023 09:23

rename to should_use_file_io

b85ca72

lemmih reviewed Aug 30, 2023

View reviewed changes

lemmih approved these changes Aug 30, 2023

View reviewed changes

cleanup consume_snapshot_file

c50b175

hanabi1224 mentioned this pull request Aug 30, 2023

chore: add const FOREST_CAR_FILE_EXTENSION to avoid hard codings #3427

Merged

4 tasks

Merge branch 'main' into hm/use-snapshot-file-as-db

8d19385

LesnyRumcajs reviewed Aug 31, 2023

View reviewed changes

hanabi1224 added 2 commits August 31, 2023 17:39

resolve comments

d54fa61

Merge branch 'main' into hm/use-snapshot-file-as-db

2a23e39

resolve comments

8d6e339

hanabi1224 force-pushed the hm/use-snapshot-file-as-db branch from d1c5c0a to 8d6e339 Compare August 31, 2023 11:10

LesnyRumcajs approved these changes Aug 31, 2023

View reviewed changes

hanabi1224 added this pull request to the merge queue Aug 31, 2023

Merged via the queue into main with commit e9ce087 Aug 31, 2023

hanabi1224 deleted the hm/use-snapshot-file-as-db branch August 31, 2023 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: serve forest CAR file as Blockstore #3365

feat: serve forest CAR file as Blockstore #3365

hanabi1224 commented Aug 14, 2023 •

edited

Loading

LesnyRumcajs Aug 16, 2023

hanabi1224 Aug 22, 2023

LesnyRumcajs Aug 16, 2023

hanabi1224 Aug 29, 2023

hanabi1224 Aug 29, 2023

lemmih Aug 19, 2023

hanabi1224 Aug 22, 2023

hanabi1224 Aug 23, 2023

lemmih left a comment

lemmih Aug 30, 2023

hanabi1224 Aug 30, 2023

lemmih left a comment

LesnyRumcajs left a comment

LesnyRumcajs Aug 31, 2023

hanabi1224 Aug 31, 2023 •

edited

Loading

LesnyRumcajs Aug 31, 2023

LesnyRumcajs Aug 31, 2023

hanabi1224 Aug 31, 2023

LesnyRumcajs Aug 31, 2023

hanabi1224 Aug 31, 2023 •

edited

Loading

LesnyRumcajs Aug 31, 2023

hanabi1224 Aug 31, 2023

hanabi1224 commented Aug 31, 2023

LesnyRumcajs commented Aug 31, 2023

hanabi1224 commented Aug 31, 2023

hanabi1224 commented Aug 31, 2023

feat: serve forest CAR file as Blockstore #3365

feat: serve forest CAR file as Blockstore #3365

Conversation

hanabi1224 commented Aug 14, 2023 • edited Loading

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

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

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

lemmih left a comment

Choose a reason for hiding this comment

LesnyRumcajs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanabi1224 Aug 31, 2023 • 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

hanabi1224 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanabi1224 commented Aug 31, 2023

LesnyRumcajs commented Aug 31, 2023

hanabi1224 commented Aug 31, 2023

hanabi1224 commented Aug 31, 2023

hanabi1224 commented Aug 14, 2023 •

edited

Loading

hanabi1224 Aug 31, 2023 •

edited

Loading

hanabi1224 Aug 31, 2023 •

edited

Loading