Split writeCurrentStakers into multiple functions
#3500
Conversation
| @@ -273,6 +274,34 @@ type diffValidator struct { | |||
| deletedDelegators map[ids.ID]*Staker | |||
| } | |||
|
|
|||
| func (d *diffValidator) WeightDiff() (ValidatorWeightDiff, error) { | |||
There was a problem hiding this comment.
This was previously implemented in writeCurrentStakersSubnetDiff.
| // | ||
| // Note: This function may return a nil public key and no error if the primary | ||
| // network validator does not have a public key. | ||
| func (s *state) getInheritedPublicKey(nodeID ids.NodeID) (*bls.PublicKey, error) { |
There was a problem hiding this comment.
This logic previously existed as part of writeCurrentStakersSubnetDiff.
| s.updateValidatorManager(updateValidators), | ||
| s.writeValidatorDiffs(height), | ||
| s.writeCurrentStakers(codecVersion), |
There was a problem hiding this comment.
While splitting these out is technically less efficient (and contains similar code). The functions were just getting too complex to understand. This is explicitly trading off some (imo insignificant) level of performance for clarity.
| func (s *state) writeCurrentStakers(updateValidators bool, height uint64, codecVersion uint16) error { | ||
| // getInheritedPublicKey returns the primary network validator's public key. | ||
| // | ||
| // Note: This function may return a nil public key and no error if the primary |
There was a problem hiding this comment.
(No action required) Under what circumstances would a primary network validator not have a public key? Doc pointers welcome.
There was a problem hiding this comment.
Prior to Durango, the AddValidatorTx was still supported and allowed validators to be registered without a BLS key (as opposed to AddPermisionlessValidatorTx which requires the public key).
So any validators previously added using AddValidtorTx that still haven't reached their end time will not have public keys. After the 1 year maximum staking duration from the activation of Durango, all primary network validators will have public keys
vms/platformvm/state/state.go
Outdated
| // validator set changes to disk. | ||
| // | ||
| // This function must be called prior to writeCurrentStakers. | ||
| func (s *state) writeValidatorDiffs(height uint64) error { |
There was a problem hiding this comment.
(No action required) I was going to suggest breaking this into 2 functions to simplify testing since it's so nicely divided between determining changes and then writing them, but then I saw there wasn't actually a test... 😄
There was a problem hiding this comment.
TestState_writeStakers is the test for this behavior.
There was a problem hiding this comment.
Ended up splitting up the function and expanded TestState_writeStakers
vms/platformvm/state/state.go
Outdated
| // of the primary network validator times. | ||
| return fmt.Errorf("%w: %s", errMissingPrimaryNetworkValidator, nodeID) | ||
| } | ||
| // writeValidatorDiffs writes the validator set diff contained by the pending |
There was a problem hiding this comment.
Just for my own understanding, is writeValidatorDiffs meant to only handle changes to the validators set of subnets before ConvertSubnetToL1Txs? I'm not clear on its usage vs updateValidatorManager
There was a problem hiding this comment.
writeValidatorDiffs will include the logic for L1 diffs in a later PR.
Why this should be merged
writeCurrentStakersis extremely complex as it is doing 3 things:As we implement ACP-77:
and
become more complex, as it is possible that the same
subnetID + nodeIDpair are removed from the pre-ACP-77 validator sets and added to the post-ACP-77 validator sets.To avoid
writeCurrentStakersfrom including ACP-77 validators as well (making one of the most complex functions even more complex), this PR moves the functionality into separate functions.How this works
Moves code around and implements helper functions where possible.
How this was tested
Existing tests.
Need to be documented in RELEASES.md?
There are no user facing changes.