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

Use fork weight instead of individual bank weight for fork selection. #7079

Merged
merged 9 commits into from Nov 21, 2019

Conversation

@aeyakovenko
Copy link
Member

aeyakovenko commented Nov 21, 2019

Problem

Summary of Changes

Fixes #

tag: @carllin

@aeyakovenko aeyakovenko force-pushed the aeyakovenko:FixReplayStage branch from a718273 to 28ef7c0 Nov 21, 2019
@aeyakovenko aeyakovenko changed the title Fix replay stage Use fork weight instead of individual bank weight for fork selection. Nov 21, 2019
@aeyakovenko aeyakovenko requested review from carllin and sagar-solana Nov 21, 2019
@aeyakovenko aeyakovenko marked this pull request as ready for review Nov 21, 2019
aeyakovenko added 2 commits Nov 21, 2019
fmt
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 21, 2019

Codecov Report

Merging #7079 into master will decrease coverage by 0.8%.
The diff coverage is 62.2%.

@@           Coverage Diff            @@
##           master   #7079     +/-   ##
========================================
- Coverage    76.5%   75.7%   -0.9%     
========================================
  Files         228     228             
  Lines       45803   46145    +342     
========================================
- Hits        35067   34934    -133     
- Misses      10736   11211    +475
aeyakovenko added 3 commits Nov 21, 2019
+ bank
.parent()
.and_then(|b| progress.get(&b.slot()))
.map(|x| x.fork_stats.fork_weight)

This comment has been minimized.

Copy link
@carllin

carllin Nov 21, 2019

Contributor

can we hard assert here that this entry must exist in the progress map, makes it easier to reason about correctness

debug!("{}: {:?} {:?}", stats.slot, stats, parents,);
});
let rv = Self::pick_best_fork(ancestors, &candidates);
candidates.sort_by_key(|b| (b.1.fork_weight, 0i64 - b.1.slot as i64));

This comment has been minimized.

Copy link
@carllin

carllin Nov 21, 2019

Contributor

Can we add a few tests here to illustrate the expected behavior in scenarios where parent-child fork weights are the same? If we are sure that parent weight > child weight in all scenarios, can we assert above that bank_weight + fork_weight > fork_weight, i.e. bank_weight > 0 ( I thnk this is true b/c we always at last simulate one vote)

This comment has been minimized.

Copy link
@aeyakovenko
@@ -200,7 +200,7 @@ fn run_network_partition(partitions: &[usize]) {
..ClusterConfig::default()
};
let now = timestamp();
let partition_start = now + 30_000;
let partition_start = now + 60_000;

This comment has been minimized.

Copy link
@carllin

carllin Nov 21, 2019

Contributor

Is the goal here to start the partition after all the validators are already in the leader schedule? If so, there's some logic in cluster_tests::sleep_n_epochs that can more precisely calculate how long to wait based on epoch len, ticks per slot, etc.

This comment has been minimized.

Copy link
@aeyakovenko
@aeyakovenko

This comment has been minimized.

Copy link
Member Author

aeyakovenko commented Nov 21, 2019

security issue: #7087

@aeyakovenko aeyakovenko merged commit d9e7a5f into solana-labs:master Nov 21, 2019
12 checks passed
12 checks passed
Summary 1 rule matches and 7 potential rules
Details
buildkite/solana Build #15513 passed (33 minutes, 28 seconds)
Details
buildkite/solana/bench Passed (12 minutes, 17 seconds)
Details
buildkite/solana/checks Passed (1 minute, 40 seconds)
Details
buildkite/solana/coverage Passed (9 minutes, 22 seconds)
Details
buildkite/solana/local-cluster Passed (14 minutes, 7 seconds)
Details
buildkite/solana/move Passed (13 seconds)
Details
buildkite/solana/pipeline-upload Passed (9 seconds)
Details
buildkite/solana/shellcheck Passed (32 seconds)
Details
buildkite/solana/stable Passed (31 minutes, 28 seconds)
Details
buildkite/solana/stable-perf Passed (9 minutes, 35 seconds)
Details
ci-gate Pull Request accepted for CI pipeline
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

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