Shutdown on upgrade#2648
Conversation
tudor-malene
left a comment
tudor-malene
left a comment
There was a problem hiding this comment.
Looks good in principle, but this is only indicative.
The branch needs to be rebased.
| return msg, nil | ||
| } | ||
|
|
||
| func FromBlockSubmissionErrorMsg(msg *generated.BlockSubmissionErrorMsg) *errutil.BlockRejectError { |
There was a problem hiding this comment.
wasn't this already merged as a separate pr?
There was a problem hiding this comment.
yeah i split it out of this
|
|
||
| for _, upgrade := range upgrades { | ||
| um.logger.Info("Upgrade detected", "featureName", upgrade.FeatureName, "featureData", upgrade.FeatureData) | ||
| blockHeight := blockHeader.Number.Uint64() + 64 |
There was a problem hiding this comment.
why not use NetworkUpgradeConfirmationDepth
| } | ||
|
|
||
| // GetNetworkUpgradesByFeature retrieves network upgrades for a specific feature | ||
| func GetNetworkUpgradesByFeature(ctx context.Context, db *sqlx.DB, featureName string) ([]*NetworkUpgrade, error) { |
There was a problem hiding this comment.
there are a bunch of unused functions here. What's up with them? Please leave a comment if they will be needed in the future, and for what
| @@ -0,0 +1,17 @@ | |||
| -- Create table for storing network upgrades | |||
There was a problem hiding this comment.
give this file a better name, like 003_netw_upgrades.sql
| block_height_final bigint, | ||
| block_height_active bigint, | ||
| created_at timestamp DEFAULT CURRENT_TIMESTAMP, | ||
| PRIMARY KEY (id) |
There was a problem hiding this comment.
use this syntax:
INDEX (feature_name),
INDEX (block_height_final, block_height_active),
INDEX USING HASH (block_hash)
instead of the "create index" stuff below.
It's easier to read
| ctx := context.Background() | ||
|
|
||
| // Ensure the current block is canonical before proceeding | ||
| isCanonical, err := um.storage.IsBlockCanonical(ctx, blockHeader.Hash()) |
There was a problem hiding this comment.
since this is called from OnL1Block, it means only canonical l1 blocks are sent in. I dont think this check is necessary, maybe just leave a comment
|
|
||
| // Load activated upgrades up to current block height | ||
| currentHeight := blockHeader.Number.Uint64() | ||
| stored, err := um.storage.GetActivatedNetworkUpgrades(ctx, currentHeight) |
There was a problem hiding this comment.
don't you have to adjust with the 64 constant?
| } | ||
|
|
||
| // filterSupportedUpgrades filters upgrades to only include supported ones | ||
| func (um *upgradeManager) filterSupportedUpgrades(ctx context.Context, allUpgrades []NetworkConfig.NetworkConfigUpgraded) []NetworkConfig.NetworkConfigUpgraded { |
There was a problem hiding this comment.
this method feels too hypothetical.
I have a feeling it will not look like this in practice.
Please leave a comment that this is an indicative implementation
| g.submitDataLock.Unlock() // lock is only guarding the enclave call, so we can release it now | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), errutil.ErrBlockAlreadyProcessed.Error()) { | ||
| if resp.RejectError != nil { |
There was a problem hiding this comment.
Wasn't this stuff already included in a pr?
| }) | ||
|
|
||
| wg.Go(func() error { | ||
| ti.issueUpgradeTransaction() |
There was a problem hiding this comment.
this needs to be removed
2 participants
Why this change is needed
We need the ability to schedule upgrades, which if old versions do not support would make them shutdown.
Still needs testing.
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks