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

Account for transaction priority when enforcing limits #10388

Merged
merged 3 commits into from
Dec 2, 2021
Merged

Account for transaction priority when enforcing limits #10388

merged 3 commits into from
Dec 2, 2021

Conversation

nazar-pc
Copy link
Contributor

@nazar-pc nazar-pc commented Nov 29, 2021 •
edited by bkchr
Loading

This extends logic used when enforcing limits on transaction pool by not only accounting for when transaction was inserted, but also what priority it has.

The logic here is that if transaction has higher priority (let's say equivocation report that has highest possible priority configured in BABE), it must not be possible to kick it out of transaction pool with transactions that have lower priority.

Polkadot address: 1vSxzbyz2cJREAuVWjhXUT1ds8vBzoxn2w4asNpusQKwjJd

skip check-dependent-cumulus

bkchr
bkchr reviewed Nov 29, 2021
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Could you please add a test?

@nazar-pc
Copy link
Contributor Author

It doesn't look like there were tests for previous behavior to replicate, but I will look into adding some tomorrow.

@bkchr
Copy link
Member

bkchr commented Nov 29, 2021

It doesn't look like there were tests for previous behavior to replicate, but I will look into adding some tomorrow.

Could be, but that just emphasis more that we should have tests ;)

@nazar-pc
Copy link
Contributor Author

Added a basic test, but there is an opportunity to write a bunch more for various scenarios

@bkchr
Copy link
Member

bkchr commented Nov 29, 2021

Ty!

@bkchr
Copy link
Member

bkchr commented Nov 30, 2021

Could you please merge master

@nazar-pc nazar-pc force-pushed the enforce-limits-account-for-priority branch from 8347af9 to b3a2849 Compare November 30, 2021 08:50
@nazar-pc nazar-pc requested a review from bkchr November 30, 2021 10:36
@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Nov 30, 2021
bkchr
bkchr approved these changes Nov 30, 2021
View reviewed changes
client/transaction-pool/src/graph/pool.rs Outdated Show resolved Hide resolved
client/transaction-pool/src/graph/base_pool.rs Outdated Show resolved Hide resolved
client/transaction-pool/src/graph/pool.rs Outdated Show resolved Hide resolved
@bkchr bkchr requested a review from tomusdrw November 30, 2021 10:44
@nazar-pc nazar-pc force-pushed the enforce-limits-account-for-priority branch from b3a2849 to 6011a11 Compare November 30, 2021 11:06
@bkchr bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Nov 30, 2021
tomusdrw
tomusdrw approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm!

client/transaction-pool/src/graph/base_pool.rs Outdated Show resolved Hide resolved
client/transaction-pool/src/graph/base_pool.rs Outdated
Comment on lines 387 to 388
/// set. We use a simplified approach to remove the transaction that occupies the pool for the
/// longest time.
/// longest time or has the lowest priority.
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually please update the comment, cause we now prefer transaction with the lowest priority and only resolve to insertion_id in case the priority is exactly the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can remember why it wasn't done like that initially, my only guess is that self.ready.fold did not traverse the entire set of transactions but rather just a subset (i.e. first-in-row transactions), but now it seems it will go through the list of all possible transactions anyway (which is a bit of a performance hurdle here, when used in the while loop).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same concern about performance, but wasn't sure if it is save to sort txs first and then slicing off some of the worst ones until we are withing limits (instead of having quadratic algorithm here) since self.remove_subtree() below might remove more than one by the looks of it and I wasn't familiar enough with the codebase to make such change yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, in regular circumstances the outer loop should complete quite fast - like we will be removing up to couple of transactions at most, so I think this is fine.

@tomusdrw
Copy link
Contributor

/tip medium

@substrate-tip-bot
Copy link

Please fix the following problems before calling the tip bot again:

  • Contributor did not properly post their Polkadot or Kusama address. Make sure the pull request has: "{network} address: {address}".

@nazar-pc
Copy link
Contributor Author

/tip medium

Thanks, added address 😺

@bkchr
Copy link
Member

bkchr commented Nov 30, 2021

/tip medium

1 similar comment
@bkchr
Copy link
Member

bkchr commented Dec 2, 2021

/tip medium

@substrate-tip-bot
Copy link

A medium tip was successfully submitted for nazar-pc (1vSxzbyz2cJREAuVWjhXUT1ds8vBzoxn2w4asNpusQKwjJd on polkadot).

https://polkadot.js.org/apps/#/treasury/tips

@tomusdrw
Copy link
Contributor

tomusdrw commented Dec 2, 2021

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Checks failed for 368f06a

@bkchr
Copy link
Member

bkchr commented Dec 2, 2021

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 40f2483 into paritytech:master Dec 2, 2021
@nazar-pc nazar-pc deleted the enforce-limits-account-for-priority branch January 15, 2022 23:39
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@nazar-pc @grishasobol
Account for transaction priority when enforcing limits (paritytech#10388
fe2b78e 
)

* Account for transaction priority when enforcing limits

* Improve `enforce_limits` comment

* Explanation comment on why comparison impl is not used for limit enforcement
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@nazar-pc @ark0f
Account for transaction priority when enforcing limits (paritytech#10388
066e742 
)

* Account for transaction priority when enforcing limits

* Improve `enforce_limits` comment

* Explanation comment on why comparison impl is not used for limit enforcement
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tomusdrw tomusdrw tomusdrw approved these changes

@bkchr bkchr bkchr approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nazar-pc @bkchr @tomusdrw