Skip to content

BEACON_BLOCK_AND_BLOBS_SIDECAR gossip topic #6465

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
Merged
tbenr merged 7 commits into from
Nov 21, 2022
Merged

BEACON_BLOCK_AND_BLOBS_SIDECAR gossip topic #6465

tbenr merged 7 commits into from
Nov 21, 2022

Conversation

@tbenr
Copy link
Contributor

@tbenr tbenr commented Nov 17, 2022 •
edited
Loading

This is the first time we have to unsubscribe to a topic and subscribe to a new one.

GossipForkSubscriptions have been refactored to leverage class inheritance but also avoid most of the code duplication when the next fork stop using previously used topics.

Basically each fork exposes methods to add GossipManager for all topic that has been introduced by that fork. Than it's up to the forks to call the corresponding add method (or delegate to super if the fork is a superset of the previous).
To make sure that forks can reenable previously disabled topics, we have to make sure to pass all possible OperationProcessors even if the current doesn't use it. It's the case of blockProcessor that may be re-enabled in future forks.

Add missing eip4844 and capella network configuration handling

fixes #6460

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@tbenr tbenr changed the title initial attempt to define new BEACON_BLOCK_AND_BLOBS_SIDECAR gossip topic Initial attempt to define new BEACON_BLOCK_AND_BLOBS_SIDECAR gossip topic Nov 17, 2022
lucassaldanha
lucassaldanha reviewed Nov 17, 2022
View reviewed changes
Copy link
Member

@lucassaldanha lucassaldanha left a comment

Choose a reason for hiding this comment

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

I have got a few comments to consider.

In terms of testing, you could look at BlsToExecutionChangeGossipIntegrationTest.java to see how I was testing the gossiping part (it isn't great but kinda gets the job done).

@tbenr tbenr marked this pull request as ready for review November 18, 2022 13:42
@tbenr tbenr changed the title Initial attempt to define new BEACON_BLOCK_AND_BLOBS_SIDECAR gossip topic BEACON_BLOCK_AND_BLOBS_SIDECAR gossip topic Nov 18, 2022
zilm13
zilm13 approved these changes Nov 20, 2022
View reviewed changes
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

LGTM

...a/tech/pegasys/teku/networking/eth2/gossip/forks/versions/GossipForkSubscriptionsPhase0.java Outdated
@Override
public void publishBlockAndBlobsSidecar(
final SignedBeaconBlockAndBlobsSidecar blockAndBlobsSidecar) {
// Does not apply to this fork.
Copy link
Contributor

@zilm13 zilm13 Nov 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd maybe go with interface defaults for this and all others (publishSyncCommitteeMessage, publishSyncCommitteeContribution, publishSignedBlsToExecutionChangeMessage, subscribeToSyncCommitteeSubnet, unsubscribeFromSyncCommitteeSubnet) to not leak them to Phase0 implementation but definitely up to you

lucassaldanha
lucassaldanha reviewed Nov 20, 2022
View reviewed changes
.../beaconchain/src/main/java/tech/pegasys/teku/services/beaconchain/BeaconChainController.java
.eventChannels(eventChannels)
.recentChainData(recentChainData)
.gossipedBlockProcessor(blockManager::validateAndImportBlock)
.gossipedBlockAndBlobsProcessor(OperationProcessor.noop())
Copy link
Member

@lucassaldanha lucassaldanha Nov 20, 2022

Choose a reason for hiding this comment

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

Is this correct? Don't we do anything with the BlockAndBlob messages we receive?

Copy link
Contributor Author

@tbenr tbenr Nov 21, 2022

Choose a reason for hiding this comment

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

yes I did it on purpose. I want to modify BlockManager and BlockValidator (and maybe BlockImporter) in a dedicated PR

@lucassaldanha
Copy link
Member

lucassaldanha commented Nov 20, 2022

Where are we forwarding the SignedBeaconBlockAndBlobsSidecar messages to other peers? Is this something that will come in another PR?

@tbenr
Copy link
Contributor Author

tbenr commented Nov 21, 2022

Where are we forwarding the SignedBeaconBlockAndBlobsSidecar messages to other peers? Is this something that will come in another PR?

yep, decided to have a better separation compared to the first draft.

@tbenr tbenr merged commit 95b05e9 into Consensys:master Nov 21, 2022
@tbenr tbenr deleted the new_4844_gossip branch November 21, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lucassaldanha lucassaldanha lucassaldanha left review comments

@zilm13 zilm13 zilm13 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement beacon_block_and_blobs_sidecar topic

3 participants

@tbenr @lucassaldanha @zilm13