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

Resolve Panic in ListValidatorBalances RPC #4051

Merged
merged 12 commits into from
Nov 22, 2019
Merged

Conversation

rauljordan
Copy link
Contributor

This resolves #4020


Description

Write why you are making the changes in this pull request

This PR resolves a bug in in ListValidatorBalances that would lead to a panic at runtime when using archival functionality:

returns rpc error: code = Unknown desc = runtime error: index out of range [345] with length 345

Specifically, this would happen under the following conditions:

  1. Current epoch is 100
  2. Request balances for epoch 50
  3. More validators exist in the state at epoch 100 than at epoch 50
  4. See the code below:
    // return everything.
    for i := 0; i < len(headState.Balances); i++ {
        res = append(res, &ethpb.ValidatorBalances_Balance{
            PublicKey: headState.Validators[i].PublicKey,
            Index:     uint64(i),
            Balance:   archivedBalances[i],
        })
    }
  1. We would loop over headState.Balances, which might have len > archivedBalances at epoch 50
  2. We would index into archivedBalances[i], which would cause a panic

Write a summary of the changes you are making

The loop should be over the archived balances, not over headState.Balances. A regression test was added to ensure the proper default responses for both archival and regular functionality so this will never happen again.

@rauljordan rauljordan self-assigned this Nov 19, 2019
@rauljordan rauljordan added Bug Something isn't working Priority: Medium Medium priority item Ready For Review A pull request ready for code review labels Nov 19, 2019
@codecov
Copy link

codecov bot commented Nov 19, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@bdbd0aa). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4051   +/-   ##
=========================================
  Coverage          ?   57.63%           
=========================================
  Files             ?      201           
  Lines             ?    12961           
  Branches          ?        0           
=========================================
  Hits              ?     7470           
  Misses            ?     4422           
  Partials          ?     1069

shayzluf
shayzluf previously approved these changes Nov 20, 2019
Copy link
Contributor

@shayzluf shayzluf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@CLAassistant
Copy link

CLAassistant commented Nov 22, 2019

CLA assistant check
All committers have signed the CLA.

@prylabs-bulldozer prylabs-bulldozer bot merged commit f461d1e into master Nov 22, 2019
@delete-merged-branch delete-merged-branch bot deleted the resolve-panic-rpc branch November 22, 2019 04:39
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 24, 2020
* adding default response
* regression test for archival
* include full regression test
* Merge branch 'master' into resolve-panic-rpc
* Merge branch 'master' into resolve-panic-rpc
* Merge branch 'master' into resolve-panic-rpc
* Merge branch 'master' into resolve-panic-rpc
* listbal
* Merge branch 'master' into resolve-panic-rpc
* Merge branch 'master' into resolve-panic-rpc
* Merge branch 'master' into resolve-panic-rpc
* Merge refs/heads/master into resolve-panic-rpc
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 28, 2020
* adding default response
* regression test for archival
* include full regression test
* Merge branch 'master' into resolve-panic-rpc
* Merge branch 'master' into resolve-panic-rpc
* Merge branch 'master' into resolve-panic-rpc
* Merge branch 'master' into resolve-panic-rpc
* listbal
* Merge branch 'master' into resolve-panic-rpc
* Merge branch 'master' into resolve-panic-rpc
* Merge branch 'master' into resolve-panic-rpc
* Merge refs/heads/master into resolve-panic-rpc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Priority: Medium Medium priority item Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ethereum API: Index out of range error when querying validator balances by epoch
4 participants