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

ipapandinas · 2024-11-25T15:40:56Z

Pull Request Summary

This PR integrates the SafeMode and TxPause pallets into the Shibuya runtime. Key configuration decisions are as follows:

SafeMode Configuration

EnterDepositAmount and ExtendDepositAmount are set to None, making safe-mode non-permissionless.
EnterDuration is set to 4 hours, with extensions allowed in increments of 2 hours.
Only accounts with EnsureRootOrHalfTechnicalCommittee origin can force enter or extend safe-mode
Only accounts with EnsureRootOrTwoThirdsTechnicalCommittee origin can force exit safe-mode
dAppStaking’s maintenance mode is triggered by Notify when entering/exiting safe-mode.

TxPause Configuration

Calls can be paused/unpaused by accounts with EnsureRootOrHalfTechnicalCommittee origin. The same origin is used by the dAppStaking maintenance manager.
All calls can be paused.

These pallets are designed to complement each other effectively, for example if halting block production after entering safe-mode is required, this can be performed by pausing System or Timestamp calls by using tx-pause.

Benchmarks

The additional DB read introduced by the TxPause base filter in the extrinsic base weight is hardcoded with 1 db read for now. This is a temporary fix, a proper benchmark must be done in the future.

Check list

added or updated unit tests
updated Astar official documentation
added OnRuntimeUpgrade hook for precompile revert code registration
added benchmarks & weights for any modified runtime logics.

Dinonard · 2024-11-27T10:29:42Z

runtime/shibuya/src/lib.rs

+            // System and Timestamp are required for block production
+            | RuntimeCall::System(_)
+            | RuntimeCall::Timestamp(_)


Dumb question - aren't these calls inherent though?
If such calls also need to be filtered, parachain system should also be included here.

If I remove Timestamp block production stops:

❌️ Mandatory inherent extrinsic returned error. Block cannot be produced.

But, I removed SafeMode since the safe-mode pallet cannot disable it's own calls and I added ParachainSystem.

Hmmm strange, have to admit that I'm surprised :)

runtime/shibuya/src/lib.rs

Dinonard · 2024-11-28T16:07:43Z

runtime/shibuya/src/lib.rs

+            // 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));


This isn't the way to do it 🙂

RocksDb weights is a config param of the frame_system and we should use it from there

That aside, this shouldn't be necessarily hardcoded but it should be benchmarked. There is a command to measure base weight and perhaps now is the time to use it.

Do you mean using the /bench command from the workflows? Should I run it specifically for frame-system and pallet-tx-pause, or should I include others as well? I’m thinking of pallets that involve dispatches, such as proxy, collective-proxy, utility, and scheduler.

There is a specific command to measure overhead, or base extrinsic weight.

E.g. using the frame-omni-bencher, it should be frame-omni-bencher v1 benchmark overhead.

I guess we'd need to adapt the CI a bit 🤔

Thanks for pointing out this command to me; I was not aware of its existence! The overhead command is not yet available in frame-omni-bencher v0.7.0 but should be released in the next Polkadot SDK stable2412 version. I've adjusted the CI to include it alongside our existing benchmarking approach. I will run it below using the /bench command.

Nice, very neat!

runtime/shibuya/src/lib.rs

ermalkaleci · 2024-12-02T12:43:55Z

bin/collator/src/parachain/service.rs

 pub type HostFunctions = (
    cumulus_client_service::ParachainHostFunctions,
    moonbeam_primitives_ext::moonbeam_ext::HostFunctions,
 );

+/// Host functions required for kitchensink runtime and Substrate node.


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

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)

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

seems like overhead doesn't include it, wonder why

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

ermalkaleci · 2024-12-02T13:06:21Z

runtime/shibuya/src/lib.rs

@@ -248,7 +249,7 @@ parameter_types! {
    pub RuntimeBlockWeights: BlockWeights = BlockWeights::builder()
        .base_block(BlockExecutionWeight::get())
        .for_class(DispatchClass::all(), |weights| {
-            weights.base_extrinsic = ExtrinsicBaseWeight::get();
+            weights.base_extrinsic = weights::base_extrinsic::ExtrinsicBaseWeight::get();


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

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?

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

@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

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.

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

It should also measure PoV.

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

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

ipapandinas · 2024-12-02T14:12:39Z

/bench shibuya-dev pallet_tx_pause

github-actions · 2024-12-02T14:13:01Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/12121139663.
Please wait for a while.
Branch: feat/tx-pause-safe-mode
SHA: 03d0f9a

github-actions · 2024-12-02T14:52:33Z

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/12121139663.

ermalkaleci · 2024-12-02T17:03:06Z

I would like to have some e2e tests for this, especially what will happen when chain receives a xcm call. Maybe we can use chopsticks to test different scenarios

Dinonard · 2024-12-02T17:38:18Z

I would like to have some e2e tests for this, especially what will happen when chain receives a xcm call. Maybe we can use chopsticks to test different scenarios

Don't the existing tests cover this?
It's the same as changing how much any existing extrinsic weighs, right?

ermalkaleci · 2024-12-02T18:08:41Z

@Dinonard No, I mean to test xcm when chain is on restriction mode

runtime/local/Cargo.toml

ipapandinas · 2024-12-04T11:18:50Z

/bench astar-dev pallet_balances

github-actions · 2024-12-04T11:19:11Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/12158641670.
Please wait for a while.
Branch: feat/tx-pause-safe-mode
SHA: 03d0f9a

github-actions · 2024-12-04T11:56:33Z

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/12158641670.

This reverts commit 03d0f9a.

This reverts commit 96a0520.

Co-authored-by: Ermal Kaleci <[email protected]>

runtime/shibuya/src/lib.rs

This reverts commit 1f400a4.

github-actions · 2024-12-04T19:05:52Z

Package	Line Rate	Branch Rate	Health
precompiles/dapp-staking/src	90%	0%	✔
precompiles/dispatch-lockdrop/src	86%	0%	✔
pallets/ethereum-checked/src	74%	0%	➖
chain-extensions/unified-accounts/src	0%	0%	❌
chain-extensions/pallet-assets/src	56%	0%	➖
pallets/collective-proxy/src	86%	0%	✔
pallets/dapp-staking/src	83%	0%	✔
precompiles/unified-accounts/src	100%	0%	✔
primitives/src	57%	0%	➖
pallets/dapp-staking/src/benchmarking	98%	0%	✔
pallets/collator-selection/src	92%	0%	✔
primitives/src/xcm	65%	0%	➖
pallets/astar-xcm-benchmarks/src	86%	0%	✔
pallets/inflation/src	89%	0%	✔
precompiles/sr25519/src	64%	0%	➖
precompiles/xcm/src	71%	0%	➖
pallets/price-aggregator/src	82%	0%	✔
pallets/dapp-staking/src/test	0%	0%	❌
chain-extensions/types/unified-accounts/src	0%	0%	❌
pallets/static-price-provider/src	85%	0%	✔
pallets/unified-accounts/src	86%	0%	✔
pallets/astar-xcm-benchmarks/src/fungible	100%	0%	✔
precompiles/substrate-ecdsa/src	74%	0%	➖
pallets/astar-xcm-benchmarks/src/generic	100%	0%	✔
precompiles/assets-erc20/src	78%	0%	➖
pallets/dapp-staking/rpc/runtime-api/src	0%	0%	❌
precompiles/dapp-staking/src/test	0%	0%	❌
chain-extensions/types/assets/src	0%	0%	❌
pallets/vesting-mbm/src	89%	0%	✔
pallets/dynamic-evm-base-fee/src	89%	0%	✔
pallets/xc-asset-config/src	50%	0%	❌
Summary	79% (3736 / 4744)	0% (0 / 0)	➖

Minimum allowed line rate is 50%

feat: tx-pause and safe-mode pallets integration

ae9ea5d

ipapandinas added shibuya related to shibuya runtime This PR/Issue is related to the topic “runtime”. labels Nov 25, 2024

ipapandinas changed the title ~~feat: tx-pause and safe-mode pallets integration~~ feat: tx-pause and safe-mode pallets integration Nov 25, 2024

ipapandinas requested review from ermalkaleci and Dinonard and removed request for ermalkaleci November 25, 2024 16:32

Dinonard reviewed Nov 27, 2024

View reviewed changes

config updated and safe-mode coupled to DappStaking via Notify

ac5d44b

Dinonard reviewed Nov 28, 2024

View reviewed changes

ipapandinas added 2 commits December 2, 2024 12:41

overhead benchmark comment added

96a0520

overhead benchmark command added - typo

03d0f9a

AstarNetwork deleted a comment from github-actions bot Dec 2, 2024

ermalkaleci reviewed Dec 2, 2024

View reviewed changes

ipapandinas mentioned this pull request Dec 2, 2024

safe-mode & tx-pause integration AstarNetwork/astar-docs#720

Open

ermalkaleci reviewed Dec 4, 2024

View reviewed changes

runtime/local/Cargo.toml Outdated Show resolved Hide resolved

ipapandinas added 3 commits December 4, 2024 15:32

Revert "overhead benchmark command added - typo"

fcbffdb

This reverts commit 03d0f9a.

Revert "overhead benchmark comment added"

9048b77

This reverts commit 96a0520.

todo comment added to use proper benchmarking in the future

815f13f

ipapandinas and others added 3 commits December 4, 2024 15:40

remove useless crate option

642f727

Co-authored-by: Ermal Kaleci <[email protected]>

remove useless crate option

c710c1e

Co-authored-by: Ermal Kaleci <[email protected]>

remove useless crates options

efd9b61

Dinonard reviewed Dec 4, 2024

View reviewed changes

runtime/shibuya/src/lib.rs Outdated Show resolved Hide resolved

DbWeight from frame_system::Config used

6218b74

Dinonard previously approved these changes Dec 4, 2024

View reviewed changes

RuntimeAdjustedWeights type to handle weights override

1f400a4

ipapandinas dismissed Dinonard’s stale review via 1f400a4 December 4, 2024 17:28

Dinonard reviewed Dec 4, 2024

View reviewed changes

runtime/shibuya/src/lib.rs Outdated Show resolved Hide resolved

Dinonard previously approved these changes Dec 4, 2024

View reviewed changes

Dinonard self-requested a review December 4, 2024 17:43

ipapandinas added 3 commits December 4, 2024 19:27

remove WeightToFee mock

633982f

Revert "RuntimeAdjustedWeights type to handle weights override"

ce4612c

This reverts commit 1f400a4.

base_extrinsic get from RuntimeBlockWeights for WeightToFee

e215684

ipapandinas dismissed Dinonard’s stale review via e215684 December 4, 2024 18:38

Dinonard approved these changes Dec 5, 2024

View reviewed changes

ermalkaleci approved these changes Dec 5, 2024

View reviewed changes

ipapandinas merged commit 17fc034 into master Dec 5, 2024
8 checks passed

ipapandinas deleted the feat/tx-pause-safe-mode branch December 5, 2024 17:19

ipapandinas linked an issue Dec 5, 2024 that may be closed by this pull request

Integrate tx-pause and safe-mode pallets #1364

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ipapandinas commented Nov 25, 2024 •

edited

Loading

Dinonard Nov 27, 2024

ipapandinas Nov 28, 2024

Dinonard Nov 28, 2024

Dinonard Nov 28, 2024

ipapandinas Nov 29, 2024

Dinonard Nov 29, 2024

ipapandinas Dec 2, 2024

Dinonard Dec 2, 2024

ermalkaleci Dec 2, 2024

ipapandinas Dec 2, 2024

ermalkaleci Dec 2, 2024 •

edited

Loading

ermalkaleci Dec 2, 2024

Dinonard Dec 2, 2024

ermalkaleci Dec 2, 2024

Dinonard Dec 2, 2024

ermalkaleci Dec 2, 2024

ermalkaleci Dec 2, 2024 •

edited

Loading

Dinonard Dec 2, 2024

Dinonard Dec 2, 2024

ipapandinas Dec 2, 2024

ipapandinas Dec 2, 2024 •

edited

Loading

ipapandinas commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

ermalkaleci commented Dec 2, 2024

Dinonard commented Dec 2, 2024

ermalkaleci commented Dec 2, 2024

ipapandinas commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

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

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

Conversation

ipapandinas commented Nov 25, 2024 • edited Loading

SafeMode Configuration

TxPause Configuration

Benchmarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ermalkaleci Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ermalkaleci Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Without base call filtering:

With base call filtering:

ipapandinas Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

ipapandinas commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

ermalkaleci commented Dec 2, 2024

Dinonard commented Dec 2, 2024

ermalkaleci commented Dec 2, 2024

ipapandinas commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

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

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

ipapandinas commented Nov 25, 2024 •

edited

Loading

ermalkaleci Dec 2, 2024 •

edited

Loading

ermalkaleci Dec 2, 2024 •

edited

Loading

ipapandinas Dec 2, 2024 •

edited

Loading