From ed0f87915a7f9929cc426a6f13fbc7371adbadc1 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Tue, 1 Aug 2023 16:50:31 +0200 Subject: [PATCH 1/5] fix: Allow VerifyVoteExtensions without ExtendVote --- baseapp/abci.go | 18 ++++++++------ baseapp/abci_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++++ baseapp/baseapp.go | 9 ------- types/context.go | 1 - 4 files changed, 68 insertions(+), 17 deletions(-) diff --git a/baseapp/abci.go b/baseapp/abci.go index 8e5610970d7b..2dccb34d8fe5 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,19 +557,18 @@ 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). WithHeaderHash(req.Hash). - WithExecMode(sdk.ExecModeVoteExtension). WithHeaderInfo(coreheader.Info{ ChainID: app.chainID, Height: req.Height, @@ -588,7 +588,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 +608,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 +635,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 4e164b812be0..4f9207924f19 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/types/context.go b/types/context.go index ab550f5effea..40326fc908c2 100644 --- a/types/context.go +++ b/types/context.go @@ -25,7 +25,6 @@ const ( ExecModeSimulate ExecModePrepareProposal ExecModeProcessProposal - ExecModeVoteExtension ExecModeFinalize ) From 5e45ebc0d358297d0f608ace25923e5b27df873a Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Tue, 1 Aug 2023 16:53:07 +0200 Subject: [PATCH 2/5] remove execModeVoteExtension --- baseapp/baseapp.go | 1 - 1 file changed, 1 deletion(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 71a297502b88..aa71fd3c435f 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -48,7 +48,6 @@ const ( execModeSimulate // Simulate a transaction execModePrepareProposal // Prepare a block proposal execModeProcessProposal // Process a block proposal - execModeVoteExtension // Extend or verify a pre-commit vote execModeFinalize // Finalize a block proposal ) From 1f9b1d0ec971e7ea6b181bb3e4b4aca65861bde1 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Tue, 1 Aug 2023 17:26:37 +0200 Subject: [PATCH 3/5] re-add exec mode --- baseapp/abci.go | 1 + baseapp/baseapp.go | 1 + types/context.go | 1 + 3 files changed, 3 insertions(+) diff --git a/baseapp/abci.go b/baseapp/abci.go index 2dccb34d8fe5..02b2d666c3a2 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -569,6 +569,7 @@ func (app *BaseApp) ExtendVote(_ context.Context, req *abci.RequestExtendVote) ( WithBlockGasMeter(storetypes.NewInfiniteGasMeter()). WithBlockHeight(req.Height). WithHeaderHash(req.Hash). + WithExecMode(sdk.ExecModeVoteExtension). WithHeaderInfo(coreheader.Info{ ChainID: app.chainID, Height: req.Height, diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index aa71fd3c435f..71a297502b88 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -48,6 +48,7 @@ const ( execModeSimulate // Simulate a transaction execModePrepareProposal // Prepare a block proposal execModeProcessProposal // Process a block proposal + execModeVoteExtension // Extend or verify a pre-commit vote execModeFinalize // Finalize a block proposal ) diff --git a/types/context.go b/types/context.go index 40326fc908c2..ab550f5effea 100644 --- a/types/context.go +++ b/types/context.go @@ -25,6 +25,7 @@ const ( ExecModeSimulate ExecModePrepareProposal ExecModeProcessProposal + ExecModeVoteExtension ExecModeFinalize ) From 8d1a0a31aa4bf3c8674446ebff824f05df4179c5 Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Tue, 1 Aug 2023 17:47:08 +0200 Subject: [PATCH 4/5] update adr and docs --- docs/architecture/adr-064-abci-2.0.md | 9 +++++---- docs/docs/core/00-baseapp.md | 3 +-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/architecture/adr-064-abci-2.0.md b/docs/architecture/adr-064-abci-2.0.md index 98651a43abfe..f392ccae1e8c 100644 --- a/docs/architecture/adr-064-abci-2.0.md +++ b/docs/architecture/adr-064-abci-2.0.md @@ -106,10 +106,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. From 94fa294fc41218b4489f60be9eceaf740c657d3c Mon Sep 17 00:00:00 2001 From: Facundo Medica Date: Tue, 1 Aug 2023 17:53:05 +0200 Subject: [PATCH 5/5] cl++ --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1fef87d337d..c9cd618671e8 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".