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

Don't commit failed out-dated votes#20850

Closed
sakridge wants to merge 1 commit intosolana-labs:masterfrom
sakridge:out-dated-votes
Closed

Don't commit failed out-dated votes#20850
sakridge wants to merge 1 commit intosolana-labs:masterfrom
sakridge:out-dated-votes

Conversation

@sakridge
Copy link
Copy Markdown
Contributor

Problem

Outdated votes can be bloated on-chain. Since votes arrive out-of-order, then older ones can be applied after later ones and also for different branches.

Summary of Changes

Don't commit failed votes which fail in the way of VoteTooOld.

Fixes #

@sakridge sakridge force-pushed the out-dated-votes branch 6 times, most recently from e6cacf4 to b7c7673 Compare October 21, 2021 16:53
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 21, 2021

Codecov Report

Merging #20850 (f4d7211) into master (140a5f6) will decrease coverage by 0.0%.
The diff coverage is 64.0%.

@@            Coverage Diff            @@
##           master   #20850     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         499      499             
  Lines      140278   140296     +18     
=========================================
- Hits       114403   114368     -35     
- Misses      25875    25928     +53     

@carllin carllin requested a review from tao-stones October 21, 2021 22:31
@carllin
Copy link
Copy Markdown
Contributor

carllin commented Oct 21, 2021

Nice, so once this goes in, we can then increase the range of leaders in the vote_listener

@tao-stones
Copy link
Copy Markdown
Contributor

The PR I had before, #19400, does similar thing, except it drops too_old vote before bank.load_and_execute_transactions(...), might be bit more efficient than doing so after bank load/execute.
However, that PR probably needs a bit more work.

@carllin
Copy link
Copy Markdown
Contributor

carllin commented Oct 21, 2021

The PR I had before, #19400, does similar thing, except it drops too_old vote before bank.load_and_execute_transactions(...), might be bit more efficient than doing so after bank load/execute. However, that PR probably needs a bit more work.

Yeah if we're going to be parsing through hundreds of thousands of votes, it might be preferable to do this before the load_and_execute_transactions, I'll take another pass at this.

@carllin
Copy link
Copy Markdown
Contributor

carllin commented Oct 22, 2021

Although each InstructionError here incurs extra account locking + copying, I think this change is a lot simpler, so might be safer to check in first.

We might want to bench how long it takes to continually read out vote account + SlotHashes sysvar (this later one is probably bigger) for a few hundred thousand votes.

The failure rate can also be mitigated if we do some more smart processing from cluster_info_vote_listener before we send, I'll see if I can spin that up

Comment thread runtime/src/bank.rs
@carllin
Copy link
Copy Markdown
Contributor

carllin commented Oct 22, 2021

Other side of cluster_info_vote_listener I'm working on: #20873

@tao-stones
Copy link
Copy Markdown
Contributor

Although each InstructionError here incurs extra account locking + copying, I think this change is a lot simpler, so might be safer to check in first.

Agree, this is much simpler.

tao-stones
tao-stones previously approved these changes Oct 22, 2021
Copy link
Copy Markdown
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mergify mergify Bot dismissed tao-stones’s stale review October 26, 2021 15:58

Pull request has been modified.

@sakridge sakridge force-pushed the out-dated-votes branch 4 times, most recently from b5c15e7 to abfefb3 Compare October 28, 2021 09:23
@stale
Copy link
Copy Markdown

stale Bot commented Jan 9, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 9, 2022
@stale
Copy link
Copy Markdown

stale Bot commented Mar 2, 2022

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale Bot closed this Mar 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

stale [bot only] Added to stale content; results in auto-close after a week.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants