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.

Sign up for GitHub

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

Jump to bottom

refactor!: reimplement PreFinalizeBlockHook as PreBlocker #17713

Merged
merged 14 commits into from
Sep 15, 2023
Merged

refactor!: reimplement PreFinalizeBlockHook as PreBlocker #17713

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
713bce4
reimplement PreFinalizeBlockHook as PreBlocker
mmsqe Aug 22, 2023
7443118
update doc
mmsqe Aug 26, 2023
0f2020f
Merge remote-tracking branch 'origin/main' into pre-begin-merge
mmsqe Sep 13, 2023
43a9d5f
Apply suggestions from code review
mmsqe Sep 13, 2023
7537d32
Apply suggestions from code review
mmsqe Sep 13, 2023
39553e1
Merge remote-tracking branch 'origin/main' into pre-begin-merge
mmsqe Sep 13, 2023
b1ae029
Update x/upgrade/CHANGELOG.md
mmsqe Sep 13, 2023
77c1e37
Merge branch 'main' into pre-begin-merge
alexanderbez Sep 13, 2023
021ead2
Update CHANGELOG.md
mmsqe Sep 13, 2023
81fafe0
Merge branch 'main' into pre-begin-merge
mmsqe Sep 14, 2023
edd52d8
change to ptr
mmsqe Sep 14, 2023
fc5c235
update doc
mmsqe Sep 14, 2023
b6a2b94
Merge remote-tracking branch 'origin/main' into re-begin-ptr
mmsqe Sep 14, 2023
82d188e
Apply suggestions from code review
mmsqe Sep 14, 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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* (types) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583), [#17372](https://github.com/cosmos/cosmos-sdk/pull/17372), [#17421](https://github.com/cosmos/cosmos-sdk/pull/17421) 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.
* (types) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583), [#17372](https://github.com/cosmos/cosmos-sdk/pull/17372), [#17421](https://github.com/cosmos/cosmos-sdk/pull/17421), [#17713](https://github.com/cosmos/cosmos-sdk/pull/17713) 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 and reimplement `PreFinalizeBlockHook` as `PreBlocker`.
mmsqe marked this conversation as resolved.
Show resolved Hide resolved
* (baseapp) [#17518](https://github.com/cosmos/cosmos-sdk/pull/17518) Utilizing voting power from vote extensions (CometBFT) instead of the current bonded tokens (x/staking) to determine if a set of vote extensions are valid.
* (config) [#17649](https://github.com/cosmos/cosmos-sdk/pull/17649) Fix `mempool.max-txs` configuration is invalid in `app.config`.
* (mempool) [#17668](https://github.com/cosmos/cosmos-sdk/pull/17668) Fix: `PriorityNonceIterator.Next()` nil pointer ref for min priority at the end of iteration.
Expand Down
8 changes: 1 addition & 7 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,13 +730,7 @@ func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.Respons
WithHeaderHash(req.Hash)
}

if app.preFinalizeBlockHook != nil {
if err := app.preFinalizeBlockHook(app.finalizeBlockState.ctx, req); err != nil {
return nil, err
}
}

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

Expand Down
18 changes: 11 additions & 7 deletions baseapp/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2035,7 +2035,7 @@ func TestABCI_HaltChain(t *testing.T) {
}
}

func TestBaseApp_PreFinalizeBlockHook(t *testing.T) {
func TestBaseApp_PreBlocker(t *testing.T) {
db := dbm.NewMemDB()
name := t.Name()
logger := log.NewTestLogger(t)
Expand All @@ -2045,9 +2045,11 @@ func TestBaseApp_PreFinalizeBlockHook(t *testing.T) {
require.NoError(t, err)

wasHookCalled := false
app.SetPreFinalizeBlockHook(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) (sdk.ResponsePreBlock, error) {
wasHookCalled = true
return nil
return sdk.ResponsePreBlock{
ConsensusParamsChanged: true,
}, nil
})
app.Seal()

Expand All @@ -2060,8 +2062,8 @@ func TestBaseApp_PreFinalizeBlockHook(t *testing.T) {
_, err = app.InitChain(&abci.RequestInitChain{})
require.NoError(t, err)

app.SetPreFinalizeBlockHook(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
return errors.New("some error")
app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) (sdk.ResponsePreBlock, error) {
return sdk.ResponsePreBlock{}, errors.New("some error")
})
app.Seal()

Expand Down Expand Up @@ -2122,7 +2124,7 @@ func TestBaseApp_VoteExtensions(t *testing.T) {
return &abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}, nil
})

app.SetPreFinalizeBlockHook(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) (sdk.ResponsePreBlock, error) {
count := uint64(0)
pricesSum := uint64(0)
for _, v := range req.Txs {
Expand All @@ -2141,7 +2143,9 @@ func TestBaseApp_VoteExtensions(t *testing.T) {
ctx.KVStore(capKey1).Set([]byte("avgPrice"), buf)
}

return nil
return sdk.ResponsePreBlock{
ConsensusParamsChanged: true,
}, nil
})
}

Expand Down
25 changes: 12 additions & 13 deletions baseapp/baseapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,17 +74,16 @@ type BaseApp struct {
anteHandler sdk.AnteHandler // ante handler for fee and auth
postHandler sdk.PostHandler // post handler, optional, e.g. for tips

initChainer sdk.InitChainer // ABCI InitChain handler
preBlocker sdk.PreBlocker // logic to run before BeginBlocker
beginBlocker sdk.BeginBlocker // (legacy ABCI) BeginBlock handler
endBlocker sdk.EndBlocker // (legacy ABCI) EndBlock handler
processProposal sdk.ProcessProposalHandler // ABCI ProcessProposal handler
prepareProposal sdk.PrepareProposalHandler // ABCI PrepareProposal
extendVote sdk.ExtendVoteHandler // ABCI ExtendVote handler
verifyVoteExt sdk.VerifyVoteExtensionHandler // ABCI VerifyVoteExtension handler
prepareCheckStater sdk.PrepareCheckStater // logic to run during commit using the checkState
precommiter sdk.Precommiter // logic to run during commit using the deliverState
preFinalizeBlockHook sdk.PreFinalizeBlockHook // logic to run before FinalizeBlock
initChainer sdk.InitChainer // ABCI InitChain handler
preBlocker sdk.PreBlocker // logic to run before BeginBlocker
beginBlocker sdk.BeginBlocker // (legacy ABCI) BeginBlock handler
endBlocker sdk.EndBlocker // (legacy ABCI) EndBlock handler
processProposal sdk.ProcessProposalHandler // ABCI ProcessProposal handler
prepareProposal sdk.PrepareProposalHandler // ABCI PrepareProposal
extendVote sdk.ExtendVoteHandler // ABCI ExtendVote handler
verifyVoteExt sdk.VerifyVoteExtensionHandler // ABCI VerifyVoteExtension handler
prepareCheckStater sdk.PrepareCheckStater // logic to run during commit using the checkState
precommiter sdk.Precommiter // logic to run during commit using the deliverState

addrPeerFilter sdk.PeerFilter // filter peers by address and port
idPeerFilter sdk.PeerFilter // filter peers by node ID
Expand Down Expand Up @@ -669,10 +668,10 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context
return ctx.WithMultiStore(msCache), msCache
}

func (app *BaseApp) preBlock() error {
func (app *BaseApp) preBlock(req *abci.RequestFinalizeBlock) error {
if app.preBlocker != nil {
ctx := app.finalizeBlockState.ctx
rsp, err := app.preBlocker(ctx)
rsp, err := app.preBlocker(ctx, req)
if err != nil {
return err
}
Expand Down
8 changes: 0 additions & 8 deletions baseapp/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,14 +216,6 @@ func (app *BaseApp) SetPrecommiter(precommiter sdk.Precommiter) {
app.precommiter = precommiter
}

func (app *BaseApp) SetPreFinalizeBlockHook(hook sdk.PreFinalizeBlockHook) {
if app.sealed {
panic("SetPreFinalizeBlockHook() on sealed BaseApp")
}

app.preFinalizeBlockHook = hook
}

func (app *BaseApp) SetAnteHandler(ah sdk.AnteHandler) {
if app.sealed {
panic("SetAnteHandler() on sealed BaseApp")
Expand Down
18 changes: 11 additions & 7 deletions docs/architecture/adr-064-abci-2.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,15 +293,15 @@ decision based on the vote extensions.

In certain contexts, it may be useful or necessary for applications to persist
data derived from vote extensions. In order to facilitate this use case, we propose
to allow app developers to define a pre-FinalizeBlock hook which will be called
to allow app developers to define a pre-Blocker hook which will be called
at the very beginning of `FinalizeBlock`, i.e. before `BeginBlock` (see below).

Note, we cannot allow applications to directly write to the application state
during `ProcessProposal` because during replay, CometBFT will NOT call `ProcessProposal`,
which would result in an incomplete state view.

```go
func (a MyApp) PreFinalizeBlockHook(ctx sdk.Context, req.RequestFinalizeBlock) error {
func (a MyApp) PreBlocker(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
voteExts := GetVoteExtensions(ctx, req.Txs)

// Process and perform some compute on vote extensions, storing any resulting
Expand Down Expand Up @@ -353,13 +353,17 @@ we can come up with new types and names altogether.
func (app *BaseApp) FinalizeBlock(req abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) {
ctx := ...

if app.preFinalizeBlockHook != nil {
if err := app.preFinalizeBlockHook(ctx, req); err != nil {
if app.preBlocker != nil {
ctx := app.finalizeBlockState.ctx
rsp, err := app.preBlocker(ctx, req)
if err != nil {
return nil, err
}
if rsp.ConsensusParamsChanged {
app.finalizeBlockState.ctx = ctx.WithConsensusParams(app.GetConsensusParams(ctx))
}
Comment on lines +362 to +364
Copy link
Member

Choose a reason for hiding this comment

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

q: Might be a silly one but... Why do we need this, couldn't we just always do it? Or are there cases in which we want to update params but not make those changes available until the next block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we only need reload when consensus params get modified in preBlocker

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but what difference would it make in practice to always reload the consensus params? That way we could discard ResponsePreBlock

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I realize I'm late to the party. This was in the previous PR, and I only thought of this now 🤦

Copy link
Member

Choose a reason for hiding this comment

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

but this is a rare case and should in reality only happen in upgrades

}

beginBlockResp := app.beginBlock(ctx, req)
beginBlockResp, err := app.beginBlock(req)
appendBlockEventAttr(beginBlockResp.Events, "begin_block")

txExecResults := make([]abci.ExecTxResult, 0, len(req.Txs))
Expand All @@ -368,7 +372,7 @@ func (app *BaseApp) FinalizeBlock(req abci.RequestFinalizeBlock) (*abci.Response
txExecResults = append(txExecResults, result)
}

endBlockResp := app.endBlock(ctx, req)
endBlockResp, err := app.endBlock(app.finalizeBlockState.ctx)
appendBlockEventAttr(beginBlockResp.Events, "end_block")

return abci.ResponseFinalizeBlock{
Expand Down
1 change: 1 addition & 0 deletions docs/architecture/adr-068-preblock.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,4 @@ The new ctx must be passed to all the other lifecycle methods.
* [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
* [4] https://github.com/cosmos/cosmos-sdk/pull/17713
2 changes: 1 addition & 1 deletion docs/docs/build/building-apps/04-vote-extensions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ will be available to the application during the subsequent `FinalizeBlock` call.
An example of how a pre-FinalizeBlock hook could look like is shown below:

```go
app.SetPreFinalizeBlockHook(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
allVEs := []VE{} // store all parsed vote extensions here
for _, tx := range req.Txs {
// define a custom function that tries to parse the tx as a vote extension
Expand Down
2 changes: 1 addition & 1 deletion runtime/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ func (a *App) Load(loadLatest bool) error {
}

// PreBlocker application updates every pre block
func (a *App) PreBlocker(ctx sdk.Context) (sdk.ResponsePreBlock, error) {
func (a *App) PreBlocker(ctx sdk.Context, _ *abci.RequestFinalizeBlock) (sdk.ResponsePreBlock, error) {
return a.ModuleManager.PreBlock(ctx)
}

Expand Down
2 changes: 1 addition & 1 deletion simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ func (app *SimApp) setPostHandler() {
func (app *SimApp) Name() string { return app.BaseApp.Name() }

// PreBlocker application updates every pre block
func (app *SimApp) PreBlocker(ctx sdk.Context) (sdk.ResponsePreBlock, error) {
func (app *SimApp) PreBlocker(ctx sdk.Context, _ *abci.RequestFinalizeBlock) (sdk.ResponsePreBlock, error) {
return app.ModuleManager.PreBlock(ctx)
}

Expand Down
17 changes: 7 additions & 10 deletions types/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,13 @@ type ExtendVoteHandler func(Context, *abci.RequestExtendVote) (*abci.ResponseExt
// pre-commit vote extension.
type VerifyVoteExtensionHandler func(Context, *abci.RequestVerifyVoteExtension) (*abci.ResponseVerifyVoteExtension, error)

// PreBlocker runs code before the `BeginBlocker`.
type PreBlocker func(Context) (ResponsePreBlock, error)
// PreBlocker runs code before the `BeginBlocker` and defines a function type alias for executing logic right
// before FinalizeBlock is called (but after its context has been set up). It is
// intended to allow applications to perform computation on vote extensions and
// persist their results in state.
//
// Note: returning an error will make FinalizeBlock fail.
type PreBlocker func(Context, *abci.RequestFinalizeBlock) (ResponsePreBlock, error)
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved

// BeginBlocker defines a function type alias for executing application
// business logic before transactions are executed.
Expand All @@ -51,14 +56,6 @@ type BeginBlocker func(Context) (BeginBlock, error)
// and allows for existing EndBlock functionality within applications.
type EndBlocker func(Context) (EndBlock, error)

// PreFinalizeBlockHook defines a function type alias for executing logic right
// before FinalizeBlock is called (but after its context has been set up). It is
// intended to allow applications to perform computation on vote extensions and
// persist their results in state.
//
// Note: returning an error will make FinalizeBlock fail.
type PreFinalizeBlockHook func(Context, *abci.RequestFinalizeBlock) error

// EndBlock defines a type which contains endblock events and validator set updates
type EndBlock struct {
ValidatorUpdates []abci.ValidatorUpdate
Expand Down
4 changes: 4 additions & 0 deletions x/upgrade/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#16511](https://github.com/cosmos/cosmos-sdk/pull/16511) `plan.DownloadURLWithChecksum` has been renamed to `plan.DownloadURL` and does not validate the URL anymore. Call `plan.ValidateURL` before calling `plan.DownloadURL` to validate the URL.
* [#16511](https://github.com/cosmos/cosmos-sdk/pull/16511) `plan.DownloadUpgrade` does not validate URL anymore. Call `plan.ValidateURL` before calling `plan.DownloadUpgrade` to validate the URL.
* [#16227](https://github.com/cosmos/cosmos-sdk/issues/16227) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey`, methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. `UpgradeHandler` now receives a `context.Context`. `GetUpgradedClient`, `GetUpgradedConsensusState`, `GetUpgradePlan` now return a specific error for "not found".

### Bug Fixes

* [#17421](https://github.com/cosmos/cosmos-sdk/pull/17421) Replace ` BeginBlock` by `PreBlock`. Read [ADR-68](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-068-preblock.md) for more information.
Loading