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

Rework Transaction Priority calculation #9834

Merged
19 commits merged into from
Oct 4, 2021
Merged

Rework Transaction Priority calculation #9834

19 commits merged into from
Oct 4, 2021

Conversation

tomusdrw
Copy link
Contributor

@tomusdrw tomusdrw commented Sep 22, 2021

Closes #5672
Closes #9317

Alternative take on #9596
(we can leave the adjuster from previous PR, but I don't think it is going to be useful at all, given no extensions change priority).

This PR changes the way Transaction Priority is calculated by following SignedExtensions:

  1. CheckWeight
  2. ChargeTransactionPayment

For CheckWeight it removes the priority calculation completely, leaving the priority as 0 for all dispatch classes. Please note that this might cause a bit of an havoc on existing chains without any extra signed extension that would alter priority (which is discouraged anyway) - all transactions would end up having the same priority (0) and the inclusion might be done at random (i.e. depending on the transaction pool ordering - currently based on the insertion time), note that the requirements (i.e. nonce) would still be respected though.

ChargeTransactionPayment is completely reworked and is now a single place responsible for calculating the overall priority (all other SignedExtensions in this repo do not alter it).
The priority depends on the dispatch class and the amount of tip-per-weight or tip-per-length (whatever is more limiting) the sender is willing to pay.
Transactions without a tip are using a minimal tip value of 1 for priority calculations to make sure we don't end up with all transactions with 0 priority. The consequence of this is that "smaller" transactions are more preferred than the "large" ones (note the behavior used to be similar with CheckWeight's prioriy).

Operational transactions are getting additional priority bump - not as high as previously (3/4 * u64::max), because that prevents other transactions from getting in front of them in the pool, but now equal to the OperationalVirtualTip amount configurable by a rutnime developer.

Companion PR: paritytech/polkadot#3901

@tomusdrw tomusdrw added A0-please_review Pull request needs code review. B7-runtimenoteworthy C3-medium PR touches the given topic and has a medium impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Sep 22, 2021
tomusdrw and others added 2 commits September 22, 2021 11:44
Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

The PR seems fine. The prioritization makes sense.
I'm just worried that this will have weird unintended consequences for downstream chains using it.
Any way we can mitigate that? Any tests we could run to see whether the behavior is sane? Could we run a few validator nodes of e.g. Kusama with this changed priority? (I know that these are runtime-level changes and thus hard to test with a canary node.)

@tomusdrw
Copy link
Contributor Author

@apopiak Please take a look once again, I've reworked the calculation after audit feedback.

I'm just worried that this will have weird unintended consequences for downstream chains using it.

This is a runtime change, so it will be intentional to upgrade to a new version. Note that CheckWeight extension was never sufficient to run a secure chain (actually it was even harmful), so I'm not sure if there is much more we can do than clearly state that in the release notes.

Any tests we could run to see whether the behavior is sane? Could we run a few validator nodes of e.g. Kusama with this changed priority?

We would need to define "sane" first :) Block authors are free to put transactions into blocks in whatever order they want, the priority is just helping them to maximize their outcomes. We have tests and audits to ensure QA and I'd argue that this change is strictly better than what we have currently deployed (mutliple extensions contributing to priority with incorrect orders of magnitude, hence preventing proper tip usage).

bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
/// range (might simply end up being zero). Hence we use a scaling factor:
/// `tip * (max_block_{weight|length} / bounded_{weight|length})`, since given current
/// state of-the-art blockchains, number of per-block transactions is expected to be in a
/// range reasonable enough to not saturate the `Balance` type while multiplying by the tip.
Copy link

@gww-parity gww-parity Sep 29, 2021

Choose a reason for hiding this comment

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

What about adding some assert in a code to ensure assumptions? (i.e. that values are in expected ranges, preventing saturation)

@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Sep 30, 2021
bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@tomusdrw
Copy link
Contributor Author

tomusdrw commented Oct 4, 2021

bot merge

@ghost
Copy link

ghost commented Oct 4, 2021

Trying merge.

@ghost ghost merged commit a30e1b2 into master Oct 4, 2021
@ghost ghost deleted the tdpriority2 branch October 4, 2021 14:25
s3krit pushed a commit that referenced this pull request Oct 5, 2021
* Add transaction validity docs.

* Re-work priority calculation.

* Fix tests.

* Update frame/transaction-payment/src/lib.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

* cargo +nightly fmt --all

* Fix an obvious mistake :)

* Re-work again.

* Fix test.

* cargo +nightly fmt --all

* Make VirtualTip dependent on the transaction size.

* cargo +nightly fmt --all

* Update frame/transaction-payment/src/lib.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

* Fix compilation.

* Update bin/node/runtime/src/lib.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
ordian added a commit that referenced this pull request Oct 5, 2021
* master: (125 commits)
  Update multiple dependencies (#9936)
  Speed up timestamp generation when logging (#9933)
  First word should be Substrate not Polkadot (#9935)
  Improved file not found error message (#9931)
  don't read events in elections anymore. (#9898)
  Remove incorrect sanity check (#9924)
  Require crypto scheme for `insert-key` (#9909)
  chore: refresh of the substrate_builder image (#9808)
  Introduce block authorship soft deadline (#9663)
  Rework Transaction Priority calculation (#9834)
  Do not propagate host RUSTFLAGS when checking for WASM toolchain (#9926)
  Small quoting comment fix (#9927)
  add clippy to CI (#9694)
  Ensure BeforeBestBlockBy voting rule accounts for base (#9920)
  rm `.maintain` lock (#9919)
  Downstream `node-template` pull (#9915)
  Implement core::fmt::Debug for BoundedVec (#9914)
  Quickly skip invalid transactions during block authorship. (#9789)
  Add SS58 prefix for Automata (#9805)
  Clean up sc-peerset (#9806)
  ...
This pull request was closed.
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. C3-medium PR touches the given topic and has a medium 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.

Rework Transaction Priority Calculation Update how transaction priority is affected by different factors
5 participants