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.

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

schemas: PoV compatible changes #1743

Merged
merged 19 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
5 changes: 5 additions & 0 deletions designdocs/schema_v2.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,8 @@ pub struct SchemaInfo {
pub settings: SchemaSettings,
}
```
### Expected PoV improvements
This PoV improvement would not extrinsic weights in this pallet, but it would directly affect any
aramikm marked this conversation as resolved.
Show resolved Hide resolved
pallet that is dependent on **Schemas** pallet. Some of these pallets are **Messages** and
**Stateful-Storage**. After these changes we are expecting see to see around 30-60KiB decrease in PoV
for `MaxEncodedLen` mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

2 changes: 1 addition & 1 deletion e2e/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions node/cli/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,9 @@ pub fn run() -> Result<()> {
Some(Subcommand::TryRuntime(cmd)) => {
aramikm marked this conversation as resolved.
Show resolved Hide resolved
use common_runtime::constants::MILLISECS_PER_BLOCK;
use try_runtime_cli::block_building_info::timestamp_with_aura_info;

let runner = cli.create_runner(cmd)?;

type HostFunctions =
(sp_io::SubstrateHostFunctions, frame_benchmarking::benchmarking::HostFunctions);

Expand Down
2 changes: 1 addition & 1 deletion pallets/schemas/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@
<CurrentSchemaIdentifierMaximum<T>>::set(n);
}

/// Build the [`SchemaInfo`] and insert it into storage
/// Inserts both the [`SchemaInfo`] and Schema Payload into storage
/// Updates the `CurrentSchemaIdentifierMaximum` storage
pub fn add_schema(
model: BoundedVec<u8, T::SchemaModelMaxBytesBoundedVecLimit>,
Expand Down Expand Up @@ -434,8 +434,8 @@
Some(response)
},
(None, Some(_)) | (Some(_), None) => {
log::error!("Corrupted state for schema {:?}, Should never happen!", schema_id);
None

Check warning on line 438 in pallets/schemas/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

pallets/schemas/src/lib.rs#L437-L438

Added lines #L437 - L438 were not covered by tests
},
(None, None) => None,
}
Expand All @@ -443,13 +443,13 @@

/// Retrieve a schema info by id
pub fn get_schema_info_by_id(schema_id: SchemaId) -> Option<SchemaInfoResponse> {
if let Some(schema_info) = Self::get_schema_info(schema_id) {
let saved_settings = schema_info.settings;

Check warning on line 447 in pallets/schemas/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

pallets/schemas/src/lib.rs#L446-L447

Added lines #L446 - L447 were not covered by tests
let settings = saved_settings.0.iter().collect::<Vec<SchemaSetting>>();
let response = SchemaInfoResponse {

Check warning on line 449 in pallets/schemas/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

pallets/schemas/src/lib.rs#L449

Added line #L449 was not covered by tests
schema_id,
model_type: schema_info.model_type,
payload_location: schema_info.payload_location,

Check warning on line 452 in pallets/schemas/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

pallets/schemas/src/lib.rs#L451-L452

Added lines #L451 - L452 were not covered by tests
settings,
};
return Some(response)
Expand Down Expand Up @@ -568,7 +568,7 @@
Self::get_schema_by_id(schema_id)
}

fn get_schema_info_by_id(schema_id: SchemaId) -> Option<SchemaInfoResponse> {
Self::get_schema_info_by_id(schema_id)
}

Check warning on line 573 in pallets/schemas/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

pallets/schemas/src/lib.rs#L571-L573

Added lines #L571 - L573 were not covered by tests
}
2 changes: 2 additions & 0 deletions pallets/schemas/src/migration/v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@
pub struct MigrateToV2<T>(PhantomData<T>);

impl<T: Config> OnRuntimeUpgrade for MigrateToV2<T> {
fn on_runtime_upgrade() -> Weight {
migrate_to_v2::<T>()
}

Check warning on line 57 in pallets/schemas/src/migration/v2.rs

View check run for this annotation

Codecov / codecov/patch

pallets/schemas/src/migration/v2.rs#L55-L57

Added lines #L55 - L57 were not covered by tests

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
Expand Down Expand Up @@ -92,13 +92,15 @@
let onchain_version = Pallet::<T>::on_chain_storage_version();
let current_version = Pallet::<T>::current_storage_version();
log::info!(target: LOG_TARGET, "onchain_version= {:?}, current_version={:?}", onchain_version, current_version);
let each_layer_access: u64 = 33 * 16;

if onchain_version < 2 {
let mut reads = 1u64;
let mut writes = 0u64;
let mut bytes = 0u64;
for (schema_id, schema) in old::Schemas::<T>::drain() {
bytes = bytes.saturating_add(schema.encode().len() as u64);
bytes = bytes.saturating_add(each_layer_access * 3); // three layers in merkle tree

let info = SchemaInfo {
model_type: schema.model_type,
Expand All @@ -125,12 +127,12 @@
log::info!(target: LOG_TARGET, "Migration Calculated weights={:?}",weights);
weights
} else {
log::info!(

Check warning on line 130 in pallets/schemas/src/migration/v2.rs

View check run for this annotation

Codecov / codecov/patch

pallets/schemas/src/migration/v2.rs#L130

Added line #L130 was not covered by tests
target: LOG_TARGET,
"Migration did not execute. This probably should be removed onchain:{:?}, current:{:?}",

Check warning on line 132 in pallets/schemas/src/migration/v2.rs

View check run for this annotation

Codecov / codecov/patch

pallets/schemas/src/migration/v2.rs#L132

Added line #L132 was not covered by tests
onchain_version,
current_version
);
T::DbWeight::get().reads(1)

Check warning on line 136 in pallets/schemas/src/migration/v2.rs

View check run for this annotation

Codecov / codecov/patch

pallets/schemas/src/migration/v2.rs#L136

Added line #L136 was not covered by tests
}
}
21 changes: 18 additions & 3 deletions pallets/schemas/src/tests/other_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,8 @@ fn schemas_migration_to_v2_should_work_as_expected() {
);
}
let old_schema_1 = v2::old::Schemas::<Test>::get(1u16).expect("should have value");
aramikm marked this conversation as resolved.
Show resolved Hide resolved
let old_schema_2 = v2::old::Schemas::<Test>::get(2u16).expect("should have value");
let old_schema_3 = v2::old::Schemas::<Test>::get(3u16).expect("should have value");

// Act
let _ = v2::migrate_to_v2::<Test>();
Expand All @@ -745,17 +747,30 @@ fn schemas_migration_to_v2_should_work_as_expected() {
let new_payload_count = SchemaPayloads::<Test>::iter().count();
let current_version = SchemasPallet::current_storage_version();

let schema_info_1 = SchemaInfos::<Test>::get(1).expect("should have value");
let schema_payload_1 = SchemaPayloads::<Test>::get(1u16).expect("should have value");

assert_eq!(old_count, 0);
assert_eq!(new_info_count, schemas.len());
assert_eq!(new_payload_count, schemas.len());
assert_eq!(current_version, StorageVersion::new(2));

let schema_info_1 = SchemaInfos::<Test>::get(1).expect("should have value");
let schema_payload_1 = SchemaPayloads::<Test>::get(1u16).expect("should have value");
assert_eq!(schema_info_1.model_type, old_schema_1.model_type);
assert_eq!(schema_info_1.payload_location, old_schema_1.payload_location);
assert_eq!(schema_info_1.settings, old_schema_1.settings);
assert_eq!(schema_payload_1.into_inner(), old_schema_1.model.into_inner());

let schema_info_2 = SchemaInfos::<Test>::get(2).expect("should have value");
let schema_payload_2 = SchemaPayloads::<Test>::get(2u16).expect("should have value");
assert_eq!(schema_info_2.model_type, old_schema_2.model_type);
assert_eq!(schema_info_2.payload_location, old_schema_2.payload_location);
assert_eq!(schema_info_2.settings, old_schema_2.settings);
assert_eq!(schema_payload_2.into_inner(), old_schema_2.model.into_inner());

let schema_info_3 = SchemaInfos::<Test>::get(3).expect("should have value");
let schema_payload_3 = SchemaPayloads::<Test>::get(3u16).expect("should have value");
assert_eq!(schema_info_3.model_type, old_schema_3.model_type);
assert_eq!(schema_info_3.payload_location, old_schema_3.payload_location);
assert_eq!(schema_info_3.settings, old_schema_3.settings);
assert_eq!(schema_payload_3.into_inner(), old_schema_3.model.into_inner());
});
}
9 changes: 6 additions & 3 deletions runtime/frequency/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,10 @@ pub type Executive = frame_executive::Executive<
frame_system::ChainContext<Runtime>,
Runtime,
AllPalletsWithSystem,
(pallet_schemas::migration::v2::MigrateToV2<Runtime>,),
(
pallet_messages::migration::v2::MigrateToV2<Runtime>,
pallet_schemas::migration::v2::MigrateToV2<Runtime>,
),
>;

/// Opaque types. These are used by the CLI to instantiate machinery that don't need to know
Expand Down Expand Up @@ -258,7 +261,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("frequency"),
impl_name: create_runtime_str!("frequency"),
authoring_version: 1,
spec_version: 61,
spec_version: 62,
impl_version: 0,
apis: RUNTIME_API_VERSIONS,
transaction_version: 1,
Expand All @@ -272,7 +275,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("frequency-rococo"),
impl_name: create_runtime_str!("frequency"),
authoring_version: 1,
spec_version: 61,
spec_version: 62,
impl_version: 0,
apis: RUNTIME_API_VERSIONS,
transaction_version: 1,
Expand Down
Loading