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

Validator client shouldn't request aggregate with no aggregators #4712

Closed
michaelsproul opened this issue Sep 7, 2023 · 2 comments
Closed
Labels
good first issue Good for newcomers low-hanging-fruit Easy to resolve, get it before someone else does! optimization Something to make Lighthouse run more efficiently. v4.6.0 ETA Q1 2024 val-client Relates to the validator client binary

Comments

@michaelsproul
Copy link
Member

Description

The validator client's logic for forming aggregate attestations will currently request an aggregate from the BN before checking whether there are any aggregators that need to sign it:

let aggregated_attestation = &self
.beacon_nodes
.first_success(
RequireSynced::No,
OfflineOnFailure::Yes,
|beacon_node| async move {
let _timer = metrics::start_timer_vec(
&metrics::ATTESTATION_SERVICE_TIMES,
&[metrics::AGGREGATES_HTTP_GET],
);
beacon_node
.get_validator_aggregate_attestation(
attestation_data.slot,
attestation_data.tree_hash_root(),
)
.await
.map_err(|e| {
format!("Failed to produce an aggregate attestation: {:?}", e)
})?
.ok_or_else(|| format!("No aggregate available for {:?}", attestation_data))
.map(|result| result.data)
},
)
.await
.map_err(|e| e.to_string())?;

This is inefficient, but also liable to cause issues with other clients, which may not be expecting to produce an aggregate when they are not subscribed to the attestation subnet. Thanks to @nflaig from Lodestar for finding and reporting this bug.

Steps to resolve

Check whether there are any aggregators (at least one selection proof that is Some) before requesting an aggregate from the BN.

@michaelsproul michaelsproul added val-client Relates to the validator client binary optimization Something to make Lighthouse run more efficiently. labels Sep 7, 2023
@michaelsproul michaelsproul added low-hanging-fruit Easy to resolve, get it before someone else does! v4.6.0 ETA Q1 2024 labels Sep 20, 2023
@michaelsproul
Copy link
Member Author

This also affects Lighthouse VC with a Prysm BN.

For easy searchability, the error produced is:

CRIT Error during attestation routine

@michaelsproul michaelsproul added the good first issue Good for newcomers label Sep 20, 2023
bors bot pushed a commit that referenced this issue Oct 5, 2023
## Issue Addressed

Closes #4712

## Proposed Changes

Exit aggregation step early if no validator is aggregator. This avoids an unnecessary request to the beacon node and more importantly fixes noisy errors if Lighthouse VC is used with other clients such as Lodestar and Prysm.

## Additional Info

Related issue ChainSafe/lodestar#5553
@jimmygchen
Copy link
Member

Completed in #4774.

Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
## Issue Addressed

Closes sigp#4712

## Proposed Changes

Exit aggregation step early if no validator is aggregator. This avoids an unnecessary request to the beacon node and more importantly fixes noisy errors if Lighthouse VC is used with other clients such as Lodestar and Prysm.

## Additional Info

Related issue ChainSafe/lodestar#5553
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
## Issue Addressed

Closes sigp#4712

## Proposed Changes

Exit aggregation step early if no validator is aggregator. This avoids an unnecessary request to the beacon node and more importantly fixes noisy errors if Lighthouse VC is used with other clients such as Lodestar and Prysm.

## Additional Info

Related issue ChainSafe/lodestar#5553
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
## Issue Addressed

Closes sigp#4712

## Proposed Changes

Exit aggregation step early if no validator is aggregator. This avoids an unnecessary request to the beacon node and more importantly fixes noisy errors if Lighthouse VC is used with other clients such as Lodestar and Prysm.

## Additional Info

Related issue ChainSafe/lodestar#5553
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers low-hanging-fruit Easy to resolve, get it before someone else does! optimization Something to make Lighthouse run more efficiently. v4.6.0 ETA Q1 2024 val-client Relates to the validator client binary
Projects
None yet
Development

No branches or pull requests

2 participants