feat: nv17 state migrations #2983

hanabi1224 · 2023-06-14T10:42:54Z

Summary of changes

Changes introduced in this pull request:

Port nv17 state migration from Go code
Port unit tests from Go code
Regression test for nv17 state migration on calibnet

Reference issue to close (if applicable)

Closes #2801

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

If it works, then it works. 🙏

LesnyRumcajs · 2023-06-28T08:23:02Z

Cargo.toml

@@ -203,6 +210,9 @@ anyhow = "1"
 protobuf-codegen = "3.2"
 walkdir = "2.3"

+[patch.crates-io]
+fvm_ipld_amt = { git = "https://github.com/hanabi1224/ref-fvm", branch = "hm/amt-diff-for-patching" }


Do we have a tracking issue to remove this? If not, let's create one.

Added ChainSafe/fil-actor-states#143

LesnyRumcajs · 2023-06-28T08:24:36Z

Dockerfile

@@ -27,7 +27,8 @@ SHELL ["/bin/bash", "-o", "pipefail", "-c"]

 # install dependencies
 RUN apt-get update && \
-    apt-get install --no-install-recommends -y build-essential clang cmake curl ca-certificates
+    apt-get install --no-install-recommends -y build-essential clang cmake curl git ca-certificates
+RUN update-ca-certificates


Why is this required for migrations?

This is needed for downloading the patched crate from github fvm_ipld_amt

LesnyRumcajs · 2023-06-28T08:26:37Z

src/shim/econ.rs

+pub use fvm_shared::econ::TokenAmount as TokenAmount_v2;
+pub use fvm_shared3::econ::TokenAmount as TokenAmount_v3;


Can we avoid making this public? It breaks the encapsulation given by the shim.

LesnyRumcajs · 2023-06-28T08:27:21Z

src/shim/piece.rs

+pub use fvm_shared::piece as piece_v2;
+pub use fvm_shared3::piece as piece_v3;


Can we avoid making this public? It breaks the encapsulation given by the shim.

LesnyRumcajs · 2023-06-28T08:28:09Z

src/shim/machine/manifest.rs

@@ -104,7 +104,7 @@ impl Manifest {
        })
    }

-    /// Returns optional actors CID
+    /// Returns actors CID


I guess we can get rid of this comment, the method's name is pretty self-describing.

Suggested change

/// Returns actors CID

LesnyRumcajs · 2023-06-28T08:34:47Z

src/state_migration/nv17/migration.rs

+            if name == MARKET_ACTOR_NAME || name == VERIFREG_ACTOR_NAME {
+                self.add_migrator(*code, Arc::new(DeferredMigrator))
+            } else {
+                let new_code = new_manifest.code_by_name(name).unwrap();


Is this 100% not supposed to fail? If it can fail, maybe adding a helpful message with enough debug information would help.

The usage of unwrap follows nv18 and vn19 code, I can change them altogether, regarding error message, code_by_name provides a verbose message already.

pub fn code_by_name(&self, name: &str) -> anyhow::Result<&Cid> { self.by_name .get(name) .ok_or_else(|| anyhow!("Failed to retrieve actor code by name {name}")) }

LesnyRumcajs · 2023-06-28T08:38:10Z

src/state_migration/nv17/migration.rs

+            }
+        });
+
+        //https://github.com/filecoin-project/go-state-types/blob/master/builtin/v9/migration/top.go#LL176C2-L176C38


It'd be better to use a permalink here. The code for v9 migrations will most likely not change, but the devs may put it into a different directory, resulting in 404 for anyone following this link.

LesnyRumcajs · 2023-06-28T09:15:32Z

src/state_migration/nv17/migration.rs

+        // Sets empty datacap actor to migrate from
+        let datacap_code = new_manifest.code_by_name(DATACAP_ACTOR_NAME)?;
+        actors_in.set_actor(
+            &fil_actors_shared::v9::builtin::DATACAP_TOKEN_ACTOR_ADDR.into(),


I don't quite get it. Aren't we migrating from v8?

Yeah, only because datacap is not defined under v8, see https://github.com/ChainSafe/fil-actor-states/blob/main/fil_actors_shared/src/v8/builtin/singletons.rs#L7

Let's note it down in the comment. Additionally, it'd be fantastic to fix it in the fil-actor-states.

Since address ids are identical across versions, maybe we should switch to version-less address in fil_actor_interface crate, like https://github.com/ChainSafe/fil-actor-states/blob/main/fil_actor_interface/src/builtin/cron/mod.rs#L13

Sounds good to me.

Yeah but the Go code handles this specially by creating an empty actor to migrate from

Is there any difference in the end? I mean, if we have a cleaner way of doing something, it's okay not to do exactly the same as Lotus does.

I can have a try

Switched to a post migrator with comments

LesnyRumcajs · 2023-06-28T09:23:20Z

src/state_migration/nv17/miner.rs

+            let policy = match &self.network {
+                NetworkChain::Mainnet => fil_actors_shared::v9::runtime::Policy::mainnet(),
+                NetworkChain::Calibnet => fil_actors_shared::v9::runtime::Policy::calibnet(),
+                NetworkChain::Devnet(_) => unimplemented!("Policy::devnet"),


It's not ideal. Theoretically we could contrive a definition somewhere in fil-actor-states, see https://github.com/ChainSafe/forest/blob/devnet-in-ci/src/networks/mod.rs#L210

That being said, it's not important at the moment. Maybe we could have a tracking issue instead.

Policy under v9 and v10 are almost identical except one uses fvm2 types the other uses fvm3 types. We could maybe implement From trait between them to reuse ChainConfig.policy

Tracking issue: ChainSafe/fil-actor-states#151

LesnyRumcajs · 2023-06-28T09:24:23Z

src/state_migration/nv17/miner.rs

+fn miner_prev_sectors_in_key(addr: &Address) -> String {
+    format!("prevSectorsIn-{addr}")
+}
+
+fn miner_prev_sectors_out_key(addr: &Address) -> String {
+    format!("prevSectorsOut-{addr}")
+}
+
+fn sectors_amt_key(cid: &Cid) -> anyhow::Result<String> {
+    Ok(format!(
+        "sectorsAmt-{}",
+        cid.to_string_of_base(Base::Base32Lower)?,
+    ))
+}


Let's format the strings in a rust_way.

lemmih

Alright. Mirroring the golang code should make the next migrations easier. In the far future, we can write idiomatic Rust code for these migrations.

src/shim/bigint.rs

lemmih · 2023-07-03T12:05:26Z

src/shim/bigint.rs

@@ -3,14 +3,24 @@

 use std::ops::{Deref, DerefMut};

-pub use fvm_shared3::bigint::bigint_ser::{BigIntDe, BigIntSer};
+pub use fvm_shared::sector::StoragePower as StoragePowerV2;


StoragePowerV2 and StoragePowerV3 are the same type. Let's just export StoragePower = BigInt.

src/state_migration/nv17/datacap.rs

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

hanabi1224 added 30 commits May 10, 2023 19:26

feat: nv17 state migration

619e3e8

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

7350016

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

e6af1fa

boilerplate code

69914fe

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

72c6699

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

2e9600c

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

fed3317

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

34bfe82

fix build

495a299

miner type convertion

e4e0939

store_empty_map

fc11335

state

8619e96

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

e839d28

datacap

dc42a68

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

94def6a

datacap

d9a8251

fix build

0176f6b

get_pending_verified_deals_and_total_size impl (partial)

d9585ab

finish get_pending_verified_deals_and_total_size impl

13b5460

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

98ed2c9

refactor

94b772a

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

5787d3d

miner

b475552

re-org files

ebc4af3

MinerMigrator constructor

2d18726

impl miner (uncompleted)

e58d50d

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

65dd1f7

fix build

11c4f2b

miner migrator impl (partial)

038b546

MarketMigrator boilerplate

3ef30e7

hanabi1224 requested review from jdjaustin, creativcoder, sudo-shashank, aatifsyed and ruseinov as code owners June 27, 2023 10:39

hanabi1224 changed the title ~~[WIP] feat: nv17 state migrations~~ feat: nv17 state migrations Jun 27, 2023

update-ca-certificates in dockerfile

cc6c4bf

hanabi1224 force-pushed the hm/nv17-migrations branch from b134da1 to cc6c4bf Compare June 27, 2023 12:10

hanabi1224 added 3 commits June 27, 2023 20:19

mainnet ActorBundleInfo

b321aeb

verbose success info

83ee87e

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

76f337a

LesnyRumcajs approved these changes Jun 28, 2023

View reviewed changes

hanabi1224 added 5 commits June 28, 2023 19:32

Merge branch 'main' into hm/nv17-migrations

e9d2bdd

review comments

5ac00f4

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

33eb3dc

no unwrap of code_by_name

157fa58

encapsulate fvm types

9f2f30b

hanabi1224 mentioned this pull request Jun 29, 2023

Impl From between fil_actors_shared::v{version}::runtime::Policy types ChainSafe/fil-actor-states#151

Open

hanabi1224 added 4 commits June 29, 2023 15:37

cleanup dead code

aec4db9

cleanup commented-out code

ba0b1c5

Re-export TokenAmount::PRECISION

5f76104

Merge remote-tracking branch 'origin/main' into hm/nv17-migrations

4c6738f

lemmih approved these changes Jul 3, 2023

View reviewed changes

hanabi1224 and others added 4 commits July 3, 2023 20:33

Update src/state_migration/nv17/datacap.rs

864fc11

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

Merge branch 'main' into hm/nv17-migrations

15d9901

re-export single StoragePower

51f3271

Merge branch 'main' into hm/nv17-migrations

bf1fa57

hanabi1224 merged commit d59a95c into main Jul 3, 2023
19 checks passed

hanabi1224 deleted the hm/nv17-migrations branch July 3, 2023 13:08

hanabi1224 mentioned this pull request Oct 12, 2023

NV21 Watermelon 🍉 support #3541

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: nv17 state migrations #2983

feat: nv17 state migrations #2983

hanabi1224 commented Jun 14, 2023 •

edited

LesnyRumcajs left a comment

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 29, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 28, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 29, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 29, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 29, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 29, 2023

hanabi1224 Jun 29, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 29, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 28, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 28, 2023 •

edited

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 28, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 28, 2023

hanabi1224 Jun 29, 2023

LesnyRumcajs Jun 29, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 28, 2023

hanabi1224 Jun 29, 2023

LesnyRumcajs Jun 28, 2023

hanabi1224 Jun 29, 2023

lemmih left a comment

lemmih Jul 3, 2023

hanabi1224 Jul 3, 2023

		pub use fvm_shared::econ::TokenAmount as TokenAmount_v2;
		pub use fvm_shared3::econ::TokenAmount as TokenAmount_v3;

		pub use fvm_shared::piece as piece_v2;
		pub use fvm_shared3::piece as piece_v3;

feat: nv17 state migrations #2983

feat: nv17 state migrations #2983

Conversation

hanabi1224 commented Jun 14, 2023 • edited

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanabi1224 Jun 28, 2023 • edited

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

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

hanabi1224 commented Jun 14, 2023 •

edited

hanabi1224 Jun 28, 2023 •

edited