[pallet_transaction_payment]: Share withdrawn tx fee credit with other pallets #9780

athei · 2025-09-18T23:09:15Z

Replaces #9590.

The audit of #9590 showed that holding the txfee as held balance and especially playing around with providers causes a lot of troubles.

This PR is a much lighter change. It keeps the original withdraw/deposit pattern. It simply stores the withdrawn Credit and allows other pallets to withdraw from it.

It is also better in terms of performance since all tx signers share a single storage item (instead of a named hold per account).

kianenigma

First glance note: This can render trait EstimateCallFee useless, as we can inspect the new storage item introduced here.

joepetrowski · 2025-09-19T05:23:19Z

substrate/frame/transaction-payment/src/lib.rs

+	/// overestimated. Drawing too much balance will cause the signer to underpay for the
+	/// transaction. Too much means that not enough is left to pay for the fee with regard


?

Suggested change

/// overestimated. Drawing too much balance will cause the signer to underpay for the

/// transaction. Too much means that not enough is left to pay for the fee with regard

/// overestimated. Drawing too much balance will cause the signer to overpay for the

/// transaction. Too little means that not enough is left to pay for the fee with regard

no I think it's correct, if you draw too much then there is not enough left to pay for what was meant to be used for the tx fee

I think it is correct. The signer underpays for the tx because the balance was taken away to pay for something else.

substrate/frame/transaction-payment/src/payment.rs

athei · 2025-09-19T06:26:29Z

It had a nasty bug which I fixed the last commt: Putting a Imbalance in storage will call its Drop implementation. Which reduces the issuance. Which we don't want. It is not really dropped logically. It will be restored later. So I worked around it by adding a new NoDrop<T> type that we can use to put T into storage without calling its Drop impl.

Co-authored-by: joe petrowski <[email protected]>

TorstenStueber

Maybe not for now: the logic to fail the transaction when too much of the credit has been drawn could be implemented in pallet-transaction-pallet instead of the pallet implementing the transaction logic: it could be a new function in the TransactionExtension trait with a no-op blanket implementation that is called before post_dispatch here and where dispatch_transaction changes res to an Error depending on the return value of that function. The code change would be minimal (apart from requiring new benchmarks).

TorstenStueber · 2025-09-22T13:11:38Z

substrate/frame/transaction-payment/src/lib.rs

 			NextFeeMultiplier::<T>::mutate(|fm| {
 				*fm = T::FeeMultiplierUpdate::convert(*fm);
 			});
+			TxPaymentCredit::<T>::kill();


Why is this required? At this point the value should always be None, correct? If not, then we would also need to drop the imbalance properly here.

Or is it just not to use any storage in between blocks?

Why is this required? At this point the value should always be None, correct?

Yes, the expectation is that it is always None here. We just kill to make sure the value is never carried between blocks.

If not, then we would also need to drop the imbalance properly here.

We cannot know for sure as the behavior is defined by the OnChargeTransaction. So yes, we should make sure to call the drop impl here. Will fix.

TorstenStueber · 2025-09-22T17:11:49Z

substrate/frame/transaction-payment/src/lib.rs

+	///
+	/// # Warning
+	///
+	/// This is only useful if a pallet knows that the pre-dispatch weight was vastly


I think that the condition here is more subtle: the pallet is allowed to draw too much balance but would need to fail the transaction if that is the case (this more relaxed condition is required for pallet-revive).

It is important that in this case any effects that the balance was drawn for during execution (e.g., storage deposits) are reverted – in order to protect against DoS.

You are right. This is what I meant by that but it should be expressed explicitly. Will change the text.

Improved the docs.

athei · 2025-09-23T10:32:16Z

Maybe not for now: the logic to fail the transaction when too much of the credit has been drawn could be implemented in pallet-transaction-pallet instead of the pallet implementing the transaction logic: it could be a new function in the TransactionExtension trait with a no-op blanket implementation that is called before post_dispatch here and where dispatch_transaction changes res to an Error depending on the return value of that function. The code change would be minimal (apart from requiring new benchmarks).

Yes will check if possible. The runtime impact will consist of an additional storage transaction in order to be able to roll back the changes done by the tx.

But should be done in a follow up. Let's keep this PR minimal as it is already touching a lot of critical stuff.

athei · 2025-09-23T11:11:40Z

@joepetrowski @TorstenStueber I improved the docs on withdraw_txfee. Please check if it makes sense to you now.

athei · 2025-09-23T16:40:52Z

cc @jakoblell

Applied the variable renaming suggestions you sent me. I applied them as-is with one exception: I renamed Pre::Charge::tip_credit to Pre::Charge::liquidity_info. Reasoning is that this type is opaque and the impl can pass whatever they want there. For example, the CurrencyAdapter does not pass the tip_credit there.

paritytech-workflow-stopper · 2025-09-23T16:43:02Z

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

athei · 2025-09-23T16:43:38Z

/cmd fmt

Make credit available to pallets

63a2e4b

athei requested a review from a team as a code owner September 18, 2025 23:09

athei mentioned this pull request Sep 18, 2025

Change fee collection to make use of hold and release pattern #9590

Closed

kianenigma reviewed Sep 19, 2025

View reviewed changes

joepetrowski reviewed Sep 19, 2025

View reviewed changes

substrate/frame/transaction-payment/src/payment.rs Outdated Show resolved Hide resolved

Add NoDrop

bb42047

rachsrl added this to Security Audit (PRs) - SRLabs Sep 19, 2025

github-project-automation bot moved this to Backlog in Security Audit (PRs) - SRLabs Sep 19, 2025

rachsrl moved this from Backlog to Scheduled in Security Audit (PRs) - SRLabs Sep 19, 2025

pgherveou approved these changes Sep 19, 2025

View reviewed changes

This was referenced Sep 22, 2025

WIP: Rework gas mapping #9803

Open

DNM: PoC for SwapAssetAdapter to use hold and release pattern #9657

Closed

Update substrate/frame/transaction-payment/src/payment.rs

dc41453

Co-authored-by: joe petrowski <[email protected]>

TorstenStueber reviewed Sep 22, 2025

View reviewed changes

athei added 2 commits September 23, 2025 13:23

Call drop impl when deleting TxPaymentCredit

bf636fe

Improved docs of withdraw_txfee

6156ddd

rachsrl moved this from Scheduled to In progress in Security Audit (PRs) - SRLabs Sep 23, 2025

Apply renaming suggestions by Jacob

ad11751

Update from github-actions[bot] running command 'fmt'

7e83f5c

		/// overestimated. Drawing too much balance will cause the signer to underpay for the
		/// transaction. Too much means that not enough is left to pay for the fee with regard

[pallet_transaction_payment]: Share withdrawn tx fee credit with other pallets #9780

Are you sure you want to change the base?

[pallet_transaction_payment]: Share withdrawn tx fee credit with other pallets #9780

Uh oh!

Conversation

athei commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

joepetrowski Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

pgherveou Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athei Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

athei commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TorstenStueber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TorstenStueber Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athei Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

TorstenStueber Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

athei Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athei Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

athei commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athei commented Sep 23, 2025

Uh oh!

athei commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paritytech-workflow-stopper bot commented Sep 23, 2025

Uh oh!

athei commented Sep 23, 2025

Uh oh!

Uh oh!

athei commented Sep 18, 2025 •

edited

Loading

pgherveou Sep 19, 2025 •

edited

Loading

athei commented Sep 19, 2025 •

edited

Loading

TorstenStueber left a comment •

edited

Loading

TorstenStueber Sep 22, 2025 •

edited

Loading

athei Sep 23, 2025 •

edited

Loading

athei commented Sep 23, 2025 •

edited

Loading

athei commented Sep 23, 2025 •

edited

Loading