From 1e245614c6c235952f9ccabbeeff27720912fa82 Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Thu, 28 Sep 2023 16:24:54 +0200 Subject: [PATCH 1/2] fix(baseapp): vote extensions on first block (#17909) (cherry picked from commit c889a07ef236d6e302df9983cc76868c641443d6) # Conflicts: # baseapp/abci.go --- baseapp/abci.go | 34 +++++++++++++++++ baseapp/abci_test.go | 89 +++++++++++++++++++++++++++++++++++++------ baseapp/abci_utils.go | 5 ++- 3 files changed, 115 insertions(+), 13 deletions(-) diff --git a/baseapp/abci.go b/baseapp/abci.go index 28b0326cf583..7ecffa2dfc05 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -549,9 +549,23 @@ func (app *BaseApp) ProcessProposal(req *abci.RequestProcessProposal) (resp *abc func (app *BaseApp) ExtendVote(_ context.Context, req *abci.RequestExtendVote) (resp *abci.ResponseExtendVote, err error) { // Always reset state given that ExtendVote and VerifyVoteExtension can timeout // and be called again in a subsequent round. +<<<<<<< HEAD emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height} ms := app.cms.CacheMultiStore() ctx := sdk.NewContext(ms, emptyHeader, false, app.logger).WithStreamingManager(app.streamingManager) +======= + var ctx sdk.Context + + // If we're extending the vote for the initial height, we need to use the + // finalizeBlockState context, otherwise we don't get the uncommitted data + // from InitChain. + if req.Height == app.initialHeight { + ctx, _ = app.finalizeBlockState.ctx.CacheContext() + } else { + ms := app.cms.CacheMultiStore() + ctx = sdk.NewContext(ms, false, app.logger).WithStreamingManager(app.streamingManager).WithChainID(app.chainID).WithBlockHeight(req.Height) + } +>>>>>>> c889a07ef (fix(baseapp): vote extensions on first block (#17909)) if app.extendVote == nil { return nil, errors.New("application ExtendVote handler not set") @@ -561,6 +575,10 @@ func (app *BaseApp) ExtendVote(_ context.Context, req *abci.RequestExtendVote) ( // error. cp := app.GetConsensusParams(ctx) + // Note: In this case, we do want to extend vote if the height is equal or + // greater than VoteExtensionsEnableHeight. This defers from the check done + // in ValidateVoteExtensions and PrepareProposal in which we'll check for + // vote extensions on VoteExtensionsEnableHeight+1. 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) @@ -611,14 +629,30 @@ func (app *BaseApp) VerifyVoteExtension(req *abci.RequestVerifyVoteExtension) (r return nil, errors.New("application VerifyVoteExtension handler not set") } +<<<<<<< HEAD emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height} ms := app.cms.CacheMultiStore() ctx := sdk.NewContext(ms, emptyHeader, false, app.logger).WithStreamingManager(app.streamingManager) +======= + var ctx sdk.Context + + // If we're verifying the vote for the initial height, we need to use the + // finalizeBlockState context, otherwise we don't get the uncommitted data + // from InitChain. + if req.Height == app.initialHeight { + ctx, _ = app.finalizeBlockState.ctx.CacheContext() + } else { + ms := app.cms.CacheMultiStore() + ctx = sdk.NewContext(ms, false, app.logger).WithStreamingManager(app.streamingManager).WithChainID(app.chainID).WithBlockHeight(req.Height) + } +>>>>>>> c889a07ef (fix(baseapp): vote extensions on first block (#17909)) // If vote extensions are not enabled, as a safety precaution, we return an // error. cp := app.GetConsensusParams(ctx) + // Note: we verify votes extensions on VoteExtensionsEnableHeight+1. Check + // comment in ExtendVote and ValidateVoteExtensions for more details. 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 VerifyVoteExtension at height %d", req.Height) diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index c7462474b307..95f8d28e0118 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -1790,16 +1790,15 @@ func TestABCI_PrepareProposal_VoteExtensions(t *testing.T) { // set up baseapp prepareOpt := func(bapp *baseapp.BaseApp) { bapp.SetPrepareProposal(func(ctx sdk.Context, req *abci.RequestPrepareProposal) (*abci.ResponsePrepareProposal, error) { + err := baseapp.ValidateVoteExtensions(ctx, valStore, req.Height, bapp.ChainID(), req.LocalLastCommit) + if err != nil { + return nil, err + } + cp := ctx.ConsensusParams() extsEnabled := cp.Abci != nil && req.Height >= cp.Abci.VoteExtensionsEnableHeight && cp.Abci.VoteExtensionsEnableHeight != 0 if extsEnabled { - err := baseapp.ValidateVoteExtensions(ctx, valStore, req.Height, bapp.ChainID(), req.LocalLastCommit) - if err != nil { - return nil, err - } - req.Txs = append(req.Txs, []byte("some-tx-that-does-something-from-votes")) - } return &abci.ResponsePrepareProposal{Txs: req.Txs}, nil }) @@ -2053,6 +2052,19 @@ func TestBaseApp_PreBlocker(t *testing.T) { // TestBaseApp_VoteExtensions tests vote extensions using a price as an example. func TestBaseApp_VoteExtensions(t *testing.T) { + ctrl := gomock.NewController(t) + valStore := mock.NewMockValidatorStore(ctrl) + + // for brevity and simplicity, all validators have the same key + privKey := secp256k1.GenPrivKey() + pubKey := privKey.PubKey() + tmPk := cmtprotocrypto.PublicKey{ + Sum: &cmtprotocrypto.PublicKey_Secp256K1{ + Secp256K1: pubKey.Bytes(), + }, + } + valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), gomock.Any()).Return(tmPk, nil).AnyTimes() + baseappOpts := func(app *baseapp.BaseApp) { app.SetExtendVoteHandler(func(sdk.Context, *abci.RequestExtendVote) (*abci.ResponseExtendVote, error) { // here we would have a process to get the price from an external source @@ -2075,7 +2087,9 @@ func TestBaseApp_VoteExtensions(t *testing.T) { app.SetPrepareProposal(func(ctx sdk.Context, req *abci.RequestPrepareProposal) (*abci.ResponsePrepareProposal, error) { txs := [][]byte{} - + if err := baseapp.ValidateVoteExtensions(ctx, valStore, req.Height, app.ChainID(), req.LocalLastCommit); err != nil { + return nil, err + } // add all VE as txs (in a real scenario we would need to check signatures too) for _, v := range req.LocalLastCommit.Votes { if len(v.VoteExtension) == 8 { @@ -2181,14 +2195,23 @@ func TestBaseApp_VoteExtensions(t *testing.T) { }, } - // add all VEs to the local last commit + // add all VEs to the local last commit, which will make PrepareProposal fail + // because it's not expecting to receive vote extensions when height == VoteExtensionsEnableHeight for _, ve := range allVEs { - prepPropReq.LocalLastCommit.Votes = append(prepPropReq.LocalLastCommit.Votes, abci.ExtendedVoteInfo{VoteExtension: ve}) + prepPropReq.LocalLastCommit.Votes = append(prepPropReq.LocalLastCommit.Votes, abci.ExtendedVoteInfo{ + VoteExtension: ve, + BlockIdFlag: cmtproto.BlockIDFlagCommit, + ExtensionSignature: []byte{}, // doesn't matter, it's just to make the next PrepareProposal fail + }) } - resp, err := suite.baseApp.PrepareProposal(prepPropReq) + require.Len(t, resp.Txs, 0) // this is actually a failure, but we don't want to halt the chain + require.NoError(t, err) // we don't error here + + prepPropReq.LocalLastCommit.Votes = []abci.ExtendedVoteInfo{} // reset votes + resp, err = suite.baseApp.PrepareProposal(prepPropReq) require.NoError(t, err) - require.Len(t, resp.Txs, 10) + require.Len(t, resp.Txs, 0) procPropRes, err := suite.baseApp.ProcessProposal(&abci.RequestProcessProposal{Height: 1, Txs: resp.Txs}) require.NoError(t, err) @@ -2197,8 +2220,50 @@ func TestBaseApp_VoteExtensions(t *testing.T) { _, err = suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{Height: 1, Txs: resp.Txs}) require.NoError(t, err) - // Check if the average price was available in FinalizeBlock's context + // The average price will be nil during the first block, given that we don't have + // any vote extensions on block 1 in PrepareProposal avgPrice := getFinalizeBlockStateCtx(suite.baseApp).KVStore(capKey1).Get([]byte("avgPrice")) + require.Nil(t, avgPrice) + _, err = suite.baseApp.Commit() + require.NoError(t, err) + + // Now onto the second block, this time we process vote extensions from the + // previous block (which we sign now) + for _, ve := range allVEs { + cve := cmtproto.CanonicalVoteExtension{ + Extension: ve, + Height: 1, + Round: int64(0), + ChainId: suite.baseApp.ChainID(), + } + + bz, err := marshalDelimitedFn(&cve) + require.NoError(t, err) + + extSig, err := privKey.Sign(bz) + require.NoError(t, err) + + prepPropReq.LocalLastCommit.Votes = append(prepPropReq.LocalLastCommit.Votes, abci.ExtendedVoteInfo{ + VoteExtension: ve, + BlockIdFlag: cmtproto.BlockIDFlagCommit, + ExtensionSignature: extSig, + }) + } + + prepPropReq.Height = 2 + resp, err = suite.baseApp.PrepareProposal(prepPropReq) + require.NoError(t, err) + require.Len(t, resp.Txs, 10) + + procPropRes, err = suite.baseApp.ProcessProposal(&abci.RequestProcessProposal{Height: 2, Txs: resp.Txs}) + require.NoError(t, err) + require.Equal(t, abci.ResponseProcessProposal_ACCEPT, procPropRes.Status) + + _, err = suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{Height: 2, Txs: resp.Txs}) + require.NoError(t, err) + + // Check if the average price was available in FinalizeBlock's context + avgPrice = getFinalizeBlockStateCtx(suite.baseApp).KVStore(capKey1).Get([]byte("avgPrice")) require.NotNil(t, avgPrice) require.GreaterOrEqual(t, binary.BigEndian.Uint64(avgPrice), uint64(10000000)) require.Less(t, binary.BigEndian.Uint64(avgPrice), uint64(11000000)) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 9a8da22c191c..3f58540687fb 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -51,7 +51,10 @@ func ValidateVoteExtensions( extCommit abci.ExtendedCommitInfo, ) error { cp := ctx.ConsensusParams() - extsEnabled := cp.Abci != nil && currentHeight >= cp.Abci.VoteExtensionsEnableHeight && cp.Abci.VoteExtensionsEnableHeight != 0 + // Start checking vote extensions only **after** the vote extensions enable + // height, because when `currentHeight == VoteExtensionsEnableHeight` + // PrepareProposal doesn't get any vote extensions in its request. + extsEnabled := cp.Abci != nil && currentHeight > cp.Abci.VoteExtensionsEnableHeight && cp.Abci.VoteExtensionsEnableHeight != 0 marshalDelimitedFn := func(msg proto.Message) ([]byte, error) { var buf bytes.Buffer if err := protoio.NewDelimitedWriter(&buf).WriteMsg(msg); err != nil { From 5430591fa7b2ddb83441b33ac848d79d3b255579 Mon Sep 17 00:00:00 2001 From: Facundo Date: Fri, 29 Sep 2023 13:35:42 +0200 Subject: [PATCH 2/2] fix conflicts --- baseapp/abci.go | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/baseapp/abci.go b/baseapp/abci.go index 7ecffa2dfc05..769f6a086ca5 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -549,11 +549,6 @@ func (app *BaseApp) ProcessProposal(req *abci.RequestProcessProposal) (resp *abc func (app *BaseApp) ExtendVote(_ context.Context, req *abci.RequestExtendVote) (resp *abci.ResponseExtendVote, err error) { // Always reset state given that ExtendVote and VerifyVoteExtension can timeout // and be called again in a subsequent round. -<<<<<<< HEAD - emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height} - ms := app.cms.CacheMultiStore() - ctx := sdk.NewContext(ms, emptyHeader, false, app.logger).WithStreamingManager(app.streamingManager) -======= var ctx sdk.Context // If we're extending the vote for the initial height, we need to use the @@ -562,10 +557,10 @@ func (app *BaseApp) ExtendVote(_ context.Context, req *abci.RequestExtendVote) ( if req.Height == app.initialHeight { ctx, _ = app.finalizeBlockState.ctx.CacheContext() } else { + emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height} ms := app.cms.CacheMultiStore() - ctx = sdk.NewContext(ms, false, app.logger).WithStreamingManager(app.streamingManager).WithChainID(app.chainID).WithBlockHeight(req.Height) + ctx = sdk.NewContext(ms, emptyHeader, false, app.logger).WithStreamingManager(app.streamingManager) } ->>>>>>> c889a07ef (fix(baseapp): vote extensions on first block (#17909)) if app.extendVote == nil { return nil, errors.New("application ExtendVote handler not set") @@ -629,11 +624,6 @@ func (app *BaseApp) VerifyVoteExtension(req *abci.RequestVerifyVoteExtension) (r return nil, errors.New("application VerifyVoteExtension handler not set") } -<<<<<<< HEAD - emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height} - ms := app.cms.CacheMultiStore() - ctx := sdk.NewContext(ms, emptyHeader, false, app.logger).WithStreamingManager(app.streamingManager) -======= var ctx sdk.Context // If we're verifying the vote for the initial height, we need to use the @@ -642,10 +632,10 @@ func (app *BaseApp) VerifyVoteExtension(req *abci.RequestVerifyVoteExtension) (r if req.Height == app.initialHeight { ctx, _ = app.finalizeBlockState.ctx.CacheContext() } else { + emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height} ms := app.cms.CacheMultiStore() - ctx = sdk.NewContext(ms, false, app.logger).WithStreamingManager(app.streamingManager).WithChainID(app.chainID).WithBlockHeight(req.Height) + ctx = sdk.NewContext(ms, emptyHeader, false, app.logger).WithStreamingManager(app.streamingManager) } ->>>>>>> c889a07ef (fix(baseapp): vote extensions on first block (#17909)) // If vote extensions are not enabled, as a safety precaution, we return an // error.