Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[FRAME Core] New pallets: safe-mode and tx-pause #12092

Merged
merged 112 commits into from
Aug 25, 2023

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Aug 23, 2022

Adds two new pallets: SafeMode and TxPause. Closes paritytech/polkadot-sdk#274, closes paritytech/polkadot-sdk#302. Depends on paritytech/polkadot-sdk#208

SafeMode pallet

The safe-mode pallet provides a big STOP button to to put the chain in safe-mode and thereby only permitting a certain subset of operations. The pallet provides a WhitelistedCalls which contains all calls that can be executed in safe-mode.
It can be permissionessly enabled by anyone by reserving a large deposit. The safe-mode pallet is used by the runtime as call filter:

impl frame_system::Config for Runtime {type BaseCallFilter = InsideBoth<DefaultFilter, SafeMode>;}

Calls

enter:
Enter the safe-mode permissionlessly for EnterDuration blocks.
Reserves an EnableDepositAmount amount of balance from the caller.
This call can be disabled by configuring EnterDepositAmount to None.

The intention is to allow heavily invested entities to stop the chain in case they are convinced that there is an ongoing attack that can be prevented or mitigated via the safe-mode. Governance would then investigate and refund the deposit afterwards. Abuse can be disincentivized through the possibility of slashing said deposit.

force_enter:
Allows only only ForceEnterOrigin to forcefully enter the safe-mode for a number of blocks that can be configured via EnsureOrigin.

This could be done by a technical governance body in order to quickly respond to an attack or exploit.

extend:
Extend the safe-mode permissionlessly for ExtendDuration more blocks.
Reserves ExtendDepositAmount from the caller's account.
This call can be disabled by configuring ExtendDepositAmount to None.

Same intention as enter, just about prolonging instead of initiating the safe-mode.

force_extend:
Allows only only ForceExtendOrigin to forcefully extend the safe-mode for a number of blocks that can be configured via EnsureOrigin.

force_exit:
Permissioned call to instantly disable the safe-mode.

release_deposit(account, block_number):
Permissionlessly repay the deposit to the account that enabled the safe-mode in block block_number. Can only be called if the safe-mode is exited and and if ReleaseDelay is configured to Some(delay) has passed.

force_slash_deposit(account, block_number):
Permissioned call to slash the deposit of the account that enabled the safe-mode in block block_number.

on_initialize:
Disables the safe-mode if its duration ran out in this block.

TxPause pallet

The TxPause pallet can be used to pause specific calls. Think of it as a dynamic call filter that can be controlled with extrinsics.
It currently features per-call pausing, but per-pallet pausing would also be possible. This is similar to what many para-chains currently have deployed.

This pallet currently operates on pallet and call names instead of indices. Depends on paritytech/polkadot-sdk#208

Can also be used as call-filter by the runtime together with the SafeMode:

impl frame_system::Config for Runtime {type BaseCallFilter = InsideBoth<TxPause, SafeMode>;}

Calls

pause_call(pallet, function):
Permissioned call to pause a specific call.

unpause_call(pallet, function):
Permissioned call to unpause a specific call.

TODOS:

  • Design finished
  • Q: How to make them work together?
  • Intern feedback
  • Parachain feedback
  • Create traits in frame-support
    • Impl
    • Doc
    • Test
  • Hooks that can be used for XCM suspension

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Aug 23, 2022
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Aug 23, 2022
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez changed the title pallet-safe-mode pallet-tx-pause Aug 23, 2022
@ggwpez ggwpez added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Aug 23, 2022
@ggwpez ggwpez changed the title pallet-tx-pause New pallets: safe-mode and transaction-pause Aug 24, 2022
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@shawntabrizi
Copy link
Member

We are missing the origin where some user can "transfer a huge amount of dot" to trigger a tx pause or safe mode for a temporary period of time.

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/parachain-technical-summit-next-steps/51/8

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/parachain-technical-summit-next-steps/51/1

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@gavofyork
Copy link
Member

I think they're fine as two pallets which can be used to work together. There should be a means of specifying an EnsureOrigin impl which can enter safe mode for a period of time (the EnsureOrigin should have a Success value which is the number of blocks that the origin may induce safe mode).

Any deposit made for inducing safe mode should not be returned by default. A simple solution would be for it to go into the treasury and be paid back only by an explicit treasury spend. A more sophisticated solution would have it be reserved and refunded by a particular Refund EnsureOrigin impl (which would likely be configured as a Gov2 origin and probably have an associated referendum curve/collective behind it).

Beyond that, I think design-wise it's good.

@ggwpez
Copy link
Member Author

ggwpez commented Aug 30, 2022

I think they're fine as two pallets which can be used to work together.

Then we should have some sane-defaults that the pallets dont ban each other.
In theory they can work together very well; 1) enable safe-mode, 2) pause offending transactions, 3) disable safe-mode.

There should be a means of specifying an EnsureOrigin impl which can enter safe mode for a period of time (the EnsureOrigin should have a Success value which is the number of blocks that the origin may induce safe mode).

Okay. I assume the required stake is either also returned by that or scales with a configured formula.

Any deposit made for inducing safe mode should not be returned by default.

Currently only a RepayOrigin can either repay or slash the reserved stake. But keeping it even more simple sounds tempting.

PS: I will probably rename enable/disable to enter/leave as that sounds nicer.

@nuke-web3 nuke-web3 mentioned this pull request Aug 31, 2022
6 tasks
Copy link
Contributor

@nuke-web3 nuke-web3 left a comment

Choose a reason for hiding this comment

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

Have a few suggestions related only to safe-mode for now, although take them with a grain of salt: perhaps I am missing something.

@ggwpez we need a impl for #[pallet::genesis_config] && #[pallet::genesis_build], are you intending on adding these?

frame/safe-mode/src/lib.rs Outdated Show resolved Hide resolved
frame/safe-mode/src/lib.rs Outdated Show resolved Hide resolved
@nuke-web3
Copy link
Contributor

nuke-web3 commented Sep 2, 2022

Doing a read here, I have some thoughts I hope it's not too much to discuss here 🙏

Doubts & Questions:

  • If done on relay chain, all paras and XCMP still operate? for example, XCMP or reserve backed transfers might be blocked. What other considerations needed?
  • In XCM, would this block all or some specific calls? (tests needed) consider fallout of no callback to known this is intentionally blocked
  • Could/should external parachain (sovereign accounts) be given special ability to execute safe-mode (without stake for example, or only to bar XCM on one chain to them. )
  • Speaking for any substrate chain: is the governance origin is always root? If not, root should be "whitelisted" such that it cannot be blocked by filter even if not explicitly configured?
  • Is there ever an internal call that could be blocked by this, like an unsigned required action... or more likely a tightly coupled pallet where this might block logic progression?
    • Offchain workers that are required in some way failing? Like oracles, etc.
    • Offchain workers triggered on hooks that cannot dispatch could lead to it being triggered every block and depending on if it expected to succeed eventually and only happen infrequently could cause problems, IE very very expensive computation and/or paid API services.
    • Other systems/bridges that assume liveness and non-failure... blocks are still produced, but in effect the network is halted (at least it errors out)
  • Once in place, should this be in the solo and parachain templates? It's a very opinionated & powerful tool to add to a chain, and could be abused by users: for example testnets could be blocked on a whim to cause chaos for "fun" if the stake isn't too high for faucet abusers to get enough. I would think not appropriate.
  • Possible exploits in on and cross chain DeFi if gains by blocking tx are larger than stake: you lock the network and exploit data to front-run.
    • Timeout functions related to block height could be exploited. Like payment channels in lightning on bitcoin: lock the network long enough (but blocks are still ticking up) to stop a justice tx being submitted.
    • related to assumptions of liveness / (eventual) completion of bridge actions.

Design changes?

  • Should there be different safe-modes possible? no, tx-pause can use pre-configured settings.
  • Should the justification for safe mode be explicitly required (and placed in storage w/ impl to query from user space)?
    • Something more like a governance proposal storage item in this pallet & flow (like a proposal on Polkassembly being fired off when this is triggered) for enacting this extremely disruptive action.
      • Could be as simple, bounded, string that is a statement of "why this safe-mode was triggered" and/or enforce a CID to a statement would be available.
  • Pause pallet has bounded vecs that are not storage items... yet?
    • The pause pallet storage of names of calls could be misconfigured as there is no such limit on naming AFAIK natively. so maybe you could make a too long name and miss this in tests that check for explicitly & exhaustively checking the filters work.
    • Adding this pallet would require you truncate names of calls, or set large enough not to fail... nothing more automatic possible?
      • Would the FRAME generated storage items be a blocker here, if you didn't want to do a migration? Rename of these -> migration in most cases.
  • Can we throw a pallet specific error instead of CallFiltered? so unambiguous to everyone (end users, future XCM callbacks, APIs) that this safe mode is the reason things are halted.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member Author

ggwpez commented Aug 24, 2023

Happy birthday 🥳

Going to merge as notlive since the audit will not start within another two months or so.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member Author

ggwpez commented Aug 24, 2023

bot bench substrate-pallet --pallet=pallet_safe_mode

@command-bot
Copy link

command-bot bot commented Aug 24, 2023

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3437154 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_safe_mode. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 6-4d486b03-ff42-4f89-a7df-2a03667e67b0 to cancel this command or bot cancel to cancel all commands in this pull request.

@ggwpez
Copy link
Member Author

ggwpez commented Aug 24, 2023

bot bench substrate-pallet --pallet=pallet_tx_pause

@command-bot
Copy link

command-bot bot commented Aug 24, 2023

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3437155 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_tx_pause. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 7-7fb643c2-f827-4323-a94c-78d6759dd974 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Aug 24, 2023

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_safe_mode has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3437154 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3437154/artifacts/download.

@command-bot
Copy link

command-bot bot commented Aug 24, 2023

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_tx_pause has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3437155 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3437155/artifacts/download.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member Author

ggwpez commented Aug 24, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: 1 review requesting changes and 2 approving reviews by reviewers with write access.

@ggwpez
Copy link
Member Author

ggwpez commented Aug 25, 2023

Bot merge

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

Safe mode Transaction pause pallet