-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
fix(baseapp): Utilizing voting power from VEs in ValidateVoteExtensions #17518
Conversation
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kek
CHANGELOG.md
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* (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. |
@alexanderbez two of the CI jobs are failing but im not sure how to correct them.
Any guidance there? |
@@ -112,19 +119,14 @@ func ValidateVoteExtensions( | |||
return fmt.Errorf("failed to verify validator %X vote extension signature", valConsAddr) | |||
} | |||
|
|||
sumVP = sumVP.Add(bondedTokens) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits.
Those have been broken since the upgrade to go 1.21. Feel free to ignore. cc @elias-orijtech |
baseapp/abci_utils.go
Outdated
@@ -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) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Updated |
…ns (#17518) Co-authored-by: Aleksandr Bezobchuk <[email protected]> (cherry picked from commit bb0c866) # Conflicts: # CHANGELOG.md # docs/architecture/adr-064-abci-2.0.md
These should be upgraded to 1.21 now. Sorry for the trouble. |
…ns (backport #17518) (#17552) Co-authored-by: David Terpay <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #17517
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
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...
!
in the type prefix if API or client breaking change