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.

Sign up for GitHub

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

Jump to bottom

Add Attestion Inclusion Distance in the HTTP API #4097

Closed
AgeManning opened this issue Mar 16, 2023 · 10 comments
Closed

Add Attestion Inclusion Distance in the HTTP API #4097

AgeManning opened this issue Mar 16, 2023 · 10 comments
Assignees
@int88
Labels
good first issue Good for newcomers gui

Comments

@AgeManning
Copy link
Member

It probably would be nice to add the attestation inclusion distance for monitored validators to display in the GUI.

This is a tracking issue for this
@AgeManning AgeManning added good first issue Good for newcomers gui labels Mar 16, 2023
@int88
Copy link
Contributor

int88 commented Mar 23, 2023

Maybe I could try this one? @AgeManning

@AgeManning
Copy link
Member Author

Yep sure if you want to take it on.

I'm not sure how much you know about lighthouse and the HTTP API. We are using warp, which makes the types kind of tricky.

I think the idea of this issue is to add an average attestation inclusion distance as an extension (or extra field) to this: #3760

You could look at that PR to figure out how to extend it. I've not yet looked into whether we store the inclusion distance in the metrics, but we could add it.

Let me know if you have any questions. I think @macladson will also be able to help out and have thoughts on how this could be implemented.

@int88
Copy link
Contributor

int88 commented Mar 24, 2023

VALIDATOR_MONITOR_PREV_EPOCH_ON_CHAIN_INCLUSION_DISTANCE seems to be the metric we want to expose, but actually it's the min attestation inclusion distance.

We should try to write some code to calculate the average distance, this method maybe a good place.

WDYT? @AgeManning

@michaelsproul
Copy link
Member

I think min inclusion distance is the right choice, the min is per validator so it just accounts for the same attestation being included in multiple blocks. E.g. if my attestation is included after 1 slot and then again after 3 slots in a different block the min distance will be 1

@AgeManning
Copy link
Member Author

Yep. If we can report the min for the last epoch in the API returned value, the UI (which will consume this endpoint) can calculate averages and stuff for us for periods greater than 1 epoch.

Just going to cc @rickimoore in on this too.

@int88
Copy link
Contributor

int88 commented Mar 29, 2023

Maybe lighthouse/analysis/attestation_performance/{index} is a better than lighthouse/ui/validator_metrics as an endpoint to get attestation inclusion distance.

Because the validator_metrics endpoint doesn't have epoch information in the query and the metrics it response are those across all epoches.

attestation_performance endpoint is the opposite, we could get attestation performance of some validators in some epoches. Also it has delay field which is quite close to the metric we want to expose, although I think it only work under Base fork version.

@AgeManning WDYT?

@michaelsproul
Copy link
Member

Using the metrics is preferable to using the analysis endpoint because they're basically "free" to compute, and will work for modern forks (nobody uses Base any more). By contrast the analysis endpoints are expensive to compute because they need to load historic states from disk, which is a) slow and b) not always guaranteed to work if those states have been deleted

@int88
Copy link
Contributor

int88 commented Mar 29, 2023

If use metrics, maybe we could just use min_inclusion_distance of the latest epoch.

Because for other metrics like attestation_target_hits is accumulated with epoches. So it's maybe inappropriate to extend the metrics endpoint that use epoch as the query parameter.

@michaelsproul WDYT?

@michaelsproul
Copy link
Member

Yeah I think that would be OK, just the min delay per validator for the most recent epoch, added to /lighthouse/ui/validator_metrics. @AgeManning can comment on whether he thinks that's sufficient for the UI

@int88 int88 mentioned this issue Mar 31, 2023
Closed
bors bot pushed a commit that referenced this issue Apr 26, 2023
@int88
add attestation inclusion distance in http api (#4148)
90d562b 
## Issue Addressed

#4097

## Proposed Changes

Add attestation inclusion distance in http api, extend `/lighthouse/ui/validator_metrics` to include it.

## Usage
```
curl -X POST "http://localhost:8001/lighthouse/ui/validator_metrics" -d '{"indices": [1]}' -H "Content-Type: application/json" | jq
```

```
{
  "data": {
    "validators": {
      "1": {
        "attestation_hits": 3,
        "attestation_misses": 1,
        "attestation_hit_percentage": 75,
        "attestation_head_hits": 3,
        "attestation_head_misses": 0,
        "attestation_head_hit_percentage": 100,
        "attestation_target_hits": 3,
        "attestation_target_misses": 0,
        "attestation_target_hit_percentage": 100,
        "attestation_inclusion_distance": 1
      }
    }
  }
}
```

## Additional Info

NA
@AgeManning
Copy link
Member Author

Resolved in #4148

ghost pushed a commit to oone-world/lighthouse that referenced this issue Jul 13, 2023
@int88
add attestation inclusion distance in http api (sigp#4148)
3eb763b 
## Issue Addressed

sigp#4097

## Proposed Changes

Add attestation inclusion distance in http api, extend `/lighthouse/ui/validator_metrics` to include it.

## Usage
```
curl -X POST "http://localhost:8001/lighthouse/ui/validator_metrics" -d '{"indices": [1]}' -H "Content-Type: application/json" | jq
```

```
{
  "data": {
    "validators": {
      "1": {
        "attestation_hits": 3,
        "attestation_misses": 1,
        "attestation_hit_percentage": 75,
        "attestation_head_hits": 3,
        "attestation_head_misses": 0,
        "attestation_head_hit_percentage": 100,
        "attestation_target_hits": 3,
        "attestation_target_misses": 0,
        "attestation_target_hit_percentage": 100,
        "attestation_inclusion_distance": 1
      }
    }
  }
}
```

## Additional Info

NA
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
@int88 @Woodpile37
add attestation inclusion distance in http api (sigp#4148)
2929662 
## Issue Addressed

sigp#4097

## Proposed Changes

Add attestation inclusion distance in http api, extend `/lighthouse/ui/validator_metrics` to include it.

## Usage
```
curl -X POST "http://localhost:8001/lighthouse/ui/validator_metrics" -d '{"indices": [1]}' -H "Content-Type: application/json" | jq
```

```
{
  "data": {
    "validators": {
      "1": {
        "attestation_hits": 3,
        "attestation_misses": 1,
        "attestation_hit_percentage": 75,
        "attestation_head_hits": 3,
        "attestation_head_misses": 0,
        "attestation_head_hit_percentage": 100,
        "attestation_target_hits": 3,
        "attestation_target_misses": 0,
        "attestation_target_hit_percentage": 100,
        "attestation_inclusion_distance": 1
      }
    }
  }
}
```

## Additional Info

NA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@int88 int88

Labels
good first issue Good for newcomers gui
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@michaelsproul @AgeManning @int88