uses current local timestamp when recording purged values

[behzadnouri]

Problem

CrdsGossipPull.purged_values is meant to record recently purged values
so that they are excluded from imminent pull requests, until the entire
cluster have synced to the updated value:
https://github.com/solana-labs/solana/blob/c826cddbb/core/src/crds_gossip_pull.rs#L449-L454

However, VersionedCrdsValue.local_timestamp represents the local time
when the value was last updated, and given that crds values may have
different timeouts based on stake, it does not necessarily represent how
recently the value was purged:
https://github.com/solana-labs/solana/blob/c826cddbb/core/src/crds.rs#L75-L76

As such, recording current local timestamp when purging values is more
appropriate. Additionally, purge_purged assumes that the purge_values is
sorted in timestamps when draining the old ones; which is not true if
those timestamps are VersionedCrdsValue.local_timestamp:
https://github.com/solana-labs/solana/blob/c826cddbb/core/src/crds_gossip_pull.rs#L563-L571

Summary of Changes

• use current local timestamp when recording purged values.

[codecov]

Codecov Report

Merging #16573 (868394f) into master (9dfe545) will decrease coverage by 0.0%.
The diff coverage is 66.6%.

@@            Coverage Diff            @@
##           master   #16573     +/-   ##
=========================================
- Coverage    80.0%    80.0%   -0.1%     
=========================================
  Files         413      413             
  Lines      112153   112150      -3     
=========================================
- Hits        89788    89774     -14     
- Misses      22365    22376     +11     

[carllin]

Good catches!

So to summarize for my understanding.

The issue is the timestamps stored for the "purged" values in the purged set are currently based on the local_timestamp, i.e. when those values were last updated in gossip. Because we keep around values from staked validators for an extended period of time, this means it could have been a long time since that value was updated.

The importance of this is that "purged" values timestamps are also used for purging the purged set in purge_purged(). Thus if the timestamps used for expiring those values from the purge set are old, then we'll immediately remove those values from the purged set. This opens us up to getting redundant entries again via pull requests shortly after.

[behzadnouri]

Good catches!

So to summarize for my understanding.

The issue is the timestamps stored for the "purged" values in the purged set are currently based on the local_timestamp, i.e. when those values were last updated in gossip. Because we keep around values from staked validators for an extended period of time, this means it could have been a long time since that value was updated.

The importance of this is that "purged" values timestamps are also used for purging the purged set in purge_purged(). Thus if the timestamps used for expiring those values from the purge set are old, then we'll immediately remove those values from the purged set. This opens us up to getting redundant entries again via pull requests shortly after.

yes, that is basically it