-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Don't vote for empty leader transmissions #3248
Merged
Merged
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
04ba184
Don't vote for empty leader transmissions
carllin f28dd29
Add is_delta flag to bank to detect empty leader transmissions
carllin 876324f
Plumb new is_votable flag through replay stage
carllin 8e37327
Fix PohRecorder tests
carllin 09fa331
Change is_delta to AtomicBool to avoid making Bank references mutable
carllin e26780d
Reset start slot in poh_recorder when working bank is cleared, so tha…
carllin 784c5c6
Use proper max tick height calculation
carllin 0b68a94
Test for not voting on empty transmission
carllin d0c5325
tests for is_votable
carllin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pub function (called from another crate) needs tests and a doc comment. At first glance, it looks like this implementation can be replaced with:
but no way to find out from within the solana_runtime crate. The test up in ReplayStage only tests one case, but this function alone has several edge cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are not equivalent, is_delta can be false when !self.accounts.is_empty() is true (f your parent bank is nonempty, the child bank will always be nonempty as well, even if the child processes no transactions). We check self.tick_height() == max_tick_height instead of is_frozen because @rob-solana pointed out we may want to check if a bank is votable before freezing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, which edge cases were you seeing? The one I can think of is that a bank that processes only ticks has self.is_delta == false, and that processing a tx sets it to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carllin there definitely should be local tests. cargo test --manifest-path runtime/Cargo.toml should cover everything in here
@garious this check is conservative and so shouldn't have edge cases. once tick_height() is max, no commit() will be called, and commit() is where we move to "is_delta".
the reason this check is down here is because otherwise, its logic would have to be duplicated in the TVU and in the TPU (which talk to the bank at different levels)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use HashQueue::hash_height()? I don't really want an answer as much as I want a test to answer on your behalf.