Dynamic Benchmarking DB Whitelist

[shawntabrizi]

polkadot companion: paritytech/polkadot#1612

This PR introduces a new get_whitelist host function. In combination with set_whitelist, we are able to create helper functions add_whitelist and remove_whitelist which can be called from a benchmark to dynamically introduce or remove items from the benchmarking whitelist.

Additionally, this PR updates the format of the whitelist to use a new struct TrackedStorageKey:

pub struct TrackedStorageKey {
	pub key: Vec<u8>,
	pub has_been_read: bool,
	pub has_been_written: bool,
}

Which includes the storage key, and whether this storage key has been read from or written to. This allows us to whitelist a key for a read, write, or both.

Here are some explicit examples of how you would use this:

Ignore Read, Track Write

Let's say that you have some storage value that is read each block, so you are able to ignore reads to that variable introduced by an extrinsic, but you still want to track the first write. For example, the LaunchPeriod of the democracy module may be placed as a storage item, and every on_initialize, we check if the block would trigger a launch period (reading it every block), but if we had an extrinsic to update the launch period, we would still want to track writes to this value. To your benchmark you would add the following:

// Ignore reads to `LaunchPeriod` since it read every `on_initialize`. But still track writes to this value.
let launch_period_key = crate::LaunchPeriod::hashed_key().to_vec();
frame_benchmarking::benchmarking::add_whitelist(TrackedStorageKey {
	key: launch_period_key,
	has_been_read: true,
	has_been_written: false,
});

Track Read, Ignore Write

In some more unique scenarios, you may want to track the first read of a storage item, but ignore any writes to it. For example the Timestamp pallet has the DidUpdate variable which is transient (created and destroyed within one block), but before we create the value, we make sure it doesn't already exist. This exist lookup should be counted as a storage read, but us writing to the value should not be counted because it will be removed at the end of the block, ultimately not committing anything to the DB. You would represent this like so:

// Ignore write to `DidUpdate` since it transient.
let did_update_key = crate::DidUpdate::hashed_key().to_vec();
frame_benchmarking::benchmarking::add_whitelist(TrackedStorageKey {
	key: did_update_key,
	has_been_read: false,
	has_been_written: true,
});

Ignore Read and Write

Finally, the most common scenario is to simply ignore both reads and writes to a storage key. For example, use a unique caller account, and want to make sure that reads/writes to the System.Account of that caller are not accounted for as they are already included in the base weight of an extrinsic. For this, we can use a shorthand to construct the TrackedStorageKey by simply converting with key.into(), like so:

// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller);
frame_benchmarking::benchmarking::add_whitelist(caller_key.into());

fixes #6802

[gui1117]

if you want to get some inspiration how to write a global variable in the runtime gav implemented one, it look like this:

substrate/frame/support/src/traits.rs

Lines 120 to 193 in 35f3ba4

	#[macro_export]
	macro_rules! impl_filter_stack {
	($target:ty, $base:ty, $call:ty, $module:ident) => {
	#[cfg(feature = "std")]
	mod $module {
	#[allow(unused_imports)]
	use super::*;
	use $crate::traits::{swap, take, RefCell, Vec, Box, Filter, FilterStack};
	thread_local! {
	static FILTER: RefCell<Vec<Box<dyn Fn(&$call) -> bool + 'static>>> = RefCell::new(Vec::new());
	}
	impl Filter<$call> for $target {
	fn filter(call: &$call) -> bool {
	<$base>::filter(call) &&
	FILTER.with(|filter| filter.borrow().iter().all(|f| f(call)))
	}
	}
	impl FilterStack<$call> for $target {
	type Stack = Vec<Box<dyn Fn(&$call) -> bool + 'static>>;
	fn push(f: impl Fn(&$call) -> bool + 'static) {
	FILTER.with(|filter| filter.borrow_mut().push(Box::new(f)));
	}
	fn pop() {
	FILTER.with(|filter| filter.borrow_mut().pop());
	}
	fn take() -> Self::Stack {
	FILTER.with(|filter| take(filter.borrow_mut().as_mut()))
	}
	fn restore(mut s: Self::Stack) {
	FILTER.with(|filter| swap(filter.borrow_mut().as_mut(), &mut s));
	}
	}
	}
	#[cfg(not(feature = "std"))]
	mod $module {
	#[allow(unused_imports)]
	use super::*;
	use $crate::traits::{swap, take, RefCell, Vec, Box, Filter, FilterStack};
	struct ThisFilter(RefCell<Vec<Box<dyn Fn(&$call) -> bool + 'static>>>);
	// NOTE: Safe only in wasm (guarded above) because there's only one thread.
	unsafe impl Send for ThisFilter {}
	unsafe impl Sync for ThisFilter {}
	static FILTER: ThisFilter = ThisFilter(RefCell::new(Vec::new()));
	impl Filter<$call> for $target {
	fn filter(call: &$call) -> bool {
	<$base>::filter(call) && FILTER.0.borrow().iter().all(|f| f(call))
	}
	}
	impl FilterStack<$call> for $target {
	type Stack = Vec<Box<dyn Fn(&$call) -> bool + 'static>>;
	fn push(f: impl Fn(&$call) -> bool + 'static) {
	FILTER.0.borrow_mut().push(Box::new(f));
	}
	fn pop() {
	FILTER.0.borrow_mut().pop();
	}
	fn take() -> Self::Stack {
	take(FILTER.0.borrow_mut().as_mut())
	}
	fn restore(mut s: Self::Stack) {
	swap(FILTER.0.borrow_mut().as_mut(), &mut s);
	}
	}
	}
	}
	}

Though you don't need to wrap in macro because you only need to have one in benchmarking.

[shawntabrizi]

Ideally, I wanted to make the whitelisting of calling account automatic. I wanted to extract the $origin from the macro parsing, and then return it in a seperate fn origin() from the Benchmarking trait. Then I can detect if the origin was Signed and then whitelist the caller

However, $origin is an $expr and has within the enum the caller value, which I believe does not evaluate anymore.

You can see my attempt here: 15ac408

It compiles and stuff, but you will notice I dont pass the $origin variable. Once you do, you will get a compile error that caller is not in scope...

Anyway, if @thiolliere you have an idea how to do this, all ears. Otherwise, this does do the functionality I want manually.

[gui1117]

looks good to me, I like that whitelisted caller account is now more explicit

[shawntabrizi]

These macro changes are just superficial. I was trying to edit them at some point, and they were impossible to read to me. So I formatted them how I would expect to seem them. No logic actually changes in the macro parsing

[shawntabrizi]

Rather than have them unimplemented I had them do nothing so they would work inside a test.

Otherwise, we would need to feature gate all get/set operations with #[cfg(not(test))]

[shawntabrizi]

@thiolliere @apopiak I think this PR is in a good spot for a review. I got all my desired features in there, so please take a look and ask any questions you might have.

[shawntabrizi]

This logic needed to be updated to support the scenario where we want to count a read, but ignore a write. (i.e. timestamp pallet)

[gui1117]

looks good to me, with not nitpicks

[apopiak]

LGTM, some nits about naming

[shawntabrizi]

bot merge

[ghost]

Missing process info; check that the PR belongs to a project column.

Merge can be attempted if:

• The PR has approval from two core-devs (or one if the PR is labelled insubstantial).
• The PR is attached to a project column and has approval from the project owner.
• The PR has approval from a member of substrateteamleads.

[shawntabrizi]

bot merge

[ghost]

Checks failed; merge aborted.

[shawntabrizi]

bot merge

[ghost]

Checks failed; merge aborted.

[shawntabrizi]

bot merge force

[ghost]

Trying merge.