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

davidterpay · 2023-08-23T21:40:54Z

Description

This PR updates the voting power calculations that are used to verify Vote Extensions in ValidateVoteExtensions. Instead of utilizing the current bonded tokens of a validator, we utilize the voting power provided by the vote extension itself. This ensures the liveness of networks using vote extensions since validator set updates take a few blocks to propagate to comet.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

davidterpay · 2023-08-23T21:59:11Z

baseapp/abci_utils.go

-	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 {


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

its totalVP a signed integer? lmfao 🤣

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

lol better safe than sorry...

alexanderbez · 2023-08-24T03:22:12Z

CHANGELOG.md

@@ -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.


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.

davidterpay · 2023-08-24T14:45:49Z

@alexanderbez two of the CI jobs are failing but im not sure how to correct them.

Detect leaking resources and bad code patterns
Warn about consensus code changes

Any guidance there?

itsdevbear · 2023-08-24T14:55:04Z

baseapp/abci_utils.go

@@ -112,19 +119,14 @@ func ValidateVoteExtensions(
 			return fmt.Errorf("failed to verify validator %X vote extension signature", valConsAddr)
 		}

-		sumVP = sumVP.Add(bondedTokens)


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

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)

ValidatorI has the pubkey we want

itsdevbear

nits.

julienrbrt · 2023-08-24T14:59:36Z

@alexanderbez two of the CI jobs are failing but im not sure how to correct them.

Detect leaking resources and bad code patterns

Warn about consensus code changes

Any guidance there?

Those have been broken since the upgrade to go 1.21. Feel free to ignore. cc @elias-orijtech

julienrbrt · 2023-08-24T15:01:58Z

baseapp/abci_utils.go

@@ -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)


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).

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.

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

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).

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

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

davidterpay · 2023-08-24T19:46:21Z

Updated BondedTokensAndPubKeyByConsAddr to GetPubKeyByConsAddr

…ns (#17518) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit bb0c866) # Conflicts: # CHANGELOG.md # docs/architecture/adr-064-abci-2.0.md

elias-orijtech · 2023-08-25T19:42:13Z

@alexanderbez two of the CI jobs are failing but im not sure how to correct them.

Detect leaking resources and bad code patterns

Warn about consensus code changes

Any guidance there?

Those have been broken since the upgrade to go 1.21. Feel free to ignore. cc @elias-orijtech

These should be upgraded to 1.21 now. Sorry for the trouble.

…ns (backport #17518) (#17552) Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

davidterpay added 2 commits August 23, 2023 17:33

utilizing voting power from VEs

fdd0c10

changelog

0326838

davidterpay marked this pull request as ready for review August 23, 2023 21:58

davidterpay requested a review from a team as a code owner August 23, 2023 21:58

davidterpay commented Aug 23, 2023

View reviewed changes

davidterpay changed the title ~~[Bug Fixes]: Utilizing voting power from VEs in ValidateVoteExtensions~~ fix(baseapp): Utilizing voting power from VEs in ValidateVoteExtensions Aug 23, 2023

davidterpay mentioned this pull request Aug 23, 2023

[Bug]: ValidateVoteExtensions Voting Power Mismatch #17517

Closed

alexanderbez approved these changes Aug 24, 2023

View reviewed changes

alexanderbez added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Aug 24, 2023

davidterpay and others added 2 commits August 24, 2023 10:44

nit

f5d0bf2

Merge branch 'main' into terpay/voting-power-check

cd4b843

itsdevbear reviewed Aug 24, 2023

View reviewed changes

itsdevbear approved these changes Aug 24, 2023

View reviewed changes

julienrbrt reviewed Aug 24, 2023

View reviewed changes

alexanderbez and others added 3 commits August 24, 2023 12:01

Merge branch 'main' into terpay/voting-power-check

e1adb6a

updating staking keeper interface

cb1648c

updating expected keeper mocks

dc416ce

github-actions bot added C:x/staking Type: ADR labels Aug 24, 2023

Merge branch 'main' into terpay/voting-power-check

f1ef63d

alexanderbez enabled auto-merge August 25, 2023 17:40

alexanderbez added this pull request to the merge queue Aug 25, 2023

Merged via the queue into cosmos:main with commit bb0c866 Aug 25, 2023
43 of 46 checks passed

mergify bot mentioned this pull request Aug 25, 2023

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

Merged

lasarojc mentioned this pull request Nov 9, 2023

V.E PoC Raneet10/cosmos-sdk#1

Open

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

davidterpay commented Aug 23, 2023 •

edited

Loading

davidterpay Aug 23, 2023

itsdevbear Aug 24, 2023

nivasan1 Aug 24, 2023

davidterpay Aug 24, 2023

alexanderbez Aug 24, 2023

davidterpay commented Aug 24, 2023 •

edited

Loading

itsdevbear Aug 24, 2023

davidterpay Aug 24, 2023

davidterpay Aug 24, 2023

itsdevbear left a comment

julienrbrt commented Aug 24, 2023

julienrbrt Aug 24, 2023 •

edited

Loading

itsdevbear Aug 24, 2023

alexanderbez Aug 24, 2023

davidterpay Aug 24, 2023 •

edited

Loading

davidterpay Aug 24, 2023

julienrbrt Aug 24, 2023 •

edited

Loading

davidterpay commented Aug 24, 2023

elias-orijtech commented Aug 25, 2023

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

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

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

Conversation

davidterpay commented Aug 23, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidterpay commented Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsdevbear left a comment

Choose a reason for hiding this comment

julienrbrt commented Aug 24, 2023

julienrbrt Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidterpay Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrbrt Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

davidterpay commented Aug 24, 2023

elias-orijtech commented Aug 25, 2023

davidterpay commented Aug 23, 2023 •

edited

Loading

davidterpay commented Aug 24, 2023 •

edited

Loading

julienrbrt Aug 24, 2023 •

edited

Loading

davidterpay Aug 24, 2023 •

edited

Loading

julienrbrt Aug 24, 2023 •

edited

Loading