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.

Sign up for GitHub

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

Jump to bottom

Take .max(1) when computing MAX_RAYON_THREADS #25940

Merged
merged 1 commit into from
Jun 13, 2022
Merged

Take .max(1) when computing MAX_RAYON_THREADS #25940

merged 1 commit into from
Jun 13, 2022

Conversation

brooksprumo
Copy link
Contributor

Problem

A third party fuzzer uncovered a divide-by-zero error in blockstore_processor::execute_batches().

solana/ledger/src/blockstore_processor.rs

Lines 356 to 361 in 3e1fd4d

let target_batch_count = get_thread_count() as u64;
let mut tx_batches: Vec<TransactionBatch> = vec![];
let mut tx_batch_costs: Vec<u64> = vec![];
let rebatched_txs = if total_cost > target_batch_count.saturating_mul(minimal_tx_cost) {
let target_batch_cost = total_cost / target_batch_count;

target_batch_count could be 0 due to the impl of get_thread_count().

Summary of Changes

Add in a .max(1) to ensure the smallest number returned is 1, not 0.

@brooksprumo brooksprumo marked this pull request as ready for review June 13, 2022 18:28
jeffwashington
jeffwashington approved these changes Jun 13, 2022
View reviewed changes
Copy link
Contributor

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo brooksprumo added the automerge Merge this Pull Request automatically once CI passes label Jun 13, 2022
@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Jun 13, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2022

automerge label removed due to a CI failure

@brooksprumo brooksprumo added the automerge Merge this Pull Request automatically once CI passes label Jun 13, 2022
@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Jun 13, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2022

automerge label removed due to a CI failure

@brooksprumo brooksprumo merged commit c419845 into solana-labs:master Jun 13, 2022
@brooksprumo brooksprumo deleted the min-valuse-for-max-rayon-threads branch June 13, 2022 19:17
@steviez steviez added the v1.10 label Aug 7, 2022
mergify bot pushed a commit that referenced this pull request Aug 7, 2022
@brooksprumo @mergify
Take .max(1) when computing MAX_RAYON_THREADS (#25940)
0446c0c 
(cherry picked from commit c419845)
@mergify mergify bot mentioned this pull request Aug 7, 2022
Merged
@steviez
Copy link
Contributor

steviez commented Aug 7, 2022

Someone hit this on v1.10.34 so marked it for backport:
https://discord.com/channels/428295358100013066/560174212967432193/1005887403157885086

mergify bot added a commit that referenced this pull request Aug 8, 2022
@mergify @brooksprumo
Take .max(1) when computing MAX_RAYON_THREADS (backport #25940) (#2…
a3d949b 
…6973)

Take `.max(1)` when computing MAX_RAYON_THREADS (#25940)

(cherry picked from commit c419845)

Co-authored-by: Brooks Prumo <brooks@solana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffwashington jeffwashington jeffwashington approved these changes

@pgarg66 pgarg66 Awaiting requested review from pgarg66

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@brooksprumo @steviez @jeffwashington