Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Allow adjusting priority coming from Signed Extensions #9596

Closed
wants to merge 13 commits into from

Conversation

tomusdrw
Copy link
Contributor

@tomusdrw tomusdrw commented Aug 20, 2021

Closes #5672

The PR introduces AdjustPriority wrapper for SignedExtension. The motivation for introducing this is defined in the original issue. Basically we want to make sure that the priorities we add (coming from different Signed Extensions) are in reasonable order of magnitude.

The idea is to divide the priority of the CheckWeight so that instead of being in [0..MAX_BLOCK_WEIGHT] (for Normal transactions) range it's in [0..10] range.

The idea is to reduce the priority of the CheckWeight extension down to 0.

@tomusdrw tomusdrw added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Aug 20, 2021
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

the code looks great, but I am still struggling to wrap my head around the 'why' of this.

Now, the priority is fee/weight ratio returned from TransactionPayment, which is some large-ish value. It seems like the range is too small to have any effect next to that, so we might as well make the priority of CheckWeight be 0. To rephrase, I am not sure where the range [0..10] comes from and why it is chosen. I presume each chain can choose their own range, but it would be good if we can give a guideline of what rationale they should use to choose the range.

@tomusdrw
Copy link
Contributor Author

Now, the priority is fee/weight ratio returned from TransactionPayment, which is some large-ish value.

Not large enough currently on some chains. The required tip to bump priority might be unreasonably high compared to the priority coming from CheckWeight (pure weight).

To rephrase, I am not sure where the range [0..10] comes from and why it is chosen.

The range is indeed arbitrary, just to have some differentiation between transactions. We can easily scale down by MAX_BLOCK_WEIGHT + 1 to make sure that Normal transactions are always 0. However the CheckWeight priority is still necessary to make sure that Operational and Mandatory transactions are prioritised correctly. Will fix.

it would be good if we can give a guideline of what rationale they should use to choose the range.

Indeed. Basically in case the ChargeTransactionPayment extension is added the original priority for Normal transactions coming from CheckWeight can be easily turned off (i.e. scaled down to 0), since the former takes into account all: weigh&tlength (fee) and the tip.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM, but needs a companion to apply sensible values to polkadot runtimes as well.

@tomusdrw
Copy link
Contributor Author

tomusdrw commented Aug 30, 2021

@kianenigma added the companion: paritytech/polkadot#3743

Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

Looks good to me. We're excited to get this one on Moonriver.

bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
JoshOrndorff added a commit to moonbeam-foundation/substrate that referenced this pull request Sep 16, 2021
@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 16, 2021
@tomusdrw
Copy link
Contributor Author

Closing in favour of #9834

@tomusdrw tomusdrw closed this Sep 22, 2021
@tomusdrw tomusdrw deleted the tdpriority branch September 22, 2021 09:49
JoshOrndorff added a commit to moonbeam-foundation/substrate that referenced this pull request Sep 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update how transaction priority is affected by different factors
6 participants