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

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

Merged
merged 8 commits into from
Aug 25, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fdd0c10
utilizing voting power from VEs
davidterpay Aug 23, 2023
0326838
changelog
davidterpay Aug 23, 2023
f5d0bf2
nit
davidterpay Aug 24, 2023
cd4b843
Merge branch 'main' into terpay/voting-power-check
davidterpay Aug 24, 2023
e1adb6a
Merge branch 'main' into terpay/voting-power-check
alexanderbez Aug 24, 2023
cb1648c
updating staking keeper interface
davidterpay Aug 24, 2023
dc416ce
updating expected keeper mocks
davidterpay Aug 24, 2023
f1ef63d
Merge branch 'main' into terpay/voting-power-check
alexanderbez 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Bug Fixes

* (types) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583), [#17372](https://github.com/cosmos/cosmos-sdk/pull/17372) Add `MigrationModuleManager` to handle migration of upgrade module before other modules, ensuring access to the updated context with consensus parameters within the same block that executes the migration.
* (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.

### API Breaking Changes

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 @@ -1791,8 +1790,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 @@ -1866,8 +1864,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 @@ -1881,7 +1878,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)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is such a footgun. bondedTokens is actually useless in the ValidatorStore interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea wasn't sure if updating the staking keeper's BondedTokensAndPubKeyByConsAddr would break anything elsewhere. alternatively looks like we could just use ValidatorByConsAddr(context.Context, sdk.ConsAddress) (ValidatorI, error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ValidatorI has the pubkey we want

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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this should ever be possible, but adding this as a sanity check.

Copy link
Contributor

Choose a reason for hiding this comment

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

its totalVP a signed integer? lmfao 🤣

I'd flip the logic around, personally. <= 0

Copy link
Contributor

Choose a reason for hiding this comment

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

lol better safe than sorry...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kek

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