replay: votes made before restart are eligible for refresh #34737
Conversation
AshwinSekar
force-pushed
the
refresh-startup
branch
from
1ac206d
to
4a011f6
Compare
AshwinSekar
changed the title
AshwinSekar
force-pushed
the
refresh-startup
branch
from
4a011f6
to
41e227a
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #34737 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 830 830
Lines 224828 224845 +17
=======================================
+ Hits 183504 183519 +15
- Misses 41324 41326 +2 |
core/src/replay_stage.rs
Outdated
| @@ -2511,6 +2516,8 @@ impl ReplayStage { | |||
| }) | |||
| .unwrap_or_else(|err| warn!("Error: {:?}", err)); | |||
| last_vote_refresh_time.last_refresh_time = Instant::now(); | |||
| } else { | |||
| tower.last_vote_tx_blockhash_failed(); | |||
There was a problem hiding this comment.
hmm we only want to mark this as failed if this is a non-voter right? It seems like there's a few other cases where generate_vote_tx can return None though.
Same is true for the other place where this is called
There was a problem hiding this comment.
non-voter is just one example. generate_vote_tx could fail for multiple reasons. In that case the vote is already in the tower under last_vote but we haven't propagated it to the network.
I think there's no point attempting to refresh the vote if the last generate_vote_tx failed as the generate_vote_tx for the refreshed vote should fail again. In steady state all of the checks in that function should be deterministic.
However if you prefer I don't mind keeping the scope to just the authorized_voter_keypair check, which is what the --no-voting flag does.
There was a problem hiding this comment.
hmmm, seems like we should differentiate a failed vote from somebody that's never voting. Grouping them together seems bound to cause avoid future confusion/bugs.
I would limit the scope if possible
core/src/consensus.rs
Outdated
| #[default] | ||
| Uninitialized, | ||
| /// Failed generation of last vote tx | ||
| Failed, |
There was a problem hiding this comment.
Seems like this is just to cover the non-voter case, can we make this more specific and call it NotVoting
AshwinSekar
force-pushed
the
refresh-startup
branch
from
41e227a
to
70bd550
Compare
github-actions
bot
added
stale
AshwinSekar
force-pushed
the
refresh-startup
branch
from
70bd550
to
1addd02
Compare
Problem
Bug in #32315 disallows the last vote to be refreshed after a restart.
This was previously allowed before that change.
Summary of Changes
Distinguish a failed vote vs an uninitialized tower from restart.