-
Notifications
You must be signed in to change notification settings - Fork 416
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
Changes from 1 commit
ae9ea5d
ac5d44b
96a0520
03d0f9a
fcbffdb
9048b77
815f13f
642f727
c710c1e
efd9b61
6218b74
1f400a4
633982f
ce4612c
e215684
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
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 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 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 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. Shouldn't be that much of a maintenance since only an additional command is executed within an existing loop, right? 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. @Dinonard if that's the case then benchmark is missing reads 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. @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 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 haven't checked the code, only the description of the command. 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. 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. If it doesn't, we should make an issue about it. It should also measure PoV. 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'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 When running the command locally, here are the results I obtained: Without base call filtering:
With base call filtering:
This represents an 8% increase. 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. However I have the following error when performing the overhead benchmark command via 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); | ||
|
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." | ||
); | ||
} | ||
} |
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.
we don't need this. The benchmark host functions is injected by benchmarking-cli itself
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 am getting this error if I remove benchmarking host functions:
PS: I will fix the comment (copied)
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.
https://github.com/paritytech/polkadot-sdk/blob/0845044454c005b577eab7afaea18583bd7e3dd3/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs#L66
Not sure why you're getting this error
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.
seems like overhead doesn't include it, wonder why
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.
We can make an issue or PR for it, probably it's small effort.