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

Correctly handle the only root case#13352

Closed
ryoqun wants to merge 1 commit intosolana-labs:masterfrom
ryoqun:the-last-tower-bug
Closed

Correctly handle the only root case#13352
ryoqun wants to merge 1 commit intosolana-labs:masterfrom
ryoqun:the-last-tower-bug

Conversation

@ryoqun
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun commented Nov 2, 2020

Problem

this is the last known bug of tower.

this is just a brain dump so that I can focus on inflation.

Summary of Changes

TBD

Fixes #

@ryoqun ryoqun requested a review from carllin November 2, 2020 19:16
@ryoqun ryoqun changed the title the only root case Correctl handle the only root case Nov 2, 2020
@ryoqun ryoqun changed the title Correctl handle the only root case Correctly handle the only root case Nov 2, 2020
Comment thread core/src/consensus.rs
slot_history.add(42);

let tower = tower.adjust_lockouts_after_replay(42, &slot_history);
assert_eq!(tower.unwrap().voted_slots(), [42]);
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.

also assert root

Comment thread core/src/consensus.rs
}

#[test]
fn test_adjust_lockouts_after_replay_vote_on_only_root() {
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.

also add future tower version of this test?

Comment thread core/src/consensus.rs

let original_votes_len = self.lockouts.votes.len();
self.initialize_lockouts(move |_| retain_flags_for_each_vote.next().unwrap());
let keep_the_only_root_vote_for_no_double_vote =
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.

comment here why this must be accepted.

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.

because we start to vote on root....

also, it's very very rare to see this on the wild. I've just come up with this possibility while working on #12671.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can also augment the check here https://github.com/solana-labs/solana/blob/master/core/src/consensus.rs#L454 to check that the vote slot > root so the is_locked_out check will fail

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 2, 2020

Codecov Report

Merging #13352 into master will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #13352   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         376      376           
  Lines       87888    87903   +15     
=======================================
+ Hits        72104    72132   +28     
+ Misses      15784    15771   -13     

@stale
Copy link
Copy Markdown

stale Bot commented Nov 10, 2020

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 Nov 10, 2020
@stale
Copy link
Copy Markdown

stale Bot commented Nov 17, 2020

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

@stale stale Bot closed this Nov 17, 2020
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.

2 participants