This repository has been archived by the owner on Jan 22, 2025. It is now read-only.
retains one node-instance per pubkey #17187
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #17187 +/- ##
=======================================
Coverage 82.7% 82.7%
=======================================
Files 421 421
Lines 117926 117976 +50
=======================================
+ Hits 97574 97640 +66
+ Misses 20352 20336 -16 |
crds table retains up to 32 node-instance values per each pubkey. This is so because if there are multiple running instances of the same node, then we want gossip to propagate node-instance values associated with both instances, therefore the corresponding label/key includes the randomly generated token in addition to the pubkey: https://github.com/solana-labs/solana/blob/9c42a89a4/core/src/crds_value.rs#L448 solana-labs#14037 As a result, the number of such values per pubkey are effectively unbounded, requiring custom mitigations implemented in: solana-labs#14467 but still taking redundant extra memory and bandwidth. This commit instead retains only one node-instance per pubkey by extending crds values override logic. If a crds value is of type node-instance, it will always override an existing one with the same key if it has more recent starting timestamp (not wallclock). As a result, gossip will always propagate the node-instance with more recent timestamp. Since the check_duplicate logic will stop the node with older timestamp, this change should preserve existing functionality.
behzadnouri
force-pushed
the
override-node-instance
branch
from
181b383 to
f299b7e
Compare
mergify bot
pushed a commit
that referenced
this pull request
May 21, 2021
crds table retains up to 32 node-instance values per each pubkey. This is so because if there are multiple running instances of the same node, then we want gossip to propagate node-instance values associated with both instances, therefore the corresponding label/key includes the randomly generated token in addition to the pubkey: https://github.com/solana-labs/solana/blob/9c42a89a4/core/src/crds_value.rs#L448 #14037 As a result, the number of such values per pubkey are effectively unbounded, requiring custom mitigations implemented in: #14467 but still taking redundant extra memory and bandwidth. This commit instead retains only one node-instance per pubkey by extending crds values override logic. If a crds value is of type node-instance, it will always override an existing one with the same key if it has more recent starting timestamp (not wallclock). As a result, gossip will always propagate the node-instance with more recent timestamp. Since the check_duplicate logic will stop the node with older timestamp, this change should preserve existing functionality. (cherry picked from commit 0aa7824)
mergify bot
added a commit
that referenced
this pull request
May 21, 2021
crds table retains up to 32 node-instance values per each pubkey. This is so because if there are multiple running instances of the same node, then we want gossip to propagate node-instance values associated with both instances, therefore the corresponding label/key includes the randomly generated token in addition to the pubkey: https://github.com/solana-labs/solana/blob/9c42a89a4/core/src/crds_value.rs#L448 #14037 As a result, the number of such values per pubkey are effectively unbounded, requiring custom mitigations implemented in: #14467 but still taking redundant extra memory and bandwidth. This commit instead retains only one node-instance per pubkey by extending crds values override logic. If a crds value is of type node-instance, it will always override an existing one with the same key if it has more recent starting timestamp (not wallclock). As a result, gossip will always propagate the node-instance with more recent timestamp. Since the check_duplicate logic will stop the node with older timestamp, this change should preserve existing functionality. (cherry picked from commit 0aa7824) Co-authored-by: behzad nouri <behzadnouri@gmail.com>
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
crds table retains up to 32 node-instance values per each pubkey. This
is so because if there are multiple running instances of the same node,
then we want gossip to propagate node-instance values associated with
both instances, therefore the corresponding label/key includes the
randomly generated token in addition to the pubkey:
https://github.com/solana-labs/solana/blob/9c42a89a4/core/src/crds_value.rs#L448
#14037
As a result, the number of such values per pubkey are effectively
unbounded, requiring custom mitigations implemented in: #14467
but still taking redundant extra memory and bandwidth.
Summary of Changes
This commit instead only retains one node-instance per pubkey by
extending crds values override logic. If a crds value is of type
node-instance, it will always override an existing one with the same key
if it has more recent starting timestamp (not wallclock). As a result,
gossip will always propagate the node-instance with more recent
timestamp. Since the check_duplicate logic will stop the node with older
timestamp, this change should preserve existing functionality.