Skip to content

feat: v0.34.x Prioritized Mempool#8695

Merged
jmalicevic merged 53 commits intov0.34.xfrom
bez/v0.34.x-prioritized-mempool
Jun 27, 2022
Merged

feat: v0.34.x Prioritized Mempool#8695
jmalicevic merged 53 commits intov0.34.xfrom
bez/v0.34.x-prioritized-mempool

Conversation

@alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Jun 3, 2022

This is a drop-in version of the prioritized mempool introduced in TM v0.35.x retrofitted for the TM v0.34.x release line. That being the case, there exists no new logic here.

Core files/diffs to review:

  • mempool/reactor.go
  • mempool/clist_mempool.go

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Just done a brief pass, are we still giving the user the option between the new and old mempool or are you modifying it so it's a straight swap?

Comment on lines +217 to +219
// mempool_error is set by Tendermint.
// ABCI applictions creating a ResponseCheckTX should not set mempool_error.
string mempool_error = 11;
Copy link
Contributor

Choose a reason for hiding this comment

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

We removed this for v0.36 but I think it's fine if we just leave it for now since it's also in v0.35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the way I'm doing this PR is just dropping in the entire mempool from 0.35.x into 0.34.x, modulo any API changes necessary.

repeated Event events = 7
[(gogoproto.nullable) = false, (gogoproto.jsontag) = "events,omitempty"];
string codespace = 8;
string sender = 9;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the SDK using this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but this is a drop-in placement of what 0.35.x has.

@alexanderbez alexanderbez changed the title feat: Prioritized Mempool feat: v0.34.x Prioritized Mempool Jun 14, 2022
@alexanderbez
Copy link
Contributor Author

Just done a brief pass, are we still giving the user the option between the new and old mempool or are you modifying it so it's a straight swap?

Straight swap. Would you like to support both?

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Jun 14, 2022

@cmwaters I have v0 and v1 introduced into this PR now. It still needs work to get it to build and update all the APIs. Honestly, it's more work than I anticipated and I don't really have the capacity to push it over the finish line.

What's left to complete:

  • Getting unit tests updated and passing
  • Updating usage of mempool construction in node.go and other places

@jmalicevic jmalicevic self-assigned this Jun 16, 2022
@jmalicevic
Copy link
Contributor

Hi @alexanderbez , @cmwaters ,
Is there a place where we wait for recheck to finish before calling reap? I saw the comment in the Update function of the mempool.
The reason I am asking is that am hitting a failure in the consensus/mempool_test and cannot pinpoint the issue. I am testing the v0 (fifo) mempool and cannot see any changes that could change the behaviour in order to cause this (even tried reverting minor changes). It seems to appear when transactions don't fit in one block.

@jmalicevic
Copy link
Contributor

The github workflows will need a quick bit of re-arranging.

I don't think I have the privileges to trigger the workflows. The plumbing is done in #8863 but I can't trigger the additional workflows.

@jmalicevic
Copy link
Contributor

We cannot merge this until #8863 is closed but as the issue is urgent, I will open it for review as all mempool related things are done and tests are passing.

@jmalicevic jmalicevic marked this pull request as ready for review June 24, 2022 09:19
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Approving, given the current gameplan that this will only be released in an RC and will not make it into any releases until #8775 is resolved.

@jmalicevic jmalicevic merged commit 6b7d30c into v0.34.x Jun 27, 2022
@jmalicevic jmalicevic deleted the bez/v0.34.x-prioritized-mempool branch June 27, 2022 09:34
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

I think the other thing we'll need to cover are the docs but we can do it as a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants