feat: refactor: actor bundling system #8838

Stebalien · 2022-06-10T03:28:51Z

Related Issues

fixes #8701

Proposed Changes

Include the builtin-actors in the lotus source tree.
Embed the bundle on build instead of downloading at runtime.
Avoid reading the bundle whenever possible by including bundle metadata (the bundle CID, the actor CIDs, etc.).
Remove everything related to dependency injection.
1. We're no longer downloading the bundle, so doing anything ahead of time doesn't really help.
2. We register the manifests on init because, unfortunately, they're global.
3. We explicitly load the current actors bundle in the genesis state-tree method.
4. For testing, we just change the in-use bundle with a bit of a hack. It's not great, but using dependency injection doesn't make any sense either because, again, the manifest information is global.
Remove the bundle.toml file. Bundles may be overridden by specifying an override path in the parameters file, or an environment variable.

Checklist

Before you mark the PR ready for review, please make sure that:

All commits have a clear commit message.
The PR title is in the form of of <PR type>: <area>: <change being made>
- example: fix: mempool: Introduce a cache for valid signatures
- PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
- area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
This PR has tests for new functionality or change in behaviour
If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
CI is green

TODO

Add a test to make sure that the generated bundle metadata matches the embedded bundle.
Test to make sure that loading from paths still works correctly.

Maybes:

Cleanup/improve the download script? Maybe add a make target?
~~Merge some of the methods in chain/actors/builtin/builtin.go with chain/actors/bundle.go, because they're basically the same.~~

build/params_2k.go

chain/actors/builtin/builtin.go

chain/consensus/filcns/upgrades.go

cmd/lotus-miner/init.go

Stebalien · 2022-06-10T03:33:50Z

cmd/lotus-seed/genesis.go

@@ -580,12 +578,7 @@ var genesisCarCmd = &cli.Command{
 		bstor := blockstore.WrapIDStore(blockstore.NewMemorySync())
 		sbldr := vm.Syscalls(ffiwrapper.ProofVerifier)

-		// load appropriate bundles
-		if err := bundle.FetchAndLoadBundles(c.Context, bstor, build.BuiltinActorReleases); err != nil {


MakeInitialStateTree now loads the actors bundle, if necessary, for us.

itests/kit/ensemble.go

lotuspond/front/src/chain/methods.json

node/builder_miner.go

paychmgr/paychget_test.go

1. Include the builtin-actors in the lotus source tree. 2. Embed the bundle on build instead of downloading at runtime. 3. Avoid reading the bundle whenever possible by including bundle metadata (the bundle CID, the actor CIDs, etc.). 4. Remove everything related to dependency injection. 1. We're no longer downloading the bundle, so doing anything ahead of time doesn't really help. 2. We register the manifests on init because, unfortunately, they're global. 3. We explicitly load the current actors bundle in the genesis state-tree method. 4. For testing, we just change the in-use bundle with a bit of a hack. It's not great, but using dependency injection doesn't make any sense either because, again, the manifest information is global. fixes #8701

arajasek

Direction generally LGTM.

build/params_debug.go

Instead, we override bundles using environment variables and overrides in the build parameter files.

1. Use mockProofs to determine the correct test bundle. 2. Don't even bother setting InsecurePoStValidation for itests, it's already set.

This gets tested: 1. When we load dynamically (by path) specified bundles. 2. When we decode the embedded bundles to generate the metadata. Effectively, this lets us test that the embedded bundles are actually correct.

arajasek

One question, but this seems like a major improvement!

build/builtin_actors.go

build/builtin_actors_gen.go

jennijuju · 2022-06-13T14:18:19Z

Makefile

@@ -295,6 +295,12 @@ actors-gen:
 	$(GOCC) run ./chain/actors/agen
 	$(GOCC) fmt ./...

+bundle-gen:


bundle-jen?

This is a must-have.

build/actors/README.md

jennijuju · 2022-06-13T14:22:56Z

build/actors/README.md

+This will:
+
+1. Download the actors bundles and pack them into the appropriate tarfile.
+2. Run `make bundle-gen` in the top-level directory to regenerate the bundle metadata file.


can one specify two versions at one time?

No, you'd have to run the command for each version/release you want to download.

Oh, I see. make bundle-gen will generate the metadata for all versions.

build/params_mainnet.go

1. Include the builtin-actors in the lotus source tree. 2. Embed the bundle on build instead of downloading at runtime. 3. Avoid reading the bundle whenever possible by including bundle metadata (the bundle CID, the actor CIDs, etc.). 4. Remove everything related to dependency injection. 1. We're no longer downloading the bundle, so doing anything ahead of time doesn't really help. 2. We register the manifests on init because, unfortunately, they're global. 3. We explicitly load the current actors bundle in the genesis state-tree method. 4. For testing, we just change the in-use bundle with a bit of a hack. It's not great, but using dependency injection doesn't make any sense either because, again, the manifest information is global. 5. Remove the bundle.toml file. Bundles may be overridden by specifying an override path in the parameters file, or an environment variable. fixes #8701

Stebalien force-pushed the feat/builtin-bundle branch 2 times, most recently from 01e4341 to e2a28d3 Compare June 10, 2022 03:44

Stebalien commented Jun 10, 2022

View reviewed changes

Stebalien force-pushed the feat/builtin-bundle branch 2 times, most recently from 3b18d2c to e9d419f Compare June 10, 2022 04:59

Stebalien commented Jun 10, 2022

View reviewed changes

paychmgr/paychget_test.go Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

Stebalien force-pushed the feat/builtin-bundle branch from e9d419f to eff32b7 Compare June 10, 2022 05:11

fix: remove duplicate actor registration for v8

fb35ad6

arajasek reviewed Jun 10, 2022

View reviewed changes

build/params_debug.go Outdated Show resolved Hide resolved

test: build: make sure we're registering the actors manifest

2f4c33c

arajasek mentioned this pull request Jun 10, 2022

docs: state: update actors_version_checklist #8844

Closed

5 tasks

fix: build: centralize bundle path override logic

e2c7407

Stebalien force-pushed the feat/builtin-bundle branch 6 times, most recently from 8d927ed to a0f907f Compare June 10, 2022 22:51

feat: build: remove the bundle.toml file

54a4097

Instead, we override bundles using environment variables and overrides in the build parameter files.

Stebalien force-pushed the feat/builtin-bundle branch from a0f907f to 54a4097 Compare June 10, 2022 22:55

Stebalien marked this pull request as ready for review June 10, 2022 23:00

Stebalien requested a review from a team as a code owner June 10, 2022 23:00

Stebalien force-pushed the feat/builtin-bundle branch from 67710eb to 089446d Compare June 10, 2022 23:50

fix: tests: correctly use mockProofs/InsecurePoStValidation

ea07648

1. Use mockProofs to determine the correct test bundle. 2. Don't even bother setting InsecurePoStValidation for itests, it's already set.

Stebalien force-pushed the feat/builtin-bundle branch from 089446d to ea07648 Compare June 11, 2022 00:00

Stebalien added 3 commits June 10, 2022 17:28

chore: test: use unified policy

33979ee

fix: make the itest config match the test bundle

ec9dd5c

feat: verify that bundles have all actors when we read the metadata

f01bd41

This gets tested: 1. When we load dynamically (by path) specified bundles. 2. When we decode the embedded bundles to generate the metadata. Effectively, this lets us test that the embedded bundles are actually correct.

arajasek approved these changes Jun 11, 2022

View reviewed changes

build/builtin_actors.go Show resolved Hide resolved

simlecode reviewed Jun 13, 2022

View reviewed changes

build/builtin_actors_gen.go Show resolved Hide resolved

arajasek added the release/backport label Jun 13, 2022

jennijuju changed the title ~~Refactor actor bundling system~~ feat: refactor: actor bundling system Jun 13, 2022

jennijuju reviewed Jun 13, 2022

View reviewed changes

build/actors/README.md Outdated Show resolved Hide resolved

jennijuju reviewed Jun 13, 2022

View reviewed changes

arajasek reviewed Jun 13, 2022

View reviewed changes

build/params_mainnet.go Show resolved Hide resolved

address review

fff677d

Stebalien merged commit 30981d0 into master Jun 13, 2022

Stebalien deleted the feat/builtin-bundle branch June 13, 2022 17:15

jennijuju removed the release/backport label Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor: actor bundling system #8838

feat: refactor: actor bundling system #8838

Stebalien commented Jun 10, 2022 •

edited

Loading

Stebalien Jun 10, 2022

This comment was marked as resolved.

arajasek left a comment

arajasek left a comment

jennijuju Jun 13, 2022

arajasek Jun 13, 2022

jennijuju Jun 13, 2022

Stebalien Jun 13, 2022

Stebalien Jun 13, 2022

feat: refactor: actor bundling system #8838

feat: refactor: actor bundling system #8838

Conversation

Stebalien commented Jun 10, 2022 • edited Loading

Related Issues

Proposed Changes

Checklist

TODO

Stebalien Jun 10, 2022

Choose a reason for hiding this comment

This comment was marked as resolved.

arajasek left a comment

Choose a reason for hiding this comment

arajasek left a comment

Choose a reason for hiding this comment

jennijuju Jun 13, 2022

Choose a reason for hiding this comment

arajasek Jun 13, 2022

Choose a reason for hiding this comment

jennijuju Jun 13, 2022

Choose a reason for hiding this comment

Stebalien Jun 13, 2022

Choose a reason for hiding this comment

Stebalien Jun 13, 2022

Choose a reason for hiding this comment

Stebalien commented Jun 10, 2022 •

edited

Loading