-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: introduce PreBlock #17421
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 97437d2
fix test
mmsqe 5df47ca
update doc
mmsqe 4cb3ead
Merge branch 'main' into pre-begin
mmsqe 875aef9
fix doc
mmsqe fa4d36c
add test
mmsqe 8f901a2
Apply suggestions from code review
mmsqe 0bf24db
force register in upgrade
mmsqe 6aef95e
Merge branch 'main' into pre-begin
mmsqe 952c619
keep UpgradeModule
mmsqe 9ab334d
Revert "keep UpgradeModule"
mmsqe 81aea05
Merge branch 'main' into pre-begin
mmsqe bded698
add proto
mmsqe 1294aba
allow sim test use latest config.PreBlockers
mmsqe 41eac61
add SetOrderPreBlockers
mmsqe 5a80397
fix doc
mmsqe b9d1447
fix test
mmsqe 5306a6f
Apply suggestions from code review
mmsqe 9fac2e9
rm dummy upgrade from begin blocker
mmsqe b5e9072
replace dep
mmsqe e9247db
decouple dep
mmsqe e40de51
use latest sdk ResponsePreBlock
mmsqe afbf503
add when dep is ready (to be reverted)
mmsqe 069488d
replace dep
mmsqe 91b8fb4
Revert "add when dep is ready (to be reverted)"
mmsqe f3d2aa6
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe 67b41af
fix replace
mmsqe 3ff3ca4
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe c2a4320
Apply suggestions from code review
mmsqe 3ffb4e9
Apply suggestions from code review
mmsqe 5b87a82
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe 19b0153
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe c4cffdc
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe 4fb0496
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe 2a9966d
reset gas meter after update cp
mmsqe 19901c9
Merge remote-tracking branch 'origin/main' into pre-begin
mmsqe f2ac765
Update docs/docs/build/building-modules/17-preblock.md
mmsqe 7a7dbaa
Merge branch 'main' into pre-begin
mmsqe 766c82d
Apply suggestions from code review
mmsqe 95a19e0
Merge branch 'main' into pre-begin
mmsqe e55d9e5
Merge branch 'main' into pre-begin
mmsqe 88b1801
Apply suggestions from code review
mmsqe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julienrbrt marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: