From 63e45150de3ee1b726bda3d1fc4150aa776f7e5a Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Tue, 1 Aug 2023 18:10:55 +0200 Subject: [PATCH] fix: Allow VerifyVoteExtensions without ExtendVote (#17251) (cherry picked from commit cb5b8cf1d3d90da22402b117c815839602bb82a4) --- CHANGELOG.md | 1 + baseapp/abci.go | 17 +++++--- baseapp/abci_test.go | 57 +++++++++++++++++++++++++++ baseapp/baseapp.go | 9 ----- docs/architecture/adr-064-abci-2.0.md | 9 +++-- docs/docs/core/00-baseapp.md | 3 +- 6 files changed, 75 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05b5d5c0a8e4..375569a24562 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* (baseapp) [#17251](https://github.com/cosmos/cosmos-sdk/pull/17251) VerifyVoteExtensions and ExtendVote initialize their own contexts/states, allowing VerifyVoteExtensions being called without ExtendVote. * (x/auth) [#17209](https://github.com/cosmos/cosmos-sdk/pull/17209) Internal error on AccountInfo when account's public key is not set. * (baseapp) [#17159](https://github.com/cosmos/cosmos-sdk/pull/17159) Validators can propose blocks that exceed the gas limit. * (x/group) [#17146](https://github.com/cosmos/cosmos-sdk/pull/17146) Rename x/group legacy ORM package's error codespace from "orm" to "legacy_orm", preventing collisions with ORM's error codespace "orm". diff --git a/baseapp/abci.go b/baseapp/abci.go index 8e5610970d7b..02b2d666c3a2 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -548,7 +548,8 @@ func (app *BaseApp) ExtendVote(_ context.Context, req *abci.RequestExtendVote) ( // Always reset state given that ExtendVote and VerifyVoteExtension can timeout // and be called again in a subsequent round. emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height} - app.setState(execModeVoteExtension, emptyHeader) + ms := app.cms.CacheMultiStore() + ctx := sdk.NewContext(ms, emptyHeader, false, app.logger).WithStreamingManager(app.streamingManager) if app.extendVote == nil { return nil, errors.New("application ExtendVote handler not set") @@ -556,14 +557,14 @@ func (app *BaseApp) ExtendVote(_ context.Context, req *abci.RequestExtendVote) ( // If vote extensions are not enabled, as a safety precaution, we return an // error. - cp := app.GetConsensusParams(app.voteExtensionState.ctx) + cp := app.GetConsensusParams(ctx) extsEnabled := cp.Abci != nil && req.Height >= cp.Abci.VoteExtensionsEnableHeight && cp.Abci.VoteExtensionsEnableHeight != 0 if !extsEnabled { return nil, fmt.Errorf("vote extensions are not enabled; unexpected call to ExtendVote at height %d", req.Height) } - app.voteExtensionState.ctx = app.voteExtensionState.ctx. + ctx = ctx. WithConsensusParams(cp). WithBlockGasMeter(storetypes.NewInfiniteGasMeter()). WithBlockHeight(req.Height). @@ -588,7 +589,7 @@ func (app *BaseApp) ExtendVote(_ context.Context, req *abci.RequestExtendVote) ( } }() - resp, err = app.extendVote(app.voteExtensionState.ctx, req) + resp, err = app.extendVote(ctx, req) if err != nil { app.logger.Error("failed to extend vote", "height", req.Height, "error", err) return &abci.ResponseExtendVote{VoteExtension: []byte{}}, nil @@ -608,9 +609,13 @@ func (app *BaseApp) VerifyVoteExtension(req *abci.RequestVerifyVoteExtension) (r return nil, errors.New("application VerifyVoteExtension handler not set") } + emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height} + ms := app.cms.CacheMultiStore() + ctx := sdk.NewContext(ms, emptyHeader, false, app.logger).WithStreamingManager(app.streamingManager) + // If vote extensions are not enabled, as a safety precaution, we return an // error. - cp := app.GetConsensusParams(app.voteExtensionState.ctx) + cp := app.GetConsensusParams(ctx) extsEnabled := cp.Abci != nil && req.Height >= cp.Abci.VoteExtensionsEnableHeight && cp.Abci.VoteExtensionsEnableHeight != 0 if !extsEnabled { @@ -631,7 +636,7 @@ func (app *BaseApp) VerifyVoteExtension(req *abci.RequestVerifyVoteExtension) (r } }() - resp, err = app.verifyVoteExt(app.voteExtensionState.ctx, req) + resp, err = app.verifyVoteExt(ctx, req) if err != nil { app.logger.Error("failed to verify vote extension", "height", req.Height, "error", err) return &abci.ResponseVerifyVoteExtension{Status: abci.ResponseVerifyVoteExtension_REJECT}, nil diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 06bdcf437014..3f70b58ed2d6 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -364,6 +364,63 @@ func TestABCI_ExtendVote(t *testing.T) { require.Equal(t, abci.ResponseVerifyVoteExtension_REJECT, vres.Status) } +// TestABCI_OnlyVerifyVoteExtension makes sure we can call VerifyVoteExtension +// without having called ExtendVote before. +func TestABCI_OnlyVerifyVoteExtension(t *testing.T) { + name := t.Name() + db := dbm.NewMemDB() + app := baseapp.NewBaseApp(name, log.NewTestLogger(t), db, nil) + + app.SetVerifyVoteExtensionHandler(func(ctx sdk.Context, req *abci.RequestVerifyVoteExtension) (*abci.ResponseVerifyVoteExtension, error) { + // do some kind of verification here + expectedVoteExt := "foo" + hex.EncodeToString(req.Hash) + strconv.FormatInt(req.Height, 10) + if !bytes.Equal(req.VoteExtension, []byte(expectedVoteExt)) { + return &abci.ResponseVerifyVoteExtension{Status: abci.ResponseVerifyVoteExtension_REJECT}, nil + } + + return &abci.ResponseVerifyVoteExtension{Status: abci.ResponseVerifyVoteExtension_ACCEPT}, nil + }) + + app.SetParamStore(¶mStore{db: dbm.NewMemDB()}) + _, err := app.InitChain( + &abci.RequestInitChain{ + InitialHeight: 1, + ConsensusParams: &cmtproto.ConsensusParams{ + Abci: &cmtproto.ABCIParams{ + VoteExtensionsEnableHeight: 200, + }, + }, + }, + ) + require.NoError(t, err) + + // Verify Vote Extensions + _, err = app.VerifyVoteExtension(&abci.RequestVerifyVoteExtension{Height: 123, VoteExtension: []byte("1234567")}) + require.ErrorContains(t, err, "vote extensions are not enabled") + + // First vote on the first enabled height + vres, err := app.VerifyVoteExtension(&abci.RequestVerifyVoteExtension{Height: 200, Hash: []byte("thehash"), VoteExtension: []byte("foo74686568617368200")}) + require.NoError(t, err) + require.Equal(t, abci.ResponseVerifyVoteExtension_ACCEPT, vres.Status) + + vres, err = app.VerifyVoteExtension(&abci.RequestVerifyVoteExtension{Height: 1000, Hash: []byte("thehash"), VoteExtension: []byte("foo746865686173681000")}) + require.NoError(t, err) + require.Equal(t, abci.ResponseVerifyVoteExtension_ACCEPT, vres.Status) + + // Reject because it's just some random bytes + vres, err = app.VerifyVoteExtension(&abci.RequestVerifyVoteExtension{Height: 201, Hash: []byte("thehash"), VoteExtension: []byte("12345678")}) + require.NoError(t, err) + require.Equal(t, abci.ResponseVerifyVoteExtension_REJECT, vres.Status) + + // Reject because the verification failed (no error) + app.SetVerifyVoteExtensionHandler(func(ctx sdk.Context, req *abci.RequestVerifyVoteExtension) (*abci.ResponseVerifyVoteExtension, error) { + return nil, errors.New("some error") + }) + vres, err = app.VerifyVoteExtension(&abci.RequestVerifyVoteExtension{Height: 201, Hash: []byte("thehash"), VoteExtension: []byte("12345678")}) + require.NoError(t, err) + require.Equal(t, abci.ResponseVerifyVoteExtension_REJECT, vres.Status) +} + func TestABCI_GRPCQuery(t *testing.T) { grpcQueryOpt := func(bapp *baseapp.BaseApp) { testdata.RegisterQueryServer( diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 586a83f7b38e..71a297502b88 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -104,11 +104,6 @@ type BaseApp struct { // previous block's state. This state is never committed. In case of multiple // consensus rounds, the state is always reset to the previous block's state. // - // - voteExtensionState: Used for ExtendVote and VerifyVoteExtension, which is - // set based on the previous block's state. This state is never committed. In - // case of multiple rounds, the state is always reset to the previous block's - // state. - // // - processProposalState: Used for ProcessProposal, which is set based on the // the previous block's state. This state is never committed. In case of // multiple rounds, the state is always reset to the previous block's state. @@ -118,7 +113,6 @@ type BaseApp struct { checkState *state prepareProposalState *state processProposalState *state - voteExtensionState *state finalizeBlockState *state // An inter-block write-through cache provided to the context during the ABCI @@ -475,9 +469,6 @@ func (app *BaseApp) setState(mode execMode, header cmtproto.Header) { case execModeProcessProposal: app.processProposalState = baseState - case execModeVoteExtension: - app.voteExtensionState = baseState - case execModeFinalize: app.finalizeBlockState = baseState diff --git a/docs/architecture/adr-064-abci-2.0.md b/docs/architecture/adr-064-abci-2.0.md index 9a47180509c2..2d83bb710ef3 100644 --- a/docs/architecture/adr-064-abci-2.0.md +++ b/docs/architecture/adr-064-abci-2.0.md @@ -105,10 +105,11 @@ type ExtendVoteHandler func(sdk.Context, abci.RequestExtendVote) abci.ResponseEx type VerifyVoteExtensionHandler func(sdk.Context, abci.RequestVerifyVoteExtension) abci.ResponseVerifyVoteExtension ``` -A new execution state, `voteExtensionState`, will be introduced and provided as -the `Context` that is supplied to both handlers. It will contain relevant metadata -such as the block height and block hash. Note, `voteExtensionState` is never -committed and will exist as ephemeral state only in the context of a single block. +An ephemeral context and state will be supplied to both handlers. The +context will contain relevant metadata such as the block height and block hash. +The state will be a cached version of the committed state of the application and +will be discarded after the execution of the handler, this means that both handlers +get a fresh state view and no changes made to it will be written. If an application decides to implement `ExtendVoteHandler`, it must return a non-nil `ResponseExtendVote.VoteExtension`. diff --git a/docs/docs/core/00-baseapp.md b/docs/docs/core/00-baseapp.md index adfa6d6b3ead..3a0aa2651d5b 100644 --- a/docs/docs/core/00-baseapp.md +++ b/docs/docs/core/00-baseapp.md @@ -90,7 +90,6 @@ Then, parameters used to define [volatile states](#state-updates) (i.e. cached s [`Commit`](#commit) and gets re-initialized on `FinalizeBlock`. * `processProposalState`: This state is updated during [`ProcessProposal`](#process-proposal). * `prepareProposalState`: This state is updated during [`PrepareProposal`](#prepare-proposal). -* `voteExtensionState`: This state is updated during [`ExtendVote`](#extendvote) & [`VerifyVoteExtension`](#verifyvoteextension). Finally, a few more important parameters: @@ -130,7 +129,7 @@ Naturally, developers can add additional `options` based on their application's ## State Updates The `BaseApp` maintains four primary volatile states and a root or main state. The main state -is the canonical state of the application and the volatile states, `checkState`, `prepareProposalState`, `processProposalState`, `voteExtensionState` and `finalizeBlockState` +is the canonical state of the application and the volatile states, `checkState`, `prepareProposalState`, `processProposalState` and `finalizeBlockState` are used to handle state transitions in-between the main state made during [`Commit`](#commit). Internally, there is only a single `CommitMultiStore` which we refer to as the main or root state.