send proposer set updates in x/staking EndBlocker ABCI validator updates#73
Conversation
|
|
||
| // Set SendFullProposerSetAbciUpdate to true to trigger a full proposer set update | ||
| // in EndBlocker | ||
| if err := k.Keeper.SetSendFullProposerSetAbciUpdate(ctx, true); err != nil { |
There was a problem hiding this comment.
Probably better to call this in keeper.SetProposers to guarantee atomicity?
| return bz[0] == 1, nil | ||
| } | ||
|
|
||
| // SetSendFullProposerSetAbciUpdate sets whether a full proposer set ABCI update is needed |
There was a problem hiding this comment.
Comment that we are ok with full send approach because CanPropose changes very infrequently
| github.com/dvsekhvalnov/jose2go v1.6.0/go.mod h1:QsHjhyTlD/lAVqn/NSbVZmSCGeDehTB/mPZadG+mhXU= | ||
| github.com/dydxprotocol/cometbft v0.38.6-0.20241126215519-69cdde955fd0 h1:KBMuBNAE91SVeULnq2XBnmSDGeimI6aM1+YxlLb0yOI= | ||
| github.com/dydxprotocol/cometbft v0.38.6-0.20241126215519-69cdde955fd0/go.mod h1:XSQX1hQbr54qaJb4/5YNNZGXkAQHHa6bi/KMcN1SQ7w= | ||
| github.com/dydxprotocol/cometbft v0.38.6-0.20250731170939-ab5587555808 h1:k6zD9fZD4O4H2sMoBsW66uMyx+MtXDpmCqjmXtlG+38= |
There was a problem hiding this comment.
What is this pointing to and why doesn't the PR build?
There was a problem hiding this comment.
this is pointing to the latest commit on comet's proposer set feature branch. build fails as it doesn't recognize CanPropose field but unit tests all are fine. will look into resolving it
| updates = append(updates, validator.ABCIValidatorUpdate(powerReduction)) | ||
| // update the validator set if power has changed or if a full proposer set update is needed | ||
| if !found || !bytes.Equal(oldPowerBytes, newPowerBytes) || sendFullProposerSetUpdate { | ||
| update := validator.ABCIValidatorUpdate(powerReduction) |
There was a problem hiding this comment.
ABCIValidatorUpdate not taking in CanUpdate isn't very clean considering it defaults canPropose to false. This reduces readability and opens up room for bugs.
Can we construct the update with CanUpdates, instead of updating the struct with update.CanPropose = .... below
There was a problem hiding this comment.
good catch! did this for my proof of concept and forgot to change this to take in CanPropose as a parameter instead
| } | ||
|
|
||
| updates = append(updates, validator.ABCIValidatorUpdateZero()) | ||
| update := validator.ABCIValidatorUpdateZero() |
There was a problem hiding this comment.
Same as above, instead of update.CanPropose = .... pass in the boolean to ABCIValidatorUpdateZero
… when constructing validator updates
| } | ||
|
|
||
| // Check if we need to send a full proposer set update | ||
| sendFullProposerSetUpdate, err := k.GetSendFullProposerSetAbciUpdate(ctx) |
There was a problem hiding this comment.
Would it be easy to do another sanity check if sendFullProposerSetUpdate is true, that we have at least X validators with canPropose =true? Just trying to reduce possibilities for chain halting state changes as much as possible.
And since this is endblocker, if invariant check fails just log error and default to all validators being able to propose or sth
Wdyt?
There was a problem hiding this comment.
good idea! can be a good fail safe that we don't expect to hit
| updates = append(updates, update) | ||
| } | ||
|
|
||
| // Set SendFullProposerSetAbciUpdate to `false` to avoid sending unnecessary validator updates. |
There was a problem hiding this comment.
Is comment accurate? We are just marking "done" after emitting updates right
There was a problem hiding this comment.
yep we are just marking "done". i was trying to say that if we don't set this to false, we would keep sending full proposer set updates every block
There was a problem hiding this comment.
updated the comment to make it clearer
* feat(proposer-set-reduction): Add proposer set state to x/staking (#71) * add proposer set state to x/staking * all validators are proposers when none is set and add more tests * get and set entire proposers list instead of each individual proposer * have a constant define min num of bonded in proposer set * fix lint and better error name * add gov msg that sets and query that reads proposer set (#72) * add gov msg that sets and query that reads proposer set * add comment on invariant checks * send proposer set updates in x/staking EndBlocker ABCI validator updates (#73) * send proposer set updates in x/staking EndBlocker ABCI validator updates * set full update flag in SetProposers keeper method and set CanPropose when constructing validator updates * update simapp's cometbft dependency * update cometbft dependency of /tests * add sanity check on full proposer set update * use cometbft commit on main (#74)
* feat(proposer-set-reduction): Add proposer set state to x/staking (dydxprotocol#71) * add proposer set state to x/staking * all validators are proposers when none is set and add more tests * get and set entire proposers list instead of each individual proposer * have a constant define min num of bonded in proposer set * fix lint and better error name * add gov msg that sets and query that reads proposer set (dydxprotocol#72) * add gov msg that sets and query that reads proposer set * add comment on invariant checks * send proposer set updates in x/staking EndBlocker ABCI validator updates (dydxprotocol#73) * send proposer set updates in x/staking EndBlocker ABCI validator updates * set full update flag in SetProposers keeper method and set CanPropose when constructing validator updates * update simapp's cometbft dependency * update cometbft dependency of /tests * add sanity check on full proposer set update * use cometbft commit on main (dydxprotocol#74)
2 participants
Description
in x/staking EndBlocker validator updates, include proposer set updates by setting
CanProposefield of each validator update.additionally introduced a flag in state
SendFullProposerSetUpdate(whichMsgSetProposersmsg handler will set to true) and when true: x/staking EndBlocker will return validator updates for all validators (besides the ones that have power changes)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.mdmake lintandmake testReviewers 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