sync: coreth PR #963,981,1009: sync package extension and related items #1679
Conversation
| MetadataDB: vm.metadataDB, | ||
| Acceptor: vm, | ||
| Parser: vm.extensionConfig.SyncableParser, | ||
| Extender: nil, |
| Start []byte `serialize:"true"` | ||
| End []byte `serialize:"true"` | ||
| Limit uint16 `serialize:"true"` | ||
| NodeType NodeType `serialize:"true"` |
There was a problem hiding this comment.
Is this a breaking change?
There was a problem hiding this comment.
For serialization? It probably is, although this didn't seem to be noted in coreth - are there reprecussions you want to avoid here? Minimizing difference with coreth was the driving goal.
Signed-off-by: Jonathan Oppenheimer <[email protected]>
Signed-off-by: Jonathan Oppenheimer <[email protected]>
JonathanOppenheimer
added
the
state-sync-migration
JonathanOppenheimer
changed the title
Signed-off-by: Jonathan Oppenheimer <[email protected]>
| Start []byte `serialize:"true"` | ||
| End []byte `serialize:"true"` | ||
| Limit uint16 `serialize:"true"` | ||
| NodeType NodeType `serialize:"true"` |
Gotcha - I can split it up further but thought the extension stuff in general could be a single PR. If you think it's worth the effort I will, otherwise will let be! |
Co-authored-by: Austin Larson <[email protected]> Signed-off-by: Jonathan Oppenheimer <[email protected]>
Co-authored-by: Austin Larson <[email protected]> Signed-off-by: Jonathan Oppenheimer <[email protected]>
Co-authored-by: Austin Larson <[email protected]> Signed-off-by: Jonathan Oppenheimer <[email protected]>
There was a problem hiding this comment.
I was in the middle of reviewing but it seems this has some invalid and unrelated changes. It also is too big and it reverts some of previous PRs.
Can we focus on each of these sync PRs one-by-one maybe not mechanically but conceptually. A directy cherry-pick could be end up with massive conflicts.
|
|
||
| // Sync settings | ||
| StateSyncEnabled bool `json:"state-sync-enabled"` | ||
| StateSyncEnabled *bool `json:"state-sync-enabled"` // Pointer distinguishes false (no state sync) and not set (state sync only at genesis). |
There was a problem hiding this comment.
why have we changed this? This is a substantial change for default value, we should have a separate PR for that if we want to align them.
There was a problem hiding this comment.
We have changed this because this was a change that Darioush made years ago to coreth, that was never synced to subnet-evm, and then was built on top off. This led to some significant divergence.
See ava-labs/coreth@cd3a14e#diff-6c4b2fd34f767685bfc35038a59ea4fbfadaa6060d70031002e3b008e73a06e7.
I have created a targeted PR for this change (and this change does need to be synced):
| if constants.ProductionNetworkIDs.Contains(networkID) { | ||
| defaultConfig := NewDefaultConfig() | ||
| if c.CommitInterval != defaultConfig.CommitInterval { | ||
| return fmt.Errorf("cannot start non-local network with commit interval %d different than %d", c.CommitInterval, defaultConfig.CommitInterval) | ||
| } | ||
| if c.StateSyncCommitInterval != defaultConfig.StateSyncCommitInterval { | ||
| return fmt.Errorf("cannot start non-local network with syncable interval %d different than %d", c.StateSyncCommitInterval, defaultConfig.StateSyncCommitInterval) | ||
| } | ||
| } |
There was a problem hiding this comment.
simirlay we should not change these here(I don't think we ever change this in those sync PRs)
| if vm.extensionConfig == nil { | ||
| // Initialize clock if not already set | ||
| if vm.clock == nil { | ||
| vm.clock = &mockable.Clock{} | ||
| } | ||
| vm.extensionConfig = &extension.Config{ | ||
| SyncSummaryProvider: &message.BlockSyncSummaryProvider{}, | ||
| SyncableParser: message.NewBlockSyncSummaryParser(), | ||
| } | ||
| // Provide a clock to the extension config before validation | ||
| vm.extensionConfig.Clock = vm.clock | ||
| } |
There was a problem hiding this comment.
I thought we resolved this in the previous PR #1743
| func (vm *VM) Connected(ctx context.Context, nodeID ids.NodeID, version *version.Application) error { | ||
| vm.vmLock.Lock() | ||
| defer vm.vmLock.Unlock() | ||
|
|
||
| if err := vm.validatorsManager.Connect(nodeID); err != nil { | ||
| return fmt.Errorf("uptime manager failed to connect node %s: %w", nodeID, err) | ||
| } | ||
| return vm.Network.Connected(ctx, nodeID, version) | ||
| } | ||
|
|
||
| func (vm *VM) Disconnected(ctx context.Context, nodeID ids.NodeID) error { | ||
| vm.vmLock.Lock() | ||
| defer vm.vmLock.Unlock() | ||
|
|
||
| if err := vm.validatorsManager.Disconnect(nodeID); err != nil { | ||
| return fmt.Errorf("uptime manager failed to disconnect node %s: %w", nodeID, err) | ||
| func (vm *VM) stateSyncEnabled(lastAcceptedHeight uint64) bool { | ||
| if vm.config.StateSyncEnabled != nil { | ||
| // if the config is set, use that | ||
| return *vm.config.StateSyncEnabled | ||
| } | ||
|
|
||
| return vm.Network.Disconnected(ctx, nodeID) |
There was a problem hiding this comment.
this does not look correct
Signed-off-by: Jonathan Oppenheimer <[email protected]>
3 participants
This PR syncs:
Note that the sync package and related items had differences that were years out of date, so this PR also brings in changes that were made in PRs in the past, and just never synced to subnet-evm.
Additionally, this PR brings in a lot of changes that aren't particular useful for subnet-evm, like the extender. This is intended, as I am trying to minimize the difference between coreth's version of these files, and subnet-evms. The eventual goal is the complete merger of all of these files, which is why I am making these changes now.
This PR introduces practically 0 "originally written by me code" - this is a culmination of the code of many other people (and in effect, has already been reviewed)
Also syncs this PR: ava-labs/coreth#1338 as surfaced by @alarso16