From 36705fa9f9d891a79a8d1b3a7ce7027c54772ed9 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 17 Aug 2026 09:26:03 -0700 Subject: [PATCH] fix(p2p): make the inbound accept rate configurable and raise its default (#3899) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manual backport of #3899 (squash d17806f6c) to release/v6.6. The UCI action refused it — the source PR carries a merge commit, from when main was merged in to resolve conflicts — and cherry-picking the squash directly mis-aligned createRouter, splicing main's signature into this branch's body. So the three affected files were reconstructed from the release/v6.6 side rather than patched out of a bad merge. The resolution is the INVERSE of the one taken on main. #3922 (which unwraps MaxDialRate/MaxAcceptRate from Option[rate.Limit] to plain rate.Limit) is on main only, so this branch keeps the Option form: - RouterOptions.MaxAcceptRate stays utils.Option[rate.Limit]. - setup.go wraps: MaxDialRate/MaxAcceptRate: utils.Some(dialRate/acceptRate). - setup_test.go asserts utils.Some(...) on both rates. - routeroptions_test.go is #3899's original fallback-based test, not the Validate-based rewrite that main's version needed. One difference is load-bearing rather than cosmetic. On main, dropping #3899's raised package-level fallback was safe because #3922 added a Validate() guard rejecting a zero rate, so a construction site that forgets the field fails loudly. This branch has no such guard, so maxAcceptRate() keeps .Or(rate.Every(10 * time.Millisecond)) — without it an unset field silently inherits 1/s and the backport ships nothing. pacingRate and p2pRouterOptions are introduced by #3899 itself, not pre-existing main drift, so both come across; createRouter here keeps its own signature (p2pMetrics first, 3 return values) and just calls p2pRouterOptions. Verified: gofmt -s and go vet clean; ./sei-tendermint/config/... and ./sei-tendermint/internal/p2p/ green; TestP2PRouterOptions_PacingAndBudgetWiring and TestRouterOptionsPacingDefaults pass. ./sei-tendermint/node/ has 3 failures (TestNodeStartStop, TestNodeRestartEventAllowsRecreate, TestNodeSetPrivValTCP) confirmed identical on pristine release/v6.6. Co-Authored-By: Claude Opus 5 (1M context) --- sei-tendermint/config/config.go | 14 ++++ sei-tendermint/config/config_test.go | 28 +++++++ sei-tendermint/config/p2p_compat_test.go | 61 +++++++++++++++ sei-tendermint/config/toml.go | 6 ++ sei-tendermint/config/toml_test.go | 19 +++++ sei-tendermint/internal/p2p/routeroptions.go | 9 ++- .../internal/p2p/routeroptions_test.go | 24 ++++++ sei-tendermint/node/setup.go | 73 ++++++++++++------ sei-tendermint/node/setup_test.go | 75 +++++++++++++++++++ 9 files changed, 286 insertions(+), 23 deletions(-) create mode 100644 sei-tendermint/config/p2p_compat_test.go create mode 100644 sei-tendermint/internal/p2p/routeroptions_test.go diff --git a/sei-tendermint/config/config.go b/sei-tendermint/config/config.go index ce47df8c84..bcbc81c042 100644 --- a/sei-tendermint/config/config.go +++ b/sei-tendermint/config/config.go @@ -140,6 +140,9 @@ func (cfg *Config) ValidateBasic() error { if err := cfg.RPC.ValidateBasic(); err != nil { return fmt.Errorf("error in [rpc] section: %w", err) } + if err := cfg.P2P.ValidateBasic(); err != nil { + return fmt.Errorf("error in [p2p] section: %w", err) + } if err := cfg.Mempool.ValidateBasic(); err != nil { return fmt.Errorf("error in [mempool] section: %w", err) } @@ -711,6 +714,10 @@ type P2PConfig struct { // How often node should dial a new peer. DialInterval time.Duration `mapstructure:"dial-interval"` + // How often node should accept a new inbound connection. A value of 0 disables + // the limiter. + AcceptInterval time.Duration `mapstructure:"accept-interval"` + // Testing params. // Force dial to fail TestDialFail bool `mapstructure:"test-dial-fail"` @@ -741,6 +748,7 @@ func DefaultP2PConfig() *P2PConfig { HandshakeTimeout: 10 * time.Second, DialTimeout: 3 * time.Second, DialInterval: 10 * time.Second, + AcceptInterval: 10 * time.Millisecond, TestDialFail: false, QueueType: "simple-priority", } @@ -761,6 +769,12 @@ func (cfg *P2PConfig) ValidateBasic() error { if cfg.RecvRate < 0 { return errors.New("recv-rate can't be negative") } + if cfg.DialInterval < 0 { + return errors.New("dial-interval can't be negative") + } + if cfg.AcceptInterval < 0 { + return errors.New("accept-interval can't be negative") + } return nil } diff --git a/sei-tendermint/config/config_test.go b/sei-tendermint/config/config_test.go index 811ba5fb55..2ea029e50b 100644 --- a/sei-tendermint/config/config_test.go +++ b/sei-tendermint/config/config_test.go @@ -11,6 +11,7 @@ import ( "github.com/sei-protocol/sei-chain/sei-tendermint/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/time/rate" ) func TestDefaultConfig(t *testing.T) { @@ -38,6 +39,16 @@ func TestConfigValidateBasic(t *testing.T) { assert.Error(t, cfg.ValidateBasic()) } +// Asserts Config.ValidateBasic routes the [p2p] section, not merely that the +// section's own checks work. +func TestConfigValidateBasicRoutesP2P(t *testing.T) { + cfg := DefaultConfig() + require.NoError(t, cfg.ValidateBasic()) + + cfg.P2P.AcceptInterval = -1 + require.Error(t, cfg.ValidateBasic()) +} + func TestTLSConfiguration(t *testing.T) { cfg := DefaultConfig() cfg.SetRoot("/home/user") @@ -229,6 +240,8 @@ func TestP2PConfigValidateBasic(t *testing.T) { "MaxPacketMsgPayloadSize", "SendRate", "RecvRate", + "DialInterval", + "AcceptInterval", } for _, fieldName := range fieldsToTest { @@ -238,6 +251,21 @@ func TestP2PConfigValidateBasic(t *testing.T) { } } +// Pins the accept-interval default exactly, so changing it is deliberate and +// visible in the diff. +func TestP2PConfigAcceptInterval(t *testing.T) { + cfg := DefaultP2PConfig() + require.NoError(t, cfg.ValidateBasic()) + + require.Equal(t, 10*time.Millisecond, cfg.AcceptInterval) + + // A zero interval is the documented escape hatch for disabling the limiter + // outright, and must stay valid rather than becoming a zero rate. + cfg.AcceptInterval = 0 + require.NoError(t, cfg.ValidateBasic()) + require.Equal(t, rate.Inf, rate.Every(cfg.AcceptInterval)) +} + // --- WalFile legacy fallback tests --- func TestWalFile_NewDefault_NoLegacy(t *testing.T) { diff --git a/sei-tendermint/config/p2p_compat_test.go b/sei-tendermint/config/p2p_compat_test.go new file mode 100644 index 0000000000..cba267a877 --- /dev/null +++ b/sei-tendermint/config/p2p_compat_test.go @@ -0,0 +1,61 @@ +package config_test + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/spf13/viper" + "github.com/stretchr/testify/require" + + tmconfig "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// readP2PConfig decodes a config.toml into a default Config: absent keys keep +// the value already in the struct. +func readP2PConfig(t *testing.T, body string) *tmconfig.P2PConfig { + t.Helper() + + path := filepath.Join(t.TempDir(), "config.toml") + require.NoError(t, os.WriteFile(path, []byte(body), 0600)) + + v := viper.New() + v.SetConfigFile(path) + require.NoError(t, v.ReadInConfig()) + + cfg := tmconfig.DefaultConfig() + require.NoError(t, v.Unmarshal(cfg)) + return cfg.P2P +} + +// dial-interval is absent from the generated template (see checkConfig in +// toml_test.go), so nothing else shows it is readable at all. +func TestP2PPacingKnobsParseFromExistingConfig(t *testing.T) { + p2p := readP2PConfig(t, ` +[p2p] +laddr = "tcp://0.0.0.0:26656" +dial-interval = "5s" +accept-interval = "20ms" +`) + + require.Equal(t, 5*time.Second, p2p.DialInterval) + require.Equal(t, 20*time.Millisecond, p2p.AcceptInterval) + require.NoError(t, p2p.ValidateBasic()) +} + +// TestP2PConfigPredatingPacingKnobsKeepsDefaults asserts a config.toml written +// before these keys existed still parses to the defaults rather than to zero, +// which rate.Every would read as "no pacing". +func TestP2PConfigPredatingPacingKnobsKeepsDefaults(t *testing.T) { + p2p := readP2PConfig(t, ` +[p2p] +laddr = "tcp://0.0.0.0:26656" +`) + + defaults := tmconfig.DefaultP2PConfig() + require.Equal(t, defaults.AcceptInterval, p2p.AcceptInterval) + require.Equal(t, defaults.DialInterval, p2p.DialInterval) + require.NotZero(t, p2p.AcceptInterval, "a zero accept-interval disables accept pacing entirely") + require.NoError(t, p2p.ValidateBasic()) +} diff --git a/sei-tendermint/config/toml.go b/sei-tendermint/config/toml.go index 4a21af33e4..3e5945e233 100644 --- a/sei-tendermint/config/toml.go +++ b/sei-tendermint/config/toml.go @@ -309,6 +309,12 @@ allow-duplicate-ip = {{ .P2P.AllowDuplicateIP }} handshake-timeout = "{{ .P2P.HandshakeTimeout }}" dial-timeout = "{{ .P2P.DialTimeout }}" +# How often the node accepts a new inbound connection. A larger interval paces +# the accept loop more slowly; if the kernel accept backlog outpaces it, arriving +# peers wait past handshake-timeout and the node silently stops acquiring inbound +# peers. A value of 0 disables the limiter. +accept-interval = "{{ .P2P.AcceptInterval }}" + # Time to wait before flushing messages out on the connection # TODO: Remove once MConnConnection is removed. flush-throttle-timeout = "{{ .P2P.FlushThrottleTimeout }}" diff --git a/sei-tendermint/config/toml_test.go b/sei-tendermint/config/toml_test.go index de4f059440..5eecfa737f 100644 --- a/sei-tendermint/config/toml_test.go +++ b/sei-tendermint/config/toml_test.go @@ -97,6 +97,25 @@ func checkConfig(t *testing.T, configFile string) { t.Errorf("config file was not expected to contain %s", e) } } + + // accept-interval is rendered deliberately: an accept rate too low to drain + // the kernel backlog silently stops a node acquiring inbound peers, and the + // template is where an operator looks. Keep it discoverable. + if !configContainsKey(configFile, "accept-interval") { + t.Errorf("config file was expected to contain accept-interval but did not") + } + + // dial-interval remains an expert-only knob, left out of the generated + // template while still being parsed from existing config files. + // See TestP2PPacingKnobsParseFromExistingConfig. + var hiddenP2PElems = []string{ + "dial-interval", + } + for _, e := range hiddenP2PElems { + if configContainsKey(configFile, e) { + t.Errorf("config file was not expected to contain %s", e) + } + } } func configContainsKey(configFile string, key string) bool { diff --git a/sei-tendermint/internal/p2p/routeroptions.go b/sei-tendermint/internal/p2p/routeroptions.go index 6f2bb04c86..2eb9c5de4d 100644 --- a/sei-tendermint/internal/p2p/routeroptions.go +++ b/sei-tendermint/internal/p2p/routeroptions.go @@ -64,7 +64,8 @@ type RouterOptions struct { // MaxDialRate limits the rate at which router is dialing peers. Defaults to 0.1/s. MaxDialRate utils.Option[rate.Limit] - // MaxAcceptRate limits the rate at which router is accepting TCP connections. Defaults to 1/s. + // MaxAcceptRate limits the sustained rate at which router is accepting TCP + // connections; the limiter's burst is MaxConcurrentAccepts. Defaults to 100/s. MaxAcceptRate utils.Option[rate.Limit] // ResolveTimeout is the timeout for resolving NodeAddress URLs. @@ -161,7 +162,11 @@ func (o *RouterOptions) maxDialRate() rate.Limit { } func (o *RouterOptions) maxAcceptRate() rate.Limit { - return o.MaxAcceptRate.Or(rate.Every(time.Second)) + // The fallback is load-bearing on this branch: unlike main, RouterOptions.Validate + // here has no "rate must be > 0" guard, so a construction site that leaves the + // field unset silently inherits this value rather than failing. 1/s was too low + // to drain the listen backlog, which is the defect #3899 fixes. + return o.MaxAcceptRate.Or(rate.Every(10 * time.Millisecond)) } func (o *RouterOptions) incomingConnectionWindow() time.Duration { diff --git a/sei-tendermint/internal/p2p/routeroptions_test.go b/sei-tendermint/internal/p2p/routeroptions_test.go new file mode 100644 index 0000000000..158467a3ea --- /dev/null +++ b/sei-tendermint/internal/p2p/routeroptions_test.go @@ -0,0 +1,24 @@ +package p2p + +import ( + "testing" + "time" + + "golang.org/x/time/rate" + + "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils" + "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils/require" +) + +// Every test harness pins MaxAcceptRate/MaxDialRate to rate.Inf and node setup +// always sets them, so these fallbacks are otherwise unexercised. +func TestRouterOptionsPacingDefaults(t *testing.T) { + var o RouterOptions + + require.Equal(t, rate.Every(10*time.Millisecond), o.maxAcceptRate()) + require.Equal(t, rate.Every(10*time.Second), o.maxDialRate()) + + // An explicit value wins over the fallback. + o.MaxAcceptRate = utils.Some(rate.Inf) + require.Equal(t, rate.Inf, o.maxAcceptRate()) +} diff --git a/sei-tendermint/node/setup.go b/sei-tendermint/node/setup.go index 2479a698c0..44806bd0aa 100644 --- a/sei-tendermint/node/setup.go +++ b/sei-tendermint/node/setup.go @@ -263,26 +263,22 @@ func buildGigaConfig( }, nil } -func createRouter( - p2pMetrics *p2p.Metrics, - nodeInfoProducer func() *types.NodeInfo, - nodeKey types.NodeKey, - validatorKey utils.Option[atypes.SecretKey], - cfg *config.Config, - app utils.Option[*proxy.Proxy], - genDoc *types.GenesisDoc, - dbProvider config.DBProvider, -) (*p2p.Router, closer, error) { - closer := func() error { return nil } - ep, err := p2p.ResolveEndpoint(nodeKey.ID().AddressString(cfg.P2P.ListenAddress)) - if err != nil { - return nil, closer, err - } - var privatePeerIDs []types.NodeID - for _, id := range tmstrings.SplitAndTrimEmpty(cfg.P2P.PrivatePeerIDs, ",", " ") { - privatePeerIDs = append(privatePeerIDs, types.NodeID(id)) - } +// pacingRate returns the rate limit for a configured pacing interval. +func pacingRate(key string, interval time.Duration) (rate.Limit, error) { + // rate.Every maps every non-positive interval to rate.Inf. A configured 0 means + // "disable the limiter" and is honoured; a negative value is a typo that would + // silently disable pacing instead. ValidateBasic rejects it wherever it runs, + // but an already-deployed config never reaches ValidateBasic, so refuse it here + // too rather than letting the two paths disagree about the same input. + if interval < 0 { + return 0, fmt.Errorf("p2p %v must not be negative, got %v", key, interval) + } + return rate.Every(interval), nil +} +// p2pRouterOptions returns the router's connection budget and pacing, derived +// from the p2p config. +func p2pRouterOptions(cfg *config.Config, ep p2p.Endpoint, privatePeerIDs []types.NodeID) (*p2p.RouterOptions, error) { // MaxConnections defaults to 64 maxConns := 64 if cfg.P2P.MaxConnections > 0 { @@ -304,10 +300,19 @@ func createRouter( connection.SendRate = cfg.P2P.SendRate connection.RecvRate = cfg.P2P.RecvRate connection.MaxPacketMsgPayloadSize = cfg.P2P.MaxPacketMsgPayloadSize - options := &p2p.RouterOptions{ + dialRate, err := pacingRate("dial-interval", cfg.P2P.DialInterval) + if err != nil { + return nil, err + } + acceptRate, err := pacingRate("accept-interval", cfg.P2P.AcceptInterval) + if err != nil { + return nil, err + } + return &p2p.RouterOptions{ Endpoint: ep, MaxIncomingConnectionAttempts: utils.Some(cfg.P2P.MaxIncomingConnectionAttempts), - MaxDialRate: utils.Some(rate.Every(cfg.P2P.DialInterval)), + MaxDialRate: utils.Some(dialRate), + MaxAcceptRate: utils.Some(acceptRate), HandshakeTimeout: utils.Some(cfg.P2P.HandshakeTimeout), DialTimeout: utils.Some(cfg.P2P.DialTimeout), PexOnHandshake: cfg.P2P.PexReactor, @@ -316,6 +321,32 @@ func createRouter( MaxOutbound: utils.Some(maxOutbound), MaxConcurrentAccepts: utils.Some(maxInbound), Connection: connection, + }, nil +} + +func createRouter( + p2pMetrics *p2p.Metrics, + nodeInfoProducer func() *types.NodeInfo, + nodeKey types.NodeKey, + validatorKey utils.Option[atypes.SecretKey], + cfg *config.Config, + app utils.Option[*proxy.Proxy], + genDoc *types.GenesisDoc, + dbProvider config.DBProvider, +) (*p2p.Router, closer, error) { + closer := func() error { return nil } + ep, err := p2p.ResolveEndpoint(nodeKey.ID().AddressString(cfg.P2P.ListenAddress)) + if err != nil { + return nil, closer, err + } + var privatePeerIDs []types.NodeID + for _, id := range tmstrings.SplitAndTrimEmpty(cfg.P2P.PrivatePeerIDs, ",", " ") { + privatePeerIDs = append(privatePeerIDs, types.NodeID(id)) + } + + options, err := p2pRouterOptions(cfg, ep, privatePeerIDs) + if err != nil { + return nil, closer, err } if addr := cfg.P2P.ExternalAddress; addr != "" { nodeAddr, err := p2p.ParseNodeAddress(nodeKey.ID().AddressString(addr)) diff --git a/sei-tendermint/node/setup_test.go b/sei-tendermint/node/setup_test.go index 97b02ca9ce..4a39092fa9 100644 --- a/sei-tendermint/node/setup_test.go +++ b/sei-tendermint/node/setup_test.go @@ -2,6 +2,7 @@ package node import ( "encoding/json" + "fmt" "os" "path/filepath" "testing" @@ -9,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/time/rate" "github.com/sei-protocol/sei-chain/sei-tendermint/abci/example/kvstore" atypes "github.com/sei-protocol/sei-chain/sei-tendermint/autobahn/types" @@ -281,3 +283,76 @@ func TestBuildGigaConfig_NodeKeyMismatch(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "node key mismatch") } + +// Every other RouterOptions construction site substitutes rate.Inf, so this +// derivation is the only place the production accept rate is exercised. +// +// The rate assertions wrap in utils.Some because MaxDialRate/MaxAcceptRate are +// still Option-typed on this branch — #3922, which unwraps them, is on main only. +func TestP2PRouterOptions_PacingAndBudgetWiring(t *testing.T) { + ep, err := p2p.ResolveEndpoint("tcp://0000000000000000000000000000000000000000@127.0.0.1:26656") + require.NoError(t, err) + + t.Run("defaults reach the router", func(t *testing.T) { + cfg := config.DefaultConfig() + opts, err := p2pRouterOptions(cfg, ep, nil) + require.NoError(t, err) + + require.Equal(t, utils.Some(rate.Every(cfg.P2P.AcceptInterval)), opts.MaxAcceptRate) + require.Equal(t, utils.Some(rate.Every(cfg.P2P.DialInterval)), opts.MaxDialRate) + }) + + // A negative value never reaches ValidateBasic on an already-deployed node, and + // rate.Every would read it as "disable". Refuse it rather than start unpaced. + for _, key := range []string{"accept-interval", "dial-interval"} { + t.Run("negative "+key+" refuses to build options", func(t *testing.T) { + cfg := config.DefaultConfig() + switch key { + case "accept-interval": + cfg.P2P.AcceptInterval = -1 * time.Second + case "dial-interval": + cfg.P2P.DialInterval = -1 * time.Second + } + _, err := p2pRouterOptions(cfg, ep, nil) + require.Error(t, err) + }) + } + + t.Run("zero interval disables the limiter", func(t *testing.T) { + cfg := config.DefaultConfig() + cfg.P2P.AcceptInterval = 0 + opts, err := p2pRouterOptions(cfg, ep, nil) + require.NoError(t, err) + require.Equal(t, utils.Some(rate.Inf), opts.MaxAcceptRate) + }) + + t.Run("operator value flows through", func(t *testing.T) { + cfg := config.DefaultConfig() + cfg.P2P.AcceptInterval = 250 * time.Millisecond + opts, err := p2pRouterOptions(cfg, ep, nil) + require.NoError(t, err) + require.Equal(t, utils.Some(rate.Every(250*time.Millisecond)), opts.MaxAcceptRate) + }) + + // Non-default totals, so the assertions track the derivation rather than + // restating DefaultP2PConfig. 50 exercises the flat 20-outbound reservation; + // 30 exercises the min(20, (maxConns+1)/2) branch, which nothing else reaches. + for _, tc := range []struct { + maxConns, wantInbound, wantOutbound int + }{ + {maxConns: 50, wantInbound: 30, wantOutbound: 20}, + {maxConns: 30, wantInbound: 15, wantOutbound: 15}, + } { + t.Run(fmt.Sprintf("budget derives from max-connections=%d", tc.maxConns), func(t *testing.T) { + cfg := config.DefaultConfig() + cfg.P2P.MaxConnections = uint(tc.maxConns) + opts, err := p2pRouterOptions(cfg, ep, nil) + require.NoError(t, err) + + require.Equal(t, utils.Some(tc.wantInbound), opts.MaxInbound) + require.Equal(t, utils.Some(tc.wantOutbound), opts.MaxOutbound) + // MaxConcurrentAccepts tracks the inbound pool, not max-connections. + require.Equal(t, utils.Some(tc.wantInbound), opts.MaxConcurrentAccepts) + }) + } +}