Navigation Menu

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

[Merged by Bors] - Fix attestation performance API InvalidValidatorIndex error #3503

Closed
wants to merge 2 commits into from

Conversation

macladson
Copy link
Member

Issue Addressed

When requesting an index which is not active during start_epoch, Lighthouse returns:

curl "http://localhost:5052/lighthouse/analysis/attestation_performance/999999999?start_epoch=100000&end_epoch=100000"
{
  "code": 500,
  "message": "INTERNAL_SERVER_ERROR: ParticipationCache(InvalidValidatorIndex(999999999))",
  "stacktraces": []
}

This error occurs even when the index in question becomes active before end_epoch which is undesirable as it can prevent larger queries from completing.

Proposed Changes

In the event the index is out-of-bounds (has not yet been activated), simply return all fields as false:

-> curl "http://localhost:5052/lighthouse/analysis/attestation_performance/999999999?start_epoch=100000&end_epoch=100000"
[
  {
    "index": 999999999,
    "epochs": {
      "100000": {
        "active": false,
        "head": false,
        "target": false,
        "source": false
      }
    }
  }
]

By doing this, we cover the case where a validator becomes active sometime between start_epoch and end_epoch.

Additional Info

Note that this error only occurs for epochs after the Altair hard fork.

@macladson macladson added the ready-for-review The code is ready for review label Aug 24, 2022
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

This looks good.

I'm wondering whether we should also update the comment here to reflect that these functions do not in fact return false when the index is out of bounds, and may return an error:

/// ## Notes
///
/// Always returns `false` for an unknown `val_index`.
pub fn is_previous_epoch_target_attester(

Or, arguably we should handle the Error::InvalidValidatorIndex(..) in all those EpochProcessingSummary functions so that the comments are accurate once again. I think this is quite safe, as those functions are only used by the analysis APIs and the validator monitor, unlike the participation cache functions themselves which are used in consensus-critical code and need to return errors for out-of-bounds validator indices.

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks good!

I've double-checked that the 4 functions modified are only used in the validator monitor and the API.

Let's merge this in v3.1.1. Will mark as blocked until v3.1.0 goes out

@michaelsproul michaelsproul added blocked ready-for-merge This PR is ready to merge. v3.1.2 Release after v3.1.0 (formerly v3.1.1) and removed ready-for-review The code is ready for review blocked labels Aug 31, 2022
@michaelsproul
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 5, 2022
## Issue Addressed

When requesting an index which is not active during `start_epoch`, Lighthouse returns: 
```
curl "http://localhost:5052/lighthouse/analysis/attestation_performance/999999999?start_epoch=100000&end_epoch=100000"
```
```json
{
  "code": 500,
  "message": "INTERNAL_SERVER_ERROR: ParticipationCache(InvalidValidatorIndex(999999999))",
  "stacktraces": []
}
```

This error occurs even when the index in question becomes active before `end_epoch` which is undesirable as it can prevent larger queries from completing.

## Proposed Changes

In the event the index is out-of-bounds (has not yet been activated), simply return all fields as `false`:

```
-> curl "http://localhost:5052/lighthouse/analysis/attestation_performance/999999999?start_epoch=100000&end_epoch=100000"
```
```json
[
  {
    "index": 999999999,
    "epochs": {
      "100000": {
        "active": false,
        "head": false,
        "target": false,
        "source": false
      }
    }
  }
]
```

By doing this, we cover the case where a validator becomes active sometime between `start_epoch` and `end_epoch`.

## Additional Info

Note that this error only occurs for epochs after the Altair hard fork.
@bors bors bot changed the title Fix attestation performance API InvalidValidatorIndex error [Merged by Bors] - Fix attestation performance API InvalidValidatorIndex error Sep 5, 2022
@bors bors bot closed this Sep 5, 2022
@macladson macladson deleted the attestation-perf-fix branch September 6, 2022 02:48
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
)

## Issue Addressed

When requesting an index which is not active during `start_epoch`, Lighthouse returns: 
```
curl "http://localhost:5052/lighthouse/analysis/attestation_performance/999999999?start_epoch=100000&end_epoch=100000"
```
```json
{
  "code": 500,
  "message": "INTERNAL_SERVER_ERROR: ParticipationCache(InvalidValidatorIndex(999999999))",
  "stacktraces": []
}
```

This error occurs even when the index in question becomes active before `end_epoch` which is undesirable as it can prevent larger queries from completing.

## Proposed Changes

In the event the index is out-of-bounds (has not yet been activated), simply return all fields as `false`:

```
-> curl "http://localhost:5052/lighthouse/analysis/attestation_performance/999999999?start_epoch=100000&end_epoch=100000"
```
```json
[
  {
    "index": 999999999,
    "epochs": {
      "100000": {
        "active": false,
        "head": false,
        "target": false,
        "source": false
      }
    }
  }
]
```

By doing this, we cover the case where a validator becomes active sometime between `start_epoch` and `end_epoch`.

## Additional Info

Note that this error only occurs for epochs after the Altair hard fork.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. v3.1.2 Release after v3.1.0 (formerly v3.1.1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants