Skip to content
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

Correct how AllValidatorsAreExited creates status request #7758

Merged
merged 6 commits into from
Nov 10, 2020

Conversation

dv8silencer
Copy link
Contributor

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?
This bug fixes #7707 -- If you look at this in validator/client/validator.go

	var publicKeys [][]byte
	for _, key := range validatingKeys {
		publicKeys = append(publicKeys, key[:])
	}
	request := &ethpb.MultipleValidatorStatusRequest{
		PublicKeys: publicKeys,
	}

What I saw during debugging is that after the range-for loop, publicKeys ends up with a set of identical keys that match the last key appended. This is likely because the iterating variable (key)'s memory address is appended and since this address will last hold the last key iterated over in validatorKeys, publicKey ends up with multiple copies of the same key. If the validator associated with the last key is exited, then this function will true that all validators are exited and this leads to the described bug. To fix this I just create a copy and append that instead. There is a regression test created as well which creates a couple of sample keys and sees if the request is as expected. This test fails on current master and passes with this PR.

Which issues(s) does this PR fix?

Fixes #7707

Other notes for review

@prylabs-bulldozer prylabs-bulldozer bot merged commit b4bce7c into prysmaticlabs:master Nov 10, 2020
@codecov
Copy link

codecov bot commented Nov 10, 2020

Codecov Report

Merging #7758 (7181cc7) into master (d4c9546) will decrease coverage by 0.01%.
The diff coverage is 52.70%.

@@            Coverage Diff             @@
##           master    #7758      +/-   ##
==========================================
- Coverage   62.15%   62.14%   -0.02%     
==========================================
  Files         429      429              
  Lines       30248    30290      +42     
==========================================
+ Hits        18802    18824      +22     
- Misses       8521     8536      +15     
- Partials     2925     2930       +5     

@prestonvanloon
Copy link
Member

Excellent find! Thanks!

@dv8silencer dv8silencer deleted the dv8-i7707 branch November 10, 2020 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All validators are exited, no more work to perform...
3 participants