Skip to content
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

Prioritized mempool follow-ups #9077

Closed
9 tasks done
Tracked by #9091 ...
thanethomson opened this issue Jul 23, 2022 · 5 comments
Closed
9 tasks done
Tracked by #9091 ...

Prioritized mempool follow-ups #9077

thanethomson opened this issue Jul 23, 2022 · 5 comments
Assignees
Labels
C:mempool Component: Mempool maintenance Bug/security fixes, correctness, reliability, performance and general QA

Comments

@thanethomson
Copy link
Contributor

thanethomson commented Jul 23, 2022

  • spec: priority mempool specification #9310
    - Link to existing ADR when prioritized mempool was introduced.
  • Performance and stability are at least as good as the mempool in v0.34 (no regression).
  • Prioritized mempool implemented and tested according to the specification.
    • Ability to provide priority to transactions in CheckTx responses, which deliver transactions to the application in priority-based order.
  • Transactions are still gossiped in FIFO order.
  • We have a clear answer to the question as to whether we need the prioritized mempool when ABCI++ lands
    • If not we need to remove the Priority field from CheckTx. This impacts the release of ABCI++.
@thanethomson thanethomson added C:mempool Component: Mempool T:tracking Tracking issue for other issues maintenance Bug/security fixes, correctness, reliability, performance and general QA labels Jul 23, 2022
@thanethomson thanethomson mentioned this issue Jul 25, 2022
39 tasks
@thanethomson thanethomson added priority A high-priority, high-level item to be shown on the priorities project board and removed T:tracking Tracking issue for other issues labels Jul 27, 2022
@thanethomson thanethomson changed the title WIP: Prioritized mempool follow-ups Prioritized mempool follow-ups Jul 28, 2022
@williambanfield
Copy link
Contributor

If Cosmos-SDK ADR-60 is accepted, it seems quite likely that the prioritized mempool will no longer be needed by the SDK with the advent of ABCI++. We may wish to keep the prioritized mempool so that other users may take advantage of it, but, at the moment, our largest consumer does not appear to need it.

@williambanfield
Copy link
Contributor

Profiling was performed and documented here, is there more profiling we need to do as part of this issue? It's fairly clear that the prioritized mempool, with the backported fixes, does not display the unbounded heap growth present in the original implementation.

@jmalicevic
Copy link
Contributor

"Performance and stability are at least as good as the mempool in v0.34 (no regression)." I don't know whether we have other metrics identified that could contribute to this particular section (Transaction throughput for example )? If not, then maybe for now this is sufficient.

@jmalicevic
Copy link
Contributor

#9388 is set to deprace the priority mempool.

@jmalicevic
Copy link
Contributor

The priority mempool was depracated. Therefore closing this issue.

@thanethomson thanethomson removed the priority A high-priority, high-level item to be shown on the priorities project board label Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:mempool Component: Mempool maintenance Bug/security fixes, correctness, reliability, performance and general QA
Projects
Status: Done/Merged
Development

No branches or pull requests

4 participants
@thanethomson @williambanfield @jmalicevic and others