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

network-gossip: Ensure sync event is processed on unknown peer roles #6553

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Nov 20, 2024

The GossipEngine::poll_next implementation polls both the notification_service and the sync_event_stream.

If both polls produce valid data to be processed (Poll::Ready(Some(..))), then the sync event is ignored when we receive NotificationEvent::NotificationStreamOpened and the role cannot be deduced.

This PR ensures both events are processed gracefully. While at it, I have added a warning to the sync engine related to notification_service producing Poll::Ready(None).

This effectively ensures that SyncEvents propagate to the network potentially fixing any state mismatch.

For more context: #6507

cc @paritytech/sdk-node

@lexnv lexnv added T0-node This PR/Issue is related to the topic “node”. I2-bug The node fails to follow expected behavior. labels Nov 20, 2024
@lexnv lexnv self-assigned this Nov 20, 2024
Signed-off-by: Alexandru Vasile <[email protected]>
Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

Good catch!

It seems this fixes the issue of sync peers not being followed (connected/disconnected) by GossipEngine through the reserved peers mechanism. It doesn't seem this fixes the issue of the peer role not being set, if there is one.

@lexnv lexnv requested a review from a team November 20, 2024 16:02
@lexnv lexnv added this pull request to the merge queue Nov 21, 2024
Merged via the queue into master with commit 7c9e34b Nov 21, 2024
197 of 201 checks passed
@lexnv lexnv deleted the lexnv/gossip-engine-poll branch November 21, 2024 10:50
Krayt78 pushed a commit to Krayt78/polkadot-sdk that referenced this pull request Dec 18, 2024
…aritytech#6553)

The `GossipEngine::poll_next` implementation polls both the
`notification_service` and the `sync_event_stream`.

If both polls produce valid data to be processed
(`Poll::Ready(Some(..))`), then the sync event is ignored when we
receive `NotificationEvent::NotificationStreamOpened` and the role
cannot be deduced.

This PR ensures both events are processed gracefully. While at it, I
have added a warning to the sync engine related to
`notification_service` producing `Poll::Ready(None)`.

This effectively ensures that `SyncEvents` propagate to the network
potentially fixing any state mismatch.


For more context: paritytech#6507

cc @paritytech/sdk-node

---------

Signed-off-by: Alexandru Vasile <[email protected]>
dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this pull request Jan 4, 2025
…aritytech#6553)

The `GossipEngine::poll_next` implementation polls both the
`notification_service` and the `sync_event_stream`.

If both polls produce valid data to be processed
(`Poll::Ready(Some(..))`), then the sync event is ignored when we
receive `NotificationEvent::NotificationStreamOpened` and the role
cannot be deduced.

This PR ensures both events are processed gracefully. While at it, I
have added a warning to the sync engine related to
`notification_service` producing `Poll::Ready(None)`.

This effectively ensures that `SyncEvents` propagate to the network
potentially fixing any state mismatch.


For more context: paritytech#6507

cc @paritytech/sdk-node

---------

Signed-off-by: Alexandru Vasile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Blocked ⛔️
Development

Successfully merging this pull request may close these issues.

3 participants