[nv16] dynamic loading of builtin actor bundles #8642

vyzo · 2022-05-11T19:33:04Z

to be resolved: properly getting repo path and network name for loading; see the TODOs; we could do nothing and keep it as is.

raulk

Light review pass, approach so far LGTM.

build/builtin_actors.go

chain/actors/bundle.go

raulk · 2022-05-11T20:35:00Z

chain/actors/bundle.go

+	return bundleFilePath, nil
+}
+
+func (b *BundleFetcher) fetchURL(url, path string) error {


nit: would it make sense to print a progress bar with loggable format when not TTY? Although defer to Lotus maintainers for hints on preferred UX.

ugh, thats hard to do actually!

https://pkg.go.dev/github.com/gosuri/uiprogress

raulk · 2022-05-11T20:36:04Z

cli/init.go

@@ -12,11 +11,9 @@ import (
 func init() {
 	// preload manifest so that we have the correct code CID inventory for cli since that doesn't
 	// go through CI
-	if len(build.BuiltinActorsV8Bundle()) > 0 {
-		bs := blockstore.NewMemory()
+	bs := blockstore.NewMemory()


Is it truly OK to load the bundle into a memory blockstore?

here yes, we only need the code CID mapping for the cli process.

Gotcha. Seems a bit wasteful, though. Can't we just load the root CID and decode the manifest? (Could do in a follow-up PR too)

the blockstore in thrown away, so it is just transient memory usage; below meh threshold ;)

Loading 30MiB into a blockstore is going to add unnecessary latency to EVERY, single, CLI, command. We don't want that.

ok, fine, but how fix that? I don't see any other way.

plus we are talking a few milliseconds, is it really such a big deal?

adding a TODO for now.

I don't see any quick improvements that can be made to avoid this. TODO and move on SGTM.

node/modules/builtin_actors.go

raulk · 2022-05-12T12:00:21Z

node/bundle/bundle.go

+	for i := 0; i < 3; i++ {
+		resp, err := http.Get(url) //nolint
+		if err != nil {
+			if isTemporary(err) {
+				log.Warnf("temporary error fetching %s: %s; retrying in 1s", url, err)
+				time.Sleep(time.Second)
+				continue
+			}
+			return xerrors.Errorf("error fetching %s: %w", url, err)
+		}
+		defer resp.Body.Close() //nolint

-	resp, err := http.Get(url) //nolint
-	if err != nil {
-		return xerrors.Errorf("error fetching %s: %w", url, err)
-	}
-	defer resp.Body.Close() //nolint
+		if resp.StatusCode != http.StatusOK {
+			log.Warnf("unexpected response fetching %s: %s (%d); retrying in 1s", url, resp.Status, resp.StatusCode)
+			time.Sleep(time.Second)
+			continue
+		}

-	if resp.StatusCode != http.StatusOK {
-		return xerrors.Errorf("error fetching %s: http response status is %d", url, resp.StatusCode)
-	}
+		f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644)
+		if err != nil {
+			return xerrors.Errorf("error opening %s for writing: %w", path, err)
+		}
+		defer f.Close() //nolint

-	f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644)
-	if err != nil {
-		return xerrors.Errorf("error opening %s for writing: %w", path, err)
-	}
-	defer f.Close() //nolint
+		if _, err := io.Copy(f, resp.Body); err != nil {
+			return xerrors.Errorf("error writing %s: %w", path, err)
+		}


I wonder if we should add HTTP resumption support. More often than not what will happen is that the download gets interrupted due to a flaky connection. In that case, we don't want to truncate and restart the download from scratch, but rather resume it?

How does go-paramfetcher do it?

maybe, but it shouldnt be much of a problem. It is a single 30M download.

We can consider in a follow up if it turns out to be a problem.

build/bundle.go

raulk · 2022-05-12T18:06:56Z

node/modules/builtin_actors.go

+	for av, rel := range build.BuiltinActorReleases {
+		key := dstore.NewKey(fmt.Sprintf("/builtin-actors/v%d/%s", av, rel))
+
+		data, err := ds.Get(ctx, key)
+		switch err {
+		case nil:
+			mfCid, err := cid.Cast(data)
+			if err != nil {
+				return result, xerrors.Errorf("error parsing cid for %s: %w", key, err)
+			}
+
+			has, err := bs.Has(ctx, mfCid)
+			if err != nil {
+				return result, xerrors.Errorf("error checking blockstore for manifest cid %s: %w", mfCid, err)
+			}
+
+			if has {
+				actors.AddManifest(av, mfCid)
+				continue
+			}
+
+		case dstore.ErrNotFound:
+
+		default:
+			return result, xerrors.Errorf("error loading %s from datastore: %w", key, err)
+		}
+
+		// ok, we don't have it -- fetch it and add it to the blockstore
+		mfCid, err := bundle.FetchAndLoadBundle(ctx, r.Path(), bs, av, rel, netw)
+		if err != nil {
 			return result, err
 		}
+
+		if err := ds.Put(ctx, key, mfCid.Bytes()); err != nil {
+			return result, xerrors.Errorf("error storing manifest CID for builtin-actors vrsion %d to the datastore: %w", av, err)
+		}


Hm, several problems here:

Users may want to delete their blockstores and reimport chain snapshots (I think this is still a very frequent operation since the splitstore still isn't out of beta). However, these keys in the datastore will prevent the bundles from being reimported again into the blockstores. Users aren't going to delete their datastores because they contain other useful stuff (like keys).

Strictly speaking, I don't think we need any of this, and I'd get rid of this extra moving part.

We are already being super bundle-import-happy by importing bundles into memory blockstores on every single CLI command process. I don't understand why the sudden frugality here.

Why don't we just reimport every bundle over a blockstore abstraction that first checks if the item exists in the underlying blockstore? (I think the buffered blockstore, or overlay blockstore should do the trick here).

Not a problem, we check whether the manifest cid is in the blockstore and if not we refetch/reload.

So users are free to delete their blockstore and import a snapshot.

This is the real blockstore and badger sucks, so we'd keep bloating on every restart.

Granted, these may not be too frequent, but still there is no need to reimport with the logic above.

Basically, for 2 this is exactly what we do: for every relevant bundle, we lookup the manifest cid in the datastore.
If it is there, we then check the blockstore to make sure it is there.
If not, or if we don't have it in the datastore, then we just go ahead and fetch/load.

Ah got it. Admittedly it's a bit hard to follow what's happening here and why this is needed (some code comments would help). But this answer satisfies me.

I still prefer a version that imports the bundle over a blockstore abstraction that blocks writes for items that already exist. I think that would give you the same effect we're looking for here, with less moving parts, and being more fail-proof.

That said, if @arajasek is happy with this, I have no objections.

I'm happy with this -- i like the idea of wiring it through a blockstore abstraction, but this gets the job done. Agreed that some comments would help, though, it takes a while to grok this.

added some comments so that the DI loader is easier to read and follow.

raulk · 2022-05-16T11:21:30Z

The final item that was left here was to sever the dependency between filecoin-ffi and the builtin-actors bundle for v7. There are some difficulties in making that happen. @vyzo and I have been discussing offband, and I'm posting my notes here.

builtin-actors v7 bundles are produced with synthetic code CIDs; that is, with CIDs of the kind fil/7/storageminer which do not content-hash over the bytecode.
That makes the car#LoadCAR() content integrity checks fail.
We could either (a) disarm those checks via an option, or (b) produce the bundle with real CIDs, import it to a memory blockstore in Lotus, and write them for real in the underlying blockstore with the synthetic Code CIDs so that ref-fvm is able to resolve them with the CID they are referenced with from the state tree.
However, synthetic CIDs use Identity multihashes (i.e. multihashes where the data is inlined in the CID), and Lotus expands such CIDs through the IDStore, without querying the underlying blockstore.
Therefore, we find that it's ultimately impossible to put the Wasm bytecode under identity multihashes in Lotus' blockstore.
The potential solutions here are:
1. Change the IDStore so that it does not expand inline CIDs when the codec is Raw.
2. Wrap the store in a remapping blockstore that holds rewrite rules such that the synthetic v7 CodeCIDs are transformed to their respective content-addressed CodeCIDs before querying the blockstore.
3. Do the rewrite fvm-side.

We don't like solution (1) because it introduces a system-wide change to address a very specific problem.
Solutions (2) and (3) require deeper changes and the tradeoff is not worth it (see next paragraph).

The realisation here is that supporting v7 bundles with FVM is only needed for testing, and only for the short time before nv16 is live. Once nv16 is live with actors v8, there is no reason we will ever want to run actors v7 with FVM. Hence, it does not make sense to implement a definitive solution for a situation that is only temporary.

In conclusion, we will keep the actors v7 dependency in filecoin-ffi and sever it once nv16 is live in mainnet and we no longer need to run canary tests.

raulk · 2022-05-16T11:23:00Z

It is worth noting that the reason why actors v7 works in Rust is because the Rust library for loading CARs is not performing content integrity checks, and the blockstore is not short-circuiting lookups for identity hashes. So this might be a smell that those Rust counterparts are underdeveloped with respect to their Go equivalents, but that's another story.

dynamic loading of builtin actor bundles

74f3002

vyzo requested review from raulk and arajasek May 11, 2022 19:33

vyzo requested a review from a team as a code owner May 11, 2022 19:33

vyzo added 3 commits May 11, 2022 22:34

bundle fetcher script does not exist any more

73f6402

fix lint

98730b9

move cli bundle loading back to init

37d94a3

vyzo changed the title ~~dynamic loading of builtin actor bundles~~ [nv16] dynamic loading of builtin actor bundles May 11, 2022

fix lint

de9ee7b

raulk mentioned this pull request May 11, 2022

nv16 development checklist filecoin-project/ref-fvm#531

Closed

48 tasks

raulk reviewed May 11, 2022

View reviewed changes

vyzo added 3 commits May 12, 2022 09:47

fix typo

da8bda3

move bundle fetcher to node/bundle instead of chain/actors

26d07fd

add retry logic for bundle fetcher

7b6f109

raulk reviewed May 12, 2022

View reviewed changes

vyzo added 6 commits May 12, 2022 16:21

LOTUS_PATH is the canonical way to get the repo path

70bf990

refactor to pass the network bundle name through ldflags from build

7be42d9

pass NetworkBundle through ldflags

14f597d

add TODO about the bundle load cost

a803021

fix lotus-seed

37b8072

use embeded toml spec for actor bundles

515a2db

raulk reviewed May 12, 2022

View reviewed changes

rename bundle.toml to bundles.toml

30d3398

arajasek approved these changes May 12, 2022

View reviewed changes

add comments in the DI bundle loader

68b280b

vyzo merged commit 8a8fb56 into feat/nv16 May 12, 2022

vyzo deleted the feat/nv16-bundles branch May 12, 2022 19:18

vyzo mentioned this pull request May 16, 2022

[nv16] add support for v7 release bundles #8653

Closed

vyzo mentioned this pull request May 16, 2022

Builtin Actor Bundling Strategy #8625

Closed

travisperson mentioned this pull request May 16, 2022

Consistent naming in fvm bundling #8659

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nv16] dynamic loading of builtin actor bundles #8642

[nv16] dynamic loading of builtin actor bundles #8642

vyzo commented May 11, 2022

raulk left a comment

raulk May 11, 2022

vyzo May 12, 2022

raulk May 12, 2022

raulk May 11, 2022

vyzo May 12, 2022

raulk May 12, 2022

vyzo May 12, 2022

raulk May 12, 2022 •

edited

Loading

vyzo May 12, 2022

vyzo May 12, 2022 •

edited

Loading

vyzo May 12, 2022

arajasek May 12, 2022

raulk May 12, 2022

raulk May 12, 2022

vyzo May 12, 2022 •

edited

Loading

raulk May 12, 2022

vyzo May 12, 2022

vyzo May 12, 2022

vyzo May 12, 2022

raulk May 12, 2022

arajasek May 12, 2022

vyzo May 12, 2022

raulk commented May 16, 2022

raulk commented May 16, 2022

[nv16] dynamic loading of builtin actor bundles #8642

[nv16] dynamic loading of builtin actor bundles #8642

Conversation

vyzo commented May 11, 2022

raulk 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

raulk May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo May 12, 2022 • 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

vyzo May 12, 2022 • 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

raulk commented May 16, 2022

raulk commented May 16, 2022

raulk May 12, 2022 •

edited

Loading

vyzo May 12, 2022 •

edited

Loading

vyzo May 12, 2022 •

edited

Loading