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

feat: tx-pause and safe-mode pallets integration #1388

Merged
merged 15 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 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 @@ -114,6 +114,7 @@ sp-arithmetic = { git = "https://github.com/paritytech/polkadot-sdk", branch = "
sp-staking = { git = "https://github.com/paritytech/polkadot-sdk", branch = "stable2407", default-features = false }
sp-externalities = { git = "https://github.com/paritytech/polkadot-sdk", branch = "stable2407", default-features = false }
sp-genesis-builder = { git = "https://github.com/paritytech/polkadot-sdk", branch = "stable2407", default-features = false }
sp-weights = { git = "https://github.com/paritytech/polkadot-sdk", branch = "stable2407", default-features = false }

# (native)
sp-blockchain = { git = "https://github.com/paritytech/polkadot-sdk", branch = "stable2407" }
Expand Down
9 changes: 9 additions & 0 deletions bin/collator/src/local/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,20 @@ use astar_primitives::*;
const GRANDPA_JUSTIFICATION_PERIOD: u32 = 512;

/// Parachain host functions
#[cfg(not(feature = "runtime-benchmarks"))]
pub type HostFunctions = (
cumulus_client_service::ParachainHostFunctions,
moonbeam_primitives_ext::moonbeam_ext::HostFunctions,
);

/// Host functions required for kitchensink runtime and Substrate node.
#[cfg(feature = "runtime-benchmarks")]
pub type HostFunctions = (
frame_benchmarking::benchmarking::HostFunctions,
cumulus_client_service::ParachainHostFunctions,
moonbeam_primitives_ext::moonbeam_ext::HostFunctions,
);

type ParachainExecutor = WasmExecutor<HostFunctions>;

type FullClient = sc_service::TFullClient<Block, RuntimeApi, ParachainExecutor>;
Expand Down
9 changes: 9 additions & 0 deletions bin/collator/src/parachain/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,20 @@ use crate::{
};

/// Parachain host functions
#[cfg(not(feature = "runtime-benchmarks"))]
pub type HostFunctions = (
cumulus_client_service::ParachainHostFunctions,
moonbeam_primitives_ext::moonbeam_ext::HostFunctions,
);

/// Host functions required for kitchensink runtime and Substrate node.
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this. The benchmark host functions is injected by benchmarking-cli itself

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 am getting this error if I remove benchmarking host functions:

Finished release [optimized] target(s) in 3m 14s
     Running `target/release/astar-collator benchmark overhead --dev --header=./.github/license-check/headers/HEADER-GNUv3`
Error: Service(Client(VersionInvalid("Other error happened while constructing the runtime: runtime requires function imports which are not present on the host: 'env:ext_benchmarking_proof_size_version_1', 'env:ext_benchmarking_add_to_whitelist_version_1', 'env:ext_benchmarking_current_time_version_1', 'env:ext_benchmarking_commit_db_version_1', 'env:ext_benchmarking_read_write_count_version_1', 'env:ext_benchmarking_wipe_db_version_1', 'env:ext_benchmarking_get_read_and_written_keys_version_1', 'env:ext_benchmarking_reset_read_write_count_version_1', 'env:ext_benchmarking_set_whitelist_version_1'")))
2024-12-02 13:52:50 Cannot create a runtime error=Other("runtime requires function imports which are not present on the host: 'env:ext_benchmarking_proof_size_version_1', 'env:ext_benchmarking_add_to_whitelist_version_1', 'env:ext_benchmarking_current_time_version_1', 'env:ext_benchmarking_commit_db_version_1', 'env:ext_benchmarking_read_write_count_version_1', 'env:ext_benchmarking_wipe_db_version_1', 'env:ext_benchmarking_get_read_and_written_keys_version_1', 'env:ext_benchmarking_reset_read_write_count_version_1', 'env:ext_benchmarking_set_whitelist_version_1'")

PS: I will fix the comment (copied)

Copy link
Contributor

@ermalkaleci ermalkaleci Dec 2, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like overhead doesn't include it, wonder why

Copy link
Member

Choose a reason for hiding this comment

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

We can make an issue or PR for it, probably it's small effort.

#[cfg(feature = "runtime-benchmarks")]
pub type HostFunctions = (
frame_benchmarking::benchmarking::HostFunctions,
cumulus_client_service::ParachainHostFunctions,
moonbeam_primitives_ext::moonbeam_ext::HostFunctions,
);

/// Parachain executor
pub type ParachainExecutor = WasmExecutor<HostFunctions>;

Expand Down
2 changes: 2 additions & 0 deletions runtime/shibuya/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ sp-session = { workspace = true }
sp-std = { workspace = true }
sp-transaction-pool = { workspace = true }
sp-version = { workspace = true }
sp-weights = { workspace = true }

# frame dependencies
frame-executive = { workspace = true }
Expand Down Expand Up @@ -264,6 +265,7 @@ std = [
"sp-std/std",
"sp-transaction-pool/std",
"sp-version/std",
"sp-weights/std",
"substrate-wasm-builder",
"vesting-mbm/std",
"xcm-builder/std",
Expand Down
4 changes: 1 addition & 3 deletions runtime/shibuya/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,7 @@ parameter_types! {
pub RuntimeBlockWeights: BlockWeights = BlockWeights::builder()
.base_block(BlockExecutionWeight::get())
.for_class(DispatchClass::all(), |weights| {
// Adjusting the base extrinsic weight to account for the additional database
// read introduced by the `tx-pause` pallet during extrinsic filtering.
weights.base_extrinsic = ExtrinsicBaseWeight::get().saturating_add(RocksDbWeight::get().reads(1));
weights.base_extrinsic = weights::base_extrinsic::ExtrinsicBaseWeight::get();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this. Why not use ExtrinsicBaseWeight and add 2 extra reads. We don't have to maintain this benchmark. Also make sure the fee calculation reflects this

Copy link
Member

Choose a reason for hiding this comment

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

I recommended to use this so I can answer - we benchmark everything else essentially, relying on automated measurements when possible.

It's easier to have something like this, then to hardcode it, IMO.
More correct & consistent also.

Shouldn't be that much of a maintenance since only an additional command is executed within an existing loop, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dinonard if that's the case then benchmark is missing reads

Copy link
Contributor

@ermalkaleci ermalkaleci Dec 2, 2024

Choose a reason for hiding this comment

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

@ipapandinas @Dinonard this overhead benchmark doesn't count any IO. It just measures the overhead of an extrinsic. So there's no use for this. We just need to manually add 2 reads and it should be fine

Copy link
Member

Choose a reason for hiding this comment

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

I haven't checked the code, only the description of the command.
According to it, it should measure everything?

Maybe it doesn't measure IO operations discretely, but it accounts for them as part of the overall measurement.

Running benchmarks before & after inclusion of the new check could show this I guess.

Copy link
Member

Choose a reason for hiding this comment

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

If it doesn't, we should make an issue about it.

It should also measure PoV.

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've checked the overhead benchmark command code.

Remark extrinsics pushed into the block used during the benchmark are executed in the client runtime via the FrameExecutive::apply_extrinsic. These extrinsics are dispatched, and the FrameSystem::BaseCallFilter is checked alongside the origin. The new SafeMode & TxPause filters are then measured.

When running the command locally, here are the results I obtained:

Without base call filtering:

type BaseCallFilter = frame_support::traits::Everything;
Average Result: 95_705 nanoseconds

With base call filtering:

type BaseCallFilter = InsideBoth<SafeMode, TxPause>;
Average Result: 103_702 nanoseconds

This represents an 8% increase.
PoV is also measured - reference: paritytech/substrate#11637

Copy link
Contributor Author

@ipapandinas ipapandinas Dec 2, 2024

Choose a reason for hiding this comment

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

However I have the following error when performing the overhead benchmark command via shibuya-dev in the Github workflow:

Finished release [optimized] target(s) in 38m 52s
2024-12-02 14:51:56 Using the chain spec instead of the runtime to generate the genesis state is deprecated. Please remove the `--chain`/`--dev`/`--local` argument, point `--runtime` to your runtime blob and set `--genesis-builder=runtime`. This warning may become a hard error any time after December 2024.    
2024-12-02 14:51:57 assembling new collators for new session 0 at #0    
2024-12-02 14:51:57 assembling new collators for new session 1 at #0    
2024-12-02 14:51:57 Loading WASM from genesis state    
[+] Benchmarking 1 Astar collator pallets.
[+] Benchmarking pallet_tx_pause with weight file ./benchmark-results/shibuya-dev/tx_pause_weights.rs
[+] Benchmarking block and extrinsic overheads...
2024-12-02 14:52:03 assembling new collators for new session 0 at #0    
2024-12-02 14:52:03 assembling new collators for new session 1 at #0    
2024-12-02 14:52:05 🔨 Initializing Genesis block/state (state: 0x81fd…3aae, header-hash: 0x69b1…ac53)    
2024-12-02 14:52:05 panicked at /home/astar-admin/.cargo/git/checkouts/polkadot-sdk-cff69157b985ed76/b98e0b3/cumulus/pallets/parachain-system/src/lib.rs:1297:30:
included head not present in relay storage proof    
Error: Client(RuntimeApiError(Application(Execution(AbortedDueToTrap(MessageWithBacktrace { message: "wasm trap: wasm `unreachable` instruction executed", backtrace: Some(Backtrace { backtrace_string: "error while executing at wasm backtrace:\n    0: 0x1680a - <unknown>!rust_begin_unwind\n    1: 0x52f5 - <unknown>!core::panicking::panic_fmt::hc677f413e2f2c888\n    2: 0x6c74b0 - <unknown>!frame_support::storage::transactional::with_transaction::hdd4b19daa5635076\n    3: 0x19b9ee - <unknown>!<cumulus_pallet_parachain_system::pallet::Call<T> as frame_support::traits::dispatch::UnfilteredDispatchable>::dispatch_bypass_filter::{{closure}}::hf5e7b72922057598\n    4: 0x15bf8b - <unknown>!<shibuya_runtime::RuntimeCall as frame_support::traits::dispatch::UnfilteredDispatchable>::dispatch_bypass_filter::hb7c74cdeb42c4a6a\n    5: 0x154f5b - <unknown>!<shibuya_runtime::RuntimeCall as sp_runtime::traits::Dispatchable>::dispatch::he9dab724d3fc532c\n    6: 0x5d1038 - <unknown>!frame_executive::Executive<System,Block,Context,UnsignedValidator,AllPalletsWithSystem,COnRuntimeUpgrade>::apply_extrinsic::hfe4db20b64a1ce4c\n    7: 0x61f955 - <unknown>!BlockBuilder_apply_extrinsic" }) })))))
2024-12-02 14:52:05 1 storage transactions are left open by the runtime. Those will be rolled back.    
2024-12-02 14:52:05 1 storage transactions are left open by the runtime. Those will be rolled back.    
[-] Failed to benchmark the block and extrinsic overheads. Error written to ./benchmark-results/shibuya-dev/bench_errors.txt; continuing...
[+] Benchmarking the machine...
[-] Benchmarks failed:  ./benchmark-results/shibuya-dev/bench_errors.txt

})
.for_class(DispatchClass::Normal, |weights| {
weights.max_total = Some(NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT);
Expand Down
82 changes: 82 additions & 0 deletions runtime/shibuya/src/weights/base_extrinsic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// This file is part of Astar.

// Copyright (C) Stake Technologies Pte.Ltd.
// SPDX-License-Identifier: GPL-3.0-or-later

// Astar is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Astar is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Astar. If not, see <http://www.gnu.org/licenses/>.

// This is just a dummy file.

use sp_core::parameter_types;
use sp_weights::{constants::WEIGHT_REF_TIME_PER_NANOS, Weight};

// parameter_types! {
// /// Time to execute a NO-OP extrinsic, for example `System::remark`.
// /// Calculated by multiplying the *Average* with `1.0` and adding `0`.
// ///
// /// Stats nanoseconds:
// /// Min, Max: 106_559, 107_788
// /// Average: 107_074
// /// Median: 107_067
// /// Std-Dev: 242.67
// ///
// /// Percentiles nanoseconds:
// /// 99th: 107_675
// /// 95th: 107_513
// /// 75th: 107_225
// pub const ExtrinsicBaseWeight: Weight =
// Weight::from_parts(WEIGHT_REF_TIME_PER_NANOS.saturating_mul(107_074), 0);
// }

parameter_types! {
/// Time to execute a NO-OP extrinsic, for example `System::remark`.
/// Calculated by multiplying the *Average* with `1.0` and adding `0`.
///
/// Stats nanoseconds:
/// Min, Max: 87_894, 99_268
/// Average: 90_705
/// Median: 90_325
/// Std-Dev: 2344.95
///
/// Percentiles nanoseconds:
/// 99th: 99_232
/// 95th: 96_405
/// 75th: 91_204
pub const ExtrinsicBaseWeight: Weight =
Weight::from_parts(WEIGHT_REF_TIME_PER_NANOS.saturating_mul(90_705), 0);
}

#[cfg(test)]
mod test_weights {
use sp_weights::constants;

/// Checks that the weight exists and is sane.
// NOTE: If this test fails but you are sure that the generated values are fine,
// you can delete it.
#[test]
fn sane() {
let w = super::ExtrinsicBaseWeight::get();

// At least 10 µs.
assert!(
w.ref_time() >= 10u64 * constants::WEIGHT_REF_TIME_PER_MICROS,
"Weight should be at least 10 µs."
);
// At most 1 ms.
assert!(
w.ref_time() <= constants::WEIGHT_REF_TIME_PER_MILLIS,
"Weight should be at most 1 ms."
);
}
}
1 change: 1 addition & 0 deletions runtime/shibuya/src/weights/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// You should have received a copy of the GNU General Public License
// along with Astar. If not, see <http://www.gnu.org/licenses/>.

pub mod base_extrinsic;
pub mod orml_oracle;
pub mod pallet_assets;
pub mod pallet_balances;
Expand Down
16 changes: 16 additions & 0 deletions scripts/run_benchmarks.sh
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,22 @@ for chain in ${chains//,/ }; do
fi
done

# Update the block and extrinsic overhead weights.
echo "[+] Benchmarking block and extrinsic overheads..."
OUTPUT=$(
./target/production/polkadot benchmark overhead \
--chain=$chain \
--wasm-execution=compiled \
--weight-path="$output_path/$chain/" \
--warmup=10 \
--repeat=100 \
--header=./.github/license-check/headers/HEADER-GNUv3
)
if [ $? -ne 0 ]; then
echo "$OUTPUT" >> "$ERR_FILE"
echo "[-] Failed to benchmark the block and extrinsic overheads. Error written to $ERR_FILE; continuing..."
fi

echo "[+] Benchmarking the machine..."
OUTPUT=$(
$ASTAR_COLLATOR benchmark machine --chain=$chain 2>&1
Expand Down
Loading