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

Discard non-validator TMSquelch message #3788

Closed
wants to merge 1 commit into from

Conversation

gregtatcam
Copy link
Collaborator

High Level Overview of Change

Discard TMSquelch message with a non-validator public key

Context of Change

Introduced in 1.7.0.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

@@ -2489,6 +2489,16 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMSquelch> const& m)
}
PublicKey key(slice);

// Ignore non-validator squelch
if (!app_.validators().listed(key))
Copy link
Contributor

Choose a reason for hiding this comment

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

If the key was listed but recently removed from manifests, one peer may have the key removed already and the other may still have the key, for a short time. So a legit unsquelch could be filtered out, but that is OK because the squelch will expire eventually. Right?
How about the case that a key is recently added, and a legit squelch message is filtered out. That should also be fine, right? Because the correctness of the algorithm should not depend on connected peers' behavior. It should only affect performance. (Sorry, the algorithm is a bit fuggy to me to work out exactly what happens.)
There are other similar cases due to not completely overlapping sets of listed keys. But the same argument applies, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ultimately the validators list is going to be synced up between a node and its peers. In the meantime, if a peer gets the squelch message for an unlisted validator it'll ignore the squelch and relay messages for this validator anyways. But the node will ignore these messages until the squelch is expired. So the only drawback is that the messages are relayed until the list is synced up but I don't think it'll happen too often.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree.

Choose a reason for hiding this comment

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

Agree

Copy link
Contributor

@pwang200 pwang200 left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Hope PeerImp.cpp could be unit tested.

@manojsdoshi manojsdoshi mentioned this pull request Sep 9, 2021
@nbougalis nbougalis mentioned this pull request Sep 9, 2021
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.

4 participants