From 58ee9ef65a8bf62cf0dcc7420aaf910cc2cfec8e Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 21 Jul 2026 16:43:17 -0700 Subject: [PATCH] server+connmgr: make outbound startup deterministic In this commit, we define TargetOutbound as the number of automatic connections and treat explicit Connect requests as additional peers. Permanent requests could previously consume connection IDs before Start sampled the shared counter. The resulting automatic count depended on goroutine scheduling, while the listener reserved the worst-case total. We now start the configured automatic count directly and cap it by the peer budget left after permanent peers. The composition test covers permanent requests on both sides of Start and pins the same automatic+permanent total for either ordering. --- connmgr/connmanager.go | 7 ++- connmgr/connmanager_test.go | 108 ++++++++++++++++++++++++++++++++++++ server.go | 31 +++++++++-- server_test.go | 33 ++++++----- 4 files changed, 157 insertions(+), 22 deletions(-) diff --git a/connmgr/connmanager.go b/connmgr/connmanager.go index 58259ee6..8e64ae2f 100644 --- a/connmgr/connmanager.go +++ b/connmgr/connmanager.go @@ -127,8 +127,9 @@ type Config struct { // behavior, while a pointer to zero disables inbound connections. MaxInbound *uint32 - // TargetOutbound is the number of outbound network connections to - // maintain. Defaults to 8. + // TargetOutbound is the number of automatic outbound network connections + // to maintain. Connections made through Connect are additional. Defaults + // to 8. TargetOutbound uint32 // RetryDuration is the duration to wait before retrying connection @@ -616,7 +617,7 @@ func (cm *ConnManager) Start() { } } - for i := atomic.LoadUint64(&cm.connReqCount); i < uint64(cm.cfg.TargetOutbound); i++ { + for i := uint32(0); i < cm.cfg.TargetOutbound; i++ { go cm.NewConnReq() } } diff --git a/connmgr/connmanager_test.go b/connmgr/connmanager_test.go index 49c5f843..2316ccf4 100644 --- a/connmgr/connmanager_test.go +++ b/connmgr/connmanager_test.go @@ -371,6 +371,114 @@ func TestTargetOutbound(t *testing.T) { cmgr.Stop() } +// TestTargetOutboundComposition verifies that explicit permanent connections +// are additional to the automatic outbound target regardless of whether they +// receive connection request IDs before or after the manager starts. +func TestTargetOutboundComposition(t *testing.T) { + const ( + targetOutbound = uint32(3) + permanentPeers = uint64(2) + ) + + tests := []struct { + name string + permanentBeforeStart bool + }{ + {name: "permanent before start", permanentBeforeStart: true}, + {name: "permanent after start"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + connected := make(chan *ConnReq, + int(targetOutbound)+int(permanentPeers)) + cmgr, err := New(&Config{ + TargetOutbound: targetOutbound, + Dial: mockDialer, + GetNewAddress: func() (net.Addr, error) { + return &net.TCPAddr{ + IP: net.ParseIP("127.0.0.1"), + Port: 18555, + }, nil + }, + OnConnection: func(c *ConnReq, _ net.Conn) { + connected <- c + }, + }) + if err != nil { + t.Fatalf("New error: %v", err) + } + + connectPermanent := func() { + for i := uint64(0); i < permanentPeers; i++ { + go cmgr.Connect(&ConnReq{ + Addr: &net.TCPAddr{ + IP: net.ParseIP("127.0.0.1"), + Port: 18556 + int(i), + }, + Permanent: true, + }) + } + } + + if test.permanentBeforeStart { + connectPermanent() + + deadline := time.After(time.Second) + for atomic.LoadUint64(&cmgr.connReqCount) < + permanentPeers { + + select { + case <-deadline: + t.Fatal("permanent requests did not receive IDs") + case <-time.After(time.Millisecond): + } + } + } + + cmgr.Start() + if !test.permanentBeforeStart { + connectPermanent() + } + + var ( + automaticCount int + permanentCount int + ) + for i := 0; i < int(targetOutbound)+int(permanentPeers); i++ { + select { + case connReq := <-connected: + if connReq.Permanent { + permanentCount++ + } else { + automaticCount++ + } + + case <-time.After(time.Second): + t.Fatal("timed out waiting for outbound connections") + } + } + + if automaticCount != int(targetOutbound) { + t.Fatalf("unexpected automatic count: got %d, want %d", + automaticCount, targetOutbound) + } + if permanentCount != int(permanentPeers) { + t.Fatalf("unexpected permanent count: got %d, want %d", + permanentCount, permanentPeers) + } + + select { + case connReq := <-connected: + t.Fatalf("unexpected extra connection: %v", connReq) + case <-time.After(10 * time.Millisecond): + } + + cmgr.Stop() + }) + } +} + // TestRetryPermanent tests that permanent connection requests are retried. // // We make a permanent connection request using Connect, disconnect it using diff --git a/server.go b/server.go index 678e3f82..f41f0e52 100644 --- a/server.go +++ b/server.go @@ -74,6 +74,24 @@ var ( // zeroHash is the zero value hash (all zeros). It is defined as a convenience. var zeroHash chainhash.Hash +// targetOutboundPeers returns the automatic outbound target for the configured +// peer mode after permanent peers reserve their portion of the total budget. +func targetOutboundPeers( + maxPeers, permanentPeers int, automaticOutbound bool, +) int { + + if !automaticOutbound || permanentPeers >= maxPeers { + return 0 + } + + available := maxPeers - permanentPeers + if available < defaultTargetOutbound { + return available + } + + return defaultTargetOutbound +} + // reservedOutboundPeers returns the outbound connection reservation for the // configured peer mode, capped at the total peer limit. func reservedOutboundPeers( @@ -3225,17 +3243,20 @@ func newServer(listenAddrs, agentBlacklist, agentWhitelist []string, } // Create a connection manager. - targetOutbound := defaultTargetOutbound - if cfg.MaxPeers < targetOutbound { - targetOutbound = cfg.MaxPeers - } permanentPeerCount := len(cfg.ConnectPeers) if permanentPeerCount == 0 { permanentPeerCount = len(cfg.AddPeers) } + automaticOutbound := newAddressFunc != nil + targetOutbound := targetOutboundPeers( + cfg.MaxPeers, permanentPeerCount, automaticOutbound, + ) + if targetOutbound == 0 { + newAddressFunc = nil + } reservedOutbound := reservedOutboundPeers( cfg.MaxPeers, targetOutbound, permanentPeerCount, - newAddressFunc != nil, + automaticOutbound, ) maxInbound := maxInboundPeers(cfg.MaxPeers, reservedOutbound) if maxInbound == 0 && len(listeners) > 0 { diff --git a/server_test.go b/server_test.go index 5639b595..3e7fb2c5 100644 --- a/server_test.go +++ b/server_test.go @@ -117,61 +117,66 @@ func TestInboundPeerReservation(t *testing.T) { tests := []struct { name string maxPeers int - targetOutbound int permanentPeers int automatic bool + wantTarget int wantReserved int wantInbound uint32 }{ { - name: "connect only", maxPeers: 8, targetOutbound: 8, + name: "connect only", maxPeers: 8, permanentPeers: 1, wantReserved: 1, wantInbound: 7, }, { name: "connect only capped", maxPeers: 8, - targetOutbound: 8, permanentPeers: 10, - wantReserved: 8, wantInbound: 0, + permanentPeers: 10, + wantReserved: 8, wantInbound: 0, }, { - name: "simnet without peers", maxPeers: 8, - targetOutbound: 8, wantReserved: 0, wantInbound: 8, + name: "simnet without peers", maxPeers: 8, wantReserved: 0, + wantInbound: 8, }, { - name: "simnet with peers", maxPeers: 8, - targetOutbound: 8, permanentPeers: 3, + name: "simnet with peers", maxPeers: 8, permanentPeers: 3, wantReserved: 3, wantInbound: 5, }, { name: "automatic without add peers", maxPeers: 125, - targetOutbound: 8, automatic: true, + automatic: true, wantTarget: 8, wantReserved: 8, wantInbound: 117, }, { name: "add peers below target", maxPeers: 125, - targetOutbound: 8, permanentPeers: 3, automatic: true, + permanentPeers: 3, automatic: true, wantTarget: 8, wantReserved: 11, wantInbound: 114, }, { name: "add peers above target", maxPeers: 10, - targetOutbound: 8, permanentPeers: 9, automatic: true, + permanentPeers: 9, automatic: true, wantTarget: 1, wantReserved: 10, wantInbound: 0, }, { name: "add peers at max peers", maxPeers: 10, - targetOutbound: 8, permanentPeers: 10, automatic: true, + permanentPeers: 10, automatic: true, wantReserved: 10, wantInbound: 0, }, { name: "add peers above max peers", maxPeers: 10, - targetOutbound: 8, permanentPeers: 12, automatic: true, + permanentPeers: 12, automatic: true, wantReserved: 10, wantInbound: 0, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { + targetOutbound := targetOutboundPeers( + test.maxPeers, test.permanentPeers, + test.automatic, + ) + require.Equal(t, test.wantTarget, targetOutbound) + reserved := reservedOutboundPeers( - test.maxPeers, test.targetOutbound, + test.maxPeers, targetOutbound, test.permanentPeers, test.automatic, ) require.Equal(t, test.wantReserved, reserved)