state: Use last height changed if validator set is empty#3560
Conversation
|
we are seeing #2723 again; restarting the build to be certain all other tests pass |
| t.Fatal(err) | ||
| } | ||
| if loadedVals.Size() == 0 { | ||
| t.Fatal("Expected validators to be non-empty") |
There was a problem hiding this comment.
This code would never be reached, right? Because panic("empty validator set") would happen before? I like that these two lines make this more explicit! (keep them, I'm just asking).
There was a problem hiding this comment.
If you remove || valInfo2.ValidatorSet == nil check, it will reach this code and fail.
With || valInfo2.ValidatorSet == nil, the code does not panic and returns non-empty validator set.
There was a problem hiding this comment.
OK, just tried it locally: I did not add the || valInfo2.ValidatorSet == nil check but added the changes in the test. The function will panic before the loadedVals.Size() == 0-check can be reached because LoadValidators calls IncrementProposerPriority anyways. Which will panic with empty validator set:
tendermint/types/validator_set.go
Lines 82 to 85 in a453628
There was a problem hiding this comment.
I think it's still fine to leave the check just in case.
liamsi
left a comment
There was a problem hiding this comment.
Thanks @melekes! LGTM 👍 I did not verify querying the rpc returns a non-empty validator set. Looking at the changes and the modifications in the test, my understanding is that it indeed fixes #3537 (comment)
|
Now failing: |
What happened: New code was supposed to fall back to last height changed when/if it failed to find validators at checkpoint height (to make release non-breaking). But because we did not check if validator set is empty, the fall back logic was never executed => resulting in LoadValidators returning an empty validator set for cases where `lastStoredHeight` is checkpoint height (i.e. almost all heights if the application does not change validator set often). How it was found: one of our users - @sunboshan reported a bug here #3537 (comment)
25807f3 to
b292900
Compare
Codecov Report
@@ Coverage Diff @@
## develop #3560 +/- ##
==========================================
+ Coverage 64.17% 64.2% +0.02%
==========================================
Files 213 213
Lines 17373 17356 -17
==========================================
- Hits 11149 11143 -6
+ Misses 5301 5291 -10
+ Partials 923 922 -1
|
liamsi
left a comment
There was a problem hiding this comment.
Thanks for adding a changelog entry 👍
…3560) What happened: New code was supposed to fall back to last height changed when/if it failed to find validators at checkpoint height (to make release non-breaking). But because we did not check if validator set is empty, the fall back logic was never executed => resulting in LoadValidators returning an empty validator set for cases where `lastStoredHeight` is checkpoint height (i.e. almost all heights if the application does not change validator set often). How it was found: one of our users - @sunboshan reported a bug here tendermint#3537 (comment) * use last height changed in validator set is empty * add a changelog entry
…3560) What happened: New code was supposed to fall back to last height changed when/if it failed to find validators at checkpoint height (to make release non-breaking). But because we did not check if validator set is empty, the fall back logic was never executed => resulting in LoadValidators returning an empty validator set for cases where `lastStoredHeight` is checkpoint height (i.e. almost all heights if the application does not change validator set often). How it was found: one of our users - @sunboshan reported a bug here tendermint#3537 (comment) * use last height changed in validator set is empty * add a changelog entry
What happened:
New code v0.31.4 was supposed to fall back to last height changed when/if it
fails to find validators at checkpoint height (to make release
non-breaking).
But because we did not check if validator set is empty (only checking if we have
an info is not enough, we save info for every block), the fall back
logic was never executed => resulting in LoadValidators returning an
empty validator set for cases where
lastStoredHeightis checkpointheight (i.e. almost all heights if the application does not change
validator set often).
How it was found:
one of our users - @sunboshan reported a bug here
#3537 (comment)