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

Only add hashes for completed blocks to recent blockhashes #24389

Merged
merged 2 commits into from
Apr 21, 2022
Merged

Only add hashes for completed blocks to recent blockhashes #24389

merged 2 commits into from
Apr 21, 2022

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Apr 15, 2022 •
edited by ryoqun
Loading

Problem

As demonstrated in #24242, the recent blockhashes queue tracks hashes for skipped blocks. The recent blockhash queue should only be storing hashes for produced blocks.

Summary of Changes

When registering ticks at slot boundaries, don't register the poh hash in the recent blockhash queue until all ticks have been completed for a bank.

some context for the justification of this change: #23949 (comment)

Fixes #24387
Feature Gate Issue: #24388

@jstarry jstarry added the feature-gate Pull Request adds or modifies a runtime feature gate label Apr 15, 2022
@jstarry jstarry mentioned this pull request Apr 15, 2022
Merged
@jstarry jstarry force-pushed the fix-recent-blockhashes branch from 74f5ab2 to 1ca2623 Compare April 15, 2022 16:31
@jstarry jstarry requested review from ryoqun and t-nelson April 15, 2022 16:31
t-nelson
t-nelson previously approved these changes Apr 15, 2022
View reviewed changes
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm! rare occurrence that we can fix anything this low-level so cleanly

@jstarry jstarry force-pushed the fix-recent-blockhashes branch from 1ca2623 to 59c9326 Compare April 16, 2022 05:59
@mergify mergify bot dismissed t-nelson’s stale review April 16, 2022 05:59

Pull request has been modified.

@jstarry
Copy link
Member Author

jstarry commented Apr 16, 2022

@t-nelson do you mind looking this over again? ProgramTest changed a bit but I managed to keep its behavior consistent even after the fix is applied

@codecov
Copy link

codecov bot commented Apr 17, 2022 •
edited
Loading

Codecov Report

Merging #24389 (c9f5a44) into master (3155270) will increase coverage by 11.9%.
The diff coverage is 73.1%.

❗ Current head c9f5a44 differs from pull request most recent head 2163f5a. Consider uploading reports for the commit 2163f5a to get more accurate results

@@             Coverage Diff             @@
##           master   #24389       +/-   ##
===========================================
+ Coverage    70.0%    82.0%    +11.9%     
===========================================
  Files          36      592      +556     
  Lines        2255   163847   +161592     
  Branches      322        0      -322     
===========================================
+ Hits         1580   134357   +132777     
- Misses        560    29490    +28930     
+ Partials      115        0      -115

@jstarry jstarry requested a review from mvines April 19, 2022 10:00
@jstarry
Copy link
Member Author

jstarry commented Apr 19, 2022

@mvines could you take a look at the program-test changes?

ryoqun
ryoqun reviewed Apr 19, 2022
View reviewed changes
runtime/src/bank.rs Outdated Show resolved Hide resolved
ryoqun
ryoqun reviewed Apr 19, 2022
View reviewed changes
sdk/program/src/clock.rs Outdated Show resolved Hide resolved
@jstarry jstarry force-pushed the fix-recent-blockhashes branch from 86d1557 to c9f5a44 Compare April 19, 2022 17:20
t-nelson
t-nelson previously approved these changes Apr 21, 2022
View reviewed changes
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

still lgtm!

@jstarry jstarry force-pushed the fix-recent-blockhashes branch from c9f5a44 to 2163f5a Compare April 21, 2022 07:05
@mergify mergify bot dismissed t-nelson’s stale review April 21, 2022 07:05

Pull request has been modified.

@jstarry jstarry merged commit d5127ab into solana-labs:master Apr 21, 2022
@jstarry jstarry deleted the fix-recent-blockhashes branch May 8, 2022 08:56
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
@jstarry @jeffwashington
Only add hashes for completed blocks to recent blockhashes (solana-la…
004ac8c 
…bs#24389)

* Only add hashes for completed blocks to recent blockhashes

* feedback
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 30, 2022
@jstarry @jeffwashington
Only add hashes for completed blocks to recent blockhashes (solana-la…
e954d73 
…bs#24389)

* Only add hashes for completed blocks to recent blockhashes

* feedback
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 30, 2022
@jstarry @jeffwashington
Only add hashes for completed blocks to recent blockhashes (solana-la…
47a74ef 
…bs#24389)

* Only add hashes for completed blocks to recent blockhashes

* feedback
@cavemanloverboy
Copy link
Contributor

does this affect replayability, i.e. if a previous tx used a blockhash that is now considered not valid?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@segfaultdoc segfaultdoc segfaultdoc left review comments

@ryoqun ryoqun ryoqun left review comments

@t-nelson t-nelson t-nelson left review comments

@mvines mvines Awaiting requested review from mvines

Assignees
No one assigned
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Skipped block hashes shouldn't be stored in the recent blockhashes queue
5 participants
@jstarry @cavemanloverboy @ryoqun @t-nelson @segfaultdoc