Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

filters crds values obtained through gossip by their shred version #18072

Merged
merged 1 commit into from
Jun 23, 2021

Conversation

behzadnouri
Copy link
Contributor

@behzadnouri behzadnouri commented Jun 18, 2021

Problem

filter_by_shred_version does not check the shred-version of the owner of
the crds-value. It only checks the shred-version of the node which is
relaying the value:
https://github.com/solana-labs/solana/blob/5cc073420/gossip/src/cluster_info.rs#L2274-L2289

So crds-values with different shred versions can still pass through this
function as long as they are relayed by a node with matching shred
version; and so, a single node can bridge different shred values
through-out the cluster.

see comments in #17940

Summary of Changes

  • Check the shred-version on the values in addition to node which sends the crds value.
  • Aggregate all shred-version checks in one place and very early in gossip handling of packets.

@behzadnouri behzadnouri force-pushed the enforce-shred-version branch from 774d3e9 to 3365bd5 Compare June 20, 2021 14:17
@codecov
Copy link

codecov bot commented Jun 20, 2021

Codecov Report

Merging #18072 (aece480) into master (3b1738c) will increase coverage by 0.0%.
The diff coverage is 51.8%.

@@           Coverage Diff           @@
##           master   #18072   +/-   ##
=======================================
  Coverage    82.3%    82.3%           
=======================================
  Files         433      433           
  Lines      120912   120901   -11     
=======================================
+ Hits        99586    99610   +24     
+ Misses      21326    21291   -35     

@behzadnouri behzadnouri requested review from sakridge and carllin June 20, 2021 16:21
@@ -3102,6 +3047,70 @@ pub fn stake_weight_peers(
ClusterInfo::sorted_stakes_with_index(peers, stakes)
}

// Filters out values from nodes with different shred-version.
fn check_shred_version(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: check_shred_version -> filter_on_shred_version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

carllin
carllin previously approved these changes Jun 23, 2021
Copy link
Contributor

@carllin carllin left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

filter_by_shred_version does not check the shred-version of the owner of
the crds-value. It only checks the shred-version of the node which is
relaying the value:
https://github.com/solana-labs/solana/blob/5cc073420/gossip/src/cluster_info.rs#L2274-L2289

So crds-values with different shred versions can still pass through this
function as long as they are relayed by a node with matching shred
version; and so, a single node can bridge different shred values
through-out the cluster.
@behzadnouri behzadnouri force-pushed the enforce-shred-version branch from 3365bd5 to aece480 Compare June 23, 2021 12:35
@mergify mergify bot dismissed carllin’s stale review June 23, 2021 12:36

Pull request has been modified.

@behzadnouri behzadnouri merged commit 69a5f0e into solana-labs:master Jun 23, 2021
@behzadnouri behzadnouri deleted the enforce-shred-version branch June 23, 2021 14:16
mergify bot pushed a commit that referenced this pull request Jun 23, 2021
…18072)

filter_by_shred_version does not check the shred-version of the owner of
the crds-value. It only checks the shred-version of the node which is
relaying the value:
https://github.com/solana-labs/solana/blob/5cc073420/gossip/src/cluster_info.rs#L2274-L2289

So crds-values with different shred versions can still pass through this
function as long as they are relayed by a node with matching shred
version; and so, a single node can bridge different shred values
through-out the cluster.

(cherry picked from commit 69a5f0e)

# Conflicts:
#	gossip/src/cluster_info.rs
mergify bot added a commit that referenced this pull request Jun 23, 2021
…ackport #18072) (#18175)

* filters crds values obtained through gossip by their shred version (#18072)

filter_by_shred_version does not check the shred-version of the owner of
the crds-value. It only checks the shred-version of the node which is
relaying the value:
https://github.com/solana-labs/solana/blob/5cc073420/gossip/src/cluster_info.rs#L2274-L2289

So crds-values with different shred versions can still pass through this
function as long as they are relayed by a node with matching shred
version; and so, a single node can bridge different shred values
through-out the cluster.

(cherry picked from commit 69a5f0e)

# Conflicts:
#	gossip/src/cluster_info.rs

* removes backport merge conflicts

Co-authored-by: behzad nouri <[email protected]>
CriesofCarrots pushed a commit to CriesofCarrots/solana that referenced this pull request Jun 28, 2021
@brooksprumo brooksprumo mentioned this pull request Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants