Add network config to connect to all validators #4164
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new network configuration option connectToAllValidators to allow connecting to all validators across all networks, bypassing the current limitation of 16 tracked subnets for non-primary network validators. This capability is designed to support ICM message signing functionality.
- Introduces
ConnectToAllValidatorsboolean config field across network, peer, and IP tracker components - Modifies connection logic to allow connections to all validators when the flag is enabled
- Updates peer list requests and IP tracking behavior to include all subnet IPs when the flag is active
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
network/config.go |
Adds the ConnectToAllValidators configuration field |
network/peer/config.go |
Propagates the config field to peer configuration |
network/peer/peer.go |
Updates peer list request logic to include all subnet IPs when flag is enabled |
network/network.go |
Modifies connection allowance and tracking logic to respect the new flag |
network/ip_tracker.go |
Updates IP tracking and connection desire logic for all validators mode |
network/ip_tracker_test.go |
Updates test initialization to include the new parameter |
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Ian Suvak <[email protected]>
| expectedChange func(*ipTracker) | ||
| skipConfigs []*testConfig |
There was a problem hiding this comment.
Is this used? Seems to always be empty so we never skip any tests
| if shouldSkipTest(test.skipConfigs, config) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
I left a few comments previously on getting rid of the skip configs, but if we kept them would we want to call t.Skip? This will silently pass the test otherwise since we've already entered t.Run.
| @@ -198,11 +258,12 @@ func TestIPTracker_ManuallyGossip(t *testing.T) { | |||
| gossipableIndices: make(map[ids.NodeID]int), | |||
| } | |||
| }, | |||
| skipConfigs: []*testConfig{&connectToAllValidatorsConfig}, | |||
There was a problem hiding this comment.
Why do we support test skipping in this PR? It seems like some of these are valid configurations (connect to all validators specified, but a non-connected untracked non-validator tries to connect to us, I think we'd expect a no-op).
| // testConfig represents different configurations for testing the IP tracker | ||
| type testConfig struct { | ||
| name string | ||
| newTracker func(t *testing.T) *ipTracker | ||
| } | ||
|
|
||
| // shouldSkipTest checks if a test case should be skipped for the given config | ||
| func shouldSkipTest(skipConfigs []*testConfig, config *testConfig) bool { | ||
| for _, skipConfig := range skipConfigs { | ||
| if skipConfig == config { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| var normalConfig = testConfig{ | ||
| name: "connect_to_tracked_only", | ||
| newTracker: newTestIPTracker, | ||
| } | ||
|
|
||
| var connectToAllValidatorsConfig = testConfig{ | ||
| name: "connect_to_all_validators", | ||
| newTracker: newTestIPTrackerConnectToAll, | ||
| } | ||
|
|
||
| // testConfigs defines all the test configurations we want to run | ||
| var testConfigs = []*testConfig{ | ||
| &normalConfig, | ||
| &connectToAllValidatorsConfig, | ||
| } |
There was a problem hiding this comment.
I think these test helpers are being added to try to not copy-paste too much code, but I think that copy-pasting the existing test-cases and changing one param (enabling connect to all validators) would actually be simpler to read than some of the boilerplate which is adding more complexity (setup functions in test tables, config passing, more test-helpers, etc).
Since we're trying to re-use the existing test coverage across two configs, it's causing some friction where we have to introduce this "skip config" abstraction since the connect-to-all and normal config when behavior expected differs.
Why this should be merged
Connecting to validators of all networks beyond the current max of 16 tracked subnets without being a primary network validator is useful for ICM message signing.
How this works
Adds a new
connectToAllValidatorsnetwork config option but doesn't expose it on the avalanchego config level.Propagates it down to
peerandiptrackerlevelsHow this was tested
Tested using E2E on a branch of
icm-servicesrepo.Added new unit tests primarily on the
ip_trackerside.Need to be documented in RELEASES.md?
No