-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Refactor - Delay visibility of program un-/re-/deployment #29654
Refactor - Delay visibility of program un-/re-/deployment #29654
Conversation
689c831
to
d0f1e9c
Compare
7c56ecf
to
d631faf
Compare
b579aa5
to
115644a
Compare
115644a
to
a729199
Compare
a729199
to
76cc1e7
Compare
a6fa38a
to
90850fb
Compare
self.executors.insert(key, Some(executor.clone())); | ||
} else { | ||
// Do not overwrite an existing entry | ||
self.executors.entry(key).or_insert(None); |
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 insert None
if not found?
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.
That is essentially an explicit invalid entry similar to a tombstone. So that if a program was not deployed before the transaction begins, and it is deployed during the transaction, that the rest of the transaction still sees it as undeployed. In other words it is necessary to get case 1 (deployment invisibility) right.
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.
Catching up on the issue, asked few questions. Will deep dive again.
90850fb
to
28f3ed2
Compare
// Executor exists and is cached, use it | ||
Some(Some(executor)) => return Ok((executor, None)), | ||
// We cached that the Executor does not exist, abort | ||
Some(None) => return Err(InstructionError::InvalidAccountData), |
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 looks a little scary since it is a new error condition that is not directly featurized. Afaict this case can only occur once the delay
feature is activated, but that is a bit distant and could potentially lead to confusion or a missed code path
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.
Hm yes, I added a comment to point out that this case only exists when the feature is active.
The thing is, if we made a condition here that would check the feature, it would still result in an error being returned.
Not much gained IMO.
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.
Yeah, comment would help
b3d1b70
to
473f9d5
Compare
3bc4d2f
to
b18c0da
Compare
b18c0da
to
9a469fe
Compare
9a469fe
to
640ef50
Compare
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.
lgtm, the deploy program macro made tests easier to read too 👍🏼
if upgrade { | ||
if delay_visibility_of_program_deployment { | ||
// Place a tombstone in the cache so that | ||
// we don't load the new version from the database as it should remain invisible |
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.
👍🏼
…s#29654) * Use three separate HashMaps instead of the enum TxBankExecutorCacheDiff. * Replaces all places which deploy programs by a macro. * Adds a feature gate. * Adjust tests. * Makes undeployment visible immediately.
## Summary solana-labs/solana#29654 introduced a delay between the slot a program is deployed and when it can be called. Let's build this into the loader now so as to remove a footgun from folks trying to call programs immediately after deploying them. ## Test Plan ``` pnpm test:live-with-test-validator ```
## Summary solana-labs/solana#29654 introduced a delay between the slot a program is deployed and when it can be called. Let's build this into the loader now so as to remove a footgun from folks trying to call programs immediately after deploying them. ## Test Plan ``` pnpm test:live-with-test-validator ```
Problem
Currently some changes to programs made by un-/re-/deployment are visible immediately, even within the same transaction. These inconsistency between top-level instructions and CPI should be removed. This is a preparation for the replacement of the current executor cache by one that supports a bitemporal model. Once that is done, the new executor cache will further delay the visibility from the end of the transaction to a slot in the future, with a constant offset from the slot the action occurred in.
Summary of Changes
HashMap
s instead of theenum
TxBankExecutorCacheDiff
Old vs New Behavior (inside the same transaction)
In other words the new consistent behavior is, that whenever a program is un-/re-/deployed,
it becomes unavailable for the rest of the transaction.
Feature Gate Issue: #30085