diff --git a/sei-tendermint/config/config.go b/sei-tendermint/config/config.go index ca46570289..d7304bbbaf 100644 --- a/sei-tendermint/config/config.go +++ b/sei-tendermint/config/config.go @@ -150,6 +150,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) } @@ -732,6 +735,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"` @@ -762,6 +769,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", } @@ -782,6 +790,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 699ac991b4..0e7adebaa9 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) { @@ -39,6 +40,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") @@ -231,6 +242,8 @@ func TestP2PConfigValidateBasic(t *testing.T) { "MaxPacketMsgPayloadSize", "SendRate", "RecvRate", + "DialInterval", + "AcceptInterval", } for _, fieldName := range fieldsToTest { @@ -240,6 +253,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 4e46f3d425..587b593803 100644 --- a/sei-tendermint/config/toml.go +++ b/sei-tendermint/config/toml.go @@ -352,6 +352,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 1b49df715a..5e9ecf1de7 100644 --- a/sei-tendermint/config/toml_test.go +++ b/sei-tendermint/config/toml_test.go @@ -99,6 +99,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 888c0bb48d..694d8645dc 100644 --- a/sei-tendermint/internal/p2p/routeroptions.go +++ b/sei-tendermint/internal/p2p/routeroptions.go @@ -68,7 +68,11 @@ type RouterOptions struct { // MaxDialRate limits the rate at which router is dialing peers. MaxDialRate rate.Limit - // MaxAcceptRate limits the rate at which router is accepting TCP connections. + // MaxAcceptRate limits the sustained rate at which router is accepting TCP + // connections; the limiter's burst is MaxConcurrentAccepts. Required — the + // default lives on the `accept-interval` config key, not here, and Validate + // rejects a zero value rather than letting a construction site inherit a rate + // too low to drain the listen backlog. MaxAcceptRate rate.Limit // ResolveTimeout is the timeout for resolving NodeAddress URLs. diff --git a/sei-tendermint/internal/p2p/routeroptions_test.go b/sei-tendermint/internal/p2p/routeroptions_test.go new file mode 100644 index 0000000000..13603dc8cb --- /dev/null +++ b/sei-tendermint/internal/p2p/routeroptions_test.go @@ -0,0 +1,38 @@ +package p2p + +import ( + "testing" + "time" + + "golang.org/x/time/rate" + + "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils/require" +) + +// #3922 removed the package-level pacing fallbacks in favour of requiring both +// rates from the construction site, so the risk this guards has moved rather +// than gone: a site that forgets to set them no longer inherits a rate too low +// to drain the listen backlog, it fails Validate. Every router test harness +// pins these to rate.Inf and node setup derives them from config, so nothing +// else exercises the unset case. +func TestRouterOptionsRequirePacingRates(t *testing.T) { + var o RouterOptions + require.Error(t, o.Validate()) + + o.MaxDialRate = rate.Every(10 * time.Second) + require.Error(t, o.Validate()) + + o.MaxAcceptRate = rate.Every(10 * time.Millisecond) + require.NoError(t, o.Validate()) + + // The accessors are plain reads now; no fallback may reappear between the + // field and the limiter. + require.Equal(t, rate.Every(10*time.Millisecond), o.maxAcceptRate()) + require.Equal(t, rate.Every(10*time.Second), o.maxDialRate()) + + // accept-interval = 0 is the documented way to disable pacing: rate.Every + // maps a non-positive interval to rate.Inf, which Validate still accepts. + o.MaxAcceptRate = rate.Every(0) + require.Equal(t, rate.Inf, o.maxAcceptRate()) + require.NoError(t, o.Validate()) +} diff --git a/sei-tendermint/node/setup.go b/sei-tendermint/node/setup.go index 8023449dbf..04766c13ab 100644 --- a/sei-tendermint/node/setup.go +++ b/sei-tendermint/node/setup.go @@ -455,27 +455,22 @@ func buildFullnodeGigaConfig( }, nil } -func createRouter( - 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, utils.Option[atypes.BlockDB], error) { - closer := func() error { return nil } - noneDB := utils.None[atypes.BlockDB]() - gigaBlockDB := noneDB - ep, err := p2p.ResolveEndpoint(nodeKey.ID().AddressString(cfg.P2P.ListenAddress)) - if err != nil { - return nil, closer, noneDB, 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 { @@ -497,11 +492,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: rate.Every(cfg.P2P.DialInterval), - MaxAcceptRate: rate.Every(time.Second), + MaxDialRate: dialRate, + MaxAcceptRate: acceptRate, HandshakeTimeout: utils.Some(cfg.P2P.HandshakeTimeout), DialTimeout: utils.Some(cfg.P2P.DialTimeout), PexOnHandshake: cfg.P2P.PexReactor, @@ -510,6 +513,33 @@ func createRouter( MaxOutbound: utils.Some(maxOutbound), MaxConcurrentAccepts: utils.Some(maxInbound), Connection: connection, + }, nil +} + +func createRouter( + 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, utils.Option[atypes.BlockDB], error) { + closer := func() error { return nil } + noneDB := utils.None[atypes.BlockDB]() + gigaBlockDB := noneDB + ep, err := p2p.ResolveEndpoint(nodeKey.ID().AddressString(cfg.P2P.ListenAddress)) + if err != nil { + return nil, closer, noneDB, 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, noneDB, 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 8aed49ae95..364a7be1e1 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" "net/url" "os" "path/filepath" @@ -10,6 +11,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" @@ -345,3 +347,73 @@ func TestPreparePersistentStateDir_EmptyStringIsNone(t *testing.T) { _, ok := cfg.PersistentStateDir.Get() require.False(t, ok, "Some(\"\") must be cleared to None for in-memory mode") } + +// Every other RouterOptions construction site substitutes rate.Inf, so this +// derivation is the only place the production accept rate is exercised. +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, rate.Every(cfg.P2P.AcceptInterval), opts.MaxAcceptRate) + require.Equal(t, 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, 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, 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) + }) + } +}