refactor: actors builder

[drahnr]

In the past, not all dependencies were tracked and cause sometimes stale or broken artifacts.

Takes pieces from substrate-wasm-builder and constructs a custom.

Outline:

Integrate the .car files and constant byte arrays. These are used whenever none are specified as part of *.toml file.

---

This change is 

[cryptoAtwill]

Reviewable status: 0 of 27 files reviewed, 2 unresolved discussions

---

fendermint/actors-builtin-car/build.rs line 125 at r1 (raw file):

    let tag = std::env::var(VERSION_OVERRIDE).unwrap_or_else(|_e| BUILTIN_ACTORS_TAG.to_owned());

    let (tmp, tmp_digest) = download_builtin_actors_bundle(tag, &out_dir).await?;

is it worthwhile to check if the file already exists and equals the checksum, if yes, skip download?

---

fendermint/actors-builtin-car/build.rs line 131 at r1 (raw file):

            // compare digests, if mismatch, replace existing with the downloaded file
            let actual = file_digest(f)?;
            if tmp_digest != actual {

can we guarantee that the tmp_digest is more correct than the actual? The digest should be hardcoded or env or passed in?

[drahnr]

Reviewable status: 0 of 27 files reviewed, 2 unresolved discussions (waiting on @cryptoAtwill)

---

fendermint/actors-builtin-car/build.rs line 125 at r1 (raw file):

Previously, cryptoAtwill wrote…

is it worthwhile to check if the file already exists and equals the checksum, if yes, skip download?

This only works if we have a hardcoded checksum.

---

fendermint/actors-builtin-car/build.rs line 131 at r1 (raw file):

Previously, cryptoAtwill wrote…

can we guarantee that the tmp_digest is more correct than the actual? The digest should be hardcoded or env or passed in?

The current approach does not guarantee reproducibility, for that, we'd have to have an anchor to check against. What's implemented now, ensures whenever the target file is changed the build.rs re-runs and downloads and ensures the upstream version is used.

[karlem]

Reviewable status: 0 of 27 files reviewed, 10 unresolved discussions (waiting on @cryptoAtwill)

---

fendermint/actors-builtin-car/build.rs line 125 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

This only works if we have a hardcoded checksum.

You could check the checksum here: .sha256, so you don’t need to download the file every time. Otherwise, the checksum of the old file becomes useless since the file can always be replaced, making verification unreliable.

---

fendermint/actors-custom-car/Cargo.toml line 2 at r1 (raw file):

[package]
# TODO rename to actors-custom-car

Why not now?

---

fendermint/vm/interpreter/src/genesis.rs line 164 at r1 (raw file):

    hardhat: Hardhat,
    /// The built in actors bundle path
    builtin_actors: &'a [u8],

👌🏻

---

fendermint/vm/snapshot/src/car/chunker.rs line 52 at r1 (raw file):

        F: Fn(usize) -> String,
        F: Send + Sync,
        F: 'static,

Nit: IMO the inline is fine.

---

fendermint/actors-builtin-car/build.rs line 72 at r1 (raw file):

    type Error = eyre::Error;
    fn try_from(value: &[u8]) -> Result<Self, Self::Error> {
        Ok(Checksum(slice_to_array::<32>(value)?))

I would just inline.

Code snippet:

let array: [u8; 32] = value.try_into().map_err(|_| eyre!("Length mismatch"))?

---

fendermint/actors-builtin-car/build.rs line 77 at r1 (raw file):

fn tempfile(out_dir: &Path) -> Result<NamedTempFile> {
    let t = NamedTempFile::new_in(out_dir)?;

Nit: IMO - this function is not needed. It does not add any extra logic.

---

fendermint/actors-builtin-car/build.rs line 89 at r1 (raw file):

    let mut tmp = tempfile(out_dir)?;

    let url = format!("https://github.com/filecoin-project/builtin-actors/releases/download/{tag}/builtin-actors-mainnet.car");

First, you can download the digest, which is part of the release, and use it to decide whether to download the file. Then, you can compare the downloaded digest with your own to verify that the data is legitimate.

---

fendermint/actors-builtin-car/build.rs line 99 at r1 (raw file):

        let piece = piece?;
        let piece = piece.slice(..);
        let mut reader = piece.clone().reader();

why not just write it like below?

Code snippet:

sha.update(&piece);
tmp.as_file_mut().write_all(&piece)?;

---

fendermint/actors/Cargo.toml line 2 at r1 (raw file):

[package]
name = "actors-umbrella"

Nit: Maybe rename to actors-core or actors-suite?

---

fendermint/actors/Cargo.toml line 9 at r1 (raw file):

description = "Depend on all individual actors to be included."

# we use the list of dependencies

This is a list of dependencies used by the builder to determine which ones to include in the .car file during the build process. It also filters for the fendermint_actor_ prefix. The all-in-one Wasm blob is not used and should not be compiled, neither for the host nor the target architecture.

[drahnr]

Reviewable status: 0 of 38 files reviewed, 10 unresolved discussions (waiting on @cryptoAtwill and @karlem)

---

fendermint/actors-builtin-car/build.rs line 89 at r1 (raw file):

Previously, karlem (Karel Moravec) wrote…

First, you can download the digest, which is part of the release, and use it to decide whether to download the file. Then, you can compare the downloaded digest with your own to verify that the data is legitimate.

I opted against fetching the remote digest, eventually we want to have a fixed artifact version, so I went with including the digest verbatim from the download.

---

fendermint/actors-builtin-car/build.rs line 99 at r1 (raw file):

Previously, karlem (Karel Moravec) wrote…

why not just write it like below?

because piece has interesting semantics, and I failed to make the above compile. slice(..) does not create a rust native slice, but a wrapper over Bytes.

---

fendermint/actors-builtin-car/build.rs line 125 at r1 (raw file):

Previously, karlem (Karel Moravec) wrote…

You could check the checksum here: .sha256, so you don’t need to download the file every time. Otherwise, the checksum of the old file becomes useless since the file can always be replaced, making verification unreliable.

Considered and disregarded, see 2 comments up

---

fendermint/actors-builtin-car/build.rs line 131 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

The current approach does not guarantee reproducibility, for that, we'd have to have an anchor to check against. What's implemented now, ensures whenever the target file is changed the build.rs re-runs and downloads and ensures the upstream version is used.

It's hardcoded now

---

fendermint/actors-custom-car/Cargo.toml line 2 at r1 (raw file):

Previously, karlem (Karel Moravec) wrote…

Why not now?

It has a few usages throughout the codebase, and I'd prefer to make a followup PR since CI finally passes on this one.

[drahnr]

Dismissed @cryptoAtwill and @karlem from 3 discussions.
Reviewable status: 0 of 38 files reviewed, 7 unresolved discussions (waiting on @cryptoAtwill and @karlem)

---

fendermint/actors/Cargo.toml line 9 at r1 (raw file):

Previously, karlem (Karel Moravec) wrote…

This is a list of dependencies used by the builder to determine which ones to include in the .car file during the build process. It also filters for the fendermint_actor_ prefix. The all-in-one Wasm blob is not used and should not be compiled, neither for the host nor the target architecture.

Isn't that what this says? I don't know what I should do with your comment

---

fendermint/actors-builtin-car/build.rs line 72 at r1 (raw file):

Previously, karlem (Karel Moravec) wrote…

I would just inline.

If I look at the above, you shift complexity around. I like the current one better for readability

---

fendermint/actors-builtin-car/build.rs line 77 at r1 (raw file):

Previously, karlem (Karel Moravec) wrote…

Nit: IMO - this function is not needed. It does not add any extra logic.

Mostly exists for readability.

---

fendermint/actors-custom-car/Cargo.toml line 2 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

It has a few usages throughout the codebase, and I'd prefer to make a followup PR since CI finally passes on this one.

Done

---

fendermint/vm/snapshot/src/car/chunker.rs line 52 at r1 (raw file):

Previously, karlem (Karel Moravec) wrote…

Nit: IMO the inline is fine.

Don't really care, I just found it easier to parse, particularly Fn(..) -> .. trait bounds

[karlem]

Reviewed 4 of 27 files at r1, 1 of 3 files at r2, 8 of 23 files at r3, 2 of 5 files at r4.
Reviewable status: 15 of 38 files reviewed, all discussions resolved (waiting on @cryptoAtwill)

---

fendermint/actors/Cargo.toml line 9 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

Isn't that what this says? I don't know what I should do with your comment

Yeah, I was just not sure when read it the first time so I have tried to adjust it.

---

fendermint/actors-builtin-car/build.rs line 89 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

I opted against fetching the remote digest, eventually we want to have a fixed artifact version, so I went with including the digest verbatim from the download.

Yeah, that's okay, but then it requires you to always download the artifact for each build. With the remote digest, you could compare it to the local one and skip the unnecessary download if they are the same.

---

fendermint/vm/snapshot/src/car/chunker.rs line 52 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

Don't really care, I just found it easier to parse, particularly Fn(..) -> .. trait bounds

Yeah not a big deal, this is a question of style. To me the multiple lines evoke multiple generic types. But you can leave it as it is.

[karlem]

Reviewable status: 15 of 38 files reviewed, all discussions resolved (waiting on @cryptoAtwill)