Increment timestamp on refreshed votes #31908

AshwinSekar · 2023-06-01T00:52:22Z

Problem

Follow up to the change #31879, now that we use vote tx timestamp as a tiebreaker in the vote queue deduplication logic, send a different timestamp everytime we refresh a vote.

Summary of Changes

On refresh increment the timestamp by 1 (using the time of refresh has potential to break the Timestamp Oracle). If the previous vote didn't have a timestamp try to estimate as best as possible.

Fixes #

carllin · 2023-06-01T19:28:14Z

core/src/consensus.rs

@@ -454,6 +455,40 @@ impl Tower {
        self.last_vote_tx_blockhash
    }

+    pub fn refresh_last_vote_timestamp(
+        &mut self,
+        heaviest_bank_on_same_fork: Slot,


nit: rename to heaviest_slot_on_same_fork

carllin · 2023-06-01T19:36:22Z

core/src/consensus.rs

+        } else if let Some(last_voted_slot) = self.last_vote.last_voted_slot() {
+            // If the previous vote did not send a timestamp due to clock error, try our
+            // best to estimate the correct timestamp for the Timestamp Oracle.


ick is this because this maybe_timestamp might return None here: https://github.com/ilya-bobyr/solana/blob/a872c0a373015a5427bdce238b32d66783595378/core/src/consensus.rs#L538

Shouldn't the if current_slot > self.last_timestamp.slot always be true since votes are always increasing? Seems like this case we can just submit the current timestamp since it should never be true

Yeah votes should always be increasing so that check should never fail. However the check below it

solana/core/src/consensus.rs

Line 593 in e0389ba

if timestamp >= self.last_timestamp.timestamp {

could fail if Utc::now().timestamp() is screwed. Previously we would return None and exclude this vote from the timestamp calculation for the oracle, but to avoid dedup we need to return a value here.

hmmm how often is Utc::now().timestamp() going to go backwards? Feel like we could just panic in that case or retry until we get a proper timestamp. Estimating the slot time here seems kind of hacky.

🤷. also if it was messed up for the original vote it could also be messed up for the estimation here.
do we want to kill everything over a missed timestamp though?

how often is Utc::now().timestamp() going to go backwards?

whenever it wants. underlying time is sampled from std::time::SystemTime (clock_gettime(CLOCK_REALTIME) on linux), which is not monotonic. std::time::Instant uses clock_gettime(CLOCK_MONOTONIC), but i don't think there's any way to get a timestamp from one.

i wonder if we'd be better off looking up the txs' rbh in status cache and comparing the slots to tiebreak. abusing the wallclock oracle is starting to feel too dirty

Bank-frozen time actually makes a lot of sense for the timestamp oracle.
Would you still want to increment by 1sec for the vote-refresh distinction?

Yeah, my idea is

Populate timestamp when freezing bank

If UTC::now fails, use the best_estimate calc. More likely to be correct because there is no vote delay, and it's an inverse of the calc used in the stake weighted mean anyway.

If we're worried about the compounding effect, denote which timestamps have been estimated and which have been observed. run best_estimate from the latest bank that has been observed.

For refresh, during vote generation add a +1

Unless I'm missing something, I would expect the estimate off an estimate to be the same as an estimate off the original latest observed bank.

But anyway, I think this approach sounds okay, with the caveat that we probably still need to ensure that the timestamp doesn't go backward immediately after a refresh, whether the new vote timestamp is generated by UTC::now or best_estimate, so that those votes do not fail to land.

i'm going to need to see this to understand it. intuitively sounds way too invasive to backport

I agree, seems way too much to backport. For now i've stuck with the +1 idea, and updated last_timestamp such that new vote timestamps cannot go backwards. Vote ts will be increasing or None.

for the future: #32135 to track how to fix this properly.

codecov · 2023-06-01T20:17:16Z

Codecov Report

Merging #31908 (24a3af8) into master (c01250f) will increase coverage by 0.0%.
The diff coverage is 69.8%.

@@           Coverage Diff            @@
##           master   #31908    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         767      767            
  Lines      208903   209002    +99     
========================================
+ Hits       171223   171349   +126     
+ Misses      37680    37653    -27

t-nelson

derp... left this comment pending 🤦

t-nelson · 2023-06-07T19:41:04Z

core/src/consensus.rs

+        } else if let Some(last_voted_slot) = self.last_vote.last_voted_slot() {
+            // If the previous vote did not send a timestamp due to clock error, try our
+            // best to estimate the correct timestamp for the Timestamp Oracle.


i'm going to need to see this to understand it. intuitively sounds way too invasive to backport

t-nelson

logic is a little complex, but i can't come up with much better.

it'd be good to get +1 from @CriesofCarrots too

(cherry picked from commit 01d3546)

…32157) Increment timestamp on refreshed votes (#31908) (cherry picked from commit 01d3546) Co-authored-by: Ashwin Sekar <[email protected]>

t-nelson · 2023-06-15T18:54:21Z

logic is a little complex, but i can't come up with much better.

it'd be good to get +1 from @CriesofCarrots too

@CriesofCarrots if you could leave a post-merge review once back in office, that'd be great! 🙏

…32156) Increment timestamp on refreshed votes (#31908) (cherry picked from commit 01d3546) Co-authored-by: Ashwin Sekar <[email protected]>

CriesofCarrots

Yeah, I can't think of better logic for now. This seems like a more preferable place to start than the previous estimation code. I do wish we had better visibility into the effects; is it worth adding some metrics that report how often each of these cases are hit?

AshwinSekar · 2023-06-20T16:40:24Z

I do wish we had better visibility into the effects; is it worth adding some metrics that report how often each of these cases are hit?

I've added some metrics here #32206

…s#31908) (solana-labs#32156) Increment timestamp on refreshed votes (solana-labs#31908) (cherry picked from commit 01d3546) Co-authored-by: Ashwin Sekar <[email protected]>

AshwinSekar force-pushed the send-later-ts-refresh branch from 4aa7dc3 to 74a05df Compare June 1, 2023 18:50

AshwinSekar added v1.14 v1.16 PRs that should be backported to v1.16 labels Jun 1, 2023

AshwinSekar marked this pull request as ready for review June 1, 2023 18:59

AshwinSekar requested review from t-nelson, CriesofCarrots, carllin, steviez and bw-solana June 1, 2023 18:59

carllin reviewed Jun 1, 2023

View reviewed changes

AshwinSekar force-pushed the send-later-ts-refresh branch from 74a05df to 8d22a9f Compare June 1, 2023 19:57

AshwinSekar mentioned this pull request Jun 2, 2023

Use timestamp to tiebreak votes in banking_stage #31925

Merged

t-nelson reviewed Jun 13, 2023

View reviewed changes

AshwinSekar force-pushed the send-later-ts-refresh branch 2 times, most recently from b535be9 to 78d2c63 Compare June 14, 2023 19:35

Increment timestamp on refreshed votes

24a3af8

AshwinSekar force-pushed the send-later-ts-refresh branch from 78d2c63 to 24a3af8 Compare June 14, 2023 19:37

AshwinSekar requested review from t-nelson and carllin June 14, 2023 19:57

t-nelson approved these changes Jun 14, 2023

View reviewed changes

carllin approved these changes Jun 15, 2023

View reviewed changes

steviez removed their request for review June 15, 2023 17:16

AshwinSekar merged commit 01d3546 into solana-labs:master Jun 15, 2023

mergify bot pushed a commit that referenced this pull request Jun 15, 2023

Increment timestamp on refreshed votes (#31908)

15b42d6

(cherry picked from commit 01d3546)

mergify bot pushed a commit that referenced this pull request Jun 15, 2023

Increment timestamp on refreshed votes (#31908)

ecdf5de

(cherry picked from commit 01d3546)

This was referenced Jun 15, 2023

v1.14: Increment timestamp on refreshed votes (backport of #31908) #32156

Merged

v1.16: Increment timestamp on refreshed votes (backport of #31908) #32157

Merged

CriesofCarrots reviewed Jun 16, 2023

View reviewed changes

wen-coding pushed a commit to wen-coding/solana that referenced this pull request Aug 15, 2023

Increment timestamp on refreshed votes (solana-labs#31908)

97ea3c5

wen-coding pushed a commit to wen-coding/solana that referenced this pull request Aug 15, 2023

Increment timestamp on refreshed votes (solana-labs#31908)

f87201d

wen-coding pushed a commit to wen-coding/solana that referenced this pull request Aug 15, 2023

Increment timestamp on refreshed votes (solana-labs#31908)

75fc21a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increment timestamp on refreshed votes #31908

Increment timestamp on refreshed votes #31908

AshwinSekar commented Jun 1, 2023 •

edited

Loading

carllin Jun 1, 2023

carllin Jun 1, 2023

AshwinSekar Jun 1, 2023

carllin Jun 1, 2023

AshwinSekar Jun 1, 2023

t-nelson Jun 2, 2023

CriesofCarrots Jun 7, 2023

AshwinSekar Jun 7, 2023

CriesofCarrots Jun 7, 2023

t-nelson Jun 7, 2023

AshwinSekar Jun 14, 2023

codecov bot commented Jun 1, 2023 •

edited

Loading

t-nelson left a comment

t-nelson Jun 7, 2023

t-nelson left a comment

t-nelson commented Jun 15, 2023

CriesofCarrots left a comment

AshwinSekar commented Jun 20, 2023 •

edited

Loading

Increment timestamp on refreshed votes #31908

Increment timestamp on refreshed votes #31908

Conversation

AshwinSekar commented Jun 1, 2023 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 1, 2023 • edited Loading

Codecov Report

t-nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson commented Jun 15, 2023

CriesofCarrots left a comment

Choose a reason for hiding this comment

AshwinSekar commented Jun 20, 2023 • edited Loading

AshwinSekar commented Jun 1, 2023 •

edited

Loading

codecov bot commented Jun 1, 2023 •

edited

Loading

AshwinSekar commented Jun 20, 2023 •

edited

Loading