Skip to content
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.

Sign up for GitHub

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

Store the domain genesis storage on the consensus chain #1824

Merged
merged 14 commits into from
Oct 3, 2023
Merged

Store the domain genesis storage on the consensus chain #1824

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a1cc353
Refactor the pallet-domain-id and export the storage key of SelfDomainId
NingLin-P Sep 13, 2023
02163c7
Introduce RawGenesis which is the genesis storage with codec and Type…
NingLin-P Sep 13, 2023
e64b92e
Store the RawGenesis in the runtime registry instead of just the runt…
NingLin-P Sep 14, 2023
0e8d82e
Use the runtime_object.raw_genesis to derive the genesis storage/stat…
NingLin-P Sep 14, 2023
3154370
Remove the domain related host function
NingLin-P Sep 14, 2023
28ce5da
Regiter the genesis domain on consensus block zero
NingLin-P Sep 14, 2023
a524100
Remove the raw_genesis_config from domain registry
NingLin-P Sep 14, 2023
fb9a367
Replace the domain subcommand build-genesis-config with build-genesis…
NingLin-P Sep 14, 2023
eb8036d
Increase the spec_version of the test evm domain runtime by one
NingLin-P Sep 14, 2023
d8fe8bf
Update both the runtime code and the genesis state in the upgrade_dom…
NingLin-P Sep 20, 2023
3cc6914
Apply review suggestions
NingLin-P Sep 21, 2023
2768da8
Merge branch 'main' of github.com:subspace/subspace into genesis-storage
NingLin-P Sep 21, 2023
245f219
Use String for runtime_name/domain_name
NingLin-P Sep 21, 2023
59f9e64
Merge branch 'main' of github.com:subspace/subspace into genesis-storage
NingLin-P Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions crates/pallet-domains/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,14 +922,18 @@ mod pallet {
origin: OriginFor<T>,
runtime_name: Vec<u8>,
runtime_type: RuntimeType,
nazar-pc marked this conversation as resolved.
Show resolved Hide resolved
code: Vec<u8>,
raw_genesis_storage: Vec<u8>,
NingLin-P marked this conversation as resolved.
Show resolved Hide resolved
) -> DispatchResult {
ensure_root(origin)?;

let block_number = frame_system::Pallet::<T>::current_block_number();
let runtime_id =
do_register_runtime::<T>(runtime_name, runtime_type.clone(), code, block_number)
.map_err(Error::<T>::from)?;
let runtime_id = do_register_runtime::<T>(
runtime_name,
runtime_type.clone(),
raw_genesis_storage,
block_number,
)
.map_err(Error::<T>::from)?;

Self::deposit_event(Event::DomainRuntimeCreated {
runtime_id,
Expand Down Expand Up @@ -1143,10 +1147,10 @@ mod pallet {
genesis_domain.runtime_name,
genesis_domain.runtime_type,
genesis_domain.runtime_version,
genesis_domain.code,
genesis_domain.raw_genesis_storage,
One::one(),
)
.expect("Genesis runtime registration must always succeed");
)
.expect("Genesis runtime registration must always succeed");

// Instantiate the genesis domain
let domain_config = DomainConfig {
Expand All @@ -1158,13 +1162,9 @@ mod pallet {
target_bundles_per_block: genesis_domain.target_bundles_per_block,
};
let domain_owner = genesis_domain.owner_account_id;
let domain_id = do_instantiate_domain::<T>(
domain_config,
domain_owner.clone(),
One::one(),
Some(genesis_domain.raw_genesis_config),
)
.expect("Genesis domain instantiation must always succeed");
let domain_id =
do_instantiate_domain::<T>(domain_config, domain_owner.clone(), One::one(), None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not need the raw_genesis_config to instantiate domain anymore. We should also remove the sudo related things from the domain_registry as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is removed in a later commit but I'm not sure what you means by remove the sudo related things from the domain_registry as well

.expect("Genesis domain instantiation must always succeed");

// Register domain_owner as the genesis operator.
let operator_config = OperatorConfig {
Expand Down
58 changes: 52 additions & 6 deletions crates/pallet-domains/src/runtime_registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use frame_support::PalletError;
use frame_system::pallet_prelude::*;
use scale_info::TypeInfo;
use sp_core::Hasher;
use sp_domains::{DomainsDigestItem, RuntimeId, RuntimeType};
use sp_domains::storage::RawGenesis;
use sp_domains::{DomainId, DomainsDigestItem, RuntimeId, RuntimeType};
use sp_runtime::traits::{CheckedAdd, Get};
use sp_runtime::DigestItem;
use sp_std::vec::Vec;
Expand All @@ -23,6 +24,8 @@ pub enum Error {
MissingRuntimeObject,
RuntimeUpgradeAlreadyScheduled,
MaxScheduledBlockNumber,
FailedToDecodeRawGenesis,
RuntimeCodeNotFoundInRawGenesis,
}

#[derive(TypeInfo, Debug, Encode, Decode, Clone, PartialEq, Eq)]
Expand All @@ -32,11 +35,28 @@ pub struct RuntimeObject<Number, Hash> {
pub runtime_upgrades: u32,
pub hash: Hash,
pub code: Vec<u8>,
// The raw gensis storage with runtime code removed.
// NOTE: don't use this field directly but `into_complete_raw_genesis` instead
pub raw_genesis: RawGenesis,
vedhavyas marked this conversation as resolved.
Show resolved Hide resolved
pub version: RuntimeVersion,
pub created_at: Number,
pub updated_at: Number,
}

impl<Number, Hash> RuntimeObject<Number, Hash> {
// Return a complete raw genesis with runtime code and domain id set properly
pub fn into_complete_raw_genesis(self, domain_id: DomainId) -> RawGenesis {
let RuntimeObject {
code,
mut raw_genesis,
..
} = self;
raw_genesis.set_runtime_code(code);
raw_genesis.set_domain_id(domain_id);
raw_genesis
}
}

#[derive(TypeInfo, Debug, Encode, Decode, Clone, PartialEq, Eq)]
pub struct ScheduledRuntimeUpgrade {
pub code: Vec<u8>,
Expand Down Expand Up @@ -73,10 +93,18 @@ pub(crate) fn can_upgrade_code(
pub(crate) fn do_register_runtime<T: Config>(
runtime_name: Vec<u8>,
runtime_type: RuntimeType,
code: Vec<u8>,
raw_genesis_storage: Vec<u8>,
at: BlockNumberFor<T>,
) -> Result<RuntimeId, Error> {
let runtime_version = runtime_version(&code)?;
let mut raw_genesis: RawGenesis = Decode::decode(&mut raw_genesis_storage.as_slice())
.map_err(|_| Error::FailedToDecodeRawGenesis)?;

// Remove the runtime code from `raw_genesis` before storing it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removing runtime code from genesis state? It would make sense to me to leave it in there so you don't need to put it back in later.

Copy link
Member Author

@NingLin-P NingLin-P Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because runtime code is updatable, it is weird to extract the code, update it, and store the updated code back to the genesis state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because runtime code is updatable, it is weird to extract the code, update it, and store the updated code back to the genesis state.

This should never be necessary, runtime code in genesis state doesn't need to be updatable. It needs to be canonical instead and come as a bundle, just like it is before you remove runtime code out of it. For updated runtime genesis state will be different too. State and code should come in pairs when it comes to instantiation.

You don't update genesis state without updating runtime code and you don't update runtime code without updating genesis state.

Copy link
Member Author

@NingLin-P NingLin-P Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I may be misled by your reply #1824 (comment), that is what I thought at first! the genesis state and the runtime code should come in pairs. This should be true for both the first registration and the later runtime upgrade. The new domain instance should use both the upgraded runtime code and genesis state to initialize its genesis block, this is the only place the genesis state is involved. The existing domain instance should just upgrade its runtime code don't care about the updated genesis state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last part of above comment is correct. They come together, but during upgrades we only upgrade code and let it change the state accordingly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still means we need to update state one step at a time when we deploy new domain. Maybe this is acceptable, but feels inefficient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with storing the runtime code along with the genesis state, but what we need to discuss is whether the genesis state should be updated as the domain runtime upgraded in the consensus chain. My opinion is yes, the domain genesis state and runtime code should come in pairs and be updated at the same time.

What you're suggesting seems to be that domain code is updated "in place", which is confusing for other domains that use the old code
I say, we store all the new runtime seperately as they are submitted but leave the first one in place. This also avoids the manually refilling them when we need the full genesis

Suppose the domain is instantiated, it commits to a runtime object with the specified RuntimeId, and uses both the runtime object's runtime code and genesis state to initialize the domain chain.

Later, the runtime object is updated (both the code and genesis state), and the existing domain instance will upgrade its runtime code unconditionally and automatically as the upgrade is derived from the consensus block deterministically. However, the existing domain doesn't care about the updated genesis state because it only needs this when building its genesis block.

Then there is a new domain instance instantiated and committed to the same runtime object with the same RuntimeId, now the new domain instance should use the newest version of both the runtime code and genesis state to initialize its genesis block. This is the reason why I think we should update the runtime object's runtime code and genesis state at the same time.

And I can't see why we need to store all the version of the runtime code in the consensus chain state to support instantiating new domain instances with an older version of the runtime code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion is yes, the domain genesis state and runtime code should come in pairs and be updated at the same time.

That is my expectation and preference as well.

And I can't see why we need to store all the version of the runtime code in the consensus chain state to support instantiating new domain instances with an older version of the runtime code.

Then whatever the latest is will have to support runtime upgrades from whatever the oldest registered domain is (even if it is not running right now) because otherwise domain will not resume. I feel like this is an issue and storing all used versions is very desirable.

Copy link
Member Author

@NingLin-P NingLin-P Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then whatever the latest is will have to support runtime upgrades from whatever the oldest registered domain is (even if it is not running right now) because otherwise domain will not resume

No, as said above the domain runtime upgrade is not controlled by the domain owner but by the runtime object, whenever the runtime object is upgraded the domain instance will upgrade its runtime unconditionally and automatically.

If the domain instance node is not running for a while, then it should catch up to the domain chain tip by deriving the domain block from the consensus block, it can't miss any consensus block since it is instantiated unless we support syncing the domain chain from other nodes, but still all the consensus block should be handled by some (but not all) domain nodes in case any potential bundle/runtime upgrade is missed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is updated with runtime code and genesis state stored together and updated together, PTAL.

let code = raw_genesis
.take_runtime_code()
.ok_or(Error::RuntimeCodeNotFoundInRawGenesis)?;

let version = runtime_version(&code)?;
let runtime_hash = T::Hashing::hash(&code);
let runtime_id = NextRuntimeId::<T>::get();

Expand All @@ -87,7 +115,8 @@ pub(crate) fn do_register_runtime<T: Config>(
runtime_type,
hash: runtime_hash,
code,
version: runtime_version,
raw_genesis,
version,
created_at: at,
updated_at: at,
runtime_upgrades: 0u32,
Expand All @@ -106,9 +135,17 @@ pub(crate) fn register_runtime_at_genesis<T: Config>(
runtime_name: Vec<u8>,
runtime_type: RuntimeType,
runtime_version: RuntimeVersion,
code: Vec<u8>,
raw_genesis_storage: Vec<u8>,
at: BlockNumberFor<T>,
) -> Result<RuntimeId, Error> {
let mut raw_genesis: RawGenesis = Decode::decode(&mut raw_genesis_storage.as_slice())
.map_err(|_| Error::FailedToDecodeRawGenesis)?;

// Remove the runtime code from `raw_genesis` before storing it
let code = raw_genesis
.take_runtime_code()
.ok_or(Error::RuntimeCodeNotFoundInRawGenesis)?;

let runtime_hash = T::Hashing::hash(&code);
let runtime_id = NextRuntimeId::<T>::get();

Expand All @@ -119,6 +156,7 @@ pub(crate) fn register_runtime_at_genesis<T: Config>(
runtime_type,
hash: runtime_hash,
code,
raw_genesis,
version: runtime_version,
created_at: at,
updated_at: at,
Expand Down Expand Up @@ -188,6 +226,7 @@ mod tests {
use frame_support::assert_ok;
use frame_support::dispatch::RawOrigin;
use frame_support::traits::OnInitialize;
use sp_domains::storage::RawGenesis;
use sp_domains::{DomainsDigestItem, RuntimeId, RuntimeType};
use sp_runtime::traits::BlockNumberProvider;
use sp_runtime::{Digest, DispatchError};
Expand All @@ -212,11 +251,16 @@ mod tests {
read_runtime_version,
));
ext.execute_with(|| {
let raw_genesis_storage = {
let mut raw_genesis = RawGenesis::default();
raw_genesis.set_runtime_code(vec![1, 2, 3, 4]);
raw_genesis.encode()
};
let res = crate::Pallet::<Test>::register_domain_runtime(
RawOrigin::Root.into(),
b"evm".to_vec(),
RuntimeType::Evm,
vec![1, 2, 3, 4],
raw_genesis_storage,
);

assert_ok!(res);
Expand All @@ -238,6 +282,7 @@ mod tests {
runtime_upgrades: 0,
hash: Default::default(),
code: vec![1, 2, 3, 4],
raw_genesis: Default::default(),
version: RuntimeVersion {
spec_name: "test".into(),
spec_version: 1,
Expand Down Expand Up @@ -381,6 +426,7 @@ mod tests {
runtime_upgrades: 0,
hash: Default::default(),
code: vec![1, 2, 3, 4],
raw_genesis: Default::default(),
version: version.clone(),
created_at: Default::default(),
updated_at: Default::default(),
Expand Down
8 changes: 7 additions & 1 deletion crates/pallet-domains/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use sp_core::crypto::Pair;
use sp_core::{Get, H256, U256};
use sp_domains::fraud_proof::FraudProof;
use sp_domains::merkle_tree::MerkleTree;
use sp_domains::storage::RawGenesis;
use sp_domains::{
BundleHeader, DomainId, DomainInstanceData, DomainsHoldIdentifier, ExecutionReceipt,
GenerateGenesisStateRoot, GenesisReceiptExtension, OpaqueBundle, OperatorId, OperatorPair,
Expand Down Expand Up @@ -347,11 +348,16 @@ pub(crate) fn run_to_block<T: Config>(block_number: BlockNumberFor<T>, parent_ha
}

pub(crate) fn register_genesis_domain(creator: u64, operator_ids: Vec<OperatorId>) -> DomainId {
let raw_genesis_storage = {
let mut raw_genesis = RawGenesis::default();
raw_genesis.set_runtime_code(vec![1, 2, 3, 4]);
raw_genesis.encode()
};
assert_ok!(crate::Pallet::<Test>::register_domain_runtime(
RawOrigin::Root.into(),
b"evm".to_vec(),
RuntimeType::Evm,
vec![1, 2, 3, 4],
raw_genesis_storage,
));

let domain_id = NextDomainId::<Test>::get();
Expand Down
4 changes: 4 additions & 0 deletions crates/sp-domains/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ targets = ["x86_64-unknown-linux-gnu"]

[dependencies]
blake2 = { version = "0.10.6", default-features = false }
frame-support = { version = "4.0.0-dev", default-features = false, git = "https://github.com/subspace/substrate", rev = "48c5ebae44b90d45863195b91e0e489eca670898" }
hash-db = { version = "0.16.0", default-features = false }
hexlit = "0.5.5"
parity-scale-codec = { version = "3.6.5", default-features = false, features = ["derive"] }
rs_merkle = { version = "1.4.1", default-features = false }
Expand Down Expand Up @@ -42,6 +44,8 @@ num-traits = "0.2.16"
default = ["std"]
std = [
"blake2/std",
"frame-support/std",
"hash-db/std",
"parity-scale-codec/std",
"rs_merkle/std",
"scale-info/std",
Expand Down
22 changes: 20 additions & 2 deletions crates/sp-domains/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@
pub mod bundle_producer_election;
pub mod fraud_proof;
pub mod merkle_tree;
pub mod storage;
#[cfg(test)]
mod tests;
pub mod transaction;

use crate::storage::{RawGenesis, StorageKey};
use bundle_producer_election::{BundleProducerElectionParams, VrfProofError};
use hexlit::hex;
use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
Expand Down Expand Up @@ -506,7 +508,7 @@ pub struct GenesisDomain<AccountId> {
pub runtime_name: Vec<u8>,
pub runtime_type: RuntimeType,
pub runtime_version: RuntimeVersion,
pub code: Vec<u8>,
pub raw_genesis_storage: Vec<u8>,

// Domain config items
pub owner_account_id: AccountId,
Expand All @@ -515,7 +517,6 @@ pub struct GenesisDomain<AccountId> {
pub max_block_weight: Weight,
pub bundle_slot_probability: (u64, u64),
pub target_bundles_per_block: u32,
pub raw_genesis_config: Vec<u8>,

// Genesis operator
pub signing_key: OperatorPublicKey,
Expand Down Expand Up @@ -607,6 +608,23 @@ impl DomainsDigestItem for DigestItem {
}
}

/// The storage key of the `SelfDomainId` storage item in the `pallet-domain-id`
///
/// Any change to the storage item name or the `pallet-domain-id` name used in the `construct_runtime`
/// macro must be reflected here.
pub fn self_domain_id_storage_key() -> StorageKey {
StorageKey(
frame_support::storage::storage_prefix(
// This is the name used for the `pallet-domain-id` in the `construct_runtime` macro
// i.e. `SelfDomainId: pallet_domain_id = 90`
"SelfDomainId".as_bytes(),
// This is the storage item name used inside the `pallet-domain-id`
"SelfDomainId".as_bytes(),
)
.to_vec(),
)
}

/// `DomainInstanceData` is used to construct `RuntimeGenesisConfig` which will be further used
/// to construct the genesis block
#[derive(PartialEq, Eq, Clone, Encode, Decode, TypeInfo)]
Expand Down
Loading