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

Return implicit delay visibility tombstone #31493

Merged
merged 3 commits into from
May 5, 2023

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented May 4, 2023

Problem

The explicit placement of delay visibility tombstone has been removed from the code. Need to update extract() to implicitly return the tombstone under correct conditions.

Summary of Changes

Return the delay visibility tombstone if the current slot is between deployment and effective slot for the cached program.
Updated the unit tests.

Fixes #

@pgarg66 pgarg66 requested a review from Lichtso May 4, 2023 19:04
@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Merging #31493 (568405a) into master (53aec4a) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #31493   +/-   ##
=======================================
  Coverage    81.4%    81.4%           
=======================================
  Files         731      731           
  Lines      208720   208738   +18     
=======================================
+ Hits       169980   170006   +26     
+ Misses      38740    38732    -8     

@@ -425,6 +425,15 @@ impl LoadedPrograms {

if current_slot >= entry.effective_slot {
return Some((key, entry.clone()));
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this condition also trigger when recompilation for feature set changes happens?

Copy link
Contributor Author

@pgarg66 pgarg66 May 4, 2023

Choose a reason for hiding this comment

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

That's correct. One potential solution is to define a const for the gap between deployed and effective slots for the delay visible programs. e.g. 1 slot. We can return the tombstone only if the gap is 1 slot, and the current slot >= deployment slot but < effective slot.

This 1 slot gap will be much less compared to the slot offset during recompilation. So we should be able to differentiate between the two use-cases.

Any other solutions to handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code and the test.

@pgarg66 pgarg66 requested a review from Lichtso May 4, 2023 23:10
@pgarg66 pgarg66 merged commit 9c6e7e0 into solana-labs:master May 5, 2023
4 checks passed
@pgarg66 pgarg66 deleted the implicit-tombstone branch May 5, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants