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

abci: add priority to checkTx response #5314

Closed

Conversation

ninjaahhh
Copy link
Contributor

@ninjaahhh ninjaahhh commented Sep 1, 2020

Description

our team is leveraging tendermint for our own development and realized there may be something worth contributing back.

after chatting with @marbar3778 and @zmanian we agreed that a good first step would be on mempool: we would like to help improve current implementation of mempool to support packing transaction based on priorities (we have a working prototype using left-leaning red–black tree), which also includes some work on generalizing current mempool interface.

Closes #1861

this minor PR will be a starting point, where the change itself should be backward-compaible and self-explanatory. we would like to also hear more feedback on how to go further (e.g. RFC process, etc.) on future non-trivial PRs.

@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #5314 into master will decrease coverage by 0.81%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##           master    #5314      +/-   ##
==========================================
- Coverage   63.29%   62.48%   -0.82%     
==========================================
  Files         181      258      +77     
  Lines       19080    27171    +8091     
==========================================
+ Hits        12077    16978    +4901     
- Misses       5973     8718    +2745     
- Partials     1030     1475     +445     
Impacted Files Coverage Δ
abci/client/client.go 41.66% <ø> (ø)
abci/client/grpc_client.go 0.00% <ø> (ø)
abci/client/local_client.go 0.00% <0.00%> (ø)
abci/types/pubkey.go 0.00% <0.00%> (ø)
abci/types/util.go 0.00% <0.00%> (ø)
behaviour/reporter.go 48.48% <ø> (ø)
blockchain/v0/pool.go 79.29% <ø> (+0.63%) ⬆️
blockchain/v2/io.go 0.00% <0.00%> (ø)
blockchain/v2/processor_context.go 63.15% <0.00%> (ø)
cmd/tendermint/commands/run_node.go 0.00% <0.00%> (ø)
... and 174 more

@melekes
Copy link
Contributor

melekes commented Sep 1, 2020

we would like to help improve current implementation of mempool to support packing transaction based on priorities (we have a working prototype using left-leaning red–black tree), which also includes some work on generalizing current mempool interface.

that's great @ninjaahhh 👍 the good first step would be an ADR outlining the pitfalls of current implementation and suggested changes. When we reach consensus, we should be able to move forward w/ the implementation. Note: it's better to break up work into smaller PRs, if possible.

Relevant issues: #2147, #2484, #3905

@tac0turtle
Copy link
Contributor

Thank you for opening this PR, prior to merging we should update the spec repo with the corresponding changes.

@tessr
Copy link
Contributor

tessr commented Sep 1, 2020

Thank you for helping out here! I agree with @melekes that we need to all agree on a design and a high-level direction first. In fact, before starting on an ADR, I would recommend that we begin with an RFC, which can address the high-level direction before getting into the details addressed in an ADR.

We haven't always been the best about writing RFCs, but we're currently in the process of cleaning up our spec and design work, and this is a great opportunity to practice and develop our RFC process. RFCs live in the spec repo and there's a template for them in that directory (linked) as well.

@ninjaahhh
Copy link
Contributor Author

thanks for the pointers! rfc PR added tendermint/spec#154

feeback will be greatly appreciated

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale for use by stalebot label Sep 14, 2020
@erikgrinaker erikgrinaker removed the stale for use by stalebot label Sep 14, 2020
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale for use by stalebot label Sep 25, 2020
@erikgrinaker erikgrinaker removed the stale for use by stalebot label Sep 25, 2020
@melekes
Copy link
Contributor

melekes commented Sep 28, 2020

Refs #1861

@github-actions
Copy link

github-actions bot commented Oct 9, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale for use by stalebot label Oct 9, 2020
@github-actions github-actions bot closed this Oct 13, 2020
@melekes melekes reopened this Oct 13, 2020
@github-actions github-actions bot closed this Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale for use by stalebot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ABCI: add Priority to ResponseCheckTx
5 participants