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

feat!: [Comet v0.38 Integration] Vote Extensions #15766

Merged
merged 36 commits into from
Apr 19, 2023
Merged

feat!: [Comet v0.38 Integration] Vote Extensions #15766

Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
3df3084
updates
alexanderbez Apr 9, 2023
8019c2e
updates
alexanderbez Apr 9, 2023
5a069e7
updates
alexanderbez Apr 10, 2023
7c1174f
updates
alexanderbez Apr 10, 2023
f894f57
updates
alexanderbez Apr 10, 2023
1010bc9
updates
alexanderbez Apr 10, 2023
dbee603
updates
alexanderbez Apr 10, 2023
19e3c9c
updates
alexanderbez Apr 10, 2023
45ec586
updates
alexanderbez Apr 11, 2023
0d9bf07
updates
alexanderbez Apr 11, 2023
36fc06d
updates
alexanderbez Apr 11, 2023
9ed60b5
updates
alexanderbez Apr 11, 2023
e9738e6
updates
alexanderbez Apr 11, 2023
2dc6194
updates
alexanderbez Apr 11, 2023
5782ff4
updates
alexanderbez Apr 9, 2023
6532c97
updates
alexanderbez Apr 9, 2023
d67c9e0
updates
alexanderbez Apr 10, 2023
10a99ba
updates
alexanderbez Apr 10, 2023
b5aa99b
updates
alexanderbez Apr 10, 2023
1061ff5
updates
alexanderbez Apr 10, 2023
d12a8ea
updates
alexanderbez Apr 10, 2023
25e32dc
updates
alexanderbez Apr 10, 2023
137267d
updates
alexanderbez Apr 11, 2023
dd57ca6
updates
alexanderbez Apr 11, 2023
2f56bbf
updates
alexanderbez Apr 11, 2023
2e32ced
updates
tac0turtle Apr 12, 2023
136403f
updates
alexanderbez Apr 11, 2023
75873b6
updates
alexanderbez Apr 11, 2023
5d72bc4
updates
alexanderbez Apr 12, 2023
01505fc
Merge branch 'bez/feature/abci-2.0-base' into bez/abci++_vote_ext
alexanderbez Apr 12, 2023
ace8648
updates
alexanderbez Apr 12, 2023
4d8c1b7
Merge branch 'bez/feature/abci-2.0-base' into bez/abci++_vote_ext
alexanderbez Apr 15, 2023
3400180
updates
alexanderbez Apr 15, 2023
925ee71
updates
alexanderbez Apr 16, 2023
6a3104a
updates
alexanderbez Apr 17, 2023
7d5ae38
updates
alexanderbez Apr 19, 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
165 changes: 146 additions & 19 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func (app *BaseApp) InitChain(_ context.Context, req *abci.RequestInitChain) (*a
// On a new chain, we consider the init chain block height as 0, even though
// req.InitialHeight is 1 by default.
initHeader := cmtproto.Header{ChainID: req.ChainId, Time: req.Time}
app.initialHeight = req.InitialHeight

app.logger.Info("InitChain", "initialHeight", req.InitialHeight, "chainID", req.ChainId)

Expand All @@ -56,13 +57,12 @@ func (app *BaseApp) InitChain(_ context.Context, req *abci.RequestInitChain) (*a
if req.InitialHeight > 1 {
initHeader.Height = req.InitialHeight
if err := app.cms.SetInitialVersion(req.InitialHeight); err != nil {
panic(err)
}
return nil, err
}

// initialize states with a correct header
app.setState(runTxModeFinalize, initHeader)
app.setState(runTxModeCheck, initHeader)
app.setState(execModeFinalize, initHeader)
app.setState(execModeCheck, initHeader)

// Store the consensus params in the BaseApp's param store. Note, this must be
// done after the finalizeBlockState and context have been set as it's persisted
Expand Down Expand Up @@ -408,14 +408,14 @@ func (app *BaseApp) legacyEndBlock(req *abci.RequestFinalizeBlock) sdk.LegacyRes
// will contain relevant error information. Regardless of tx execution outcome,
// the ResponseCheckTx will contain relevant gas execution context.
func (app *BaseApp) CheckTx(_ context.Context, req *abci.RequestCheckTx) (*abci.ResponseCheckTx, error) {
var mode runTxMode
var mode execMode

switch {
case req.Type == abci.CheckTxType_New:
mode = runTxModeCheck
mode = execModeCheck

case req.Type == abci.CheckTxType_Recheck:
mode = runTxModeReCheck
mode = execModeReCheck

default:
return nil, fmt.Errorf("unknown RequestCheckTx type: %s", req.Type)
Expand Down Expand Up @@ -453,11 +453,19 @@ func (app *BaseApp) PrepareProposal(_ context.Context, req *abci.RequestPrepareP
return nil, errors.New("PrepareProposal method not set")
}

// always reset state given that PrepareProposal can timeout and be called again
emptyHeader := cmtproto.Header{ChainID: app.chainID}
app.setState(runTxPrepareProposal, emptyHeader)
// Always reset state given that PrepareProposal can timeout and be called
// again in a subsequent round.
header := cmtproto.Header{
ChainID: app.chainID,
Height: req.Height,
Time: req.Time,
ProposerAddress: req.ProposerAddress,
NextValidatorsHash: req.NextValidatorsHash,
}
app.setState(execModePrepareProposal, header)

// CometBFT must never call PrepareProposal with a height of 0.
//
// Ref: https://github.com/cometbft/cometbft/blob/059798a4f5b0c9f52aa8655fa619054a0154088c/spec/core/state.md?plain=1#L37-L38
if req.Height < 1 {
return nil, errors.New("PrepareProposal called with invalid height")
Expand Down Expand Up @@ -488,7 +496,8 @@ func (app *BaseApp) PrepareProposal(_ context.Context, req *abci.RequestPrepareP

resp, err = app.prepareProposal(app.prepareProposalState.ctx, req)
if err != nil {
return nil, err
app.logger.Error("failed to prepare proposal", "height", req.Height, "error", err)
return &abci.ResponsePrepareProposal{Txs: [][]byte{}}, nil
}

return resp, nil
Expand Down Expand Up @@ -520,9 +529,25 @@ func (app *BaseApp) ProcessProposal(_ context.Context, req *abci.RequestProcessP
return nil, errors.New("ProcessProposal called with invalid height")
}

// always reset state given that ProcessProposal can timeout and be called again
emptyHeader := cmtproto.Header{ChainID: app.chainID}
app.setState(runTxProcessProposal, emptyHeader)
// Always reset state given that ProcessProposal can timeout and be called
// again in a subsequent round.
header := cmtproto.Header{
ChainID: app.chainID,
Height: req.Height,
Time: req.Time,
ProposerAddress: req.ProposerAddress,
NextValidatorsHash: req.NextValidatorsHash,
}
app.setState(execModeProcessProposal, header)

// Since the application can get access to FinalizeBlock state and write to it,
// we must be sure to reset it in case ProcessProposal timeouts and is called
// again in a subsequent round. However, we only want to do this after we've
// processed the first block, as we want to avoid overwriting the finalizeState
// after state changes during InitChain.
if req.Height > app.initialHeight {
app.setState(execModeFinalize, header)
}
Comment on lines +506 to +513
Copy link
Member

Choose a reason for hiding this comment

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

Okay, now I get what you meant the other day. So it's not possible that ProcessProposal gets called more than once during the first 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.

So it's not possible that ProcessProposal gets called more than once during the first block?

It is possible. Why does that matter? This conditional is irrelevant to the round number in the first block.

Copy link
Member

Choose a reason for hiding this comment

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

Then I'm not so sure I understood you the other day lol. In other words, why this wouldn't cause an issue? I understand that we would be having multiple writes to state without discarding what was done in the previous processProposal.

1. InitChain writes to FinalizeState
2. ProcessProposal writes to FinalizeState
3. ProcessProposal gets called again and writes to FinalizeState

So step 3 would be seeing the results of steps 1+2, but I think it should only be seeing results of step 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not the full correct understanding of what's happening. What is actually happening is, in the context of the first block:

1. InitChain writes to FinalizeState
2. ProcessProposal writes to a CACHED copy of FinalizeState
3. If ProcessProposal from (2) times out, (2) is called again

So step (3) would be a on a fresh cached copy of FinalizeState from step (1).

The big caveat here though is that a chain should NOT write to FinalizeState on the first block. I'll update the godoc. This isn't a concern really though because vote extensions can be used until at least the block after the 1st block.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh thanks for taking the time to explain me, I was a bit too worried 😅


app.processProposalState.ctx = app.getContextForProposal(app.processProposalState.ctx, req.Height).
WithVoteInfos(app.voteInfos).
Expand Down Expand Up @@ -550,12 +575,114 @@ func (app *BaseApp) ProcessProposal(_ context.Context, req *abci.RequestProcessP

resp, err = app.processProposal(app.processProposalState.ctx, req)
if err != nil {
return nil, err
app.logger.Error("failed to process proposal", "height", req.Height, "error", err)
return &abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT}, nil
}

return resp, nil
}

// ExtendVote implements the ExtendVote ABCI method and returns a ResponseExtendVote.
// It calls the application's ExtendVote handler which is responsible for performing
// application-specific business logic when sending a pre-commit for the NEXT
// block height. The extensions response may be non-deterministic but must always
// be returned, even if empty.
//
// Agreed upon vote extensions are made available to the proposer of the next
// height and are committed in the subsequent height, i.e. H+2. An error is
// returned if vote extensions are not enabled or if extendVote fails or panics.
//
// Note, an error returned from ExtendVote will not cause the CometBFT to panic,
// so it is safe to return errors upon failure.
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
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.
emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height}
app.setState(execModeVoteExtension, emptyHeader)

// If vote extensions are not enabled, as a safety precaution, we return an
// error.
cp := app.GetConsensusParams(app.voteExtensionState.ctx)
if cp.Abci != nil && cp.Abci.VoteExtensionsEnableHeight <= 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a consensus module migration here? Or can we capture this somehow so we don't forget

Copy link
Contributor Author

@alexanderbez alexanderbez Apr 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

An app would have to set the VoteExtensionsEnableHeight in it's upgrade handler, yes. I dont think this is something we can do for them. That being said, you mean should we set default values in x/consensus? If so, yeah I think we can do that.

return nil, fmt.Errorf("vote extensions are not enabled; unexpected call to ExtendVote at height %d", req.Height)
}

if app.extendVote == nil {
return nil, errors.New("application ExtendVote handler not set")
}

app.voteExtensionState.ctx = app.voteExtensionState.ctx.
WithConsensusParams(cp).
WithBlockGasMeter(storetypes.NewInfiniteGasMeter()).
WithBlockHeight(req.Height).
WithHeaderHash(req.Hash)

// add a deferred recover handler in case extendVote panics
defer func() {
if err := recover(); err != nil {
Fixed Show fixed Hide fixed
app.logger.Error(
"panic recovered in ExtendVote",
"height", req.Height,
"hash", fmt.Sprintf("%X", req.Hash),
"panic", err,
)
err = fmt.Errorf("recovered application panic in ExtendVote: %w", err)
Fixed Show fixed Hide fixed
}
}()

resp, err = app.extendVote(app.voteExtensionState.ctx, req)
if err != nil {
app.logger.Error("failed to extend vote", "height", req.Height, "error", err)
return nil, fmt.Errorf("failed to extend vote: %w", err)
}

return resp, err
}

// VerifyVoteExtension implements the VerifyVoteExtension ABCI method and returns
// a ResponseVerifyVoteExtension. It calls the applications' VerifyVoteExtension
// handler which is responsible for performing application-specific business
// logic in verifying a vote extension from another validator during the pre-commit
// phase. The response MUST be deterministic. An error is returned if vote
// extensions are not enabled or if verifyVoteExt fails or panics.
//
// Note, an error returned from VerifyVoteExtension will not cause the CometBFT
// to panic, so it is safe to return errors upon failure.
func (app *BaseApp) VerifyVoteExtension(_ context.Context, req *abci.RequestVerifyVoteExtension) (resp *abci.ResponseVerifyVoteExtension, err error) {
// If vote extensions are not enabled, as a safety precaution, we return an
// error.
cp := app.GetConsensusParams(app.voteExtensionState.ctx)
if cp.Abci != nil && cp.Abci.VoteExtensionsEnableHeight <= 0 {
return nil, fmt.Errorf("vote extensions are not enabled; unexpected call to VerifyVoteExtension at height %d", req.Height)
}

if app.verifyVoteExt == nil {
return nil, errors.New("application VerifyVoteExtension handler not set")
}

// add a deferred recover handler in case verifyVoteExt panics
defer func() {
if err := recover(); err != nil {
Fixed Show fixed Hide fixed
app.logger.Error(
"panic recovered in VerifyVoteExtension",
"height", req.Height,
"hash", fmt.Sprintf("%X", req.Hash),
"validator", fmt.Sprintf("%X", req.ValidatorAddress),
"panic", err,
)
err = fmt.Errorf("recovered application panic in VerifyVoteExtension: %w", err)
Fixed Show fixed Hide fixed
}
}()

resp, err = app.verifyVoteExt(app.voteExtensionState.ctx, req)
if err != nil {
app.logger.Error("failed to verify vote extension", "height", req.Height, "error", err)
return nil, fmt.Errorf("failed to verify vote extension: %w", err)
}

return resp, err
}

func (app *BaseApp) legacyDeliverTx(tx []byte) *abci.ExecTxResult {
gInfo := sdk.GasInfo{}
resultStr := "successful"
Expand Down Expand Up @@ -584,7 +711,7 @@ func (app *BaseApp) legacyDeliverTx(tx []byte) *abci.ExecTxResult {
telemetry.SetGauge(float32(gInfo.GasWanted), "tx", "gas", "wanted")
}()

gInfo, result, anteEvents, err := app.runTx(runTxModeFinalize, tx)
gInfo, result, anteEvents, err := app.runTx(execModeFinalize, tx)
if err != nil {
resultStr = "failed"
resp = sdkerrors.ResponseExecTxResultWithEvents(
Expand Down Expand Up @@ -633,7 +760,7 @@ func (app *BaseApp) FinalizeBlock(_ context.Context, req *abci.RequestFinalizeBl
// already be initialized in InitChain. Otherwise app.finalizeBlockState will be
// nil, since it is reset on Commit.
if app.finalizeBlockState == nil {
app.setState(runTxModeFinalize, header)
app.setState(execModeFinalize, header)
} else {
// In the first block, app.finalizeBlockState.ctx will already be initialized
// by InitChain. Context is now updated with Header information.
Expand Down Expand Up @@ -719,7 +846,7 @@ func (app *BaseApp) Commit(_ context.Context, _ *abci.RequestCommit) (*abci.Resp
//
// NOTE: This is safe because CometBFT holds a lock on the mempool for
// Commit. Use the header from this latest block.
app.setState(runTxModeCheck, header)
app.setState(execModeCheck, header)

app.finalizeBlockState = nil

Expand Down Expand Up @@ -915,7 +1042,7 @@ func (app *BaseApp) FilterPeerByID(info string) *abci.ResponseQuery {
// ProcessProposal. We use finalizeBlockState on the first block to be able to
// access any state changes made in InitChain.
func (app *BaseApp) getContextForProposal(ctx sdk.Context, height int64) sdk.Context {
if height == 1 {
if height == app.initialHeight {
ctx, _ = app.finalizeBlockState.ctx.CacheContext()

// clear all context data set during InitChain to avoid inconsistent behavior
Expand Down
Loading