-
Notifications
You must be signed in to change notification settings - Fork 752
Align minCompatibleTime settings across TestNetwork and Network #3842
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
base: master
Are you sure you want to change the base?
Conversation
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.
Could we instead remove minCompatibilityTime from NewNetwork
altogether, and use FortunaTime in all instances? That way the activation time used by the test network is covered by general Network
upgrade tests (i.e. e2e tests that run full nodes).
I debated that, or adding the I think the Either way, I agree minimizing the number of places updates need to be made should be the goal in order to reduce the chance that any are missed in the future. |
I updated the PR to this approach now. I agree it's less error prone since there's only one place where the minCompatibleTime is defined, and should be covered in the E2E tests. The downside is needing to pass the full I'm pretty indifferent between the options myself really, open to everyone's input. |
@@ -175,7 +176,7 @@ type network struct { | |||
// NewNetwork returns a new Network implementation with the provided parameters. | |||
func NewNetwork( | |||
config *Config, | |||
minCompatibleTime time.Time, | |||
upgradeConfig upgrade.Config, |
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 change in the to depend on upgrade
looks unrelated to the intent of this PR to me... can we remove changes that aren't in test_network.go
form the diff?
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.
See Cam's comment above for the context here. We can do this just by updating test_network.go
(as we had originally), but that would make it such that multiple places need to be updated for every network upgrade to ensure compatibility, which is more error prone. This approach makes it so that the minCompatibleTime
only needs to be updated in a single place, but requires then passing the full upgrade.Config
to NewNetwork
.
Open to whichever is preferred on your side.
Why this should be merged
Aligns
TestNetwork
compatibility with theNetwork
created for full AvalancheGo nodes here. This should be updated in the future when required network upgrades are scheduled.How this works
Passes the full
UpgradeConfig
toNewNetwork
such that all callers (currently justnode.go
andtest_network.go
will use the same upgrade as the minimum compatible timestamp. This reduces the number of places where changes need to be made for each network upgrade in order to haveNetwork
instances be compatible.It also results in the min compatible time of the
TestNetwork
being bumped to theFortunaTime
.How this was tested
N/A
Need to be documented in RELEASES.md?
No