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.

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

Expose gossip future #8735

Merged
merged 12 commits into from
Oct 25, 2024
Merged

Expose gossip future #8735

merged 12 commits into from
Oct 25, 2024

Conversation

tbenr
Copy link
Contributor

@tbenr tbenr commented Oct 16, 2024 •
edited
Loading

Allow block and blobs gossip publishing to forward the libp2p Future signaling when the first message (block or blob) has been successfully published over the wire (or error).

The exposed SafeFuture<Void> is designed to never fail. If the underlining gossip fails, it in any case completes successfully.

I also refactored gossip error handling to have a common logic used everywhere. So now we will have:

2024-10-23 19:32:01.022 WARN  - Failed to publish beacon_aggregate_and_proof(s) for slot 23; No peers for message topics [/eth2/6c6f623a/beacon_aggregate_and_proof/ssz_snappy] found

2024-10-23 19:32:01.067 WARN  - Failed to publish sync_committee_contribution_and_proof(s) for slot 23; No peers for message topics [/eth2/6c6f623a/sync_committee_contribution_and_proof/ssz_snappy] found

2024-10-23 19:31:57.263 WARN  - Failed to publish blob_sidecar for slot 23; No peers for message topics [/eth2/6c6f623a/blob_sidecar_0/ssz_snappy] found

2024-10-23 19:31:51.029 WARN  - Failed to publish beacon_block for slot 22; No peers for message topics [/eth2/6c6f623a/beacon_block/ssz_snappy] found

2024-10-23 19:31:51.034 WARN  - Failed to publish sync_committee_message(s) for slot 22; No peers for message topics [/eth2/6c6f623a/sync_committee_0/ssz_snappy] found

2024-10-23 19:31:51.037 WARN  - Failed to publish attestation(s) for slot 22; No peers for message topics [/eth2/6c6f623a/beacon_attestation_6/ssz_snappy] found

related to an improvement over #8682

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 mentioned this pull request Oct 17, 2024
Merged
2 tasks
zilm13
zilm13 reviewed Oct 22, 2024
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.

Great improvement!
Just few nits

...ing/eth2/src/main/java/tech/pegasys/teku/networking/eth2/gossip/forks/GossipForkManager.java Show resolved Hide resolved
networking/p2p/src/main/java/tech/pegasys/teku/networking/p2p/libp2p/gossip/GossipHandler.java Outdated Show resolved Hide resolved
...tor/src/main/java/tech/pegasys/teku/validator/coordinator/publisher/BlockPublisherDeneb.java Show resolved Hide resolved
@tbenr tbenr force-pushed the expose-gossip-future branch from e166cc3 to fb0f4a4 Compare October 23, 2024 17:33
@tbenr
Copy link
Contributor Author

tbenr commented Oct 23, 2024

@zilm13 @rolfyone I improved gossip error handling here.

zilm13
zilm13 approved these changes Oct 24, 2024
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

...king/p2p/src/test/java/tech/pegasys/teku/networking/p2p/libp2p/gossip/GossipHandlerTest.java Outdated Show resolved Hide resolved
...ing/p2p/src/main/java/tech/pegasys/teku/networking/p2p/libp2p/gossip/LibP2PTopicChannel.java Show resolved Hide resolved
...ing/eth2/src/test/java/tech/pegasys/teku/networking/eth2/gossip/GossipFailureLoggerTest.java Show resolved Hide resolved
...ing/eth2/src/main/java/tech/pegasys/teku/networking/eth2/gossip/forks/GossipForkManager.java Outdated
@@ -163,12 +165,13 @@ public synchronized void publishAttestation(final ValidatableAttestation attesta
GossipForkSubscriptions::publishAttestation);
}

public synchronized void publishBlock(final SignedBeaconBlock block) {
publishMessage(block.getSlot(), block, "block", GossipForkSubscriptions::publishBlock);
public synchronized SafeFuture<Void> publishBlock(final SignedBeaconBlock block) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Something is synchronized here, something not (publishSyncCommitteeContribution). Maybe move all synchronizations to publishMessage and publishMessageWithFeedback? Or I missed something and features like publishSyncCommitteeContribution shouldn't be synchronized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh.. I haven't noticed the difference. good catch. I actually don't know, seems to me like the synchronized has been forgotten.
It happened here: https://github.com/Consensys/teku/pull/4888/files#diff-8b65d2863b6d9404c913fa07957bc9f5e638ccb31cda39bfd22df59594208fa7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cleaned up the synchronizations

networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/gossip/GossipFailureLogger.java Outdated Show resolved Hide resolved
networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/gossip/GossipFailureLogger.java Outdated Show resolved Hide resolved
@tbenr tbenr force-pushed the expose-gossip-future branch from b23aa54 to 9b37f3e Compare October 24, 2024 17:40
@tbenr tbenr merged commit 735f664 into Consensys:master Oct 25, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolfyone rolfyone rolfyone 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.
3 participants
@tbenr @rolfyone @zilm13