filter out unstaked NodeInstance from sent PullRequests #2637
Conversation
gossip/src/cluster_info.rs
Outdated
| | CrdsData::RestartLastVotedForkSlots(_) | ||
| | CrdsData::NodeInstance(_) => { | ||
| let stake = stakes.get(&value.pubkey()).copied(); | ||
| stake.unwrap_or_default() >= MIN_STAKE_FOR_GOSSIP |
There was a problem hiding this comment.
I think you can leave this part as is, but add an extra branch before line 437:
CrdsData::NodeInstance(_) if !drop_unstaked_node_instance => true,
gossip/src/cluster_info.rs
Outdated
| @@ -1646,7 +1656,7 @@ impl ClusterInfo { | |||
| .add_relaxed(num_nodes as u64); | |||
| if self.require_stake_for_gossip(stakes) { | |||
| push_messages.retain(|_, data| { | |||
| retain_staked(data, stakes); | |||
| retain_staked(data, stakes, false); | |||
There was a problem hiding this comment.
for someone reading this code later, it is not clear what each of these false/true means.
I think it would be good to add some inline comments as in:
retain_staked(data, stakes, /*drop_unstaked_node_instance:*/ false);
| @@ -1646,7 +1651,7 @@ impl ClusterInfo { | |||
| .add_relaxed(num_nodes as u64); | |||
| if self.require_stake_for_gossip(stakes) { | |||
| push_messages.retain(|_, data| { | |||
| retain_staked(data, stakes); | |||
| retain_staked(data, stakes, /* drop_unstaked_node_instance */ false); | |||
There was a problem hiding this comment.
i purposefully left this false since I wanted to minimize crds table differences between nodes on this first PR. If we stop propagating staked NodeInstance via push, then the crds tables will differ more between the non upgraded nodes and the upgraded nodes. This will result in more unstaked NodeInstances getting sent from non upgraded to upgraded nodes in PullResponses. Which is the same issue we are trying to fix (although less extreme)
|
Step (2) completed in: #3653 |
* filter out unstaked NodeInstance from sent PullRequests * add descriptor, refactor retain_staked() --------- Co-authored-by: greg <[email protected]>
PR #2511 causes an issue where non upgraded nodes consistently send unstaked
NodeInstances inPullResponses that the receiving node (that has this PR) drops. This increases the network's gossip traffic since the sender of thePullResponsekeeps thinking the receiver doesn't have and wants the unstakedNodeInstance.Summary of Changes
Revert the PR that stops propagating unstaked
NodeInstances.There are two steps in this process of getting to the point where we do not propagate any unstaked
NodeInstanceStep 1) This PR: Do not propagate unstaked
NodeInstances inPullResponses. UnstakedNodeInstances are propagated as normal in sent/receivedPushMessages.Step 2) Once all nodes have upgraded to this Step 1 PR, then we fully remove unstaked
NodeInstancepropagation. This will ensure that we don't run into the issue created by PR #2511. Nodes that have not upgraded to the Step 2 PR will still propagate unstakedNodeInstances but will not send them inPullResponses, which resulted in a spike in gossip traffic.NOTE: I tested this incremental update process on a cluster with 40 validators, 25 rpcs, and 1 bootstrap and it did not increase the gossip traffic as seen in PR #2511.