Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Align minCompatibleTime settings across TestNetwork and Network #3842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b475d24
test network takes min compat time arg
cam-schultz Mar 26, 2025
dfe4b83
Set minCompatibleTime for TestNetwork
michaelkaplan13 Mar 26, 2025
5a9e93b
Remove minCompatibleTime parameter
michaelkaplan13 Mar 26, 2025
65a355d
Merge branch 'master' into test-network-default-min-compatible
michaelkaplan13 Apr 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions network/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/ava-labs/avalanchego/snow/networking/router"
"github.com/ava-labs/avalanchego/snow/networking/sender"
"github.com/ava-labs/avalanchego/subnets"
"github.com/ava-labs/avalanchego/upgrade"
"github.com/ava-labs/avalanchego/utils/bloom"
"github.com/ava-labs/avalanchego/utils/constants"
"github.com/ava-labs/avalanchego/utils/ips"
Expand Down Expand Up @@ -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,
Copy link
Contributor

@joshua-kim joshua-kim Mar 27, 2025 •
edited
Loading

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?

Copy link
Contributor Author

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.

msgCreator message.Creator,
metricsRegisterer prometheus.Registerer,
log logging.Logger,
Expand Down Expand Up @@ -268,7 +269,7 @@ func NewNetwork(
InboundMsgThrottler: inboundMsgThrottler,
Network: nil, // This is set below.
Router: router,
VersionCompatibility: version.GetCompatibility(minCompatibleTime),
VersionCompatibility: version.GetCompatibility(upgradeConfig.FortunaTime), // Must be updated for each upgrade
MyNodeID: config.MyNodeID,
MySubnets: config.TrackedSubnets,
Beacons: config.Beacons,
Expand Down
10 changes: 5 additions & 5 deletions network/network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ func newFullyConnectedTestNetwork(t *testing.T, handlers []router.InboundHandler
var connected set.Set[ids.NodeID]
net, err := NewNetwork(
config,
upgrade.InitiallyActiveTime,
upgrade.Default,
msgCreator,
registry,
logging.NoLog{},
Expand Down Expand Up @@ -517,7 +517,7 @@ func TestTrackDoesNotDialPrivateIPs(t *testing.T) {

net, err := NewNetwork(
config,
upgrade.InitiallyActiveTime,
upgrade.Default,
msgCreator,
registry,
logging.NoLog{},
Expand Down Expand Up @@ -595,7 +595,7 @@ func TestDialDeletesNonValidators(t *testing.T) {

net, err := NewNetwork(
config,
upgrade.InitiallyActiveTime,
upgrade.Default,
msgCreator,
registry,
logging.NoLog{},
Expand Down Expand Up @@ -748,7 +748,7 @@ func TestAllowConnectionAsAValidator(t *testing.T) {

net, err := NewNetwork(
config,
upgrade.InitiallyActiveTime,
upgrade.Default,
msgCreator,
registry,
logging.NoLog{},
Expand Down Expand Up @@ -809,7 +809,7 @@ func TestGetAllPeers(t *testing.T) {
configs[0].Validators = validators.NewManager()
nonValidatorNetwork, err := NewNetwork(
configs[0],
upgrade.InitiallyActiveTime,
upgrade.Default,
newMessageCreator(t),
prometheus.NewRegistry(),
logging.NoLog{},
Expand Down
2 changes: 1 addition & 1 deletion network/test_network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func NewTestNetwork(

return NewNetwork(
cfg,
upgrade.InitiallyActiveTime,
upgrade.GetConfig(cfg.NetworkID),
msgCreator,
metrics,
log,
Expand Down
2 changes: 1 addition & 1 deletion node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ func (n *Node) initNetworking(reg prometheus.Registerer) error {

n.Net, err = network.NewNetwork(
&n.Config.NetworkConfig,
n.Config.UpgradeConfig.FortunaTime,
n.Config.UpgradeConfig,
n.msgCreator,
reg,
n.Log,
Expand Down