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

retains crds values if the origin is still active#16576

Merged
behzadnouri merged 2 commits intosolana-labs:masterfrom
behzadnouri:update-record-timestamps
Apr 23, 2021
Merged

retains crds values if the origin is still active#16576
behzadnouri merged 2 commits intosolana-labs:masterfrom
behzadnouri:update-record-timestamps

Conversation

@behzadnouri
Copy link
Copy Markdown
Contributor

Problem

Local timestamps are updated for records associated with a pubkey if the
origin is still active:
https://github.com/solana-labs/solana/blob/c8ed14c64/core/src/crds.rs#L301-L311

However this is done inconsistently on some gossip paths (pull requests
and pull responses) but not all (e.g. push messages). Additionally
update_record_timestamp is inefficient since there can be ~800 values
associated with each pubkey.

This commit updates records timestamps only on contact-infos; and,
instead utilizes origin's timestamp when purging old values.

Summary of Changes

  • Update record timestamp only on contact-info if it exists.
  • Utilize origin's timestamp when purging old values.
  • Retain crds values if the origin is still active.

@behzadnouri behzadnouri force-pushed the update-record-timestamps branch from aa809f0 to 7a813bf Compare April 15, 2021 19:25
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 15, 2021

Codecov Report

Merging #16576 (a155f70) into master (0924c2d) will increase coverage by 0.0%.
The diff coverage is 100.0%.

❗ Current head a155f70 differs from pull request most recent head 82616e0. Consider uploading reports for the commit 82616e0 to get more accurate results

@@           Coverage Diff           @@
##           master   #16576   +/-   ##
=======================================
  Coverage    82.9%    82.9%           
=======================================
  Files         416      416           
  Lines      115439   115477   +38     
=======================================
+ Hits        95784    95831   +47     
+ Misses      19655    19646    -9     

@behzadnouri behzadnouri marked this pull request as ready for review April 18, 2021 16:54
@behzadnouri behzadnouri force-pushed the update-record-timestamps branch 2 times, most recently from 872c9bf to 4fcac53 Compare April 20, 2021 11:24
@behzadnouri behzadnouri force-pushed the update-record-timestamps branch from e9f2851 to a155f70 Compare April 21, 2021 14:56
Local timestamps are updated for records associated with a pubkey if the
origin is still active:
https://github.com/solana-labs/solana/blob/c8ed14c64/core/src/crds.rs#L301-L311

However this is done inconsistently on some gossip paths (pull requests
and pull responses) but not all (e.g. push messages). Additionally
update_record_timestamp is inefficient since there can be ~800 values
associated with each pubkey.

This commit updates records timestamps only on contact-infos; and,
instead utilizes origin's timestamp when purging old values.
@behzadnouri behzadnouri force-pushed the update-record-timestamps branch from a155f70 to 82616e0 Compare April 21, 2021 19:14
Copy link
Copy Markdown
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.

Nice!

Small disambiguation on the problem description: by "origin timestamp" we don't mean the timestamp marked by the original sender, we mean the timestamp from the last refresh on the origin's CrdsValueLabel::ContactInfo, which is now refreshed on every message update from that origin.

@behzadnouri
Copy link
Copy Markdown
Contributor Author

Small disambiguation on the problem description: by "origin timestamp" we don't mean the timestamp marked by the original sender, we mean the timestamp from the last refresh on the origin's CrdsValueLabel::ContactInfo, which is now refreshed on every message update from that origin.

yeah, that is correct

@behzadnouri behzadnouri merged commit 2c82f21 into solana-labs:master Apr 23, 2021
@behzadnouri behzadnouri deleted the update-record-timestamps branch April 23, 2021 15:14
mergify Bot pushed a commit that referenced this pull request Apr 23, 2021
Local timestamps are updated for records associated with a pubkey if the
origin is still active:
https://github.com/solana-labs/solana/blob/c8ed14c64/core/src/crds.rs#L301-L311

However this is done inconsistently on some gossip paths (pull requests
and pull responses) but not all (e.g. push messages). Additionally
update_record_timestamp is inefficient since there can be ~800 values
associated with each pubkey.

This commit updates records timestamps only on contact-infos; and,
instead utilizes origin's timestamp when purging old values.

(cherry picked from commit 2c82f21)
mergify Bot added a commit that referenced this pull request Apr 23, 2021
Local timestamps are updated for records associated with a pubkey if the
origin is still active:
https://github.com/solana-labs/solana/blob/c8ed14c64/core/src/crds.rs#L301-L311

However this is done inconsistently on some gossip paths (pull requests
and pull responses) but not all (e.g. push messages). Additionally
update_record_timestamp is inefficient since there can be ~800 values
associated with each pubkey.

This commit updates records timestamps only on contact-infos; and,
instead utilizes origin's timestamp when purging old values.

(cherry picked from commit 2c82f21)

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
@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