Skip to content

Commit

Permalink
fix(baseapp): Utilizing voting power from VEs in ValidateVoteExtensio…
Browse files Browse the repository at this point in the history 
…ns (backport #17518) (#17552)

Co-authored-by: David Terpay <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
  • Loading branch information
@mergify @davidterpay @julienrbrt
3 people authored Aug 28, 2023
1 parent 2cef4a1 commit b922a37
Show file tree
Hide file tree
Showing 10 changed files with 231 additions and 214 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ Ref: https://keepachangelog.com/en/1.0.0/

## [Unreleased]

### Bug Fixes

* (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.

## [v0.50.0-rc.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.0-rc.0) - 2023-08-18

### Features
Expand Down
26 changes: 20 additions & 6 deletions baseapp/abci_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (

errorsmod "cosmossdk.io/errors"
"cosmossdk.io/log"
"cosmossdk.io/math"
pruningtypes "cosmossdk.io/store/pruning/types"
"cosmossdk.io/store/snapshots"
snapshottypes "cosmossdk.io/store/snapshots/types"
Expand Down Expand Up @@ -1786,8 +1785,7 @@ func TestABCI_PrepareProposal_VoteExtensions(t *testing.T) {
}

consAddr := sdk.ConsAddress(addr.String())
valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), consAddr.Bytes()).Return(math.NewInt(667), tmPk, nil)
valStore.EXPECT().TotalBondedTokens(gomock.Any()).Return(math.NewInt(1000), nil).AnyTimes()
valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), consAddr.Bytes()).Return(tmPk, nil)

// set up baseapp
prepareOpt := func(bapp *baseapp.BaseApp) {
Expand Down Expand Up @@ -1861,8 +1859,7 @@ func TestABCI_PrepareProposal_VoteExtensions(t *testing.T) {
{
Validator: abci.Validator{
Address: consAddr.Bytes(),
// this is being ignored by our validation function
Power: sdk.TokensToConsensusPower(math.NewInt(1000000), sdk.DefaultPowerReduction),
Power: 666,
},
VoteExtension: ext,
ExtensionSignature: extSig,
Expand All @@ -1876,7 +1873,24 @@ func TestABCI_PrepareProposal_VoteExtensions(t *testing.T) {
require.Equal(t, 1, len(resPrepareProposal.Txs))

// now vote extensions but our sole voter doesn't reach majority
valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), consAddr.Bytes()).Return(math.NewInt(666), tmPk, nil)
reqPrepareProposal = abci.RequestPrepareProposal{
MaxTxBytes: 1000,
Height: 3, // this value can't be 0
LocalLastCommit: abci.ExtendedCommitInfo{
Round: 0,
Votes: []abci.ExtendedVoteInfo{
{
Validator: abci.Validator{
Address: consAddr.Bytes(),
Power: 666,
},
VoteExtension: ext,
ExtensionSignature: extSig,
BlockIdFlag: cmtproto.BlockIDFlagNil, // This will ignore the vote extension
},
},
},
}
resPrepareProposal, err = suite.baseApp.PrepareProposal(&reqPrepareProposal)
require.NoError(t, err)
require.Equal(t, 0, len(resPrepareProposal.Txs))
Expand Down
36 changes: 19 additions & 17 deletions baseapp/abci_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ type (
// extension signatures. Typically, this will be implemented by the x/staking
// module, which has knowledge of the CometBFT public key.
ValidatorStore interface {
TotalBondedTokens(ctx context.Context) (math.Int, error)
BondedTokensAndPubKeyByConsAddr(context.Context, sdk.ConsAddress) (math.Int, cmtprotocrypto.PublicKey, error)
GetPubKeyByConsAddr(context.Context, sdk.ConsAddress) (cmtprotocrypto.PublicKey, error)
}

// GasTx defines the contract that a transaction with a gas limit must implement.
Expand Down Expand Up @@ -62,8 +61,16 @@ func ValidateVoteExtensions(
return buf.Bytes(), nil
}

sumVP := math.NewInt(0)
var (
// Total voting power of all vote extensions.
totalVP int64
// Total voting power of all validators that submitted valid vote extensions.
sumVP int64
)

for _, vote := range extCommit.Votes {
totalVP += vote.Validator.Power

// Only check + include power if the vote is a commit vote. There must be super-majority, otherwise the
// previous block (the block vote is for) could not have been committed.
if vote.BlockIdFlag != cmtproto.BlockIDFlagCommit {
Expand All @@ -86,12 +93,12 @@ func ValidateVoteExtensions(
}

valConsAddr := sdk.ConsAddress(vote.Validator.Address)
bondedTokens, cmtPubKeyProto, err := valStore.BondedTokensAndPubKeyByConsAddr(ctx, valConsAddr)
pubKeyProto, err := valStore.GetPubKeyByConsAddr(ctx, valConsAddr)
if err != nil {
return fmt.Errorf("failed to get validator %X info (bonded tokens and public key): %w", valConsAddr, err)
return fmt.Errorf("failed to get validator %X public key: %w", valConsAddr, err)
}

cmtPubKey, err := cryptoenc.PubKeyFromProto(cmtPubKeyProto)
cmtPubKey, err := cryptoenc.PubKeyFromProto(pubKeyProto)
if err != nil {
return fmt.Errorf("failed to convert validator %X public key: %w", valConsAddr, err)
}
Expand All @@ -112,19 +119,14 @@ func ValidateVoteExtensions(
return fmt.Errorf("failed to verify validator %X vote extension signature", valConsAddr)
}

sumVP = sumVP.Add(bondedTokens)
sumVP += vote.Validator.Power
}

// Ensure we have at least 2/3 voting power that submitted valid vote
// extensions.
totalVP, err := valStore.TotalBondedTokens(ctx)
if err != nil {
return fmt.Errorf("failed to get total bonded tokens: %w", err)
}

percentSubmitted := math.LegacyNewDecFromInt(sumVP).Quo(math.LegacyNewDecFromInt(totalVP))
if percentSubmitted.LT(VoteExtensionThreshold) {
return fmt.Errorf("insufficient cumulative voting power received to verify vote extensions; got: %s, expected: >=%s", percentSubmitted, VoteExtensionThreshold)
if totalVP > 0 {
percentSubmitted := math.LegacyNewDecFromInt(math.NewInt(sumVP)).Quo(math.LegacyNewDecFromInt(math.NewInt(totalVP)))
if percentSubmitted.LT(VoteExtensionThreshold) {
return fmt.Errorf("insufficient cumulative voting power received to verify vote extensions; got: %s, expected: >=%s", percentSubmitted, VoteExtensionThreshold)
}
}

return nil
Expand Down
56 changes: 27 additions & 29 deletions baseapp/abci_utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ import (
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/suite"

"cosmossdk.io/math"

"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/baseapp/testutil/mock"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -46,10 +44,10 @@ func newTestValidator() testValidator {
}
}

func (t testValidator) toValidator() abci.Validator {
func (t testValidator) toValidator(power int64) abci.Validator {
return abci.Validator{
Address: t.consAddr.Bytes(),
Power: 0, // ignored for now
Power: power,
}
}

Expand Down Expand Up @@ -78,10 +76,9 @@ func NewABCIUtilsTestSuite(t *testing.T) *ABCIUtilsTestSuite {
s.valStore = valStore

// set up mock
s.valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), s.vals[0].consAddr.Bytes()).Return(math.NewInt(333), s.vals[0].tmPk, nil).AnyTimes()
s.valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), s.vals[1].consAddr.Bytes()).Return(math.NewInt(333), s.vals[1].tmPk, nil).AnyTimes()
s.valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), s.vals[2].consAddr.Bytes()).Return(math.NewInt(334), s.vals[2].tmPk, nil).AnyTimes()
s.valStore.EXPECT().TotalBondedTokens(gomock.Any()).Return(math.NewInt(1000), nil).AnyTimes()
s.valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), s.vals[0].consAddr.Bytes()).Return(s.vals[0].tmPk, nil).AnyTimes()
s.valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), s.vals[1].consAddr.Bytes()).Return(s.vals[1].tmPk, nil).AnyTimes()
s.valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), s.vals[2].consAddr.Bytes()).Return(s.vals[2].tmPk, nil).AnyTimes()

// create context
s.ctx = sdk.Context{}.WithConsensusParams(cmtproto.ConsensusParams{
Expand All @@ -92,7 +89,7 @@ func NewABCIUtilsTestSuite(t *testing.T) *ABCIUtilsTestSuite {
return s
}

func TestACITUtilsTestSuite(t *testing.T) {
func TestABCIUtilsTestSuite(t *testing.T) {
suite.Run(t, NewABCIUtilsTestSuite(t))
}

Expand Down Expand Up @@ -122,19 +119,19 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsHappyPath() {
Round: 0,
Votes: []abci.ExtendedVoteInfo{
{
Validator: s.vals[0].toValidator(),
Validator: s.vals[0].toValidator(333),
VoteExtension: ext,
ExtensionSignature: extSig0,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
},
{
Validator: s.vals[1].toValidator(),
Validator: s.vals[1].toValidator(333),
VoteExtension: ext,
ExtensionSignature: extSig1,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
},
{
Validator: s.vals[2].toValidator(),
Validator: s.vals[2].toValidator(334),
VoteExtension: ext,
ExtensionSignature: extSig2,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
Expand Down Expand Up @@ -168,18 +165,18 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsSingleVoteAbsent() {
Round: 0,
Votes: []abci.ExtendedVoteInfo{
{
Validator: s.vals[0].toValidator(),
Validator: s.vals[0].toValidator(333),
VoteExtension: ext,
ExtensionSignature: extSig0,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
},
// validator of power >1/3 is missing, so commit-info shld still be valid
// validator of power <1/3 is missing, so commit-info shld still be valid
{
Validator: s.vals[1].toValidator(),
Validator: s.vals[1].toValidator(333),
BlockIdFlag: cmtproto.BlockIDFlagAbsent,
},
{
Validator: s.vals[2].toValidator(),
Validator: s.vals[2].toValidator(334),
VoteExtension: ext,
ExtensionSignature: extSig2,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
Expand Down Expand Up @@ -213,18 +210,18 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsSingleVoteNil() {
Round: 0,
Votes: []abci.ExtendedVoteInfo{
{
Validator: s.vals[0].toValidator(),
Validator: s.vals[0].toValidator(333),
VoteExtension: ext,
ExtensionSignature: extSig0,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
},
// validator of power <1/3 is missing, so commit-info shld still be valid
// validator of power <1/3 is missing, so commit-info should still be valid
{
Validator: s.vals[1].toValidator(),
Validator: s.vals[1].toValidator(333),
BlockIdFlag: cmtproto.BlockIDFlagNil,
},
{
Validator: s.vals[2].toValidator(),
Validator: s.vals[2].toValidator(334),
VoteExtension: ext,
ExtensionSignature: extSig2,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
Expand All @@ -248,26 +245,27 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsTwoVotesNilAbsent() {
bz, err := marshalDelimitedFn(&cve)
s.Require().NoError(err)

extSig2, err := s.vals[2].privKey.Sign(bz)
extSig0, err := s.vals[0].privKey.Sign(bz)
s.Require().NoError(err)

llc := abci.ExtendedCommitInfo{
Round: 0,
Votes: []abci.ExtendedVoteInfo{
// validator of power >2/3 is missing, so commit-info shld still be valid
// validator of power >2/3 is missing, so commit-info should not be valid
{
Validator: s.vals[0].toValidator(),
BlockIdFlag: cmtproto.BlockIDFlagCommit,
Validator: s.vals[0].toValidator(333),
BlockIdFlag: cmtproto.BlockIDFlagCommit,
VoteExtension: ext,
ExtensionSignature: extSig0,
},
{
Validator: s.vals[1].toValidator(),
Validator: s.vals[1].toValidator(333),
BlockIdFlag: cmtproto.BlockIDFlagNil,
},
{
Validator: s.vals[2].toValidator(),
VoteExtension: ext,
ExtensionSignature: extSig2,
BlockIdFlag: cmtproto.BlockIDFlagAbsent,
Validator: s.vals[2].toValidator(334),
VoteExtension: ext,
BlockIdFlag: cmtproto.BlockIDFlagAbsent,
},
},
}
Expand Down
47 changes: 15 additions & 32 deletions baseapp/testutil/mock/mocks.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit b922a37

Please sign in to comment.