Timestamp inherent doesn't pay fees

[JoshOrndorff]

This PR marks pallet timestamp's set extrinsic as Pays::No. Inherents never pay fees because there is no origin and in general the block author is not associated with a runtime account. Therefore this makes no changes in terms of actual fee collection. Rather this just makes it easier to do offchain accounting work to track fees by block.

Should this be handled somewhere deeper in FRAME so it applies to all inherents?

cc @crystalin

[thiolliere]

this makes sending a signed extrinsic with the call set free, which is not good, even if it will fails directly it can still fill some block for free

[thiolliere]

if we want to use Pays::No then we need some validation code which makes the call invalid (invalid so it can't be included in a block).

sidenote: Maybe we could improve on this using some pallet::inherent attribute which would automatically generate some validation code which makes the extrinsic invalid but this is not easy.

Rather this just makes it easier to do offchain accounting work to track fees by block.

maybe the best is to make a more precise calculation ?

[crystalin]

I'm in favor of not putting pay fee to yes if there is no fee paid.

[thiolliere]

I'm in favor of not putting pay fee to yes if there is no fee paid.

if somebody send a signed extrinsic with the call timestamp::set then it will pay fees. I think it important that this extrinsic is not free otherwise somebody can spam the chain for free

EDIT: if we want to make the extrinsic free, then there should be code to ensure the extrinsic is invalid and can't go into a block.

[crystalin]

@thiolliere I understand this part and agree. Isn't there a way to change how the Extrinsic event reports if the fee are being paid in that case?
I don't think there is an easy way to know otherwise if the fees have been paid or not

[JoshOrndorff]

What if we mark it as Pays::No in post-dispatch phase? That would address @thiolliere 's concerns.

Would that help you @crystalin ? Could you tell us in more detail, why this would help you?

[crystalin]

The use case is simply to list all the collected fees of block.

[JoshOrndorff]

I've updated this PR to mark the extrinsic as Pays::No in the post-dispatch phase. What do you think @thiolliere ?

[bkchr]

The use case is simply to list all the collected fees of block.

How you are doing that currently?

[crystalin]

@bkchr I take all the extrinsics in the block, filter those with Pay::Yes and api.rpc.payment.queryInfo on them.
(I also filter the ethereum one which are computed differently, and if I want to skip the timestamp I also filter non Mandatory)

[thiolliere]

It seems we should fix transaction-payment instead.
transaction-payment withdraw 0 for unsigned transactions but return some fee in the implementation of the method query_info.

Instead it should return the correct fee depending on whereas the extrinsic is signed, so something roughly like this:

diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs
index e3a3bccc3d..5b85ad1f47 100644
--- a/frame/transaction-payment/src/lib.rs
+++ b/frame/transaction-payment/src/lib.rs
@@ -377,13 +377,16 @@ where
        ///
        /// All dispatchables must be annotated with weight and will have some fee info. This function
        /// always returns.
-       pub fn query_info<Extrinsic: GetDispatchInfo>(
+       pub fn query_info<Extrinsic: GetDispatchInfo + sp_runtime::traits::Extrinsic>(
                unchecked_extrinsic: Extrinsic,
                len: u32,
        ) -> RuntimeDispatchInfo<BalanceOf<T>>
        where
                T::Call: Dispatchable<Info = DispatchInfo>,
        {
+               if unchecked_extrinsic.is_signed().map_or(false, |is_signed| is_signed == false) {
+                       return 0;
+               }
                // NOTE: we can actually make it understand `ChargeTransactionPayment`, but would be some
                // hassle for sure. We have to make it aware of the index of `ChargeTransactionPayment` in
                // `Extra`. Alternatively, we could actually execute the tx's per-dispatch and record the
@@ -405,6 +408,9 @@ where
        where
                T::Call: Dispatchable<Info = DispatchInfo>,
        {
+               if unchecked_extrinsic.is_signed().map_or(false, |is_signed| is_signed == false) {
+                       return 0;
+               }
                let dispatch_info = <Extrinsic as GetDispatchInfo>::get_dispatch_info(&unchecked_extrinsic);
                Self::compute_fee_details(len, &dispatch_info, 0u32.into())
        }

[bkchr]

@thiolliere looks like the correct way to do it.

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

[thiolliere]

closed in favor of #10107