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

PoV Reclaim Runtime Side #3002

Merged
merged 32 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
9bacb18
Add SignedExtension, Manual reclaim
skunert Jan 19, 2024
fca700f
Remove unnecessary bound
skunert Jan 19, 2024
fd3b613
Merge branch 'master' into reclaim-again
skunert Jan 29, 2024
f808645
Fix taplo + clippy
skunert Jan 29, 2024
088dd8d
More CI fixes
skunert Jan 30, 2024
ce33ece
Provide `reclaim_with_meter`
skunert Jan 30, 2024
0468623
Merge branch 'master' into reclaim-again
skunert Jan 30, 2024
72b8818
Reclaimer should not reclaim directly, now only subtracts from meter
skunert Feb 2, 2024
ec26a45
Update cumulus/parachains/common/src/storage_weight_reclaim.rs
skunert Feb 2, 2024
fb86674
Apply suggestions from code review
skunert Feb 15, 2024
00dbfb6
Improve naming
skunert Feb 15, 2024
102c17e
Merge branch 'master' into reclaim-again
skunert Feb 15, 2024
903a026
Add reclaim extension to template runtime
skunert Feb 15, 2024
d9550e3
Prdoc
skunert Feb 15, 2024
a54f670
Reset recorder in PVF
skunert Feb 16, 2024
93c5570
Apply suggestions from code review
skunert Feb 19, 2024
a257a52
Review comments
skunert Feb 19, 2024
9433f16
Use docify
skunert Feb 19, 2024
2f8244e
Merge branch 'master' into reclaim-again
skunert Feb 19, 2024
0f44605
deps
skunert Feb 19, 2024
b308598
Revert "deps"
skunert Feb 20, 2024
872f170
Add small test for reset
skunert Feb 20, 2024
5386108
Comments
skunert Feb 20, 2024
6af533b
Incorporate unspent value and add more tests
skunert Feb 21, 2024
fb523e5
Merge branch 'master' into reclaim-again
skunert Feb 21, 2024
03074da
Format features & small fixes
skunert Feb 21, 2024
f6bb796
Upgrade try-runtime & fix test-service
skunert Feb 22, 2024
eb79558
Test-service once more
skunert Feb 22, 2024
b8012be
Fix PrDoc and feature flags
skunert Feb 22, 2024
2a5cfba
Merge branch 'master' into reclaim-again
skunert Feb 23, 2024
fdfa266
Enable import proof recording in cumulus test client
skunert Feb 23, 2024
408f7c7
Use cumulus-test-runtime for tests
skunert Feb 23, 2024
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
4 changes: 3 additions & 1 deletion .gitlab/pipeline/check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,10 @@ check-toml-format:
export RUST_LOG=remote-ext=debug,runtime=debug

echo "---------- Downloading try-runtime CLI ----------"
curl -sL https://github.com/paritytech/try-runtime-cli/releases/download/v0.5.0/try-runtime-x86_64-unknown-linux-musl -o try-runtime
curl -sL https://github.com/paritytech/try-runtime-cli/releases/download/v0.5.4/try-runtime-x86_64-unknown-linux-musl -o try-runtime
chmod +x ./try-runtime
echo "Using try-runtime-cli version:"
./try-runtime --version

echo "---------- Building ${PACKAGE} runtime ----------"
time cargo build --release --locked -p "$PACKAGE" --features try-runtime
Expand Down
23 changes: 23 additions & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ members = [
"cumulus/primitives/core",
"cumulus/primitives/parachain-inherent",
"cumulus/primitives/proof-size-hostfunction",
"cumulus/primitives/storage-weight-reclaim",
"cumulus/primitives/timestamp",
"cumulus/primitives/utility",
"cumulus/test/client",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

//! The actual implementation of the validate block functionality.

use super::{trie_cache, MemoryOptimizedValidationParams};
use super::{trie_cache, trie_recorder, MemoryOptimizedValidationParams};
use cumulus_primitives_core::{
relay_chain::Hash as RHash, ParachainBlockData, PersistedValidationData,
};
Expand All @@ -34,12 +34,14 @@ use sp_externalities::{set_and_run_with_externalities, Externalities};
use sp_io::KillStorageResult;
use sp_runtime::traits::{Block as BlockT, Extrinsic, HashingFor, Header as HeaderT};
use sp_std::prelude::*;
use sp_trie::MemoryDB;
use sp_trie::{MemoryDB, ProofSizeProvider};
use trie_recorder::SizeOnlyRecorderProvider;

type TrieBackend<B> = sp_state_machine::TrieBackend<
MemoryDB<HashingFor<B>>,
HashingFor<B>,
trie_cache::CacheProvider<HashingFor<B>>,
SizeOnlyRecorderProvider<HashingFor<B>>,
>;

type Ext<'a, B> = sp_state_machine::Ext<'a, HashingFor<B>, TrieBackend<B>>;
Expand All @@ -48,6 +50,9 @@ fn with_externalities<F: FnOnce(&mut dyn Externalities) -> R, R>(f: F) -> R {
sp_externalities::with_externalities(f).expect("Environmental externalities not set.")
}

// Recorder instance to be used during this validate_block call.
environmental::environmental!(recorder: trait ProofSizeProvider);

/// Validate the given parachain block.
///
/// This function is doing roughly the following:
Expand Down Expand Up @@ -120,6 +125,7 @@ where

sp_std::mem::drop(storage_proof);

let mut recorder = SizeOnlyRecorderProvider::new();
let cache_provider = trie_cache::CacheProvider::new();
// We use the storage root of the `parent_head` to ensure that it is the correct root.
// This is already being done above while creating the in-memory db, but let's be paranoid!!
Expand All @@ -128,6 +134,7 @@ where
*parent_header.state_root(),
cache_provider,
)
.with_recorder(recorder.clone())
.build();

let _guard = (
Expand Down Expand Up @@ -167,9 +174,11 @@ where
.replace_implementation(host_default_child_storage_next_key),
sp_io::offchain_index::host_set.replace_implementation(host_offchain_index_set),
sp_io::offchain_index::host_clear.replace_implementation(host_offchain_index_clear),
cumulus_primitives_proof_size_hostfunction::storage_proof_size::host_storage_proof_size
.replace_implementation(host_storage_proof_size),
);

run_with_externalities::<B, _, _>(&backend, || {
run_with_externalities_and_recorder::<B, _, _>(&backend, &mut recorder, || {
Copy link
Member

Choose a reason for hiding this comment

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

Dumb question: do we need access to the recorder in this closure as well?
Shouldn't we need access to the recorder only in the scope where we call execute_block only?
(i.e. in the call below)
Or we need it to check_inherents as well?

Another dumb question: why we need two calls to run_with_externalities_and_recorder?
Can't we unify the closures in a single one?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we should not need it.

Copy link
Member

Choose a reason for hiding this comment

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

Let me phrase it better, we should NOT use the recorder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean we need to supply the recorder everywhere where host functions are potentially being called. And since the CheckInherent type is user supplied, host functions could be called there.

The two closures are separate because we want to start with a fresh overlay and externalities. But this is a good point, I now reset the recorder too between these two code sections.

let relay_chain_proof = crate::RelayChainStateProof::new(
PSC::SelfParaId::get(),
inherent_data.validation_data.relay_parent_storage_root,
Expand All @@ -190,7 +199,7 @@ where
}
});

run_with_externalities::<B, _, _>(&backend, || {
run_with_externalities_and_recorder::<B, _, _>(&backend, &mut recorder, || {
let head_data = HeadData(block.header().encode());

E::execute_block(block);
Expand Down Expand Up @@ -265,15 +274,17 @@ fn validate_validation_data(
);
}

/// Run the given closure with the externalities set.
fn run_with_externalities<B: BlockT, R, F: FnOnce() -> R>(
/// Run the given closure with the externalities and recorder set.
fn run_with_externalities_and_recorder<B: BlockT, R, F: FnOnce() -> R>(
backend: &TrieBackend<B>,
recorder: &mut SizeOnlyRecorderProvider<HashingFor<B>>,
execute: F,
) -> R {
let mut overlay = sp_state_machine::OverlayedChanges::default();
let mut ext = Ext::<B>::new(&mut overlay, backend);
recorder.reset();
Copy link
Member

Choose a reason for hiding this comment

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

Actually realizing that my comment last time was not correct 🙈

Given that we reset the recorder here, it is fine to also use this function for check_inherents. So can you do this? :D It should not be used there, but better than having it fail the validation.


set_and_run_with_externalities(&mut ext, || execute())
recorder::using(recorder, || set_and_run_with_externalities(&mut ext, || execute()))
}

fn host_storage_read(key: &[u8], value_out: &mut [u8], value_offset: u32) -> Option<u32> {
Expand Down Expand Up @@ -305,6 +316,10 @@ fn host_storage_clear(key: &[u8]) {
with_externalities(|ext| ext.place_storage(key.to_vec(), None))
}

fn host_storage_proof_size() -> u64 {
recorder::with(|rec| rec.estimate_encoded_size()).expect("Recorder is always set; qed") as _
}

fn host_storage_root(version: StateVersion) -> Vec<u8> {
with_externalities(|ext| ext.storage_root(version))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ fn call_validate_block(
}

fn create_test_client() -> (Client, Header) {
let client = TestClientBuilder::new().build();
let client = TestClientBuilder::new().enable_import_proof_recording().build();

let genesis_header = client
.header(client.chain_info().genesis_hash)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,21 @@ pub(crate) struct SizeOnlyRecorderProvider<H: Hasher> {
}

impl<H: Hasher> SizeOnlyRecorderProvider<H> {
/// Create a new instance of [`SizeOnlyRecorderProvider`]
pub fn new() -> Self {
Self {
seen_nodes: Default::default(),
encoded_size: Default::default(),
recorded_keys: Default::default(),
}
}

/// Reset the internal state.
pub fn reset(&self) {
self.seen_nodes.borrow_mut().clear();
*self.encoded_size.borrow_mut() = 0;
self.recorded_keys.borrow_mut().clear();
}
}

impl<H: trie_db::Hasher> sp_trie::TrieRecorderProvider<H> for SizeOnlyRecorderProvider<H> {
Expand Down Expand Up @@ -281,6 +289,9 @@ mod tests {
reference_recorder.estimate_encoded_size(),
recorder_for_test.estimate_encoded_size()
);

recorder_for_test.reset();
assert_eq!(recorder_for_test.estimate_encoded_size(), 0)
}
}
}
3 changes: 2 additions & 1 deletion cumulus/parachain-template/node/src/command.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::net::SocketAddr;

use cumulus_client_service::storage_proof_size::HostFunctions as ReclaimHostFunctions;
use cumulus_primitives_core::ParaId;
use frame_benchmarking_cli::{BenchmarkCmd, SUBSTRATE_REFERENCE_HARDWARE};
use log::info;
Expand Down Expand Up @@ -183,7 +184,7 @@ pub fn run() -> Result<()> {
match cmd {
BenchmarkCmd::Pallet(cmd) =>
if cfg!(feature = "runtime-benchmarks") {
runner.sync_run(|config| cmd.run::<sp_runtime::traits::HashingFor<Block>, ()>(config))
runner.sync_run(|config| cmd.run::<sp_runtime::traits::HashingFor<Block>, ReclaimHostFunctions>(config))
} else {
Err("Benchmarking wasn't enabled when building the node. \
You can enable it with `--features runtime-benchmarks`."
Expand Down
8 changes: 6 additions & 2 deletions cumulus/parachain-template/node/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ use substrate_prometheus_endpoint::Registry;
pub struct ParachainNativeExecutor;

impl sc_executor::NativeExecutionDispatch for ParachainNativeExecutor {
type ExtendHostFunctions = frame_benchmarking::benchmarking::HostFunctions;
type ExtendHostFunctions = (
cumulus_client_service::storage_proof_size::HostFunctions,
Copy link
Member

@davxy davxy Feb 3, 2024

Choose a reason for hiding this comment

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

If we propose this host function in the template. Maybe we should also need to add the StorageWeigthReclaim signed extension to the runtime signed extensions tuple? (i.e. here)

Otherwise what is its usage?

IIUC the StorageWeightReclaimer alone can't work correctly if we don't also return the extra weight to frame_system::BlockWeight ( by means of this signed extension hooks right?
Or I'm missing some other means of doing this? E.g. the user directly altering frame_system::BlockWeight in some other hook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC the StorageWeightReclaimer alone can't work correctly if we don't also return the extra weight to frame_system::BlockWeight ( by means of this signed extension hooks right?
Or I'm missing some other means of doing this? E.g. the user directly altering frame_system::BlockWeight in some other hook.

The StorageWeightReclaimer is a separate mechanism from the SignedExtension. It is designed to be used inside the on_idle hook, where weight management happens manually via WeightMeter. Users need to check whether the operation they plan fits into the remaining weight. In the end, on_idle will return the consumed weight and it will be accounted for after the hook finished here:

let used_weight = <AllPalletsWithSystem as OnIdle<BlockNumberFor<System>>>::on_idle(
block_number,
remaining_weight,
);
<frame_system::Pallet<System>>::register_extra_weight_unchecked(
used_weight,
DispatchClass::Mandatory,
);

This means that the weightreclaimer should only modify the value in the weightmeter, not the storage value itself.

If we propose this host function in the template. Maybe we should also need to add the StorageWeigthReclaim signed extension to the runtime signed extensions tuple? (i.e. here)

Otherwise what is its usage?

This is a good point, I will add it.

frame_benchmarking::benchmarking::HostFunctions,
);

fn dispatch(method: &str, data: &[u8]) -> Option<Vec<u8>> {
parachain_template_runtime::api::dispatch(method, data)
Expand Down Expand Up @@ -100,10 +103,11 @@ pub fn new_partial(config: &Configuration) -> Result<Service, sc_service::Error>
let executor = ParachainExecutor::new_with_wasm_executor(wasm);

let (client, backend, keystore_container, task_manager) =
sc_service::new_full_parts::<Block, RuntimeApi, _>(
sc_service::new_full_parts_record_import::<Block, RuntimeApi, _>(
config,
telemetry.as_ref().map(|(_, telemetry)| telemetry.handle()),
executor,
true,
)?;
let client = Arc::new(client);

Expand Down
2 changes: 2 additions & 0 deletions cumulus/parachain-template/runtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ cumulus-pallet-xcm = { path = "../../pallets/xcm", default-features = false }
cumulus-pallet-xcmp-queue = { path = "../../pallets/xcmp-queue", default-features = false }
cumulus-primitives-core = { path = "../../primitives/core", default-features = false }
cumulus-primitives-utility = { path = "../../primitives/utility", default-features = false }
cumulus-primitives-storage-weight-reclaim = { path = "../../primitives/storage-weight-reclaim", default-features = false }
pallet-collator-selection = { path = "../../pallets/collator-selection", default-features = false }
parachains-common = { path = "../../parachains/common", default-features = false }
parachain-info = { package = "staging-parachain-info", path = "../../parachains/pallets/parachain-info", default-features = false }
Expand All @@ -87,6 +88,7 @@ std = [
"cumulus-pallet-xcm/std",
"cumulus-pallet-xcmp-queue/std",
"cumulus-primitives-core/std",
"cumulus-primitives-storage-weight-reclaim/std",
"cumulus-primitives-utility/std",
"frame-benchmarking?/std",
"frame-executive/std",
Expand Down
1 change: 1 addition & 0 deletions cumulus/parachain-template/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ pub type SignedExtra = (
frame_system::CheckNonce<Runtime>,
frame_system::CheckWeight<Runtime>,
pallet_transaction_payment::ChargeTransactionPayment<Runtime>,
cumulus_primitives_storage_weight_reclaim::StorageWeightReclaim<Runtime>,
);

/// Unchecked extrinsic type as expected by this runtime.
Expand Down
3 changes: 2 additions & 1 deletion cumulus/polkadot-parachain/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use crate::{
},
service::{new_partial, Block},
};
use cumulus_client_service::storage_proof_size::HostFunctions as ReclaimHostFunctions;
use cumulus_primitives_core::ParaId;
use frame_benchmarking_cli::{BenchmarkCmd, SUBSTRATE_REFERENCE_HARDWARE};
use log::info;
Expand Down Expand Up @@ -584,7 +585,7 @@ pub fn run() -> Result<()> {
match cmd {
BenchmarkCmd::Pallet(cmd) =>
if cfg!(feature = "runtime-benchmarks") {
runner.sync_run(|config| cmd.run::<sp_runtime::traits::HashingFor<Block>, ()>(config))
runner.sync_run(|config| cmd.run::<sp_runtime::traits::HashingFor<Block>, ReclaimHostFunctions>(config))
} else {
Err("Benchmarking wasn't enabled when building the node. \
You can enable it with `--features runtime-benchmarks`."
Expand Down
13 changes: 9 additions & 4 deletions cumulus/polkadot-parachain/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,15 @@ use substrate_prometheus_endpoint::Registry;
use polkadot_primitives::CollatorPair;

#[cfg(not(feature = "runtime-benchmarks"))]
type HostFunctions = sp_io::SubstrateHostFunctions;
type HostFunctions =
(sp_io::SubstrateHostFunctions, cumulus_client_service::storage_proof_size::HostFunctions);

#[cfg(feature = "runtime-benchmarks")]
type HostFunctions =
(sp_io::SubstrateHostFunctions, frame_benchmarking::benchmarking::HostFunctions);
type HostFunctions = (
sp_io::SubstrateHostFunctions,
cumulus_client_service::storage_proof_size::HostFunctions,
frame_benchmarking::benchmarking::HostFunctions,
);

type ParachainClient<RuntimeApi> = TFullClient<Block, RuntimeApi, WasmExecutor<HostFunctions>>;

Expand Down Expand Up @@ -274,10 +278,11 @@ where
.build();

let (client, backend, keystore_container, task_manager) =
sc_service::new_full_parts::<Block, RuntimeApi, _>(
sc_service::new_full_parts_record_import::<Block, RuntimeApi, _>(
config,
telemetry.as_ref().map(|(_, telemetry)| telemetry.handle()),
executor,
true,
)?;
let client = Arc::new(client);

Expand Down
4 changes: 3 additions & 1 deletion cumulus/primitives/proof-size-hostfunction/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#![cfg_attr(not(feature = "std"), no_std)]

#[cfg(feature = "std")]
use sp_externalities::ExternalitiesExt;

use sp_runtime_interface::runtime_interface;
Expand All @@ -35,7 +36,8 @@ pub const PROOF_RECORDING_DISABLED: u64 = u64::MAX;
pub trait StorageProofSize {
/// Returns the current storage proof size.
fn storage_proof_size(&mut self) -> u64 {
self.extension::<ProofSizeExt>().map_or(u64::MAX, |e| e.storage_proof_size())
self.extension::<ProofSizeExt>()
.map_or(PROOF_RECORDING_DISABLED, |e| e.storage_proof_size())
}
}

Expand Down
Loading
Loading