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

refactor!: remove v0 mempool #1388

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ninabarbakadze
Copy link
Member

@ninabarbakadze ninabarbakadze commented Jun 11, 2024

Description

Fixes #1387

@ninabarbakadze ninabarbakadze marked this pull request as ready for review June 12, 2024 08:57
@ninabarbakadze ninabarbakadze requested a review from a team as a code owner June 12, 2024 08:57
@ninabarbakadze ninabarbakadze requested review from rootulp and cmwaters and removed request for a team June 12, 2024 08:57
@cmwaters
Copy link
Contributor

Hmm I want this to only be apart of v3. Which might be okay since v0.34.x is the branch for v1 and v2.

I would ideally like to be sure of our v3 release strategy i.e. will v3 have a v2 of celestia-core. Will we diverge from cometbft in our release naming because we already released a v1, will this be compatible with the SDK and our ambitions of no longer needing a fork etc...

cc @evan-forbes @rootulp

In any case it's probably fine to still go ahead and deprecate this. I just want to give ample time to double check. We should be in no rush to have to remove v0

@ninabarbakadze
Copy link
Member Author

Hmm I want this to only be apart of v3. Which might be okay since v0.34.x is the branch for v1 and v2.

I would ideally like to be sure of our v3 release strategy i.e. will v3 have a v2 of celestia-core. Will we diverge from cometbft in our release naming because we already released a v1, will this be compatible with the SDK and our ambitions of no longer needing a fork etc...

cc @evan-forbes @rootulp

In any case it's probably fine to still go ahead and deprecate this. I just want to give ample time to double check. We should be in no rush to have to remove v0

should i convert this into a draft?

@evan-forbes
Copy link
Member

we just need to avoid backporting it, and can keep all of our changes for v3 in main

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

should i convert this into a draft?

IMO yes. I personally haven't been convinced that we should deprecate and remove the v0 mempool. Is there anything I can read about how difficult it is to maintain?

@evan-forbes
Copy link
Member

I personally haven't been convinced that we should deprecate and remove the v0 mempool. Is there anything I can read about how difficult it is to maintain?

my understanding is that we just wanted to save some time implementing bestTx things multiple times, and no one is using the v0 mempool. This is a good point though. When we look upstream, we see that they exclusively use v0, so this might make backporting certain test require further changes.

@ninabarbakadze ninabarbakadze marked this pull request as draft June 13, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate v0 mempool
4 participants