Improve vote-account for logical flow/reasoning#15237
Improve vote-account for logical flow/reasoning#15237ryoqun merged 3 commits intosolana-labs:masterfrom
Conversation
|
|
||
| writeln!( | ||
| f, | ||
| "- ... (truncated older history covering {} credits/rooted votes)", |
There was a problem hiding this comment.
bike shedding: covering or including or containing?
There was a problem hiding this comment.
omitting {} past rooted votes, which have already been credited ?
|
|
||
| // Maximum number of credits history to keep around | ||
| const MAX_EPOCH_CREDITS_HISTORY: usize = 64; | ||
| pub const MAX_EPOCH_CREDITS_HISTORY: usize = 64; |
There was a problem hiding this comment.
Making this const pub shouldn't create much burden for us for maintainability or compatibility as this is very sensitive to the abi. So, we won't change this in near future.
| writeln!( | ||
| f, | ||
| " credits range: [{}..{})", | ||
| " credits range: ({}..{}]", |
There was a problem hiding this comment.
as this run.sh sample shows this is more proper range notation... I was wrong at #13374 in hindsight after more deeper understanding of staking:
$ ./target/release/solana -ul vote-account 3vZMFHQPt618pUnjEnnw5yXbNgEK6PMzLNHEZrroe8Qn
Account Balance: 0.02685864 SOL
Validator Identity: A9ZHywzYR3G17utyPs8kb5Tm7pcrbuaGC56AWuuWLNEp
Authorized Voters: {0: "A9ZHywzYR3G17utyPs8kb5Tm7pcrbuaGC56AWuuWLNEp"}
Authorized Withdrawer: A9ZHywzYR3G17utyPs8kb5Tm7pcrbuaGC56AWuuWLNEp
Credits: 1
Commission: 100%
Root Slot: 0
Recent Timestamp: 2021-02-10T05:49:58Z from slot 31
Recent Votes (using 31/31 entries):
- slot: 31 (confirmation count: 1)
- slot: 30 (confirmation count: 2)
- slot: 29 (confirmation count: 3)
- slot: 28 (confirmation count: 4)
- slot: 27 (confirmation count: 5)
- slot: 26 (confirmation count: 6)
- slot: 25 (confirmation count: 7)
- slot: 24 (confirmation count: 8)
- slot: 23 (confirmation count: 9)
- slot: 22 (confirmation count: 10)
- slot: 21 (confirmation count: 11)
- slot: 20 (confirmation count: 12)
- slot: 19 (confirmation count: 13)
- slot: 18 (confirmation count: 14)
- slot: 17 (confirmation count: 15)
- slot: 16 (confirmation count: 16)
- slot: 15 (confirmation count: 17)
- slot: 14 (confirmation count: 18)
- slot: 13 (confirmation count: 19)
- slot: 12 (confirmation count: 20)
- slot: 11 (confirmation count: 21)
- slot: 10 (confirmation count: 22)
- slot: 9 (confirmation count: 23)
- slot: 8 (confirmation count: 24)
- slot: 7 (confirmation count: 25)
- slot: 6 (confirmation count: 26)
- slot: 5 (confirmation count: 27)
- slot: 4 (confirmation count: 28)
- slot: 3 (confirmation count: 29)
- slot: 2 (confirmation count: 30)
- slot: 1 (confirmation count: 31)
- ... (truncated older 1 votes, which was rooted and became credits)
All Epoch Voting History (using 1/64 entries):
* missed credits include slots unavailable to vote on due to delinquent leaders
- epoch: 0
credits range: (0..1]
credits/slots: 1/128
| )?; | ||
| } | ||
|
|
||
| if !epoch_voting_history.is_empty() { |
There was a problem hiding this comment.
Well, originally, showed titles for no good reason even if is_empty()...
| if let Some(newest) = epoch_voting_history.iter().rev().next() { | ||
| writeln!( | ||
| f, | ||
| "- ... (truncated older {} votes, which was rooted and became credits)", |
There was a problem hiding this comment.
This serves two purposes:
- Actually indicate this list is truncated
- Also, as a logical cue for the relation of rooted votes <=> credits and popping-out nature
There was a problem hiding this comment.
Also, another word bike shedding:
became or converted or translated to? (I wanted to indicate the 1-by-1 nature of votes/credits)
There was a problem hiding this comment.
How about truncated {} rooted votes, which have been credited ?
| writeln!(f, "- slot: {}", vote.slot)?; | ||
| writeln!(f, " confirmation count: {}", vote.confirmation_count)?; |
There was a problem hiding this comment.
Condensed this two lines to one for the precious vertical screen estate. :)
There was a problem hiding this comment.
NIT: What do you think about saving the per-line field name redundancy with a header line at the top?
There was a problem hiding this comment.
Hmm, I actually like this epoch-info-style (key: value). I might do this once I got enough motivation in the future. :)
| (if epoch_voting_history.len() < MAX_EPOCH_CREDITS_HISTORY { | ||
| "All" | ||
| } else { | ||
| "Recent" | ||
| }), |
There was a problem hiding this comment.
Made this section dynamic; similar treatment was applied to the votes section for consistentcy. ... so that we can quickly determine the existence of truncation by looking either at the top or bottom of possibly long list. :)
Codecov Report
@@ Coverage Diff @@
## master #15237 +/- ##
=========================================
- Coverage 79.5% 79.5% -0.1%
=========================================
Files 402 402
Lines 102337 102359 +22
=========================================
Hits 81446 81446
- Misses 20891 20913 +22 |
| writeln!( | ||
| f, | ||
| "* missed credits include slots unavailable to vote on due to delinquent leaders", | ||
| "{} Votes (using {}/{} entries):", |
There was a problem hiding this comment.
The subtle thing here is that 29/31 is possible even if vote truncation started because fork switch. So, the Recent/All title switching and this number indicator isn't strictly-speaking redundant information.
t-nelson
left a comment
t-nelson
left a comment
There was a problem hiding this comment.
Thanks! This looks much cleaner!
LGTM as is, I did offer some rewording suggestions where you had questions and a couple nits where we can take the cleanup a little further should you feel like it
| if let Some(newest) = epoch_voting_history.iter().rev().next() { | ||
| writeln!( | ||
| f, | ||
| "- ... (truncated older {} votes, which was rooted and became credits)", |
There was a problem hiding this comment.
How about truncated {} rooted votes, which have been credited ?
|
|
||
| writeln!( | ||
| f, | ||
| "- ... (truncated older history covering {} credits/rooted votes)", |
There was a problem hiding this comment.
omitting {} past rooted votes, which have already been credited ?
| writeln!(f, "- slot: {}", vote.slot)?; | ||
| writeln!(f, " confirmation count: {}", vote.confirmation_count)?; |
There was a problem hiding this comment.
NIT: What do you think about saving the per-line field name redundancy with a header line at the top?
| )?; | ||
| } | ||
|
|
||
| for entry in epoch_voting_history.iter().rev() { |
There was a problem hiding this comment.
NIT: Maybe give this block the single-line treatment as well?
There was a problem hiding this comment.
well, I rather keep this as-is. Maybe, the single-line style condenses too much information in a single line.
mergify
Bot
dismissed
t-nelson’s stale review
Pull request has been modified.
|
@t-nelson thanks for the review. :) |
2 participants
Problem
My last periodic inflation checkup was mislead to a false bug hunting due to my hurried look at
solana vote-account.I'd say this subcommand needs more love. ;)
Summary of Changes
watch solana vote-account-ing for awhile.(Originally, I wanted to hide these additions under verbose, but I found that wiring takes some more work. I don't think this new by-default output impedes understanding for general consumption.)
BEFORE:
AFTER: