-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: nv17 state migrations #2983
Conversation
b134da1
to
cc6c4bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works, then it works. 🙏
@@ -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" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a tracking issue to remove this? If not, let's create one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this required for migrations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed for downloading the patched crate from github fvm_ipld_amt
src/shim/econ.rs
Outdated
pub use fvm_shared::econ::TokenAmount as TokenAmount_v2; | ||
pub use fvm_shared3::econ::TokenAmount as TokenAmount_v3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid making this public? It breaks the encapsulation given by the shim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/shim/piece.rs
Outdated
pub use fvm_shared::piece as piece_v2; | ||
pub use fvm_shared3::piece as piece_v3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid making this public? It breaks the encapsulation given by the shim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/shim/machine/manifest.rs
Outdated
@@ -104,7 +104,7 @@ impl Manifest { | |||
}) | |||
} | |||
|
|||
/// Returns optional actors CID | |||
/// Returns actors CID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can get rid of this comment, the method's name is pretty self-describing.
/// Returns actors CID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this 100% not supposed to fail? If it can fail, maybe adding a helpful message with enough debug information would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}"))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
}); | ||
|
||
//https://github.com/filecoin-project/go-state-types/blob/master/builtin/v9/migration/top.go#LL176C2-L176C38 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// 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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get it. Aren't we migrating from v8
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's note it down in the comment. Additionally, it'd be fantastic to fix it in the fil-actor-states
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but the Go code handles this specially by creating an empty actor to migrate from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can have a try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to a post migrator with comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
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"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking issue: ChainSafe/fil-actor-states#151
src/state_migration/nv17/miner.rs
Outdated
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)?, | ||
)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's format the strings in a rust_way
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StoragePowerV2
and StoragePowerV3
are the same type. Let's just export StoragePower = BigInt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #2801
Other information and links
Change checklist