Skip to content

Conversation

@paulhauner
Copy link
Member

@paulhauner paulhauner commented Aug 15, 2020

Issue Addressed

NA

Proposed Changes

Moves beacon block processing over to the newly-added GossipProcessor. This moves the task off the core executor onto the blocking one.

Additional Info

  • With this PR, gossip blocks are being ignored during sync.

@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Aug 15, 2020
@paulhauner paulhauner marked this pull request as ready for review August 17, 2020 05:34
@paulhauner paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Aug 17, 2020
block: Box<SignedBeaconBlock<E>>,
) -> Self {
Self {
drop_during_sync: true,
Copy link
Member

Choose a reason for hiding this comment

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

Are we confident that sync can handle getting to the head reliably without the help of gossip blocks? I thought I'd noticed that my node sometimes got kicked out of a sync loop by a new gossip block

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm let's ask @AgeManning

Copy link
Member

@AgeManning AgeManning Aug 17, 2020

Choose a reason for hiding this comment

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

Oh yeah. So there is a heuristic where we look at gossip blocks even when we are still synced. If it is close to head its valuable.

See this condition: https://github.com/sigp/lighthouse/blob/master/beacon_node/network/src/sync/manager.rs#L453

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed offline with @AgeManning, I've changed it to always process gossip blocks, even if syncing.

@paulhauner
Copy link
Member Author

All comments addressed except for the one for @AgeManning. I'll await his response, perhaps you could approve pending his approval? :)

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!

Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

LGTM

@paulhauner
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Aug 17, 2020
## Issue Addressed

NA

## Proposed Changes

Moves beacon block processing over to the newly-added `GossipProcessor`. This moves the task off the core executor onto the blocking one.

## Additional Info

- With this PR, gossip blocks are being ignored during sync.
@bors
Copy link

bors bot commented Aug 17, 2020

@bors bors bot changed the title Process gossip blocks on the GossipProcessor [Merged by Bors] - Process gossip blocks on the GossipProcessor Aug 17, 2020
@bors bors bot closed this Aug 17, 2020
@paulhauner paulhauner deleted the block-worker branch September 18, 2020 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review The code is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants