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

VC does not fallback for sync sigs when primary BN is optimistic #3612

Closed
paulhauner opened this issue Sep 27, 2022 · 2 comments
Closed

VC does not fallback for sync sigs when primary BN is optimistic #3612

paulhauner opened this issue Sep 27, 2022 · 2 comments
Labels
low-hanging-fruit Easy to resolve, get it before someone else does!

Comments

@paulhauner
Copy link
Member

Description

When the VC attempts to publish a sync committee signature it will try and get the head block root from the first BN that returns a response, regardless if that BN is optimistic or not:

// Fetch `block_root` and `execution_optimistic` for `SyncCommitteeContribution`.
let response = self
.beacon_nodes
.first_success(RequireSynced::Yes, OfflineOnFailure::Yes,|beacon_node| async move {
beacon_node.get_beacon_blocks_root(BlockId::Head).await
})
.await
.map_err(|e| e.to_string())?
.ok_or_else(|| format!("No block root found for slot {}", slot))?;
let block_root = response.data.root;
if let Some(execution_optimistic) = response.execution_optimistic {
if execution_optimistic {
warn!(
log,
"Refusing to sign sync committee messages for optimistic head block";
"slot" => slot,
);
return Ok(());
}
} else if let Some(bellatrix_fork_epoch) = self.duties_service.spec.bellatrix_fork_epoch {
// If the slot is post Bellatrix, do not sign messages when we cannot verify the
// optimistic status of the head block.
if slot.epoch(E::slots_per_epoch()) > bellatrix_fork_epoch {
warn!(
log,
"Refusing to sign sync committee messages for a head block with an unknown \
optimistic status";
"slot" => slot,
);
return Ok(());
}
}

I think the ideal behaviour would be trying to find a BN that has a non-optimistic head block root. At first glance I think this would be pretty easy to achieve by pushing the execution_optimistic checks inside the first_success function.

@paulhauner paulhauner added the low-hanging-fruit Easy to resolve, get it before someone else does! label Sep 27, 2022
@clifton
Copy link
Contributor

clifton commented Oct 27, 2022

is ERRO Unable to publish proposer preparation a related issue? I get that when one of my beacon nodes is optimistic and the other is synced.

@michaelsproul
Copy link
Member

@clifton Yeah, it is. That also falls under the scope of the tracking issue to improve VC fallback behaviour: #3613

bors bot pushed a commit that referenced this issue Nov 9, 2022
## Issue Addressed

Closes #3612

## Proposed Changes

- Iterates through BNs until it finds a non-optimistic head.

A slight change in error behavior: 
- Previously: `spawn_contribution_tasks` did not return an error for a non-optimistic block head. It returned `Ok(())` logged a warning.
- Now: `spawn_contribution_tasks` returns an error if it cannot find a non-optimistic block head. The caller of `spawn_contribution_tasks` then logs the error as a critical error.


Co-authored-by: Michael Sproul <[email protected]>
macladson pushed a commit to macladson/lighthouse that referenced this issue Jan 5, 2023
## Issue Addressed

Closes sigp#3612

## Proposed Changes

- Iterates through BNs until it finds a non-optimistic head.

A slight change in error behavior: 
- Previously: `spawn_contribution_tasks` did not return an error for a non-optimistic block head. It returned `Ok(())` logged a warning.
- Now: `spawn_contribution_tasks` returns an error if it cannot find a non-optimistic block head. The caller of `spawn_contribution_tasks` then logs the error as a critical error.


Co-authored-by: Michael Sproul <[email protected]>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
## Issue Addressed

Closes sigp#3612

## Proposed Changes

- Iterates through BNs until it finds a non-optimistic head.

A slight change in error behavior: 
- Previously: `spawn_contribution_tasks` did not return an error for a non-optimistic block head. It returned `Ok(())` logged a warning.
- Now: `spawn_contribution_tasks` returns an error if it cannot find a non-optimistic block head. The caller of `spawn_contribution_tasks` then logs the error as a critical error.


Co-authored-by: Michael Sproul <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low-hanging-fruit Easy to resolve, get it before someone else does!
Projects
None yet
Development

No branches or pull requests

3 participants