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

excludes node's pubkey from bloom filter of pruned origins #2990

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

behzadnouri
Copy link

Problem

Bloom filter of pruned origins can return false positive for a node's own pubkey but a node should always be able to push its own values to other nodes in the cluster.

Summary of Changes

Excludes node's pubkey from bloom filter of pruned origins

Bloom filter of pruned origins can return false positive for a node's
own pubkey but a node should always be able to push its own values to
other nodes in the cluster.
// Bloom filter can return false positive for origin == pubkey
// but a node should always be able to push its own values.
!bloom_filter.contains(origin)
|| (pubkey_eq_origin && &pubkey != node)

Choose a reason for hiding this comment

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

not sure I understand this check:

&pubkey != node

When would a node have its own pubkey as the value in the PushActiveSetEntry index map?
Is it possible for a node to add itself to its PushActiveSet? Even if it is, wouldn't we not want to send the message to ourselves if &pubkey == node?

Copy link
Author

Choose a reason for hiding this comment

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

When would a node have its own pubkey as the value in the PushActiveSetEntry index map?

The code here does not have any guarantees that pubkey != node.
So we need to check here not to send our own messages back to ourselves.

Is it possible for a node to add itself to its PushActiveSet?

We exclude that here:
https://github.com/anza-xyz/agave/blob/ce158213f/gossip/src/crds_gossip.rs#L352
but nonetheless the code here does not need to rely on that presumption.

Even if it is, wouldn't we not want to send the message to ourselves if &pubkey == node?

not sure I understand this.
&& &pubkey != node prevents sending messages to ourselves.

Choose a reason for hiding this comment

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

ok that makes sense.
and ya you're right I screwed up what we wanted to return in the filter.

Copy link

@gregcusack gregcusack left a comment

Choose a reason for hiding this comment

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

lgtm!

@behzadnouri behzadnouri merged commit bce28c0 into anza-xyz:master Sep 27, 2024
40 checks passed
@behzadnouri behzadnouri deleted the prune-origin branch September 27, 2024 20:28
@behzadnouri behzadnouri added the v2.0 Backport to v2.0 branch label Sep 27, 2024
Copy link

mergify bot commented Sep 27, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify bot pushed a commit that referenced this pull request Sep 27, 2024
Bloom filter of pruned origins can return false positive for a node's
own pubkey but a node should always be able to push its own values to
other nodes in the cluster.

(cherry picked from commit bce28c0)
willhickey pushed a commit that referenced this pull request Sep 27, 2024
Bloom filter of pruned origins can return false positive for a node's
own pubkey but a node should always be able to push its own values to
other nodes in the cluster.

(cherry picked from commit bce28c0)
mergify bot added a commit that referenced this pull request Sep 30, 2024
…kport of #2990) (#3015)

excludes node's pubkey from bloom filter of pruned origins (#2990)

Bloom filter of pruned origins can return false positive for a node's
own pubkey but a node should always be able to push its own values to
other nodes in the cluster.

(cherry picked from commit bce28c0)

Co-authored-by: behzad nouri <[email protected]>
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
…2990)

Bloom filter of pruned origins can return false positive for a node's
own pubkey but a node should always be able to push its own values to
other nodes in the cluster.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.0 Backport to v2.0 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants