-
Notifications
You must be signed in to change notification settings - Fork 41
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(node): consistent genesis #1016
Changes from 1 commit
19f93f5
fcdc70f
3d8f00f
8a8efd6
3890de4
14e6dea
4dc0482
02b183f
d88face
2a4d428
0e31699
0d1c55c
d551aed
22219f1
6a1b4dd
7c6276b
c4b7e66
fa0f330
b0038cc
22e5958
0319141
7a86832
79d1264
0ae95c8
fa372d6
5332a31
ed66780
272c29c
0db23d2
ef2740c
4ba0219
47556e9
37a5522
38fbeed
86d1cf9
707060b
0ebe60f
29099f1
a3b3c4a
501a256
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,6 +147,28 @@ pub enum GenesisIpcCommands { | |
Gateway(GenesisIpcGatewayArgs), | ||
/// Fetch the genesis parameters of a subnet from the parent. | ||
FromParent(Box<GenesisFromParentArgs>), | ||
/// Seal the genesis state from the genesis parameter file | ||
SealState(SealGenesisArgs), | ||
} | ||
|
||
#[derive(Args, Debug, Clone)] | ||
pub struct SealGenesisArgs { | ||
/// The built in actors bundle path | ||
#[arg(long, short)] | ||
pub builtin_actors_path: PathBuf, | ||
|
||
/// The custom actors bundle path | ||
#[arg(long, short)] | ||
pub custom_actors_path: PathBuf, | ||
|
||
/// The ipc artifacts output path. If you are using ipc-monorepo, it should be the `out` folder | ||
/// of `make build` | ||
#[arg(long, short)] | ||
pub ipc_artifacts_path: PathBuf, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we can embed these artifacts in the build, and load them from the target directory instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cryptoAtwill, any thoughts here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops, missed this. I think we have discussed this before, if I recall correctly, it's because |
||
|
||
/// The sealed, i.e. finalized genesis state CAR file dump path | ||
#[arg(long, short)] | ||
pub sealed_genesis_path: PathBuf, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's simply call this output_path. |
||
} | ||
|
||
#[derive(Args, Debug, Clone)] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ use fendermint_vm_genesis::{ | |
ipc, Account, Actor, ActorMeta, Collateral, Genesis, Multisig, PermissionMode, SignerAddr, | ||
Validator, ValidatorKey, | ||
}; | ||
use fendermint_vm_interpreter::genesis::GenesisCreator; | ||
|
||
use crate::cmd; | ||
use crate::options::genesis::*; | ||
|
@@ -93,6 +94,8 @@ cmd! { | |
set_ipc_gateway(&genesis_file, args), | ||
GenesisIpcCommands::FromParent(args) => | ||
new_genesis_from_parent(&genesis_file, args).await, | ||
GenesisIpcCommands::SealState(args) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if it makes sense to have this under IPC? I understand that it does deploy IPC-related contracts, but it also deploys built-in contracts that might be useful without IPC. Maybe it can be moved as a root genesis command, with FEVM contracts as an optional parameter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently the separation btw ipc and non-ipc is blurred. I would actually argue we track this in another issue, because it's the downstream implementation that assumes an |
||
seal_state(&genesis_file, args).await, | ||
} | ||
} | ||
} | ||
|
@@ -279,6 +282,19 @@ fn set_ipc_gateway(genesis_file: &PathBuf, args: &GenesisIpcGatewayArgs) -> anyh | |
}) | ||
} | ||
|
||
async fn seal_state(genesis_file: &PathBuf, args: &SealGenesisArgs) -> anyhow::Result<()> { | ||
let genesis = read_genesis(genesis_file)?; | ||
|
||
let genesis_creator = GenesisCreator::new( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think should be fine with |
||
args.builtin_actors_path.clone(), | ||
args.custom_actors_path.clone(), | ||
args.ipc_artifacts_path.clone(), | ||
args.sealed_genesis_path.clone(), | ||
); | ||
|
||
genesis_creator.create(genesis).await | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These APIs are weird. We have a Genesis and the GenesisCreator. You would imagine the GenesisCreator creates a Genesis, but in reality the Genesis already exists and the GenesisCreator wants it as an input. I think we need to adjust this a little.
In practice: let mut output = // create the file
let gen = GenesisBuilder::from_params(genesis_params_path)
.with_builtin_actors(...)
.with_custom_actors(...)
.with_system_contracts(...)
.write_into(&output) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works for me. Just a side note that probably
should be added to constructor as they are required parameters, otherwise it feels it's not really required. |
||
} | ||
|
||
async fn new_genesis_from_parent( | ||
genesis_file: &PathBuf, | ||
args: &GenesisFromParentArgs, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
extern crate core; | ||
|
||
// Copyright 2022-2024 Protocol Labs | ||
// SPDX-License-Identifier: Apache-2.0, MIT | ||
mod app; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ use anyhow::anyhow; | |
use fvm_shared::bigint::{BigInt, Integer}; | ||
use serde::{Deserialize, Serialize}; | ||
use serde_with::serde_as; | ||
use std::path::PathBuf; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed alr |
||
|
||
use fendermint_actor_eam::PermissionModeParams; | ||
use fvm_shared::version::NetworkVersion; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,6 +177,17 @@ where | |
} | ||
} | ||
|
||
/// Flush the data to the block store. Returns the state root cid and the underlying state store. | ||
pub fn finalize(self) -> anyhow::Result<(Cid, DB)> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth to either explain the difference between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will remove |
||
match self.stage { | ||
Stage::Tree(_) => Err(anyhow!("invalid finalize state")), | ||
Stage::Exec(exec_state) => match exec_state.commit()? { | ||
(_, _, true) => bail!("FVM parameters are not expected to be updated in genesis"), | ||
(cid, _, _) => Ok((cid, self.store)), | ||
}, | ||
} | ||
} | ||
|
||
/// Replaces the built in actor with custom actor. This assumes the system actor is already | ||
/// created, else it would throw an error. | ||
pub fn replace_builtin_actor( | ||
|
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.
maybe?
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 think this is fine as it is.