feat: inline actor bundles #3276

hanabi1224 · 2023-07-25T15:51:14Z

Summary of changes

Changes introduced in this pull request:

Inline actor bundles
Re-enable state migration unit tests

Reference issue to close (if applicable)

Closes #2765

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.

LesnyRumcajs

We need to make it less of a burden for developers. This approach forces builds even if there are no changes.

That is to say:

❯ cargo build
   Compiling forest-filecoin v0.11.1 (/home/rumcajs/prj/forest2)
    Finished dev [unoptimized] target(s) in 15.04s

forest2 on  hm/inline-actor-bundles [$?] is 📦 v0.11.1 via  v18.16.1 via 🦀 v1.71.0 took 15s
❯ cargo build
   Compiling forest-filecoin v0.11.1 (/home/rumcajs/prj/forest2)
    Finished dev [unoptimized] target(s) in 15.13s

as opposed to

❯ cargo build
   Compiling forest-filecoin v0.11.1 (/home/rumcajs/prj/forest)
    Finished dev [unoptimized] target(s) in 20.02s

forest on  query-cid-2 [$!?] is 📦 v0.11.1 via  v18.16.1 via 🦀 v1.71.0 took 20s
❯ cargo build
    Finished dev [unoptimized] target(s) in 0.15s

Notice how I need to build the binary again, even though I didn't do any changes. On most machines, unless some custom configuration is used, it will be even slower.

LesnyRumcajs · 2023-07-26T07:17:12Z

actor_bundles/.gitignore

@@ -0,0 +1,2 @@
+*


Could we just add actor_bundles to the root .gitignore? In general, it's a bit surprising to suddenly have such directory. Maybe we could use target/?

Moved to target/actor_bundles/

hanabi1224 · 2023-07-26T11:10:31Z

This approach forces builds even if there are no changes.

@LesnyRumcajs Fixed by tweaking cargo:rerun-if-changed lines, it should now only rebuild when bundle car cache is changed

LesnyRumcajs

Let's also remove the logic from places like https://github.com/ChainSafe/forest/blob/hm/inline-actor-bundles/src/networks/devnet/mod.rs#L112 . To my understanding, this is no longer needed.

LesnyRumcajs · 2023-07-27T07:27:23Z

build.rs

+    );
+
+    let mut zstd_encoder = ZstdEncoder::with_quality(
+        async_fs::File::create(Path::new(&env::var("OUT_DIR")?).join("actor_bundles.car.zst"))


This is also defined in src/daemon/bundle.rs. Let's have a single source of truth.

Not sure how to share the code, include_bytes!(concat!(env!("OUT_DIR"), "/actor_bundles.car.zst")); requires str literals and is evaluated at compile time

Below code does not compile, only literals (like "foo", -42 and 3.14) can be passed to concat!()

const BUNDLE_NAME: &str = "/actor_bundles.car.zst"; pub const ACTOR_BUNDLES_CAR_ZST: &[u8] = include_bytes!(concat!(env!("OUT_DIR"), BUNDLE_NAME));

LesnyRumcajs · 2023-07-27T07:29:47Z

build.rs

 use walkdir::WalkDir;

 const PROTO_DIR: &str = "proto";
 const CARGO_OUT_DIR: &str = "proto";

-fn main() -> anyhow::Result<()> {
+const ACTOR_BUNDLE_CACHE_DIR: &str = "target/actor_bundles/";


It'd be great to have it as a PathBuf. The less we operate on strings and more on strongly typed objects, the less likely we'll introduce a bug.

LesnyRumcajs · 2023-07-27T07:36:48Z

src/daemon/bundle.rs

+    #[tokio::test]
+    async fn test_load_actor_bundles() {
+        let db = fvm_ipld_blockstore::MemoryBlockstore::new();
+        load_actor_bundles(&db).await.unwrap();


Let's add some assertions here, even with hard-coded Cids.

hanabi1224 · 2023-07-27T09:54:52Z

Let's also remove the logic from places like https://github.com/ChainSafe/forest/blob/hm/inline-actor-bundles/src/networks/devnet/mod.rs#L112 . To my understanding, this is no longer needed.

@LesnyRumcajs Fixed

LesnyRumcajs · 2023-07-27T10:38:52Z

src/mod.rs

+    pub url: Url,
+}
+
+lazy_static! {


Perhaps we could avoid this macro with once_cell::sync::Lazy

LesnyRumcajs · 2023-07-27T10:42:20Z

src/networks/calibnet/mod.rs

-            manifest: Cid::try_from("bafy2bzacedbedgynklc4dgpyxippkxmba2mgtw7ecntoneclsvvl4klqwuyyy").unwrap(),
-            url: Url::parse("https://github.com/filecoin-project/builtin-actors/releases/download/v9.0.3/builtin-actors-calibrationnet.car").unwrap()
-        })
+        bundle: Some(Cid::try_from("bafy2bzacedbedgynklc4dgpyxippkxmba2mgtw7ecntoneclsvvl4klqwuyyy").unwrap())


Do we still need this? I mean, we have this defined here and in src/mod.rs

We still need to map the cids to the network and height and use them in migrate functions. Moving everything to mod.rs might be too much

hanabi1224 added 6 commits July 25, 2023 23:49

feat: inline actor bundles

4cb4710

Merge remote-tracking branch 'origin/main' into hm/inline-actor-bundles

8c29d60

enable state migration tests

4a2d6d5

fix tests

6c5d366

rerun condition

ef5774b

fix

072487b

hanabi1224 marked this pull request as ready for review July 25, 2023 17:22

hanabi1224 requested a review from a team as a code owner July 25, 2023 17:22

hanabi1224 requested review from lemmih and jdjaustin and removed request for a team July 25, 2023 17:22

LesnyRumcajs requested changes Jul 26, 2023

View reviewed changes

fix cargo:rerun-if-changed

d4e32dc

move bundle cache to target/actor_bundles

7d48e50

LesnyRumcajs requested changes Jul 27, 2023

View reviewed changes

hanabi1224 added 3 commits July 27, 2023 15:58

Merge remote-tracking branch 'origin/main' into hm/inline-actor-bundles

6521afd

assert in test_load_actor_bundles

616b0c9

ACTOR_BUNDLE_CACHE_DIR: PathBuf

9565bb0

cargo:rerun-if-changed=src/mod.rs

532e35f

LesnyRumcajs reviewed Jul 27, 2023

View reviewed changes

hanabi1224 added 2 commits July 27, 2023 20:13

Favor Lazy over lazy_static

edf0f09

Merge branch 'main' into hm/inline-actor-bundles

ba36277

LesnyRumcajs approved these changes Jul 27, 2023

View reviewed changes

jdjaustin approved these changes Jul 27, 2023

View reviewed changes

lemmih approved these changes Jul 31, 2023

View reviewed changes

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

hanabi1224 removed this pull request from the merge queue due to a manual request Jul 31, 2023

Merge remote-tracking branch 'origin/main' into hm/inline-actor-bundles

4ded979

hanabi1224 enabled auto-merge July 31, 2023 07:16

hanabi1224 mentioned this pull request Jul 31, 2023

feat: add blockstore that is the union of several CAR stores #3288

Merged

4 tasks

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

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 31, 2023

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

Merged via the queue into main with commit 1fcea0c Jul 31, 2023

hanabi1224 deleted the hm/inline-actor-bundles branch July 31, 2023 08:17

sudo-shashank pushed a commit that referenced this pull request Aug 3, 2023

feat: inline actor bundles (#3276)

c82f3f8

LesnyRumcajs mentioned this pull request Oct 19, 2023

Download actor bundles at runtime #3614

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: inline actor bundles #3276

feat: inline actor bundles #3276

hanabi1224 commented Jul 25, 2023 •

edited

Loading

LesnyRumcajs left a comment •

edited

Loading

LesnyRumcajs Jul 26, 2023

hanabi1224 Jul 26, 2023

hanabi1224 commented Jul 26, 2023

LesnyRumcajs left a comment

LesnyRumcajs Jul 27, 2023

hanabi1224 Jul 27, 2023

hanabi1224 Jul 27, 2023

LesnyRumcajs Jul 27, 2023

hanabi1224 Jul 27, 2023

LesnyRumcajs Jul 27, 2023

hanabi1224 Jul 27, 2023

hanabi1224 commented Jul 27, 2023

LesnyRumcajs Jul 27, 2023

hanabi1224 Jul 27, 2023

LesnyRumcajs Jul 27, 2023

hanabi1224 Jul 27, 2023 •

edited

Loading

feat: inline actor bundles #3276

feat: inline actor bundles #3276

Conversation

hanabi1224 commented Jul 25, 2023 • edited Loading

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

LesnyRumcajs left a comment • 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 Jul 26, 2023

LesnyRumcajs 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanabi1224 commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanabi1224 Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

hanabi1224 commented Jul 25, 2023 •

edited

Loading

LesnyRumcajs left a comment •

edited

Loading

hanabi1224 Jul 27, 2023 •

edited

Loading