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

fix(x/consensus): harden consensus params proposal #20381

Merged
merged 6 commits into from
May 28, 2024

Conversation

tac0turtle
Copy link
Member

Description

replaces: #20314


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

This comment has been minimized.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

Can we get a changelog?
I the rationale: it is consensus breaking but as the chain would have halted if that wrongly gov proposal were submitted, we consider it safe to backport?

@julienrbrt julienrbrt changed the title fix: consensus params fix(x/consensus): harden consensus params proposal May 14, 2024
@tac0turtle
Copy link
Member Author

Can we get a changelog? I the rationale: it is consensus breaking but as the chain would have halted if that wrongly gov proposal were submitted, we consider it safe to backport?

I need to spend some time seeing if it's breaking. I think the current design is fine but need to check as well

@julienrbrt
Copy link
Member

julienrbrt commented May 15, 2024

I think compared to other consensus breaking where the chain would have panicked immediately, this will require a rollback of the latest block for those crashing, instead of simply switching binaries.

However you could predict this issue is going to reach a chain miles ahead, as it is via a proposal.

@testinginprod
Copy link
Contributor

I think it's ok to backport to 0.50 even if it is consensus breaking.

@julienrbrt
Copy link
Member

@tac0turtle tac0turtle enabled auto-merge (squash) May 28, 2024 10:12
@tac0turtle tac0turtle merged commit d180df8 into release/v0.50.x May 28, 2024
44 of 45 checks passed
@tac0turtle tac0turtle deleted the sergio/fix-consensus-params branch May 28, 2024 10:25
@julienrbrt julienrbrt mentioned this pull request May 28, 2024
12 tasks
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.

None yet

5 participants