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

ClusterInfoVoteListener send only missing votes to BankingStage#20873

Merged
carllin merged 13 commits intosolana-labs:masterfrom
carllin:FixVotes
Nov 18, 2021
Merged

ClusterInfoVoteListener send only missing votes to BankingStage#20873
carllin merged 13 commits intosolana-labs:masterfrom
carllin:FixVotes

Conversation

@carllin
Copy link
Copy Markdown
Contributor

@carllin carllin commented Oct 22, 2021

Problem

  1. ClusterInfoVoteListener has too short of a window (20 slots) for sending votes to BankingStage
  2. ClusterInfoVoteListener sends all votes to BankingStage, even outdated/ones on wrong fork
  3. ClusterInfoVoteListener may send votes out of order

Summary of Changes

Only send newer votes on the same fork, in the right order to BankingStage

Fixes #

@carllin carllin force-pushed the FixVotes branch 4 times, most recently from 2fb1441 to faed3f5 Compare October 28, 2021 11:47
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 28, 2021

Codecov Report

Merging #20873 (34577e1) into master (8910254) will decrease coverage by 0.0%.
The diff coverage is 96.0%.

@@            Coverage Diff            @@
##           master   #20873     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         501      501             
  Lines      140699   140911    +212     
=========================================
+ Hits       114751   114903    +152     
- Misses      25948    26008     +60     

@carllin carllin force-pushed the FixVotes branch 8 times, most recently from 207754e to ac74b2c Compare October 31, 2021 17:52
@carllin carllin marked this pull request as ready for review October 31, 2021 23:03

#[test]
#[serial]
fn test_votes_land_in_fork_during_long_partition() {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Local cluster test fails without these changes, passes with these fixes

Comment thread core/src/cluster_info_vote_listener.rs
Comment thread core/src/cluster_info_vote_listener.rs Outdated
Comment thread core/src/cluster_info_vote_listener.rs
Comment thread core/src/cluster_info_vote_listener.rs
Comment thread core/src/cluster_info_vote_listener.rs
Comment thread core/src/cluster_info_vote_listener.rs Outdated
Comment thread core/src/cluster_info_vote_listener.rs
Comment thread core/src/verified_vote_packets.rs
tao-stones
tao-stones previously approved these changes Nov 4, 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 November 4, 2021 22:23

Pull request has been modified.

@carllin carllin force-pushed the FixVotes branch 5 times, most recently from 2b35245 to 86c903c Compare November 17, 2021 23:48
@carllin carllin merged commit b30c94c into solana-labs:master Nov 18, 2021
frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021
@carllin carllin added the v1.8 label Dec 28, 2021
mergify Bot pushed a commit that referenced this pull request Dec 28, 2021
(cherry picked from commit b30c94c)

# Conflicts:
#	core/src/cluster_info_vote_listener.rs
#	core/src/verified_vote_packets.rs
#	gossip/src/cluster_info.rs
#	local-cluster/tests/local_cluster.rs
#	runtime/src/bank.rs
carllin added a commit that referenced this pull request Dec 28, 2021
(cherry picked from commit b30c94c)

# Conflicts:
#	core/src/cluster_info_vote_listener.rs
#	core/src/verified_vote_packets.rs
#	gossip/src/cluster_info.rs
#	local-cluster/tests/local_cluster.rs
#	runtime/src/bank.rs
carllin added a commit that referenced this pull request Dec 30, 2021
(cherry picked from commit b30c94c)

# Conflicts:
#	core/src/cluster_info_vote_listener.rs
#	core/src/verified_vote_packets.rs
#	gossip/src/cluster_info.rs
#	local-cluster/tests/local_cluster.rs
#	runtime/src/bank.rs
mergify Bot added a commit that referenced this pull request Dec 30, 2021
…port #20873) (#22135)

* ClusterInfoVoteListener send only missing votes to BankingStage (#20873)

(cherry picked from commit b30c94c)

# Conflicts:
#	core/src/cluster_info_vote_listener.rs
#	core/src/verified_vote_packets.rs
#	gossip/src/cluster_info.rs
#	local-cluster/tests/local_cluster.rs
#	runtime/src/bank.rs

* Resolve conflicts

* Remove Select

* Fixup tests

Co-authored-by: carllin <carl@solana.com>
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