Fix treasury benchmarks when no SpendOrigin

[bgallois]

Issue

It was impossible to benchmark the pallet_treasury when SpendOrigin = frame_support::traits::NeverEnsureOrigin<u64>; was specified.

Done

• Use weight = 0 for all extrinsics that are un-callable with no SpendOrigin.
• Fix benchmarks for extrinsics requiring a Spend even if SpendOrigin = frame_support::traits::NeverEnsureOrigin<u64>;

[cla-bot-2021]

User @bgallois, please sign the CLA here.

[bgallois]

I'm unable to sign the CLA: Content Encoding Error. An error occurred during a connection to cla.parity.io.

[ggwpez]

Thanks!

[ggwpez]

I'm unable to sign the CLA: Content Encoding Error. An error occurred during a connection to cla.parity.io.

Hm, can you try again?
Not sure whom to ping here @mutantcornholio maybe?

[bgallois]

I'm unable to sign the CLA: Content Encoding Error. An error occurred during a connection to cla.parity.io.

Hm, can you try again? Not sure whom to ping here @mutantcornholio maybe?

Still no luck.

[bkchr]

This is actually not correct. You are right that the benchmarks are not correct the way they are written, but these calls clearly don't have zero weight when SpendOrigin fails. If you look at the functions of the dispatchables, you will see that we access Spends aka reading from the DB which is one of the most expensive operations we have.

[bgallois]

This is actually not correct. You are right that the benchmarks are not correct the way they are written, but these calls clearly don't have zero weight when SpendOrigin fails. If you look at the functions of the dispatchables, you will see that we access Spends aka reading from the DB which is one of the most expensive operations we have.

In fact, these extrinsics should not be callable when SpendOrigin = frame_support::traits::NeverEnsureOrigin<u64>;. In this case, we can authorize weight=0 so that the benchmark can pass for this pallet (or that is the expected behavior I understand from the benchmarks already written).

Another solution is to benchmark the path until failure occurs and use this weight as the default weight, and these extrinsics could be callable even with SpendOrigin = frame_support::traits::NeverEnsureOrigin<u64>;.

[ggwpez]

You are right that the benchmarks are not correct the way they are written, but these calls clearly don't have zero weight when SpendOrigin fails.

Yes true. But he is fixing the case that SpendOrigin is configured to be impossible to pass. There should be no DB reads in that case, right?
Otherwise the try_success_origin for benchmarks is advised to return something that always works by using a feature flag. Although its better to go through the normal setup to also benchmark the effort of the try_success_origin.

[bkchr]

polkadot-sdk/substrate/frame/treasury/src/lib.rs

Lines 812 to 815 in acd043b

	pub fn payout(origin: OriginFor<T>, index: SpendIndex) -> DispatchResult {
	ensure_signed(origin)?;
	let mut spend = Spends::<T, I>::get(index).ok_or(Error::<T, I>::InvalidIndex)?;
	let now = frame_system::Pallet::<T>::block_number();

polkadot-sdk/substrate/frame/treasury/src/lib.rs

Lines 855 to 861 in acd043b

	pub fn check_status(origin: OriginFor<T>, index: SpendIndex) -> DispatchResultWithPostInfo {
	use PaymentState as State;
	use PaymentStatus as Status;
	ensure_signed(origin)?;
	let mut spend = Spends::<T, I>::get(index).ok_or(Error::<T, I>::InvalidIndex)?;
	let now = frame_system::Pallet::<T>::block_number();

polkadot-sdk/substrate/frame/treasury/src/lib.rs

Lines 909 to 911 in acd043b

	pub fn void_spend(origin: OriginFor<T>, index: SpendIndex) -> DispatchResult {
	T::RejectOrigin::ensure_origin(origin)?;
	let spend = Spends::<T, I>::get(index).ok_or(Error::<T, I>::InvalidIndex)?;

As we can see, there are reads. If SpendOrigin isn't set, this prevents that any value is stored in Spends. However, it doesn't prevent that these other functions are doing a read.

[bgallois]

You are right that the benchmarks are not correct the way they are written, but these calls clearly don't have zero weight when SpendOrigin fails.

Yes true. But he is fixing the case that SpendOrigin is configured to be impossible to pass. There should be no DB reads in that case, right? Otherwise the try_success_origin for benchmarks is advised to return something that always works by using a feature flag. Although its better to go through the normal setup to also benchmark the effort of the try_success_origin.

Alright, but in this case, we need to adapt the three benchmarks(check_status, void_spend and payout) that always failed not because of a SpendOrigin check, but because the extrinsic will fail when reading the Spend storage, which will always be empty. For these, the weight will be one signed origin check and then one read to the database.

[bkchr]

Alright, but in this case, we need to adapt the three benchmarks(check_status, void_spend and payout) that always failed not because of a SpendOrigin check,

Yes, I actually wanted this. I should have been more clear :D

[mutantcornholio]

I'm unable to sign the CLA: Content Encoding Error. An error occurred during a connection to cla.parity.io.

Hm, can you try again? Not sure whom to ping here @mutantcornholio maybe?

Thanks for report, I'm on it

[bkchr]

This is not correct. What you should do is to skip adding something to Spend when T::SpendOrigin::try_successful_origin() fails.

So,

		if let Ok(origin) = T::SpendOrigin::try_successful_origin() {
		Treasury::<T, _>::spend(
			origin,
		force_add_spend::<T, _>(
			Box::new(asset_kind.clone()),
			amount,
			Box::new(beneficiary_lookup),
			None,
		)?;
}

is the logic I would expect for these benchmarks below.

[bgallois]

If we omit adding something to the spend, the benchmark will fail during execution with the 'InvalidIndex' error. While it is probably possible to bypass this error using a solution like this, it appears error-prone with an extrinsic that will sometime complete and sometime fail.

As extrinsic benchmark cover worst case scenario, It seems not far-fetched that even with SpendOrigin = frame_support::traits::NeverEnsureOrigin<u64> we take this impossible case.

[bkchr]

I mean we should be able to use the same condition as above to check either on success or error?

CC @ggwpez

[bgallois]

I know that the #[extrinsic_call] cannot be included inside a conditional block.

If there is a way to perform a benchmark with a condition and two expected results, I have never seen it.

[ggwpez]

You can use #[block] and put the if inside:

#[block]
{
    if condition {
        ...
    } else {
        ...
    }
}

Condition should be a bool already to not mess up the timing. Otherwise you can create a new benchmark just for the failing case and then use the maximum of the two as weight.

[bgallois]

The more general case (forcing a spend then processing it) has a weight that is superior to the edge case (one read then an error), thus posing no risk of spamming without overly complicating the benchmark.

Let take the case of:

polkadot-sdk/substrate/frame/treasury/src/lib.rs

Lines 812 to 831 in 8a8f6f9

	pub fn payout(origin: OriginFor<T>, index: SpendIndex) -> DispatchResult {
	ensure_signed(origin)?;
	let mut spend = Spends::<T, I>::get(index).ok_or(Error::<T, I>::InvalidIndex)?;
	let now = frame_system::Pallet::<T>::block_number();
	ensure!(now >= spend.valid_from, Error::<T, I>::EarlyPayout);
	ensure!(spend.expire_at > now, Error::<T, I>::SpendExpired);
	ensure!(
	matches!(spend.status, PaymentState::Pending | PaymentState::Failed),
	Error::<T, I>::AlreadyAttempted
	);
	let id = T::Paymaster::pay(&spend.beneficiary, spend.asset_kind.clone(), spend.amount)
	.map_err(|_| Error::<T, I>::PayoutError)?;
	spend.status = PaymentState::Attempted { id };
	Spends::<T, I>::insert(index, spend);
	Self::deposit_event(Event::<T, I>::Paid { index, payment_id: id });
	Ok(())

The worst-case scenario occurs when a valid spend is found and paid. Therefore, even if SpendOrigin = frame_support::traits::NeverEnsureOrigin<u64>, and in reality, no spend can be added, we can still benchmark the worst case by forcibly adding a spend (what I have done in the last commit).

It is analog to an extrinsic than necessitate a root origin, we write a benchmark using the root origin and always charge the weight of this path. We did not split the benchmark to one with success when root and one with a fail when not root. The only time we split the benchmark is when the extrinsic will always fail and when there is no valid path, that is not the case with payout, check_status, and void_spend, but is it the case for spend, spend_local.

[bkchr]

The worst-case scenario occurs when a valid spend is found and paid.

But this isn't the worst-case when you have SpendOrigin = NeverEnsureOrigin<u64>;. Then the worst-case is basically the best case as well and just means that there is only one read of the storage.

[bgallois]

The worst-case scenario occurs when a valid spend is found and paid.

But this isn't the worst-case when you have SpendOrigin = NeverEnsureOrigin<u64>;. Then the worst-case is basically the best case as well and just means that there is only one read of the storage.

From the extrinsic point of view, it is the worst path because there is no mention of SpendOrigin in its implementation. In all the benchmarks that I found in Substrate, the worst path is determined from the extrinsic perspective, rather than considering all the (sometimes complicated) interference of all the parameters that can play a role outside the extrinsic implementation (and that can be quite complicated to anticipate when writing the benchmark, see this error for example).

I think that the implementation you're asking is suboptimal by adding unnecessary complexity to the benchmark for something that doesn't pose any threat to the chain and just add a slight overweight. However, you seem very committed to this specific approach, so I will move this pull request to draft status and I will implement it according to your suggestions when I have some time.

[bkchr]

Not sure your arguments are correct here. I mean the problem here is that the benchmark is failing because it is written in an incorrect way. The benchmark is "abusing" SpendOrigin to setup the state and the benchmark is failing because SpendOrigin is set to NeverEnsureOrigin. Now if we fix the benchmark, we should do it properly that it generates the correct weights. The thing I'm proposing here is also not really that complicated and can be implemented in some minutes.

[bgallois]

Yes, I agree that the benchmark is failing because it was written incorrectly. In my opinion, this failure directly stems from not approaching it from the extrinsic point of view, as I explained before.

No, it's not complicated. However, since I'm paid to work on another Substrate chain, this fix isn't a priority in my work schedule that is quite full at the moment. Feel free to complete it if you want it done rapidly (i.e. before the end of a week).

[mutantcornholio]

@bgallois could you try signing the CLA again please?

[bgallois]

@bgallois could you try signing the CLA again please?

Thanks it is working :)

[Hugo-Trentesaux]

The thing I'm proposing here is also not really that complicated and can be implemented in some minutes.

@bkchr, those minutes transformed into months and the benchmarks are still broken.

If you are still interested by a fix, we have it on our fork: https://github.com/duniter/duniter-polkadot-sdk/commits/duniter-substrate-v1.14.0/ (latest commit here).

[bkchr]

@Hugo-Trentesaux I have done now the changes as I had requested.

[bgallois]

Thank you for the fix.

There is still a small attack vector if a chain is improbably (mis)configured with SpendOrigin set to NeverOrigin, but RejectOrigin can be successful. In this case, the code from remove_approval could be executed without any weight at all.

[bkchr]

Ty! #5713

[paritytech-cmd-bot-polkadot-sdk]

Git push to origin failed for stable2407 with exitcode 1

[paritytech-cmd-bot-polkadot-sdk]

Successfully created backport PR for stable2409:

• [stable2409] Backport #3049 #5730

[paritytech-cmd-bot-polkadot-sdk]

Created backport PR for stable2407:

• [stable2407] Backport #3049 #5734 with remaining conflicts!

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-3049-to-stable2407
git worktree add --checkout .worktree/backport-3049-to-stable2407 backport-3049-to-stable2407
cd .worktree/backport-3049-to-stable2407
git reset --hard HEAD^
git cherry-pick -x 51f336711a0391987db69d6281c9b57bfe49d925
git push --force-with-lease

[paritytech-cmd-bot-polkadot-sdk]

Git push to origin failed for stable2409 with exitcode 1