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 #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 2 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 (comet) instead of the current bonded tokens (baseapp) to determine if a set of vote extensions are valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* (baseapp) [#17518](https://github.com/cosmos/cosmos-sdk/pull/17518) Utilizing voting power from vote extensions (comet) instead of the current bonded tokens (baseapp) to determine if a set of vote extensions are valid.
* (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
25 changes: 20 additions & 5 deletions baseapp/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1791,8 +1791,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().BondedTokensAndPubKeyByConsAddr(gomock.Any(), consAddr.Bytes()).Return(math.NewInt(0), tmPk, nil)

// set up baseapp
prepareOpt := func(bapp *baseapp.BaseApp) {
Expand Down Expand Up @@ -1866,8 +1865,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 +1879,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
30 changes: 16 additions & 14 deletions baseapp/abci_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ 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)
}

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,7 +93,7 @@ func ValidateVoteExtensions(
}

valConsAddr := sdk.ConsAddress(vote.Validator.Address)
bondedTokens, cmtPubKeyProto, err := valStore.BondedTokensAndPubKeyByConsAddr(ctx, valConsAddr)
_, cmtPubKeyProto, err := valStore.BondedTokensAndPubKeyByConsAddr(ctx, valConsAddr)
Copy link
Member

@julienrbrt julienrbrt Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we actually need to ignore the value, we should delete this method from x/staking and update the ValidatorStore interface.
It was added recently and has never been released (see #17164).

Copy link
Contributor

Choose a reason for hiding this comment

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

yea agreed, though I think we should think in more detail around the footgun of CometBFT power vs whats int he staking module being different and either document the weirdness or define access patterns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto. Let's address this @davidterpay and we can merge.

Copy link
Contributor Author

@davidterpay davidterpay Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll add some docs on why we are utilizing the voting power on CometBFT instead of most recent bonded tokens on base app. But at a high level, liveness of the network is dependent on the consensus power each validator has as derived by base app. But this voting power has a delay in terms of relaying to CometBFT. What this means is that for vote extensions to be in sync with the underlying consensus engine, they should default to voting power from the VEs themselves - not from baseapp at the current block. Otherwise you have liveness issues.

Copying this over from the open issue related to this.

Say there are two validators in the network, val1 and val2:

Block H - 1:

  • Val1 has a voting power of 80 (from the perspective of the consensus engine CometBFT)
  • Val2 has a voting power of 20 (from the perspective of the consensus engine CometBFT)

Only val1 signs off on block H - 1 which constitutes a valid block (> 2/3+). In block H-1, there is a redelegation transaction that moves 30 voting power from val1 to val2.

Block H:

  • is currently being constructed and contains the signed vote extensions from Block H - 1.

Within ValidateVoteExtensions,

  • Val1 has voting power of 50 (from the perspective of the application)
  • Val2 has a voting power of 50 (from the perspective of the application)

When validate vote extensions is called in prepare/process proposal, it will see that Val1 only has a voting power of 50 and will reject the set of vote extensions (even though it should still be 80 since this is what comet is utilizing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think this should be added to the ADR or within the code itself?

Copy link
Member

@julienrbrt julienrbrt Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'd say both 😬 or link the ADR in the code for explanation.

if err != nil {
return fmt.Errorf("failed to get validator %X info (bonded tokens and 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
54 changes: 27 additions & 27 deletions baseapp/abci_utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,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 +78,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().BondedTokensAndPubKeyByConsAddr(gomock.Any(), s.vals[0].consAddr.Bytes()).Return(math.NewInt(0), s.vals[0].tmPk, nil).AnyTimes()
s.valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), s.vals[1].consAddr.Bytes()).Return(math.NewInt(0), s.vals[1].tmPk, nil).AnyTimes()
s.valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), s.vals[2].consAddr.Bytes()).Return(math.NewInt(0), s.vals[2].tmPk, nil).AnyTimes()

// create context
s.ctx = sdk.Context{}.WithConsensusParams(cmtproto.ConsensusParams{
Expand All @@ -92,7 +91,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 +121,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 +167,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 +212,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 +247,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
31 changes: 8 additions & 23 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.

2 changes: 1 addition & 1 deletion tests/integration/staking/keeper/vote_extensions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func TestValidateVoteExtensions(t *testing.T) {
ve := abci.ExtendedVoteInfo{
Validator: abci.Validator{
Address: valbz,
Power: v.ConsensusPower(sdk.DefaultPowerReduction),
Power: 1000,
},
VoteExtension: voteExt,
ExtensionSignature: sig,
Expand Down
Loading