Skip to content

Conversation

@tthebst
Copy link
Contributor

@tthebst tthebst commented Oct 2, 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.

@michaelsproul michaelsproul added ready-for-review The code is ready for review v3.3.0 Minor release following v3.2.0 labels Oct 19, 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.

Nice!

Sorry it took so long for us to review, will merge this with the typo corrected.

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Nov 9, 2022
@michaelsproul
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request 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 <micsproul@gmail.com>
@michaelsproul
Copy link
Member

bors r-

@bors
Copy link

bors bot commented Nov 9, 2022

Canceled.

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-merge This PR is ready to merge. labels Nov 9, 2022
@michaelsproul
Copy link
Member

noticed that the simulator is failing semi-consistently with errors about sync aggregates. it may not be related to this PR but I think we should look into it before merging

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 to go now! Good catch on the flipped condition, sorry I missed that!

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Nov 13, 2022
@michaelsproul
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Nov 13, 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 <micsproul@gmail.com>
@bors bors bot changed the title Sync committee sign bn fallback [Merged by Bors] - Sync committee sign bn fallback Nov 14, 2022
@bors bors bot closed this Nov 14, 2022
@tthebst
Copy link
Contributor Author

tthebst commented Nov 14, 2022

The if statement below that checked if optimistic_execution==None was also an issue. This case is now an error in and not returned in the first_sucess iterator.

@michaelsproul
Copy link
Member

It'll be a breaking change for old versions of Lighthouse that don't have that flag, but should only affect Gnosis chain (which is still unmerged).

@michaelsproul michaelsproul added the backwards-incompat Backwards-incompatible API change label Nov 14, 2022
macladson pushed a commit to macladson/lighthouse that referenced this pull request 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 <micsproul@gmail.com>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request 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 <micsproul@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backwards-incompat Backwards-incompatible API change ready-for-merge This PR is ready to merge. v3.3.0 Minor release following v3.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants