fix: enable automatic upgrades on the server side (#1981)#1983
Open
fix: enable automatic upgrades on the server side (#1981)#1983
Conversation
# What ❔ <!-- What are the changes this PR brings about? --> <!-- Example: This PR adds a PR template to the repo. --> <!-- (For bigger PRs adding more context is appreciated) --> ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. # What ❔ <!-- What are the changes this PR brings about? --> <!-- Example: This PR adds a PR template to the repo. --> <!-- (For bigger PRs adding more context is appreciated) --> ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated.
kelemeno
requested review from
StanislavBreadless and
vladbochok
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96116eaa79
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
121
to
123
| function _onlyAdminOrChainTypeManagerOrUpgradeExecutor() internal view { | ||
| if (msg.sender != s.admin && msg.sender != s.chainTypeManager && !s.validators[msg.sender] && false) { // kl todo | ||
| revert Unauthorized(msg.sender); |
There was a problem hiding this comment.
The new onlyAdminOrChainTypeManagerOrUpgradeExecutor guard is effectively a no‑op because the condition includes && false, which makes the if unreachable. That means any address can pass the modifier, and since upgradeChainFromVersion now uses it, any caller can execute a pending upgrade as soon as the cut hash matches, bypassing the intended admin/chain-type-manager/upgrade-executor restriction and any timing controls. This allows unauthorized or premature upgrades whenever upgradeCutHash is set.
Useful? React with 👍 / 👎.
…s into kl/automatic-upgrades
kelemeno
force-pushed
the
kl/automatic-upgrades
branch
from
e527e38 to
0262c7a
Compare
kelemeno
changed the title
…s into kl/automatic-upgrades
…s into kl/automatic-upgrades
Collaborator
StanislavBreadless
left a comment
StanislavBreadless
left a comment
There was a problem hiding this comment.
also, we should add a test for upgrading via the validator timelock, etc
| @@ -140,4 +148,10 @@ interface IValidatorTimelock is IExecutor { | |||
| uint256 _processBatchTo, | |||
| bytes calldata _batchData | |||
| ) external; | |||
| /// @dev Make a call to the zkChain diamond contract to upgrade from a specific protocol version. | |||
Collaborator
There was a problem hiding this comment.
nit: should be a newline before new function declaration
| /// @param _cutData The diamond cut parameters that is executed in the upgrade | ||
| function upgradeChainFromVersion(uint256 _protocolVersion, Diamond.DiamondCutData calldata _cutData) external; | ||
| function upgradeChainFromVersion( | ||
| address _chainAddress, |
Collaborator
There was a problem hiding this comment.
I know that you have not yet worked on this change on the server side, but this is a breaking change for zkstack cli etc
| rotateCommitterRole: false, | ||
| rotateReverterRole: true, | ||
| rotateProverRole: true, | ||
| rotateExecutorRole: true |
Collaborator
There was a problem hiding this comment.
we never in this script set that the upgrader role is true, I am not sure it is usable for the platform team
Contributor
Author
There was a problem hiding this comment.
Tomasz tried it and he said it worked somehow
Collaborator
There was a problem hiding this comment.
most likely because he uses ValidatorTimelock.addValidator which sets all the roles
Collaborator
There was a problem hiding this comment.
We still need to rotate it here somehow
| @@ -140,4 +148,10 @@ interface IValidatorTimelock is IExecutor { | |||
| uint256 _processBatchTo, | |||
| bytes calldata _batchData | |||
| ) external; | |||
| /// @dev Make a call to the zkChain diamond contract to upgrade from a specific protocol version. | |||
| function upgradeChainFromVersion( | |||
Collaborator
There was a problem hiding this comment.
another small note:
- the interface is defined inside IAdmin.sol, but ValidatorTimelock does not inherit it.
- this introduces a risk that we may accidentally change the function signature and not notice it. the tests help, but whenever we have an opportunity to use compile time checks, we should do so
I think the best solutions are either:
- move this function to executor
- create a new small upgrade-only interface that will be inherited by both IAdmin and IValidatorTimelock
|
|
||
| /// @notice Checks that the message sender is an active admin | ||
| modifier onlyAdmin() { | ||
| _onlyAdmin(); |
Collaborator
There was a problem hiding this comment.
why did we introduce so many changes in this file?
…s into kl/automatic-upgrades
…s into kl/automatic-upgrades
…s into kl/automatic-upgrades
Contributor
|
Coverage after merging kl/automatic-upgrades into draft-v31 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
What ❔
enable automatic upgrades on the server side
Why ❔
Checklist
entries from PRs).
What ❔
Why ❔
Checklist