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

fix(baseapp): Utilizing voting power from VEs in ValidateVoteExtensions (backport #17518) #17552

Merged
merged 2 commits into from
Aug 28, 2023
Merged

fix(baseapp): Utilizing voting power from VEs in ValidateVoteExtensions (backport #17518) #17552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e22a6e6
fix(baseapp): Utilizing voting power from VEs in ValidateVoteExtensio…
davidterpay Aug 25, 2023
5424880
fix conflicts
julienrbrt Aug 25, 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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
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.
Copy link
Member

Choose a reason for hiding this comment

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

I leave so changelog so that people on rc.0 can check what changed by looking at the commit before rc.1.
However this will not be included in the collapsed changelog of rc.1 as this is still part of the same vote extension addition compared to v0.47. Possibly we can link the PR to abci 2.0 implementation.

Copy link
Member

Choose a reason for hiding this comment

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

leaving for now, we can merge later on

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's what I meant 😀


## [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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop

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

Loading