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

Remove istanbul.v1 code #1999

Merged
merged 10 commits into from
Feb 28, 2023
Merged

Remove istanbul.v1 code #1999

merged 10 commits into from
Feb 28, 2023

Conversation

hbandura
Copy link
Contributor

Remove all code and references of the deprecated istanbul v1 consensus code. The only remnant is the v1 PreparedCertificate since it's still used in storage.

@hbandura hbandura requested a review from a team as a code owner January 18, 2023 17:39
@hbandura hbandura requested review from mcortesi and nategraf and removed request for a team January 18, 2023 17:39
@piersy
Copy link
Contributor

piersy commented Jan 18, 2023

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 7f6fe11

coverage: 51.1% of statements across all listed packages
coverage:  63.4% of statements in consensus/istanbul
coverage:  42.9% of statements in consensus/istanbul/announce
coverage:  56.1% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  66.9% of statements in consensus/istanbul/core
coverage:  50.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.4% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random
CommentID: 3146592755

@palango
Copy link
Contributor

palango commented Jan 19, 2023

@hbandura Have you thought about removing all V2 suffixes as well?

e2e_test/e2e_test.go Outdated Show resolved Hide resolved
@hbandura
Copy link
Contributor Author

@hbandura Have you thought about removing all V2 suffixes as well?

I have, but since we are already retaining the old msg codes (though deprecating them), I thought of just leaving the 'v2', making it explicit that this is a new version of the consensus code.

If we were to remove that suffix, I'd still do it in a different PR since I'd like to avoid the mess in the pr diff.

@palango
Copy link
Contributor

palango commented Feb 15, 2023

I have, but since we are already retaining the old msg codes (though deprecating them), I thought of just leaving the 'v2', making it explicit that this is a new version of the consensus code.

What do you mean with retaining the old message codes?

If we were to remove that suffix, I'd still do it in a different PR since I'd like to avoid the mess in the pr diff.

Sounds reasonable to me.

@hbandura
Copy link
Contributor Author

I have, but since we are already retaining the old msg codes (though deprecating them), I thought of just leaving the 'v2', making it explicit that this is a new version of the consensus code.

What do you mean with retaining the old message codes?

The istanbul msg codes, PreprepareV1 and RoundChangeV1 . I left them with a prepended 'deprecated' in the name.

I'm not against removing all references to v2, but I'd rather be sure that at least all validators are running versions that already removed all v1 code.

Copy link
Contributor

@gastonponti gastonponti left a comment

Choose a reason for hiding this comment

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

Check the question but other than that, LGTM

@@ -238,7 +223,7 @@ func (c *core) handleRoundChangeV2(msg *istanbul.Message) error {
// ----------------------------------------------------------------------------

// CurrentRoundChangeSet returns the current round change set summary.
func (c *core) CurrentRoundChangeSetV2() *RoundChangeSetSummary {
func (c *core) CurrentRoundChangeSet() *RoundChangeSetSummary {
Copy link
Contributor

Choose a reason for hiding this comment

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

You've been maintaining the V2 suffix in all the PR except this one. Just checking if that was on purpose or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was. Since the CRCS was used from an api call, and we had to check which one to return based on the block, both were available. Now that only one should be there, I decided to change this one now.

@hbandura hbandura enabled auto-merge (squash) February 28, 2023 17:39
@hbandura hbandura merged commit bac7831 into master Feb 28, 2023
@hbandura hbandura deleted the hbandura/remv1 branch February 28, 2023 19:57
@palango palango mentioned this pull request Mar 10, 2023
palango pushed a commit that referenced this pull request Apr 13, 2023
* Remove v1 types

* Add backlog tests (remove v1)

* Remove v1 references from istanbul core core.go

* Remove v1 code from handler & handler_test

* remove v1 code from preprepare files

* remove v1 code from roundchange files

* Remove most of v1 code

* Remove isConsensusFork

* Remove all mentions to V2Block

* Remove useless testing suite function
@palango palango mentioned this pull request Apr 13, 2023
gastonponti pushed a commit that referenced this pull request Apr 17, 2023
* Remove v1 types

* Add backlog tests (remove v1)

* Remove v1 references from istanbul core core.go

* Remove v1 code from handler & handler_test

* remove v1 code from preprepare files

* remove v1 code from roundchange files

* Remove most of v1 code

* Remove isConsensusFork

* Remove all mentions to V2Block

* Remove useless testing suite function
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.

4 participants