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

feat: introduce PreBlock #17421

Merged
merged 42 commits into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
9944fe1
replace RunMigrationBeginBlock with PreBeginBlock
mmsqe Aug 17, 2023
97437d2
fix test
mmsqe Aug 17, 2023
5df47ca
update doc
mmsqe Aug 17, 2023
4cb3ead
Merge branch 'main' into pre-begin
mmsqe Aug 17, 2023
875aef9
fix doc
mmsqe Aug 17, 2023
fa4d36c
add test
mmsqe Aug 17, 2023
8f901a2
Apply suggestions from code review
mmsqe Aug 17, 2023
0bf24db
force register in upgrade
mmsqe Aug 18, 2023
6aef95e
Merge branch 'main' into pre-begin
mmsqe Aug 18, 2023
952c619
keep UpgradeModule
mmsqe Aug 18, 2023
9ab334d
Revert "keep UpgradeModule"
mmsqe Aug 19, 2023
81aea05
Merge branch 'main' into pre-begin
mmsqe Aug 19, 2023
bded698
add proto
mmsqe Aug 19, 2023
1294aba
allow sim test use latest config.PreBlockers
mmsqe Aug 19, 2023
41eac61
add SetOrderPreBlockers
mmsqe Aug 19, 2023
5a80397
fix doc
mmsqe Aug 19, 2023
b9d1447
fix test
mmsqe Aug 19, 2023
5306a6f
Apply suggestions from code review
mmsqe Aug 19, 2023
9fac2e9
rm dummy upgrade from begin blocker
mmsqe Aug 19, 2023
b5e9072
replace dep
mmsqe Aug 20, 2023
e9247db
decouple dep
mmsqe Aug 20, 2023
e40de51
use latest sdk ResponsePreBlock
mmsqe Aug 20, 2023
afbf503
add when dep is ready (to be reverted)
mmsqe Aug 20, 2023
069488d
replace dep
mmsqe Aug 21, 2023
91b8fb4
Revert "add when dep is ready (to be reverted)"
mmsqe Aug 21, 2023
f3d2aa6
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe Aug 21, 2023
67b41af
fix replace
mmsqe Aug 21, 2023
3ff3ca4
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe Aug 21, 2023
c2a4320
Apply suggestions from code review
mmsqe Aug 21, 2023
3ffb4e9
Apply suggestions from code review
mmsqe Aug 21, 2023
5b87a82
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe Aug 24, 2023
19b0153
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe Aug 26, 2023
c4cffdc
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe Aug 29, 2023
4fb0496
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe Aug 30, 2023
2a9966d
reset gas meter after update cp
mmsqe Aug 30, 2023
19901c9
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe Sep 6, 2023
f2ac765
Update docs/docs/build/building-modules/17-preblock.md
mmsqe Sep 6, 2023
7a7dbaa
Merge branch 'main' into pre-begin
mmsqe Sep 13, 2023
766c82d
Apply suggestions from code review
mmsqe Sep 13, 2023
95a19e0
Merge branch 'main' into pre-begin
mmsqe Sep 13, 2023
e55d9e5
Merge branch 'main' into pre-begin
mmsqe Sep 13, 2023
88b1801
Apply suggestions from code review
mmsqe Sep 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,10 @@ func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.Respons
}
}

if err := app.preBlock(); err != nil {
return nil, err
}

Comment on lines 736 to +742
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).FinalizeBlock (baseapp/abci.go:669)

beginBlock, err := app.beginBlock(req)
if err != nil {
return nil, err
Expand Down
22 changes: 13 additions & 9 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -669,17 +669,12 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context
return ctx.WithMultiStore(msCache), msCache
}

func (app *BaseApp) beginBlock(req *abci.RequestFinalizeBlock) (sdk.BeginBlock, error) {
var (
resp sdk.BeginBlock
err error
)

ctx := app.finalizeBlockState.ctx
func (app *BaseApp) preBlock() error {
if app.preBlocker != nil {
ctx := app.finalizeBlockState.ctx
rsp, err := app.preBlocker(ctx)
if err != nil {
return sdk.BeginBlock{}, err
return err
}
// rsp.ConsensusParamsChanged is true from preBlocker means ConsensusParams in store get changed
// write the consensus parameters in store to context
Expand All @@ -691,8 +686,17 @@ func (app *BaseApp) beginBlock(req *abci.RequestFinalizeBlock) (sdk.BeginBlock,
app.finalizeBlockState.ctx = ctx
}
}
return nil
}

func (app *BaseApp) beginBlock(req *abci.RequestFinalizeBlock) (sdk.BeginBlock, error) {
var (
resp sdk.BeginBlock
err error
)

if app.beginBlocker != nil {
resp, err = app.beginBlocker(ctx)
resp, err = app.beginBlocker(app.finalizeBlockState.ctx)
if err != nil {
return resp, err
}
Expand Down
60 changes: 60 additions & 0 deletions docs/architecture/adr-068-preblock.md
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# ADR 068: Preblock

## Changelog

* Sept 13, 2023: Initial Draft

## Status

DRAFT

## Abstract

Introduce `PreBlock`, which runs before begin blocker other modules, and allows to modify consensus parameters, and the changes are visible to the following state machine logics.

## Context
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved

When upgrading to sdk 0.47, the storage format for consensus parameters changed, but in the migration block, `ctx.ConsensusParams()` is always `nil`, because it fails to load the old format using new code, it's supposed to be migrated by the `x/upgrade` module first, but unfortunately, the migration happens in `BeginBlocker` handler, which runs after the `ctx` 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.

## Alternatives

The first alternative solution introduced a `MigrateModuleManager`, which only includes the `x/upgrade` module right now, and baseapp will run their `BeginBlocker`s before the other modules, and reload context's consensus parameters in between.

## Decision

Suggested this new lifecycle method.

### `PreBlocker`

There are two semantics around the new lifecycle method:

- It runs before the `BeginBlocker` of all modules
- It can modify consensus parameters in storage, and signal the caller through the return value.

When it returns `ConsensusParamsChanged=true`, the caller must refresh the consensus parameter in the finalize context:
```
app.finalizeBlockState.ctx = app.finalizeBlockState.ctx.WithConsensusParams(app.GetConsensusParams())
```

The new ctx must be passed to all the other lifecycle methods.


## Consequences

### Backwards Compatibility

### Positive

### Negative

### Neutral

## Further Discussions

## Test Cases

## References
* [1] https://github.com/cosmos/cosmos-sdk/issues/16494
* [2] https://github.com/cosmos/cosmos-sdk/pull/16583
* [3] https://github.com/cosmos/cosmos-sdk/pull/17421
5 changes: 3 additions & 2 deletions x/upgrade/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,9 @@ func NewAppModule(keeper *keeper.Keeper, ac address.Codec) AppModule {
}

var (
_ appmodule.AppModule = AppModule{}
_ module.HasGenesis = AppModule{}
_ appmodule.AppModule = AppModule{}
_ appmodule.HasPreBlocker = AppModule{}
_ module.HasGenesis = AppModule{}
)

// IsOnePerModuleType implements the depinject.OnePerModuleType interface.
Expand Down
Loading