-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
go: Add consensus transaction prioritization #4911
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4911 +/- ##
==========================================
- Coverage 66.64% 66.61% -0.03%
==========================================
Files 464 464
Lines 51204 51218 +14
==========================================
- Hits 34127 34121 -6
- Misses 12891 12904 +13
- Partials 4186 4193 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
dd9d1d4
to
3964c45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have some tests that these priorities work correctly?
It would probably be better if these priorities would be emitted similar to how gas stuff is done. E.g. there being some SetPriority in the context that one could query via GetPriority during CheckTx. Then priority could be configured as part of the regular execution flow instead of introducing another method.
d357e81
to
c9e49b7
Compare
c9e49b7
to
34729a1
Compare
@@ -82,6 +82,8 @@ type Context struct { // nolint: maligned | |||
blockCtx *BlockContext | |||
initialHeight int64 | |||
|
|||
priority int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be moved up into the group where the data, events and gasAccountant are.
34729a1
to
68bbdf0
Compare
68bbdf0
to
b844f58
Compare
This PR adds support for consensus transaction prioritization using the V1 tendermint mempool implementation.
Here are the base priorities for our consensus apps:
The reason why the
scheduler
andsupplementarysanity
have a priority of 0 is that they have no transactions.Node registrations have a slightly higher priority of 60000 instead of the base 50000 used for other registry transactions.
I also experimented a bit with bumping various priorities based on the transaction contents -- e.g. roothash executor commits and proposer timeouts based on runtime stake, but I don't think it's worth the extra complexity.
I suggest we first try the basic version as implemented in this PR and we can always add the extra priority fudging later, once we're sure this works as intended.
In case of errors, the returned priority is 0, which I think makes most sense, so people won't be able to submit faulty transactions to fill up the high-priority part of the queue.