WIP safe mode and tx pause {continued}

[nuke-web3]

Targets #12092

• Changes the tx pause pallet to accept input as RuntimeCalls, instead of stringy names. This was decided to be a bad idea, based on feedback.

  • The storage is still stringy names, as it allows for less headache on migration and safety
  • The UI can now help you as in the Proxy pallet have dropdown selections for the possible calls, and there should be no possibility of user error, or inserting calls that do not explicitly exist in the runtime. Example:
    
  • The above likely means we have to over specify calls with "real" values, but all we really want here is the call without dummy data. TBD on behavior and if a fix is possible

• tests for tx pause and safe mode working as written 🎉

  • ensuring indirect calls from pallets, like batching via util pallet (and perhaps proxy) {tested manually to work as expected in the kitchensink via polkadot.js apps UI}

• benchmarks configured and dummy files genereated 🎉

• naming tweaks for intuitive understanding and common terms between pallets.

Considerations (for abandoned RuntimeCall variation on these pallets)

One issue with tx pause pallet to accept input as RuntimeCalls: on runtime upgrades changing names or removing pallets that are paused will be inaccessible to remove from tx pause storage, as the runtime metadata won't include the matching stringy names from before.

Possible solution: a "raw" call in this pallet to insert or drop pauses by stringy names explicitly as a privileged call for the unpause origin. Alternatively, just a note on how to remove a raw storage key if you missed it in a migration (likely not possible from the unpause origin!)

Its more of a corner case IMHO, and a very simple migration would be needed if you wanted to keep a modified call paused. No issue leaving an irrelevant call name in storage either. Not an DOS attachable thing or something likely to case any issue if forgotten

TODO

• Update kitchen sink runtime config for the safe mode origins needed (not correct or working as it stands now)
• ~Add preset call filter for the tx pause pallet such that what amounts to a "batch" of items will be placed into storage for tx pause in a single call. There should be no need to analyze and form a manual pause_call batch. ~ in a folllowup PR
  • Can this be derived from a match statement ergonomically, so you can specify pallets inclusive of all calls without explicitly naming each?
  • Desired behavior is being able to selectively unpause calls that are placed in this preset batch of calls to add to the vec of paused calls.

[nuke-web3]

To ensure that the RuntimeCall type used as a field needed to be set by end users, I wanted to make sure it wasn't too difficult and that it would prevent "footguns" using 917961b I have made an MVP demo of using the tx-pause with subxt: https://github.com/NukeManDan/pause-subxt

As the OP hoped, the behavior using the apps ui is the same as proxy: you have drop downs on the only selections you can make. The only grip is that you are forced to used correct values here generally in the "call" you supply (like an account and value for a balance transfer) to make the UI and subxt happy.

@ggwpez wdyt? Not too bad for the extra safety and less need to lookup the correct string to place into storage for the pallet and the function/call as it was previously?

[Polkadot-Forum]

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

https://forum.polkadot.network/t/safe-mode-and-transaction-pause-pallets/636/1

[ggwpez]

@ggwpez wdyt? Not too bad for the extra safety and less need to lookup the correct string to place into storage for the pallet and the function/call as it was previously?

We could already safe-guard against wrong pallet names by going through all available pallets. This is maybe also possible for calls. I think requiring additional argument data which we will then ignore is not good.
It also gives off the impression that we could filter a complete by arguments, since they are provided. But we ignore them later on.
I will ask @shawntabrizi today in a meeting what he thinks.

PS: A front-end UI could also real all Pallet and Call names from metadata, so we should be good with just strings.

[nuke-web3]

Agreed that extraneous call data is not ideal. Dumb question: is it possible to specify a type that is inclusive of only the call without values? Perhaps its somewhere in the macro logic to generate the RuntimeCall enum or maybe a way to cheaply impl a type that does this from the fully generated RuntimeCall enum...?

Perhaps yes: https://stackoverflow.com/questions/32554285/compare-enums-only-by-variant-not-value
Will look into this

[nuke-web3]

Not having to create a custom ui just to handle this pallet was my goal in using the Call variants though (in part) although I agree its an option.

I want to test behavior of sending a raw encoded call with incorrect call data specified to pause, as at least to me it's unclear if that extrinsic would be rejected. I do think so though, as we need to be able to read the pallet name and call name, and dummy call data set in the pause extrinsics should fail to extract this (i hope). Stingy names can be set without a check and succeed (as it is now) to set an arbitrary, possibly meaningless via typo, filter.

Perhaps it is possible to use stringy names that are checked at execution time to be correct? Static at compile time imho is still preferred.

[Polkadot-Forum]

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

https://forum.polkadot.network/t/safe-mode-and-transaction-pause-pallets/636/4

[xlc]

I will prefer to make it

/// An origin that can enable the safe-mode by force.
 pub enum ForceActivateOrigin {
 	Weak = 5,
 	Medium = 7,
 	Strong = 11,
 }

so that frontend doesn't need to hardcod those number once more

[nuke-web3]

Cool - TIL https://doc.rust-lang.org/reference/items/enumerations.html#custom-discriminant-values-for-fieldless-enumerations is a thing

So this example diff works to yield an integer castable item, so BlockNumber works fine.

But you cannot assign the same value for any origin this way as it would conflict:

enum SharedDiscriminantError {
    SharedA = 1,
    SharedB = 1
}

I think this restriction would be OK, but hope there is a better method to get there... Open to ideas.

[Polkadot-Forum]

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

https://forum.polkadot.network/t/safe-mode-and-transaction-pause-pallets/636/6

[nuke-web3]

Removed RuntimeCall call values per feedback.

Introduced FullNameOf makes a handy Contains impl for calls and/or pallets to be filtered:

substrate/bin/node/runtime/src/lib.rs

Lines 217 to 241 in b95b286

	use pallet_tx_pause::FullNameOf;
	pub struct UnfilterableCallNames;
	/// Make Balances::transfer_keep_alive unfilterable, accept all others.
	impl Contains<FullNameOf<Runtime>> for UnfilterableCallNames {
	fn contains(full_name: &FullNameOf<Runtime>) -> bool {
	let unpausables: Vec<FullNameOf<Runtime>> = vec![
	(
	b"Balances".to_vec().try_into().unwrap(),
	Some(b"transfer_keep_alive".to_vec().try_into().unwrap()),
	),
	];
	for unpausable_call in unpausables {
	let (pallet_name, maybe_call_name) = full_name;
	if pallet_name == &unpausable_call.0 {
	if unpausable_call.1.is_none() {
	return true
	}
	return maybe_call_name == &unpausable_call.1
	}
	}
	false
	}
	}