Skip to content

Commit

Permalink
perf(types): Speedup valset.GetByAddress by noticing we do not need t…
Browse files Browse the repository at this point in the history
…o copy the validator object (cometbft#3129)

Speedup valset.GetByAddress by noticing we do not need to copy the
validator object in ~every single usage, as it operates on read-only
instances.

This acts as a very small speedup to ValidateBlock, and calculating
statistics for enter prevote. In the future we can change data
structures for bigger impact.

If we think this needs more tests to feel comfortable, I think we should
close this PR. Was a driveby change, and there are more impactful things
we can do

<!--

Please add a reference to the issue that this PR addresses and indicate
which
files are most critical to review. If it fully addresses a particular
issue,
please include "Closes #XXX" (where "XXX" is the issue number).

If this PR is non-trivial/large/complex, please ensure that you have
either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests.
The team
can be reached via GitHub Discussions or the Cosmos Network Discord
server in
the #cometbft channel. GitHub Discussions is preferred over Discord as
it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions

If the work in this PR is not aligned with the team's current
priorities, please
be advised that it may take some time before it is merged - especially
if it has
not yet been discussed with the team.

See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1

-->

---

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Andy Nogueira <[email protected]>
(cherry picked from commit 2e0a6db)
  • Loading branch information
ValarDragon authored and mergify[bot] committed Jun 3, 2024
1 parent dcca445 commit 643cdf0
Show file tree
Hide file tree
Showing 7 changed files with 25 additions and 11 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- [`types`] Make a new method `GetByAddressMut` for `ValSet`, which does not copy the returned validator.
([\#3119](https://github.com/cometbft/cometbft/issues/3119)
4 changes: 2 additions & 2 deletions consensus/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -1834,7 +1834,7 @@ func (cs *State) recordMetrics(height int64, block *types.Block) {
)
for _, ev := range block.Evidence.Evidence {
if dve, ok := ev.(*types.DuplicateVoteEvidence); ok {
if _, val := cs.Validators.GetByAddress(dve.VoteA.ValidatorAddress); val != nil {
if _, val := cs.Validators.GetByAddressMut(dve.VoteA.ValidatorAddress); val != nil {
byzantineValidatorsCount++
byzantineValidatorsPower += val.VotingPower
}
Expand Down Expand Up @@ -2381,7 +2381,7 @@ func (cs *State) calculatePrevoteMessageDelayMetrics() {

var votingPowerSeen int64
for _, v := range pl {
_, val := cs.Validators.GetByAddress(v.ValidatorAddress)
_, val := cs.Validators.GetByAddressMut(v.ValidatorAddress)
votingPowerSeen += val.VotingPower
if votingPowerSeen >= cs.Validators.TotalVotingPower()*2/3+1 {
cs.metrics.QuorumPrevoteDelay.With("proposer_address", cs.Validators.GetProposer().Address.String()).Set(v.Timestamp.Sub(cs.Proposal.Timestamp).Seconds())
Expand Down
4 changes: 2 additions & 2 deletions consensus/types/round_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func (rs *RoundState) RoundStateSimple() RoundStateSimple {
}

addr := rs.Validators.GetProposer().Address
idx, _ := rs.Validators.GetByAddress(addr)
idx, _ := rs.Validators.GetByAddressMut(addr)

return RoundStateSimple{
HeightRoundStep: fmt.Sprintf("%d/%d/%d", rs.Height, rs.Round, rs.Step),
Expand All @@ -140,7 +140,7 @@ func (rs *RoundState) RoundStateSimple() RoundStateSimple {
// NewRoundEvent returns the RoundState with proposer information as an event.
func (rs *RoundState) NewRoundEvent() types.EventDataNewRound {
addr := rs.Validators.GetProposer().Address
idx, _ := rs.Validators.GetByAddress(addr)
idx, _ := rs.Validators.GetByAddressMut(addr)

return types.EventDataNewRound{
Height: rs.Height,
Expand Down
2 changes: 1 addition & 1 deletion internal/test/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func MakeCommit(blockID types.BlockID, height int64, round int32, valSet *types.
}
addr := pk.Address()

idx, _ := valSet.GetByAddress(addr)
idx, _ := valSet.GetByAddressMut(addr)
if idx < 0 {
return nil, fmt.Errorf("validator with address %s not in validator set", addr)
}
Expand Down
2 changes: 1 addition & 1 deletion types/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func verifyCommitBatch(
if lookUpByIndex {
val = vals.Validators[idx]
} else {
valIdx, val = vals.GetByAddress(commitSig.ValidatorAddress)
valIdx, val = vals.GetByAddressMut(commitSig.ValidatorAddress)

// if the signature doesn't belong to anyone in the validator set
// then we just skip over it
Expand Down
20 changes: 16 additions & 4 deletions types/validator_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -268,9 +268,21 @@ func (vals *ValidatorSet) HasAddress(address []byte) bool {
// GetByAddress returns an index of the validator with address and validator
// itself (copy) if found. Otherwise, -1 and nil are returned.
func (vals *ValidatorSet) GetByAddress(address []byte) (index int32, val *Validator) {
i, val := vals.GetByAddressMut(address)
if i == -1 {
return -1, nil
}
return i, val.Copy()
}

// GetByAddressMut returns an index of the validator with address and the
// direct validator object if found. Mutations on this return value affect the validator set.
// This method should be used by callers who will not mutate Val.
// Otherwise, -1 and nil are returned.
func (vals *ValidatorSet) GetByAddressMut(address []byte) (index int32, val *Validator) {
for idx, val := range vals.Validators {
if bytes.Equal(val.Address, address) {
return int32(idx), val.Copy()
return int32(idx), val
}
}
return -1, nil
Expand Down Expand Up @@ -431,7 +443,7 @@ func verifyUpdates(
) (tvpAfterUpdatesBeforeRemovals int64, err error) {

delta := func(update *Validator, vals *ValidatorSet) int64 {
_, val := vals.GetByAddress(update.Address)
_, val := vals.GetByAddressMut(update.Address)
if val != nil {
return update.VotingPower - val.VotingPower
}
Expand Down Expand Up @@ -478,7 +490,7 @@ func numNewValidators(updates []*Validator, vals *ValidatorSet) int {
func computeNewPriorities(updates []*Validator, vals *ValidatorSet, updatedTotalVotingPower int64) {
for _, valUpdate := range updates {
address := valUpdate.Address
_, val := vals.GetByAddress(address)
_, val := vals.GetByAddressMut(address)
if val == nil {
// add val
// Set ProposerPriority to -C*totalVotingPower (with C ~= 1.125) to make sure validators can't
Expand Down Expand Up @@ -543,7 +555,7 @@ func verifyRemovals(deletes []*Validator, vals *ValidatorSet) (votingPower int64
removedVotingPower := int64(0)
for _, valUpdate := range deletes {
address := valUpdate.Address
_, val := vals.GetByAddress(address)
_, val := vals.GetByAddressMut(address)
if val == nil {
return removedVotingPower, fmt.Errorf("failed to find validator %X to remove", address)
}
Expand Down
2 changes: 1 addition & 1 deletion types/vote_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ func (voteSet *VoteSet) GetByAddress(address []byte) *Vote {
}
voteSet.mtx.Lock()
defer voteSet.mtx.Unlock()
valIndex, val := voteSet.valSet.GetByAddress(address)
valIndex, val := voteSet.valSet.GetByAddressMut(address)
if val == nil {
panic("GetByAddress(address) returned nil")
}
Expand Down

0 comments on commit 643cdf0

Please sign in to comment.