-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat: introduce PreBlock #17421
feat: introduce PreBlock #17421
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super a fan personally of this new notion of pre-begin blocker.
With PreCommitter, PrepareCheckStaters, I feel like it makes the module API more complicated.
What was the issue with the previous implementation? And why does it need to be generalized? What kind of module other than upgrade could take advantage of this?
CHANGELOG.md
Outdated
@@ -55,6 +55,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
* (version) [#17096](https://github.com/cosmos/cosmos-sdk/pull/17096) Improve `getSDKVersion()` to handle module replacements | |||
* (x/staking) [#17164](https://github.com/cosmos/cosmos-sdk/pull/17164) Add `BondedTokensAndPubKeyByConsAddr` to the keeper to enable vote extension verification. | |||
* (x/genutil) [#17296](https://github.com/cosmos/cosmos-sdk/pull/17296) Add `MigrateHandler` to allow reuse migrate genesis related function. | |||
* (types) [#17421](https://github.com/cosmos/cosmos-sdk/pull/17421) Replace `RunMigrationBeginBlock` with `PreBeginBlock`, which runs before begin blocker other modules, and allows to modify consensus parameters, and the changes are visible to the following state machine logics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was never released so no changelog needed, best to edit the other changelog and add this PR number next to it.
types/module/module.go
Outdated
@@ -222,6 +222,11 @@ type HasConsensusVersion interface { | |||
ConsensusVersion() uint64 | |||
} | |||
|
|||
type PreBeginBlockAppModule interface { | |||
AppModule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be on core/appmodule and named Has<...>. We shouldn't introduce module related interface in types/module unless we need an sdk dependency.
the main issue with the previous implementation is it treat some begin blockers different from the other begin blockers, but they are all named as |
* to runs before begin blocker, and be more generic than hardcode with migration * allowed to modify consensus parameters and the changes are visible to the following state machine logics
25002c4
to
8c0cccd
Compare
@mmsqe could you resolve conflicts? lets see about getting this merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we can do it after the fact, a short adr would be good on this feature so we have it documented
} | ||
} | ||
|
||
if err := app.preBlock(); err != nil { | ||
return nil, err | ||
} | ||
|
There was a problem hiding this comment.
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).FinalizeBlock (baseapp/abci.go:669)
ctx = ctx.WithConsensusParams(app.GetConsensusParams(ctx)) | ||
// GasMeter must be set after we get a context with updated consensus params. | ||
gasMeter := app.getBlockGasMeter(ctx) | ||
ctx = ctx.WithBlockGasMeter(gasMeter) | ||
app.finalizeBlockState.ctx = ctx | ||
} | ||
} |
There was a problem hiding this comment.
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).beginBlock (baseapp/baseapp.go:685)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).FinalizeBlock (baseapp/baseapp.go:669)
Head branch was pushed to by a user without write access
(cherry picked from commit 4eb0185) # Conflicts: # CHANGELOG.md # UPGRADING.md # docs/docs/build/building-apps/03-app-upgrade.md # docs/docs/build/building-modules/01-module-manager.md # docs/docs/develop/advanced/00-baseapp.md # runtime/builder.go # simapp/go.mod # simapp/go.sum # simapp/gomod2nix.toml # tests/go.mod # tests/go.sum # tests/starship/tests/go.mod # tests/starship/tests/go.sum # testutil/mock/types_mock_appmodule.go # types/module/module.go # types/module/module_test.go # x/feegrant/go.mod # x/upgrade/abci.go # x/upgrade/abci_test.go # x/upgrade/go.mod # x/upgrade/go.sum
@mmsqe, are you up to submit this PR https://github.com/mmsqe/cosmos-sdk/pull/2/files now to main? |
Co-authored-by: mmsqe <[email protected]> Co-authored-by: Julien Robert <[email protected]>
finalizeBlockState.ctx
, since only ctx withinbeginBlock
scope was changed, it did not update theConsensusParams
offinalizeBlockState.ctx
which is used inendBlock
and now it doesConsensusParamsChanged
check to avoid refreshingConsensusParams
of ctx every timeDescription
Updates #16583
Context
The Original Problem
When upgrading to sdk 0.47, the storage format for consensus parameters changed, but in the migration block,
ctx.ConsensusParams()
is alwaysnil
, because it fails to load the old format using new code, it's supposed to be migrated by thex/upgrade
module first, but unfortunately, the migration happens inBeginBlocker
handler, which runs after thectx
is initialized.When we try to solve this, we find the
x/upgrade
module can't modify the context to make the consensus parameters visible for the other modules, the context is passed by value, and sdk team want to keep it that way, that's good for isolations between modules.The first alternative solution introduced a
MigrateModuleManager
, which only includes thex/upgrade
module right now, and baseapp will run theirBeginBlocker
s before the other modules, and reload context's consensus parameters in between, but I think it's too hacky, and suggested this new lifecycle method.PreBlocker
There are two semantics around the new lifecycle method:
BeginBlocker
of all modulesWhen it returns
ConsensusParamsChanged=true
, the caller must refresh the consensus parameter in the deliver context:The new ctx must be passed to all the other lifecycle methods.
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
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...
!
in the type prefix if API or client breaking change