Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow voting on empty banks #6719

Merged
merged 5 commits into from Nov 6, 2019
Merged

Allow voting on empty banks #6719

merged 5 commits into from Nov 6, 2019

Conversation

@carllin
Copy link
Contributor

carllin commented Nov 4, 2019

Problem

Disallowing votes on empty banks leads can stall the network when their parents are also non-votable due to threshold failures. This is possible when the parents have included all the consumable transactions on the network.

Summary of Changes

  1. Allow votes on empty banks
  2. Add test to check that even when there is no activity on the network, banks still chain correctly

Fixes #

@carllin carllin requested a review from sagar-solana Nov 4, 2019
pub fn is_votable(&self) -> bool {
self.is_delta.load(Ordering::Relaxed) && self.tick_height() == self.max_tick_height
pub fn is_empty(&self) -> bool {
self.is_delta.load(Ordering::Relaxed)

This comment has been minimized.

Copy link
@garious

garious Nov 4, 2019

Member

is_delta == is_empty? Confusing terminology. Anything you can do here? is "delta" the right word?

This comment has been minimized.

Copy link
@carllin

carllin Nov 4, 2019

Author Contributor

hehe b/c I messed up, it should be!self.is_delta.load(Ordering::Relaxed), does that make more sense?

This comment has been minimized.

Copy link
@garious

garious Nov 4, 2019

Member

Makes a little more sense. A bit disturbing though. Did you updated the tests to reflect the implementation rather than updating the implementation to reflect better tests?

This comment has been minimized.

Copy link
@carllin

carllin Nov 4, 2019

Author Contributor

Even worse, I updated the tests and assumed it would work, thinking that is_empty == is_votable which is complete nonsense 😛

Updated the test, thanks!

@carllin carllin force-pushed the carllin:FixReplayStage branch from 954c7ab to 641146b Nov 4, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 4, 2019

Codecov Report

Merging #6719 into master will decrease coverage by 14.2%.
The diff coverage is 90%.

@@            Coverage Diff            @@
##           master   #6719      +/-   ##
=========================================
- Coverage    79.4%   65.1%   -14.3%     
=========================================
  Files         216     216              
  Lines       41375   50378    +9003     
=========================================
- Hits        32855   32838      -17     
- Misses       8520   17540    +9020
@carllin carllin added the v0.20 label Nov 5, 2019
@carllin carllin force-pushed the carllin:FixReplayStage branch from abc0199 to 6b73d13 Nov 5, 2019
Carl
@carllin carllin merged commit 24102a7 into solana-labs:master Nov 6, 2019
12 checks passed
12 checks passed
Rule: v0.20 backport (backport) Backports have been created
Details
Summary 2 rules match and 6 potential rules
Details
buildkite/solana Build #14339 passed (32 minutes)
Details
buildkite/solana/bench Passed (17 minutes, 58 seconds)
Details
buildkite/solana/checks Passed (1 minute, 51 seconds)
Details
buildkite/solana/coverage Passed (10 minutes, 51 seconds)
Details
buildkite/solana/local-cluster Passed (17 minutes, 31 seconds)
Details
buildkite/solana/pipeline-upload Passed (8 seconds)
Details
buildkite/solana/shellcheck Passed (31 seconds)
Details
buildkite/solana/stable Passed (29 minutes, 48 seconds)
Details
buildkite/solana/stable-perf Passed (6 minutes, 17 seconds)
Details
ci-gate Pull Request accepted for CI pipeline
mergify bot pushed a commit that referenced this pull request Nov 6, 2019
* Allow votes on empty banks

* Remove making first bank is_delta true, no longer necessary for idling

* Remove votable from ledger tool

(cherry picked from commit 24102a7)
solana-grimes added a commit that referenced this pull request Nov 6, 2019
automerge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.