Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply_authorized_upgrade: Implement feeless_if #7812

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Mar 5, 2025

This pr implements the feeless_if feature for apply_authorized_upgrade. The call would be feeless if the given code is authorized.

Besides that the pr is doing some clean ups.

This pr implements the `feeless_if` feature for `apply_authorized_upgrade`. The call would be feeless if the given code is authorized.

Besides that the pr is doing some clean ups.
@bkchr bkchr added the T2-pallets This PR/Issue is related to a particular pallet. label Mar 5, 2025
@bkchr bkchr requested a review from a team as a code owner March 5, 2025 21:59
@bkchr
Copy link
Member Author

bkchr commented Mar 5, 2025

/cmd prdoc --audience runtime_user --bump major

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13686410325
Failed job name: run-frame-omni-bencher

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13686410367
Failed job name: test-linux-stable-runtime-benchmarks

@@ -859,12 +860,25 @@ pub mod pallet {
/// All origins are allowed.
#[pallet::call_index(11)]
#[pallet::weight((T::SystemWeightInfo::apply_authorized_upgrade(), DispatchClass::Operational))]
#[pallet::feeless_if(|_: &OriginFor<T>, code: &Vec<u8>| -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Finally, a real usage example. I will also adjust my PR accordingly.

But iiuc, feeless_if won't work unless we add SkipCheckIfFeeless and pallet_skip_feeless_payment to the runtimes, right? Please see my related questions here: #7592 (comment).

So, iiuc, feeless_if won't have any effect without SkipCheckIfFeeless, but in this case, execution will still be free because of pays_fee: Pays::No, correct?

Next, if we add SkipCheckIfFeeless and pallet_skip_feeless_payment, and we change DispatchResultWithPostInfo to DispatchResult, then we no longer need the PostDispatchInfo { pays_fee: Pays::No, ... } code, right? (Probably because of prevent further transactions we want to keep it as it is here)

And lastly, when we make feeless_if + SkipCheckIfFeeless work with TransactionExtensions, how does it behave in the case of an extrinsic error? For example, if feeless_if passes and returns true, but fn apply_authorized_upgrade(..) fails on Self::can_set_code(..), will the user still be charged, or will it remain free?

Copy link
Contributor

Choose a reason for hiding this comment

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

But iiuc, feeless_if won't work unless we add SkipCheckIfFeeless and pallet_skip_feeless_payment to the runtimes, right? Please see my related questions here: #7592 (comment).

Yes feeless_if is effective only if SkipCheckIfFeeless is used, usually wrapping the transaction payment extension.

So, iiuc, feeless_if won't have any effect without SkipCheckIfFeeless, but in this case, execution will still be free because of pays_fee: Pays::No, correct?

It would be free only if successful, people will have to pay for it, and they get refunded at the end of the transaction if is successful.

Next, if we add SkipCheckIfFeeless and pallet_skip_feeless_payment, and we change DispatchResultWithPostInfo to DispatchResult, then we no longer need the PostDispatchInfo { pays_fee: Pays::No, ... } code, right? (Probably because of prevent further transactions we want to keep it as it is here)

Yes, but it doesn't harm.

And lastly, when we make feeless_if + SkipCheckIfFeeless work with TransactionExtensions, how does it behave in the case of an extrinsic error? For example, if feeless_if passes and returns true, but fn apply_authorized_upgrade(..) fails on Self::can_set_code(..), will the user still be charged, or will it remain free?

if feeless_if passes, the user is never charged, (the transaction payment happens in the transaction extension in prepare phase, this payment will be skipped).
So if the transaction fails the changes in the call dispatch is reverted. So probably people will be able to spam a failing transaction for free.
Signer will not be charged after the failing of the transaction.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gui1117 Thank you :)

So if the transaction fails the changes in the call dispatch is reverted. So probably people will be able to spam a failing transaction for free.
Signer will not be charged after the failing of the transaction.

This is exactly what I was worried about :), but we probably just need to ensure that feeless_if is correct and covers all the necessary validations, right?

Also, another thing, here we are using/expecting an unsigned call: ValidateUnsigned::validate_unsigned(Call::apply_authorized_upgrade(..)). So, is this the spam protection mechanism that it doesn’t even add call to the transaction pool?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants