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(baseapp): allow empty consensus params #18531

Merged
merged 4 commits into from
Nov 28, 2023
Merged

Conversation

facundomedica
Copy link
Member

@facundomedica facundomedica commented Nov 21, 2023

Description

This PR attempts to fix an issue when skipping v0.47 while upgrading to v0.50. FinalizeBlock panics because it tries to read consensus params in the new format before the PreBlock which is where the params migration happens.

The solution proposed here involves not erroring if no consensus params were found and returning an empty struct.

Negative: Preblock runs for a single block without consensus params.
It could be an issue for other modules with their own preblock as even if the consensus params are migrated, they won't get the new params until the next block.
I don't think this would be an issue for VoteExtensions, as the chain would presumably enable vote extensions once the v0.50 upgrade has been done.

Positive: FinalizeBlock doesn't panic and consensus params will be present during the first block at any time with the exception of PreBlock.


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
  • added ! to 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
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • 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 ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling in consensus parameter retrieval to prevent application panic and ensure smooth upgrades.
    • Corrected public key usage during transaction simulation.
    • Ensured FinalizeBlock calls are properly passed to ABCIListeners.
    • Addressed a data race issue in BaseApp's context management.
  • Tests

    • Added tests to verify the retrieval of empty consensus parameters and correct maximum block gas value.
  • Documentation

    • Updated CHANGELOG.md with recent bug fixes and improvements.

Copy link
Contributor

coderabbitai bot commented Nov 21, 2023

Warning

Rate Limit Exceeded

@facundomedica has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 13 minutes and 26 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between d7b9a1b and ef1a3b3.

Walkthrough

The changes involve updates to the cosmos/cosmos-sdk project, specifically within the BaseApp component. Error handling in the GetConsensusParams function has been improved to log errors and return an empty struct instead of panicking, facilitating smoother upgrades. A new test ensures this behavior is as expected. Additionally, the changelog notes fixes and improvements, including correct pubkey usage in transaction simulation, proper handling of FinalizeBlock calls, and a resolution to a data race issue.

Changes

File Summary
baseapp/baseapp.go Modified GetConsensusParams to log errors and return an empty cmtproto.ConsensusParams struct instead of panicking.
baseapp/baseapp_test.go Added TestGetEmptyConsensusParams to verify the new error handling behavior in GetConsensusParams.
CHANGELOG.md Updated with entries detailing fixes and improvements, including the GetConsensusParams fix, pubkey usage in transaction simulation, FinalizeBlock call handling, and a data race resolution.

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Comment on lines 522 to 529

cp, err := app.paramStore.Get(ctx)
if err != nil {
panic(fmt.Errorf("consensus key is nil: %w", err))
// This could happen while migrating from v0.45/v0.46 to v0.50, we should
// allow it to happen so during preblock the upgrade plan can be executed
// and the consensus params set for the first time in the new format.
app.logger.Error("couldn't get consensus params", "err", err)
return cmtproto.ConsensusParams{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).GetConsensusParams (baseapp/baseapp.go:518)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).PrepareProposal (baseapp/baseapp.go:402)

@tac0turtle
Copy link
Member

is this still needed?

@facundomedica facundomedica marked this pull request as ready for review November 27, 2023 15:59
@facundomedica facundomedica requested a review from a team as a code owner November 27, 2023 15:59

This comment has been minimized.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 8644e6b and 37c5f95.
Files selected for processing (2)
  • baseapp/baseapp.go (1 hunks)
  • baseapp/baseapp_test.go (1 hunks)
Additional comments: 2
baseapp/baseapp.go (1)
  • 522-532: The changes to the GetConsensusParams function to handle a nil consensus key by logging an error and returning an empty cmtproto.ConsensusParams struct are appropriate for the upgrade scenario described. This prevents a panic during the upgrade process, allowing the consensus parameters to be set in the new format during the PreBlock execution of the first block post-upgrade. Ensure that all other parts of the system that rely on consensus parameters can handle an empty struct being returned.
baseapp/baseapp_test.go (1)
  • 729-731: The test TestGetEmptyConsensusParams correctly checks for the expected behavior when no consensus parameters are found.

@@ -720,6 +720,17 @@ func TestGetMaximumBlockGas(t *testing.T) {
require.Panics(t, func() { suite.baseApp.GetMaximumBlockGas(ctx) })
}

func TestGetEmptyConsensusParmas(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a typo in the test function name TestGetEmptyConsensusParmas; it should be TestGetEmptyConsensusParams.

- func TestGetEmptyConsensusParmas(t *testing.T) {
+ func TestGetEmptyConsensusParams(t *testing.T) {

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func TestGetEmptyConsensusParmas(t *testing.T) {
func TestGetEmptyConsensusParams(t *testing.T) {

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.

lgtm! We should add a changelog entry.

@julienrbrt julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Nov 27, 2023
@julienrbrt julienrbrt changed the title fix: allow empty consensus params fix(baseapp): allow empty consensus params Nov 27, 2023
baseapp/baseapp.go Outdated Show resolved Hide resolved
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

makes sense thank you.

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Comment on lines 522 to +529

cp, err := app.paramStore.Get(ctx)
if err != nil {
panic(fmt.Errorf("consensus key is nil: %w", err))
// This could happen while migrating from v0.45/v0.46 to v0.50, we should
// allow it to happen so during preblock the upgrade plan can be executed
// and the consensus params set for the first time in the new format.
app.logger.Error("failed to get consensus params", "err", err)
return cmtproto.ConsensusParams{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).GetConsensusParams (baseapp/baseapp.go:518)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).PrepareProposal (baseapp/baseapp.go:402)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 37c5f95 and 1f9b400.
Files selected for processing (1)
  • baseapp/baseapp.go (1 hunks)
Additional comments: 1
baseapp/baseapp.go (1)
  • 522-532: The change to log an error and return an empty cmtproto.ConsensusParams struct instead of panicking is a significant change in behavior. Ensure that all parts of the system that rely on GetConsensusParams can handle an empty cmtproto.ConsensusParams struct without any adverse effects, especially during the upgrade process.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 1f9b400 and d7b9a1b.
Files selected for processing (2)
  • CHANGELOG.md (1 hunks)
  • baseapp/baseapp_test.go (1 hunks)
Additional comments: 2
CHANGELOG.md (1)
  • 70-70: The changelog entries accurately reflect the changes made in the associated pull requests and provide clear links to the issues addressed.
baseapp/baseapp_test.go (1)
  • 720-721: The test TestGetMaximumBlockGas correctly asserts that the function panics when a negative MaxGas is stored, which aligns with the expected behavior.

Comment on lines +723 to +732
func TestGetEmptyConsensusParams(t *testing.T) {
suite := NewBaseAppSuite(t)
_, err := suite.baseApp.InitChain(&abci.RequestInitChain{})
require.NoError(t, err)
ctx := suite.baseApp.NewContext(true)

cp := suite.baseApp.GetConsensusParams(ctx)
require.Equal(t, cmtproto.ConsensusParams{}, cp)
require.Equal(t, uint64(0), suite.baseApp.GetMaximumBlockGas(ctx))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The test TestGetEmptyConsensusParams should also verify that an error is logged when no consensus parameters are found, as described in the PR summary. Consider using a mock logger to capture and assert the log output.

@facundomedica facundomedica added this pull request to the merge queue Nov 28, 2023
Merged via the queue into main with commit fa5280f Nov 28, 2023
59 of 60 checks passed
@facundomedica facundomedica deleted the facu/allow-empty-cp branch November 28, 2023 08:37
mergify bot pushed a commit that referenced this pull request Nov 28, 2023
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
(cherry picked from commit fa5280f)
julienrbrt pushed a commit that referenced this pull request Nov 28, 2023
Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com>
@faddat faddat mentioned this pull request Nov 8, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants