Refactor OnChargeEVMTransaction

[icodezjb]

No description provided.

[tgmichel]

I don't understand this change. The priority fee is never subtracted twice from the sender, and is up to the pay_priority_fee implementor to whether tip it to the block author or not - the later meaning is just burned.

If we completely remove pay_priority_fee, there is no optional tip to the author.

For example:

+--------+----------------+---------+
| MaxFee | MaxPriorityFee | BaseFee |
+--------+----------------+---------+
|     20 |             10 |      15 |
+--------+----------------+---------+

+----------------+-----------------------+------+--------+--------+
| ActualPriority | ActualFee (withdrawn) | Burn | Tipped | Refund |
+----------------+-----------------------+------+--------+--------+
|              5 |                    20 |   15 |      5 |      0 |
+----------------+-----------------------+------+--------+--------+

In this case, 20 is deducted initially from the sender. Then 15 burned, 5 tipped and 0 refunded.

In this same case, if the implementation for pay_priority_fee is no-op, 20 is burned instead.

+--------+----------------+---------+
| MaxFee | MaxPriorityFee | BaseFee |
+--------+----------------+---------+
|     20 |             10 |       5 |
+--------+----------------+---------+

+----------------+-----------------------+------+--------+--------+
| ActualPriority | ActualFee (withdrawn) | Burn | Tipped | Refund |
+----------------+-----------------------+------+--------+--------+
|             10 |                    15 |    5 |     10 |      5 |
+----------------+-----------------------+------+--------+--------+

In this other case, 20 is deducted initially from the sender. Then 5 burn, 10 tipped and 5 refunded.

In this same case, if the implementation for pay_priority_fee is no-op, 15 is burned instead and 5 still refunded.

[icodezjb]

I don't understand this change. The priority fee is never subtracted twice from the sender, and is up to the pay_priority_fee implementor to whether tip it to the block author or not - the later meaning is just burned.

If we completely remove pay_priority_fee, there is no optional tip to the author.

For example:

+--------+----------------+---------+
| MaxFee | MaxPriorityFee | BaseFee |
+--------+----------------+---------+
|     20 |             10 |      15 |
+--------+----------------+---------+

+----------------+-----------------------+------+--------+--------+
| ActualPriority | ActualFee (withdrawn) | Burn | Tipped | Refund |
+----------------+-----------------------+------+--------+--------+
|              5 |                    20 |   15 |      5 |      0 |
+----------------+-----------------------+------+--------+--------+

In this case, 20 is deducted initially from the sender. Then 15 burned, 5 tipped and 0 refunded.

In this same case, if the implementation for pay_priority_fee is no-op, 20 is burned instead.

+--------+----------------+---------+
| MaxFee | MaxPriorityFee | BaseFee |
+--------+----------------+---------+
|     20 |             10 |       5 |
+--------+----------------+---------+

+----------------+-----------------------+------+--------+--------+
| ActualPriority | ActualFee (withdrawn) | Burn | Tipped | Refund |
+----------------+-----------------------+------+--------+--------+
|             10 |                    15 |    5 |     10 |      5 |
+----------------+-----------------------+------+--------+--------+

In this other case, 20 is deducted initially from the sender. Then 5 burn, 10 tipped and 5 refunded.

In this same case, if the implementation for pay_priority_fee is no-op, 15 is burned instead and 5 still refunded.

(1) Your examples are all correct.
(2) In the function correct_and_deposit_fee:

case 1: 20 is deducted initially from the sender. 0 refunded, 15 burned + 5 tipped = adjusted_paid
case 2: 20 is deducted initially from the sender. 5 refunded, 5 burned + 10 tipped = adjusted_paid

For the substrate blockchain which total issuance is fixed, and the upper implementation of OU::on_unbalanced(adjusted_pa​​id) is reward to block author, the tip has been processed.
if call pay_priority_fee after correct_and_deposit_fee, the total issuance will be increase.

fn correct_and_deposit_fee(
		who: &H160,
		corrected_fee: U256,
		already_withdrawn: Self::LiquidityInfo,
	) {
		if let Some(paid) = already_withdrawn {
			let account_id = T::AddressMapping::into_account_id(*who);

			// Calculate how much refund we should return
			let refund_amount = paid
				.peek()
				.saturating_sub(corrected_fee.low_u128().unique_saturated_into());
			// refund to the account that paid the fees. If this fails, the
			// account might have dropped below the existential balance. In
			// that case we don't refund anything.
			let refund_imbalance = C::deposit_into_existing(&account_id, refund_amount)
				.unwrap_or_else(|_| C::PositiveImbalance::zero());

			// Make sure this works with 0 ExistentialDeposit
			// https://github.com/paritytech/substrate/issues/10117
			// If we tried to refund something, the account still empty and the ED is set to 0,
			// we call `make_free_balance_be` with the refunded amount.
			let refund_imbalance = if C::minimum_balance().is_zero()
				&& refund_amount > C::Balance::zero()
				&& C::total_balance(&account_id).is_zero()
			{
				// Known bug: Substrate tried to refund to a zeroed AccountData, but
				// interpreted the account to not exist.
				match C::make_free_balance_be(&account_id, refund_amount) {
					SignedImbalance::Positive(p) => p,
					_ => C::PositiveImbalance::zero(),
				}
			} else {
				refund_imbalance
			};

			// merge the imbalance caused by paying the fees and refunding parts of it again.
			let adjusted_paid = paid
				.offset(refund_imbalance)
				.same()
				.unwrap_or_else(|_| C::NegativeImbalance::zero());
			OU::on_unbalanced(adjusted_paid);
		}
	}

Maybe correct_and_deposit_fee should be fixed.

[tgmichel]

the upper implementation of OU::on_unbalanced(adjusted_pa​​id) is reward to block author, the tip has been processed.

I see, then the issue is not on the default OnChargeEVMTransaction implementation or the runner code - which assumes the block author tipping is done through T::OnChargeTransaction::pay_priority_fee -, but rather the OnUnbalanced implementor that you are using for pallet-evm's OnChargeTransaction associated type in your runtime.

[icodezjb]

@tgmichel please review again

[tgmichel]

Thank you @icodezjb .

Although I understand your problem and the change you are proposing, I still fail to see the difference or the value added comparing to what is currently implemented.

Let me reiterate, you can achieve what you want at the runtime level. In your project, if you want pay_priority_fee to be no-op - because you handle the tip somewhere else or you just wish to burn it - you can have your custom OnChargeEVMTransaction (see Moonbeam).

[icodezjb]

Archive this PR.
(1) if base_fee and tip are both burned or reward to block author, see Moonbeam
(2) if with fixed total issuance, base_fee to be burned, but tip reward to block author, see this PR.