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

mempool: Deprecate the priority mempool #9388

Closed
Tracked by #9428
thanethomson opened this issue Sep 7, 2022 · 4 comments
Closed
Tracked by #9428

mempool: Deprecate the priority mempool #9388

thanethomson opened this issue Sep 7, 2022 · 4 comments
Assignees
Labels
C:mempool Component: Mempool feature Feature work that definitely changes system behavior stale for use by stalebot

Comments

@thanethomson
Copy link
Contributor

In light of ABCI++ landing, we need to strongly consider deprecating and removing the priority mempool as the same functionality can be provided by implementing an app-side mempool with ABCI++.

Definition of Done

When we've either decided that we still need the priority mempool (in which case this issue can just be closed), or we've clearly deprecated it and scheduled it for removal in a specific future release.

Follow-up here, if we decide to remove it, would be to create a separate issue for tracking its removal.

@thanethomson thanethomson added C:mempool Component: Mempool feature Feature work that definitely changes system behavior labels Sep 7, 2022
@cmwaters
Copy link
Contributor

Even without an app-side mempool, applications don't need a priority mempool. The proposer would simply request all the txs in prepare proposal, sort through them accordingly and return what it wants in a block.

It seems from talking with others that it's pretty unanimous that abci++ supersedes priority mempool. We don't need to rush this. I would go about adding a deprecation notice in v0.38 and removing it in v0.39. This would also involve modifying CheckTxResponse.

@thanethomson
Copy link
Contributor Author

thanethomson commented Oct 13, 2022

It seems reasonable to deprecate priority mempool support in favor of what's possible with ABCI++ Prepare/Process Proposal.

We could mark it as deprecated in v0.37 (deprecated due to ABCI++) and remove it in v0.38? Othewise we'll need to support it (and all the weirdness it could cause in interaction with ABCI++ uses) through the v0.38 release cycle.

@rootulp
Copy link
Contributor

rootulp commented Nov 10, 2022

Even without an app-side mempool, applications don't need a priority mempool. The proposer would simply request all the txs in prepare proposal, sort through them accordingly and return what it wants in a block.

Is there an attack vector where a malicious user could spam the mempool with many low fee paying transactions? Without a prioritized mempool, I assume the proposer would need to request all txs in prepare proposal and be forced to sort through all these low fee paying transactions.

Is there a benefit to the prioritization occurring in the mempool rather than in prepare proposal?

@adizere
Copy link
Contributor

adizere commented Dec 14, 2022

Relevant -- SDK progress on using a no-op mempool: cosmos/cosmos-sdk#14288

@thanethomson thanethomson added backlog-priority A priority issue in the backlog and removed priority A high-priority, high-level item to be shown on the priorities project board labels Dec 19, 2022
@thanethomson thanethomson removed the backlog-priority A priority issue in the backlog label Dec 22, 2022
@github-actions github-actions bot added the stale for use by stalebot label Mar 4, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:mempool Component: Mempool feature Feature work that definitely changes system behavior stale for use by stalebot
Projects
Status: Done/Merged
Development

No branches or pull requests

5 participants