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

Send message out only if it is valid #189

Merged
merged 6 commits into from
May 7, 2024

Conversation

boyuan-chen
Copy link

@boyuan-chen boyuan-chen commented May 4, 2024

📋 Add associated issues, tickets, docs URL here.

Overview

Describe what your Pull Request is about in a few sentences.

There are two possible ways to fix this invalid payload is shared to other nodes.

  1. We can gossip the payload only if it is valid.
  2. We can drop the invalid payload if it is from the gossip (unsafe block)

This PR implements the first method and this #188 implements the second method. The first method is more reasonable.

Changes

Describe your changes and implementation choices. More details make PRs easier to review.

  • Send out the message only if it is valid

Testing

Describe how to test your new feature/bug fix and if possible, a step by step guide on how to demo this.

Copy link

@jyellick jyellick left a comment

Choose a reason for hiding this comment

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

The logic changes seem good to me. I'm a little confused why agossip.Clear() is called in all paths, even on success. But the existing code seems to be doing so.

@boyuan-chen boyuan-chen marked this pull request as ready for review May 6, 2024 17:16
@boyuan-chen boyuan-chen temporarily deployed to op-stack-production May 6, 2024 20:28 — with GitHub Actions Inactive
@boyuan-chen boyuan-chen merged commit 278b865 into develop May 7, 2024
70 checks passed
@boyuan-chen boyuan-chen deleted the gossip-only-payload-is-valid branch May 7, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants