fix(p2p): make the inbound accept rate configurable and raise its default

[bdchatham]

Problem

Router paces its accept loop with a rate limiter, but the limit has never been reachable from config. createRouter sets MaxDialRate from the p2p dial-interval key and leaves MaxAcceptRate unset, so every node falls through to the compiled-in default:

func (o *RouterOptions) maxAcceptRate() rate.Limit {
	return o.MaxAcceptRate.Or(rate.Every(time.Second))   // 1 accept/s
}

node/setup.go is the only non-test construction site for RouterOptions; every other site is a test harness that pins MaxAcceptRate: rate.Inf. So the production value is both unreachable and unexercised.

One accept per second is too low for a public-facing node. The kernel keeps completing three-way handshakes into the listen backlog while the loop drains it at 1/s. Once the backlog is more than a few entries deep, an arriving peer waits longer than handshake-timeout and never completes its handshake. The node stops acquiring inbound peers while continuing to serve its established ones — so it stays at chain head and reports healthy while inbound is effectively closed. From the dialer's side this looks like a peer that accepts TCP and then does nothing.

Why it is 1/s

Not a decision about inbound behaviour:

• PeerManager rewrite #2539 (PeerManager rewrite) introduced the limiter at 2/s, rate.Every(500ms).
• made the peer dialing less aggressive #2799 — subject "made the peer dialing less aggressive", body describing DialRate — halved MaxAcceptRate to 1/s as a secondary line item, exposed the dial knob to config.toml, and left the accept knob compiled in.

Change

• Add accept-interval to P2PConfig (struct field + default + ValidateBasic) and render it in the generated template. dial-interval stays unrendered, but this key is the reason the PR exists, so it belongs where an operator looks.
• Wire it: MaxAcceptRate: utils.Some(rate.Every(cfg.P2P.AcceptInterval)).
• Raise the package default in maxAcceptRate() from 1/s to 10ms (100/s), so no construction path inherits a rate too low to drain the backlog, and default accept-interval to the same value. Concurrency is already bounded by MaxConcurrentAccepts, which setup sets to maxInbound (max-connections minus the outbound reservation) so the global limiter should be a backstop, not the binding constraint. Note max-incoming-connection-attempts does not fill that role: connTracker.AddConn caps concurrent connections per source IP at 100 (the 100ms window applies only once a source drops to zero), and 100 exceeds the default inbound pool of 80, so it does not bound a single source below the pool.
• rate.Every returns rate.Inf for a non-positive interval, so accept-interval = 0 disables the limiter.

Behavior change: [p2p] validation now runs

Config.ValidateBasic routed every section except [p2p], so P2PConfig.ValidateBasic had never executed in production — its send-rate, recv-rate, flush-throttle-timeout and max-packet-msg-payload-size checks were dead for their whole life. This PR routes the section, which makes them load-bearing.

Who is affected. Traced both entry points:

• seid's normal boot (sei-cosmos/server/util.go interceptConfigs) calls ValidateBasic only on the config-creation branch, so an already-deployed node with an existing config.toml is unaffected.
• commands.ParseConfig does call it, and sei-cosmos/server/util.go:342 invokes that from AddCommands with panic(err) on failure.
• confix's CheckValid (sei-tendermint/scripts/confix/confix.go:163) also reaches it. It unmarshals into a zero-valued Config and all [p2p] checks are < 0, so it only trips on a file carrying an explicit negative.

Because an already-deployed node never reaches ValidateBasic, p2pRouterOptions additionally clamps a negative pacing interval to the default — rate.Every maps any non-positive interval to rate.Inf, so a typo would otherwise disable pacing silently. A configured 0 still disables the limiter, as documented.

So the realistic worst case is a seid command tree that panics at startup rather than returning a config error, for an operator whose config.toml carries a negative value in one of those four pre-existing fields — previously tolerated silently. Low likelihood, but it belongs in release notes, not buried in a pacing PR.

Tests

• TestP2PConfigAcceptInterval — pins the default exactly; the rationale lives in the doc comment.
• TestP2PConfigValidateBasic — AcceptInterval added to fieldsToTest; dial-interval now gets the same negative check, since it also becomes rate.Inf.
• TestEnsureRoot/checkConfig — asserts accept-interval is present in the rendered template, and dial-interval still absent.
• p2p_compat_test.go — both keys parse from an existing config.toml; a template-shaped file without them still yields the defaults.
• TestP2PRouterOptions_PacingAndBudgetWiring — asserts the cfg → RouterOptions derivation directly, over non-default max-connections values (50 → 30/20, 30 → 15/15).

Each guard was checked to fail on the defect it targets, not merely to pass. ./node/ has three failures (TestNodeStartStop, TestNodeRestartEventAllowsRecreate, TestNodeSetPrivValTCP) that reproduce on unmodified origin/main.

gofmt -s -l and goimports -l are clean on all four files.

./node/ has three failures — TestNodeStartStop, TestNodeRestartEventAllowsRecreate, TestNodeSetPrivValTCP — which reproduce identically on unmodified origin/main. Pre-existing, not from this change.

Notes for review

• Default value is the judgement call. 10ms/100 per second is argued above from the surrounding limiters rather than measured; if you want it more conservative, the knob now exists and only the constant moves.
• AGENTS.md deviation: the guide prefers libs/utils/require over testify, but sei-tendermint/config is entirely testify with no utils/require usage. I matched the package. Happy to convert if you would rather the new test lead the migration.
• Follow-up not in this PR: the router test harnesses (testonly.go, router_test.go, giga_router_validator_test.go) all substitute rate.Inf, so no test exercises the production accept rate end-to-end. That substitution is why a 1/s cap on a mainnet listener went unnoticed. Worth a dedicated test at the router level.
• max-connections reaches the router only through the maxInbound/maxOutbound derivation in p2pRouterOptions; it is not itself a RouterOptions field. MaxConcurrentAccepts is set to maxInbound, so the connection budget does track max-connections — just via that computation rather than directly.

🤖 Generated with Claude Code

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Aug 13, 2026, 3:02 PM

[cursor]

PR Summary

Medium Risk
Changes default inbound accept behavior for all nodes and enables [p2p] validation on startup paths that previously skipped it, which can surface latent invalid config values.

Overview
Adds accept-interval under [p2p] so operators can pace how fast the router accepts inbound TCP connections. createRouter now sets MaxAcceptRate from that knob ( dial-interval was already wired for outbound dials). The compiled-in fallback when unset jumps from ~1/s to 100/s (10ms default), addressing inbound peers timing out while the accept loop drains the backlog too slowly.

accept-interval = 0 disables the limiter via rate.Inf. Negative accept-interval / dial-interval are rejected in P2PConfig.ValidateBasic and again in pacingRate at router build time so typos do not silently disable pacing on nodes that never run full config validation.

Config.ValidateBasic now validates the [p2p] section (previously skipped), so existing negative values in other P2P fields can start failing validation on code paths that call it. The generated config.toml documents accept-interval; dial-interval stays template-hidden but still parses from older files.

^{Reviewed by Cursor Bugbot for commit 8f5a204. Bugbot is set up for automated code reviews on this repo. Configure here.}

[seidroid]

Small, well-motivated change that closes a real defect: MaxAcceptRate was unreachable from config and every node ran the compiled-in 1 accept/s, which cannot drain the accept backlog on a public listener. The wiring is correct and I verified the safe-fallback claims (absent key preserves the default; config.toml is not rewritten over a hand-added key) — remaining findings are doc-comment accuracy, a test assertion looser than its stated intent, and a per-IP concurrency gap now exposed by the higher rate.

Findings: 0 blocking | 9 non-blocking | 4 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's second-opinion pass produced no output — cursor-review.md is empty, so this review merges only Claude's and Codex's findings.
• Nothing replaces the global accept limiter as flood control, and that trade-off deserves an explicit follow-up. The limiter is constructed as rate.NewLimiter(maxAcceptRate(), maxAccepts()) (sei-tendermint/internal/p2p/router.go:190), so a burst of maxAccepts (= maxInbound, ~80 by default) was always permitted; what 1/s prevented was sustaining it. Holding 80 handshake slots full against a 10s handshake-timeout needs ~8 accepts/s, which 1/s denied and 100/s comfortably allows. connTracker is a per-IP rate limit (100 per 100ms ⇒ ~1000/s) applied after AcceptOrClose, and there is no per-IP concurrency cap, so one source can now keep every handshake slot occupied. Raising the default is still the right call — 1/s breaks legitimate inbound — but the residual gap (per-IP concurrent-handshake cap) is worth filing rather than leaving implicit.
• No test covers the wiring itself. The PR's own diagnosis is that a 1/s production cap survived because every RouterOptions construction site except node/setup.go substitutes rate.Inf. The fix adds a config-level test but leaves createRouter's cfg.P2P.AcceptInterval → MaxAcceptRate mapping unasserted, so the same class of regression (dropping the field, or wiring the wrong config key) stays invisible. A node/setup_test.go assertion on the constructed RouterOptions would close it cheaply; the author acknowledges this as deferred, which seems reasonable, but it is the specific gap that produced this bug.
• Verified, no action needed (recording so a later reviewer need not re-derive it): omitting accept-interval from config.toml does not silently yield 0/unlimited. sei-cosmos/server/util.go:255 starts from tmcfg.DefaultConfig() and rootViper.Unmarshal(conf) leaves absent keys untouched, and WriteConfigFile only runs when config.toml does not already exist (sei-cosmos/server/util.go:257-273), so a hand-added key also survives restarts.
• PR description is internally inconsistent on max-connections: the Change section says MaxConcurrentAccepts is "set from MaxConnections", while Notes for review says max-connections "feeds only the MaxInbound/MaxOutbound computation." sei-tendermint/node/setup.go:510 shows the former is closer — it is set to maxInbound, itself derived from max-connections. Worth correcting so the follow-up note isn't acted on from a wrong premise.
• 4 suggestion(s)/nit(s) flagged inline on specific lines.

[codecov]

Codecov Report

❌ Patch coverage is 80.55556% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.47%. Comparing base (8bbac80) to head (8f5a204).

Files with missing lines	Patch %	Lines
sei-tendermint/node/setup.go	75.00%	3 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3899      +/-   ##
==========================================
- Coverage   59.48%   58.47%   -1.02%     
==========================================
  Files        2325     2229      -96     
  Lines      198647   188038   -10609     
==========================================
- Hits       118160   109950    -8210     
+ Misses      69258    67700    -1558     
+ Partials    11229    10388     -841     

Flag	Coverage Δ
sei-chain-pr	72.48% <80.55%> (?)
sei-db	70.41% <ø> (ø)
sei-db-state-db	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
sei-tendermint/config/config.go	77.20% <100.00%> (+0.32%)	⬆️
sei-tendermint/config/toml.go	57.62% <ø> (ø)
sei-tendermint/internal/p2p/routeroptions.go	81.25% <100.00%> (ø)
sei-tendermint/node/setup.go	59.03% <75.00%> (+1.08%)	⬆️

... and 97 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bdchatham]

Added the golden/compat test coverage this key was missing. accept-interval follows dial-interval in staying out of the generated template, which left "deliberately not rendered" and "not readable at all" indistinguishable from the suite's point of view.

The statesync knobs already establish both halves of this convention; the p2p pacing knobs had neither, so this pins dial-interval too rather than adding a second unpinned key beside it.

Absent from the template — checkConfig in toml_test.go, alongside the existing hiddenStateSyncElems block:

// The p2p pacing knobs are likewise expert-only and stay out of the
// generated template, while still being parsed from existing config files.
var hiddenP2PElems = []string{"dial-interval", "accept-interval"}

Still parseable — new p2p_compat_test.go, mirroring statesync_compat_test.go (same viper/ParseConfig shape, same non-parallel caveat):

• TestHiddenP2PKnobsStillParseFromExistingConfig — both keys read out of an existing config.toml.
• TestFreshP2PConfigKeepsDefaultPacing — a template-shaped file without them still yields the defaults. This guards both directions: a zeroed AcceptInterval means rate.Every(0) == rate.Inf and disables accept pacing outright, while an oversized one throttles the accept loop below peer arrival rate. Neither is visible in the rendered config.

Both guards were checked to actually discriminate rather than merely pass:

Injected break	Resulting failure
mapstructure:"accept-interval-typo"	expected: 20ms, actual: 10ms
accept-interval added to the template	config file was not expected to contain accept-interval

The first is the one worth having — a wrong tag doesn't error, it silently ignores the operator's value and falls back to the default, which is the same class of silent-drop failure that made the sei-config companion necessary.

Companion sei-protocol/sei-config#47 has merged, so the key now survives a WriteConfigToDir round trip instead of being stripped by seictl/sei-k8s-controller.

[bdchatham]

All four inline findings addressed in c704f4d72, replied individually. Picking up the two non-blocking summary items that needed more than a reply:

"No test covers the wiring itself." This was the sharpest finding — the PR diagnosed that a 1/s production rate survived because every RouterOptions site except node/setup.go substitutes rate.Inf, then left the mapping unasserted. I had deferred it; you were right that it is the specific gap that produced the bug, so it is closed here rather than filed.

Extracted the budget/pacing derivation out of createRouter into p2pRouterOptions(cfg, ep, privatePeerIDs) — a pure function over cfg, so it is testable without a proxy, DB provider, or genesis doc. TestP2PRouterOptions_PacingAndBudgetWiring then asserts the derivation directly, using a rate.Limit(-1) sentinel so an unset field is distinguishable from a plausible value:

• defaults reach the router, and specifically are not the 1/s package fallback
• an operator-set accept-interval flows through
• max-connections = 100 yields MaxConcurrentAccepts = 80, pinning the derivation rather than the knob

Verified it discriminates: deleting the MaxAcceptRate line fails with expected: 100, actual: -1 instead of silently falling back.

Per-IP concurrent-handshake cap. Confirmed and not addressed here — agreed it wants its own change rather than being folded in. Your analysis matches what a security review of this reached independently: the slot is taken in acquire() before the STS handshake, and connTracker.AddConn runs one line after AcceptOrClose, so the per-IP control is a rate limit (~1000/s) with no concurrency bound. Raising the global rate removes what was incidentally masking that. The mitigation set worth considering: reject-at-capacity instead of hanging the connection (the transport_mconn.go:37-39 FIXME's own suggestion), a shorter pre-handshake read deadline, and per-IP concurrency accounting after authentication. I will file it separately — flagging that it is worth handling as a security issue rather than a public tracking issue, given it is remotely reachable pre-auth on internet-facing listeners.

Cursor's empty second-opinion pass — noted; no action available on my side.

Also corrected the PR description's max-connections inconsistency you flagged. It now says the same thing in both places: max-connections reaches the router through the maxInbound/maxOutbound derivation, and MaxConcurrentAccepts is set to maxInbound — so the budget does track it, via that computation rather than directly.

[seidroid]

Sound, well-scoped fix: accept-interval is added to P2PConfig with a default, validation, template rendering, and wiring into MaxAcceptRate, and the p2pRouterOptions extraction makes the derivation testable without changing other behavior. No blockers; findings are test-quality nits, a stale PR description, and a couple of consistency/structural suggestions.

Findings: 0 blocking | 10 non-blocking | 4 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• PR description is stale relative to the diff and should be updated before merge. It states accept-interval is "not rendered into the generated template — same treatment as its sibling", but the diff does render it in toml.go and toml_test.go asserts it is present (only dial-interval stays hidden). The Tests section also lists only two tests while the diff adds p2p_compat_test.go (2 tests), the toml_test.go assertions, and TestP2PRouterOptions_PacingAndBudgetWiring. Rendering it is the better call — just make the description match, since it is what a reader relies on for release notes.
• Structural: per AGENTS.md ("guard at the choke point, never at each caller"), the durable correction for the diagnosed failure mode — "any RouterOptions field left unset silently falls back to a package default rather than failing" — is a guard inside p2p (e.g. requiring MaxAcceptRate/MaxDialRate in RouterOptions.Validate(), or dropping the silent .Or() fallbacks for pacing), not a test in node that enumerates fields by hand. The new test is a convention the next field-adder can forget; a validation in the single construction path every router goes through is an invariant they cannot. Worth a follow-up even if out of scope here.
• Validation asymmetry: ValidateBasic now rejects a negative accept-interval but still accepts a negative dial-interval, which also becomes rate.Inf (unbounded dialing) via rate.Every. Adding the sibling check would make the pair consistent and is a one-liner alongside this change.
• Security framing worth stating explicitly: raising the global limiter 100x makes it non-binding, so accept-slot exhaustion now rests entirely on MaxConcurrentAccepts. In router.go:193-214 the semaphore slot is taken before connTracker.AddConn (the per-IP check) and held for the whole handshake (handshake-timeout = 10s), so time-to-fill all 80 slots drops from ~80s to <1s. Steady-state exposure is unchanged — an attacker saturates the slots under either rate — so this is not a regression, but the PR's argument that the per-IP limiter and MaxConcurrentAccepts make the global one "just a backstop" holds only because of that steady-state equivalence, not because the per-IP limiter constrains slot exhaustion.
• Agreeing with the author's own follow-up note: the gap that let a 1/s mainnet accept cap go unnoticed is that testonly.go, router_test.go, and giga_router_validator_test.go all substitute rate.Inf, so no test exercises the real accept rate at the router level. TestP2PRouterOptions_PacingAndBudgetWiring closes the wiring half but not the behavioral half. Worth filing rather than leaving in the PR body.
• Codex reported no material findings. The Cursor review file (cursor-review.md) is empty, so that pass produced no output and contributed nothing to this synthesis.
• 4 suggestion(s)/nit(s) flagged inline on specific lines.

[bdchatham]

Round 2 addressed in 0ccc3ffdf; replied on each inline thread. Three summary items needed more than a reply.

The structural finding — taken, and it changed the shape of the fix. You cited AGENTS.md "Guard at the choke point, never at each caller" against the previous round. I checked the citation rather than taking it, and it is AGENTS.md:60, in a Structural corrections section I had not read — I read that file while my worktree was still on a stale branch, so I had been working from an older copy. My mistake, and worth stating since it means the first two rounds were reviewed against conventions I had not fully seen.

Applying it changed the fix. Wiring MaxAcceptRate in node setup corrects one caller; the hazard is the package default every embedder falls through to. So maxAcceptRate() itself now returns rate.Every(10*time.Millisecond) instead of rate.Every(time.Second). The config key and wiring stay, but they now tune a safe default rather than rescue a dangerous one — and an embedder constructing RouterOptions directly can no longer inherit 1/s.

I did not take the stronger form you offered (requiring the pacing fields in RouterOptions.Validate()). That removes the documented .Or() default, which is the package's idiom, and peermanager_test.go:136 validates option sets that deliberately leave pacing unset. Raising the default gets the same guarantee at the same choke point without changing the contract. Happy to go further if you disagree.

Validation asymmetry — added; dial-interval gets the same negative check.

Security framing — agreed and worth having in the record: raising the global limiter makes it non-binding, so slot exhaustion now rests entirely on MaxConcurrentAccepts, and time-to-fill drops from ~80s to <1s. Steady-state exposure is unchanged, so it is not a regression, but you are right that the "backstop" argument holds because of that equivalence rather than because the per-IP limiter constrains slot exhaustion. Still routing the per-IP concurrency gap separately, as a security issue rather than a public one.

Behavioral half of the test gap (testonly.go/router_test.go/giga_router_validator_test.go all substituting rate.Inf) — agreed it belongs in a filed issue, not this PR. TestP2PRouterOptions_PacingAndBudgetWiring closes the wiring half only.

PR description is now synced to the diff — the "not rendered" claim and the Tests section were both stale.

[seidroid]

Well-motivated fix: accept-interval is wired through to RouterOptions.MaxAcceptRate, the 1/s package default is raised to 100/s, and the derivation is extracted into a testable p2pRouterOptions with reasonable coverage of the config default, the template rendering, and the wiring. No correctness bugs found; the notes are about doc comments that misstate what max-incoming-connection-attempts actually bounds, stale comments in the new test file, and one uncovered validation branch.

Findings: 0 blocking | 9 non-blocking | 4 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's second-opinion pass produced no output (cursor-review.md is empty); Codex reported no material issues. Only this pass contributed findings.
• Security judgement worth making explicit: with the global accept limiter no longer the binding constraint, the effective per-source brake is max-incoming-connection-attempts = 100 concurrent per IP, which is above the default inbound pool (maxInbound = 80 for max-connections = 100). A single IP can therefore occupy the whole inbound handshake semaphore in well under a second, where at 1/s it took ~80s. Still strictly better than a node that accepts nothing, but consider tightening that default in the same change or stating that the risk is accepted.
• The new 100/s default remains unexercised end-to-end: testonly.go, router_test.go and giga_router_validator_test.go all substitute rate.Inf, which is exactly why the 1/s production value went unnoticed. The PR description defers the router-level test; worth filing it as a tracked follow-up rather than leaving it only in the PR body.
• The failure mode this fixes is silent (node stays at head, reports healthy, acquires no inbound peers). Consider logging the effective accept rate and MaxConcurrentAccepts once at router start, so a pathological operator-set accept-interval is diagnosable from logs instead of only from peer counts.
• ValidateBasic now rejects a negative dial-interval, which makes a previously-accepted (if nonsensical) existing config fatal at startup. Almost certainly harmless, but it is an unadvertised behavior change beyond the accept-interval work.
• 4 suggestion(s)/nit(s) flagged inline on specific lines.

[bdchatham]

Round 3 in b816df210, replied on each thread.

The connTracker finding was the important one, and I verified it in source rather than accepting it. AddConn compares against rat.cache[address], which RemoveConn decrements — a concurrent connection count, not attempts — and the window check sits in an else if num == 0 branch, so the 100ms gate is a re-connect delay that only applies when a source currently holds zero connections.

The consequence is what matters: 100 concurrent per IP exceeds the default inbound pool of 80, so that limiter does not bound a single source below the pool and cannot be the backstop I described. Corrected in the doc comment, the template, and the PR description, which carried "~1000/s from a single address" — a claim that was load-bearing for the safety argument and simply wrong.

It also sharpens the follow-up I have been deferring: a single IP can occupy the entire inbound pool under the current defaults, independent of anything this PR changes. That strengthens the case for per-IP concurrency accounting, which I am routing separately as a security item rather than a public issue.

The other three were all self-inflicted staleness from round 2 — I rendered accept-interval, then left a test named TestHiddenP2PKnobs... asserting the opposite, and claimed dial-interval coverage in the description while never adding the field to the slice. All three now fixed and, in the test cases, verified to fail on the defect they target rather than merely pass.

One reframing worth flagging beyond the literal fix: TestFreshP2PConfigKeepsDefaultPacing never mirrored a fresh template. What it actually pins is a config.toml written before these keys existed — which is the state every already-deployed node is in, since seid does not rewrite an existing config.toml. Renamed to TestP2PConfigPredatingPacingKnobsKeepsDefaults to say what it covers, since that is the more valuable case.

[seidroid]

Well-scoped change: accept-interval is added to P2PConfig, rendered in the template, wired into RouterOptions, and the package default is raised from 1/s to 100/s, with the cfg → RouterOptions derivation extracted into a testable p2pRouterOptions and covered by new tests. No correctness bugs found; the notable issues are that the new ValidateBasic checks are never reached in production (confirmed) and that the higher accept rate interacts with a pre-existing per-IP cap gap.

Findings: 0 blocking | 7 non-blocking | 2 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Security/capacity note (not blocking, but worth a follow-up): raising the accept rate from 1/s to 100/s while max-incoming-connection-attempts defaults to 100 concurrent connections per source IP — above the 80-slot default inbound pool (max-connections 100 minus 20 outbound) — means a single source can now re-saturate the MaxConcurrentAccepts semaphore in well under a second instead of over ~80s. The burst (rate.NewLimiter(maxAcceptRate, maxAccepts) in router.go:190) already allowed initial saturation, so this widens sustained-pressure surface rather than creating it, and the PR documents the gap in both the struct and template comments. Consider a follow-up bringing the per-IP cap below maxInbound so the accept pipeline can't be monopolized by one source.
• Test gap the author already calls out: every router-level harness (testonly.go, router_test.go, giga_router_validator_test.go) pins MaxAcceptRate: rate.Inf, so nothing exercises the real accept rate end-to-end — which is precisely why a 1/s cap survived on a mainnet listener. TestP2PRouterOptions_PacingAndBudgetWiring closes the wiring half; the router-behaviour half is still uncovered. Worth landing the dedicated test rather than leaving it as a note.
• The Cursor second-opinion pass produced no output (cursor-review.md is empty), so this review reflects only my own pass merged with Codex's single finding.
• No prompt-injection attempts or suspicious directives found in the PR title, body, or diff.
• testutil/configtest does not cover sei-tendermint/config, so the new AcceptInterval field needs no characterization row — noting it explicitly since AGENTS.md flags added keys as a place the completeness check can be silent.
• 2 suggestion(s)/nit(s) flagged inline on specific lines.

[bdchatham]

Round 4 in be9ad9057.

The [p2p] routing gap is the most important finding on this PR, and it invalidates a verification I reported two rounds ago. Config.ValidateBasic routes seven sections and omits [p2p]; before this commit the only callers of P2PConfig.ValidateBasic anywhere in the tree were tests this PR added.

So when I added the negative-interval guards, ran the deletion check, watched TestP2PConfigValidateBasic fail, and reported them verified — that test calls ValidateBasic() on a *P2PConfig directly. It proved the section owns its checks while nothing in production ran them. accept-interval = "-1s" would have started successfully and become rate.Inf, disabling the accept limiter — the exact failure the guard was written to prevent. My deletion check was real but tested the wrong level.

The gap predates this PR and covers send-rate, recv-rate, flush-throttle-timeout and max-packet-msg-payload-size too, so routing the section makes that whole block load-bearing for the first time.

The new test asserts the routing, not the section, because asserting the section is precisely what passed while nothing called it. Blast radius checked rather than assumed: no config in the tree relied on the gap — config, cmd/..., internal/p2p and the root module all build and pass.

That is AGENTS.md's choke-point rule for the third time on this PR, each at a different level: wire the field → raise the package default → route the section. Each round found the previous fix sitting one level below the path everything actually takes. Worth noting as a pattern rather than three separate nits.

Duplicated default — cross-referenced both ways. I looked at a real drift test in node (which imports both packages), but maxAcceptRate() is unexported, so there is nothing to assert against without widening the API for a test. Noted the alternative in the thread rather than trading a dependency question for a nit.

[seidroid]

Sound, well-tested fix: accept-interval is properly threaded from config to RouterOptions, the extraction of p2pRouterOptions makes the derivation testable, and the compat/template tests cover the paths that matter. No blockers; the notes below are about an undisclosed ValidateBasic behavior change bundled in, a duplicated default with no drift guard, and the security/operator framing of the 100× default increase.

Findings: 0 blocking | 8 non-blocking | 3 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Security framing of the default change: the accept limiter is the only global (non-per-IP) pacing on pre-authentication inbound handshakes. Going 1/s -> 100/s raises the sustainable churn rate of unauthenticated handshake attempts 100x. max-incoming-connection-attempts caps concurrent connections per source (100), not rate, so it does not compensate; only MaxConcurrentAccepts (80 by default) bounds in-flight work, and that ceiling is unchanged. This looks like the right availability/exposure tradeoff, and per-handshake cost is small, but the PR argues the default purely from the drain-the-backlog side — worth stating the DoS side explicitly so the choice is recorded as deliberate.
• PR-narrative nit (doesn't affect the fix): the limiter's burst is maxAccepts = maxInbound = 80 by default, so even at 1/s a cold listener could accept 80 connections immediately. The failure mode the PR describes is therefore steady-state churn / reconnect storms after the burst drains, not initial pool fill. Adjusting the problem statement would make the diagnosis reproducible for the next reader.
• The author-acknowledged gap is the one worth closing in this PR rather than deferring: every router harness pins MaxAcceptRate: rate.Inf, which is precisely why a 1/s mainnet default went unnoticed. internal/p2p/transport_test.go:43 (TestRouter_MaxConcurrentAccepts) is a close template for a router-level test that exercises a finite accept rate. The config- and setup-level tests added here pin the wiring but still leave the router's own behaviour under a real rate unexercised.
• Cursor's second-opinion file (cursor-review.md) is empty — that pass produced no output. Codex reported no material issues, which matches my reading.
• Nit (sei-tendermint/config/toml_test.go): hiddenP2PElems is a one-element slice while accept-interval gets a bespoke if. It mirrors hiddenStateSyncElems so it's defensible, but a matching expectedP2PElems slice would make the pair read symmetrically.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

Tree read succeeded (merge ref ee9f21f). Review below.

1. Blocking

None. The change is a self-contained P2P accept-pacing knob: it adds AcceptInterval (default 10ms), wires it through the new p2pRouterOptions/pacingRate into MaxAcceptRate, routes P2PConfig.ValidateBasic into Config.ValidateBasic, and raises the maxAcceptRate fallback from 1/s to 100/s. I traced each hunk into its callees:

• pacingRate (setup.go:459) correctly refuses negative intervals (which rate.Every would silently turn into rate.Inf = unpaced) and honours 0 as the documented disable. Its error is propagated up through p2pRouterOptions → createRouter, so a bad already-deployed config fails startup rather than starting unpaced. Verified createRouter returns the error at setup.go:541.
• The refactor extracting p2pRouterOptions preserves behaviour: privatePeerIDs is threaded through as a parameter, and the returned RouterOptions fields match the pre-refactor literal.
• Config.ValidateBasic now calls cfg.P2P.ValidateBasic() (config.go:153), which was previously never routed. All checks are < 0 guards on values that are non-negative in DefaultP2PConfig, so no previously-valid default config is newly rejected — confirmed by TestConfigValidateBasicRoutesP2P.
• The limiter is consumed at router.go:190 as rate.NewLimiter(maxAcceptRate(), maxAccepts()) with burst = MaxConcurrentAccepts; a rate.Inf limit (0 interval) makes Wait return immediately, leaving the semaphore as the sole concurrency bound. Consistent with the "0 disables the limiter" contract.

2. Security

None found. I checked the classes that apply: the accept rate is node-local P2P plumbing, not consensus-observed, so the wall-clock/rate behaviour raises no determinism concern across validators. The 100× accept-rate increase (1/s → 100/s) is bounded downstream by the concurrent-handshake semaphore (MaxConcurrentAccepts) and per-IP connTracker (maxIncomingConnectionAttempts=100 / 100ms window), so it does not weaken an existing DoS boundary. No injection, deserialization, path-traversal, or secret surface is touched.

3. Non-blocking

None.

4. Summary

I read the full 470-line diff and the changed files under the PR merge tree (ee9f21f), tracing AcceptInterval from config through pacingRate/p2pRouterOptions into the router's accept loop and the ValidateBasic chain. The change is coherent, defensively handles the negative-interval-vs-rate.Inf footgun in the one function every path passes through, keeps 0 as a valid disable, and is thoroughly characterized by new tests covering routing, defaults, template rendering, backward-compat parsing, and the derivation wiring. The sole scout (codex) produced no reading, so there was nothing to reconcile. I found nothing blocking, no security issues, and no non-blocking concerns worth posting.

{"decision": "approve",
 "summary": "Self-contained P2P accept-pacing knob (AcceptInterval, default 10ms) wired through a new pacingRate/p2pRouterOptions with correct negative-interval refusal, 0-as-disable, ValidateBasic routing, and thorough tests. No blocking, security, or non-blocking findings.",
 "findings": []}

_{seidroid xreview · decision approve · session cb7ef106883e473bb53df977a2a8e589 · turn resp_claude_413354083b04f00221a1ea991a690357 · item ea0b3bde10b14e12b4fc6005f6928c8f}

[seidroid]

Solid, well-tested fix: accept-interval is correctly threaded config → RouterOptions.MaxAcceptRate, the package default is raised in lockstep, and the extracted p2pRouterOptions makes the derivation directly testable. No correctness bugs found; remaining notes are a DoS-surface question the PR itself raises, a widened ValidateBasic behavior change, and some godoc/comment style deviations from AGENTS.md.

Findings: 0 blocking | 9 non-blocking | 5 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• The Cursor second-opinion pass produced no output (cursor-review.md is empty). Codex's two findings are both incorporated below.
• No test exercises a finite MaxAcceptRate end-to-end at the router level — testonly.go:296, router_test.go:273 and giga_router_validator_test.go:112 all pin rate.Inf. The PR correctly identifies that substitution as the reason a 1/s mainnet cap went unnoticed; that follow-up is worth doing soon rather than later, since the accept path is now the only pacing knob operators can get wrong.
• Codex's testify point: sei-tendermint/AGENTS.md:8 asks for libs/utils/require over testify/require. The new tests use testify, matching the surrounding packages (config and node are entirely testify today). Matching the package is the defensible call here; noting it for a maintainer decision only.
• Per AGENTS.md ("Document the subject, not the system"), the new AcceptInterval godoc (config.go:730-738) and the template comment (toml.go:349-356) each spend most of their length describing other knobs (MaxConcurrentAccepts, max-incoming-connection-attempts, kernel backlog behaviour). The operator-facing template comment arguably earns it; the struct godoc could shrink to what the field is plus the 0-disables-it escape hatch.
• 5 suggestion(s)/nit(s) flagged inline on specific lines.

[monty-sei]

✅ Approving this one, the diagnosis lines up exactly with something I ran into last week while verifying the new seed defaults, so it was great to see a fix already in flight!

One thing worth checking before this goes in, the seed profile sets max-connections to 1000, so maxInbound and MaxConcurrentAccepts both derive to 980, and going from 1 accept per second to 100 puts two orders of magnitude more sustained traffic through exactly the path that unbounded read sits on. The burst is unchanged (the limiter is already built with burst = maxAccepts) so it isn't a new hole, it just widens the existing one a fair bit.

Two smaller things, neither blocking:

accept-interval has no upper bound: validation only rejects negatives, so setting it to 1s quietly puts you back in the exact state this PR fixes, and by your own reasoning there's no signal at all when that happens. A warning above some threshold would be nice to have.

Config.ValidateBasic now routing [p2p]: this newly enforces four checks that have never actually run in production. I had a look and the blast radius is small (they're all negative only, and config unmarshals onto DefaultConfig so missing keys keep their defaults), but it does change startup behaviour so it's probably worth a line in the release notes?

[bdchatham]

Round 5 pushed (e56a4791c, 0c95de0f8). Both findings addressed, one of them differently than suggested.

Duplicated default — collapsed rather than tested. One premise in the suggestion was off, and correcting it opened a better fix: config does import internal/p2p, at config/autobahn.go:10. So the config → p2p direction already exists, and instead of a test asserting two copies agree, internal/p2p now exports DefaultAcceptInterval and DefaultP2PConfig() sets accept-interval from it. One value, two references, no drift to detect.

The suggested drift test also would not have compiled — maxAcceptRate is unexported, so neither node nor an external p2p_test package can reach it without widening the API for a test.

[p2p] routing — documented in the PR body with the path analysis, and I confirmed the panic(err) at sei-cosmos/server/util.go:342. Left the panic alone: it is sei-cosmos and a separate concern, though a config error panicking is a poor failure mode regardless of which section is routed.

---

Two things worth flagging beyond the review, since main was merged into this branch mid-review:

The new AGENTS.md Godoc section applies to this PR, and several comments here violated it — DefaultAcceptInterval explained why it was exported, p2pRouterOptions recorded why it was split out of createRouter, and two field comments toured the surrounding system. Rewritten from scratch in 0c95de0f8 per the section's own "rewrite, don't patch" rule. The operator-facing rationale that was removed already lives in the config.toml template, which is where an operator actually reads it.

There is a tension between two AGENTS.md sections that a maintainer may want to resolve. Structural corrections says "the step name carries the what, and the doc comment carries the why... extract the step and move the rationale to its doc comment." The new Godoc section says "godocs say what a thing is, not why... rationale belongs in an inline comment at the line that needs them, or nowhere." I read the Godoc section as the more specific rule for exported API and followed it, but the two give opposite instructions for where rationale lives, and I would rather flag it than pick silently.

Confirmed no golden update is needed. No .golden in the tree captures the tendermint config.toml template — they are all app.toml-side (app/, sei-db/, sei-cosmos/), and testutil/configtest does not reach P2PConfig. Rendering accept-interval therefore breaks no golden.

Branch is BEHIND main after the merge; happy to rebase again before merge if you want it current.

[seidroid]

Well-scoped fix that makes a previously unreachable p2p accept-rate limit configurable and raises a default that was accidentally low; the wiring, the backwards-compat path, and the p2pRouterOptions extraction all check out, and the PR body's own analysis of connTracker and the boot path is accurate. No blockers — findings are a godoc/comment-hygiene cluster (including "this PR" phrasing that goes stale on merge), a validation gap that lets a large accept-interval silently reproduce the very outage being fixed, an untested 0-disables-the-limiter path, and a pre-existing connTracker.lastConnect leak whose growth ceiling this change raises 100×.

Findings: 0 blocking | 11 non-blocking | 6 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's second-opinion pass produced no output — cursor-review.md is empty, so this review merges only Claude's findings with Codex's single godoc nit.
• The PR body's release-note warning looks unreachable, worth confirming before it ships as one. AddCommands calls commands.ParseConfig during root-command construction (cmd/seid/cmd/root.go:158 → sei-cosmos/server/util.go:342), which runs strictly before any PersistentPreRunE. ParseConfig reads the global viper singleton, and nothing populates that with config.toml at construction time — interceptConfigs reads into serverCtx.Viper, and sei-tendermint/libs/cli/setup.go:85 (the only global-viper reader) is itself a pre-run hook. So ParseConfig should see bare DefaultConfig() and always validate clean; the described panic-on-negative-send-rate can't fire from an operator's file. Good news for the risk section — but it also means the flip side of the PR's framing is the accurate one: routing [p2p] is correct hygiene, yet operator-supplied [p2p] values still aren't validated anywhere on the seid boot path, so calling those four checks "load-bearing" now overstates it. Worth settling, since this paragraph is what release notes will be written from.
• Layering: sei-tendermint/config now imports sei-tendermint/internal/p2p for a single duration constant, which permanently forbids internal/p2p from importing config. No cycle today (I checked internal/p2p's import closure), and it matches the existing mempoolcfg precedent in the same file, so this is fine as-is — just noting the constraint it locks in. Sharing the constant is still better than the duplicated 10 * time.Second literal that DialInterval/maxDialRate() carry.
• Verified and correct, for the record: the p2pRouterOptions extraction is behaviour-preserving (the ExternalAddress branch and everything after still mutate the returned struct in the same order); interceptConfigs does start from DefaultConfig() before Unmarshal, so an existing config.toml without the key inherits the default exactly as TestP2PConfigPredatingPacingKnobsKeepsDefaults asserts; the max-incoming-connection-attempts analysis matches connTracker.AddConn; p2p_compat_test.go mirrors the established statesync_compat_test.go shape, and no test in the package calls t.Parallel, so the global-viper mutation is safe. Adding the template key needs no testutil/configtest row — that suite covers app.toml/AppOpts reads, not Tendermint [p2p]. scripts/confix goldens are static fixtures and the two docker/*/config/config.toml files carry only positive values in the newly-live fields, so nothing breaks there.
• The named follow-up is the right one and still open: with every router harness substituting rate.Inf, nothing exercises the production accept rate end-to-end, so the actual failure mode — accept loop not draining the kernel backlog — remains unexercised. The wiring test proves the value reaches RouterOptions, not that the value works.
• 6 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

Well-scoped fix: accept-interval is correctly plumbed from config into RouterOptions, the extraction of p2pRouterOptions is behavior-preserving, and the new tests follow existing package conventions (statesync_compat_test.go, hiddenStateSyncElems). No blockers found; a handful of suggestions around the untested package default, negative-value handling, and the template asymmetry.

Findings: 0 blocking | 10 non-blocking | 5 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Verified the PR's claims independently: sei-cosmos/server/util.go:262 does only validate on the config-creation branch, and util.go:342 panics on commands.ParseConfig error. Worth noting the blast radius is even narrower than the PR states — that ParseConfig call unmarshals the global viper singleton at command-tree construction time, which is not populated with the node's config.toml (seid's boot path uses the per-command rootViper). So a deployed node with a negative [p2p] value most likely never reaches that panic. The release-note call-out is still right to make.
• Security/robustness trade-off worth stating explicitly in the PR body: the accept limiter's burst is maxAccepts and the semaphore (maxInbound, 80 by default) is acquired before limiter.Wait, so raising the rate 1/s→100/s drops the time for a single source to saturate all 80 concurrent handshake slots from ~80s to <1s. Under a slow-loris the semaphore then becomes the binding constraint (~8 slots/s freed at a 10s handshake-timeout), so the ceiling is unchanged and the change is defensible — but the PR's own observation that connTracker's per-IP concurrent cap (100) exceeds the inbound pool (80) means nothing bounds one source below the pool. Worth a tracked follow-up rather than leaving it as a prose aside.
• scripts/confix/confix.go:163 (CheckValid) is a third caller of Config.ValidateBasic not covered by the PR's trace. It unmarshals into a zero-valued config.Config, and all six [p2p] checks are < 0, so zero values pass and the tool is unaffected unless a migrated file carries an explicit negative — same class as the other paths, but worth listing for completeness.
• Cursor's second-opinion pass (cursor-review.md) is empty — that review produced no output. Codex reported no material findings, which matches my read.
• I could not execute the test suite in this environment (go test/go vet were not permitted), so the new tests were reviewed statically. The reflection-based fieldsToTest addition is sound: DialInterval/AcceptInterval are time.Duration, so SetInt(-1) works, and the reset-to-0 between iterations keeps each error attributable to the intended field.
• 5 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

The change is well-motivated and correctly wired: accept-interval reaches RouterOptions through the single production construction site, the package default is raised in maxAcceptRate() so no path inherits 1/s, and negatives are clamped where validation cannot reach. No blockers — remaining notes are documentation contradictions around the negative-value contract, a silent clamp, and some accuracy corrections to the PR's justification.

Findings: 0 blocking | 13 non-blocking | 5 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's second-opinion review file (cursor-review.md) is empty — that pass produced no output.
• On Codex's [p2p]-validation concern: agreed that four previously dormant checks becoming load-bearing is a behavior change worth release notes (the PR body documents it), but the exposure is narrower than either Codex or the PR describes. server.AddCommands calls commands.ParseConfig(tmcfg.DefaultConfig()), which unmarshals from the global viper singleton; seid never loads config.toml into it (interceptConfigs uses its own rootViper, and cmd/seid/cmd/root.go has no global-viper population outside tests). So that call validates pure defaults and cannot panic on an operator's file. Realistic newly-reachable paths are confix's CheckValid and the standalone tendermint binary. Not a blocker, and not a reason to change the code — just don't over-claim the risk in release notes.
• The problem statement slightly overstates the cold-start failure. router.go:190 builds the limiter as rate.NewLimiter(maxAcceptRate(), maxAccepts()), i.e. burst = MaxConcurrentAccepts = maxInbound (80 with the default max-connections = 100). A freshly started node therefore fills its entire inbound pool immediately even at 1/s; the 1/s cap binds on sustained churn — replacement after peer drops, and failed handshakes, each of which still consumes a token. The fix is right either way, but the argument for 10ms should rest on churn/failure throughput rather than on backlog drain at startup.
• Log amplification: the accept loop logs logger.Info("r.runConn(inbound)", ...) once per inbound connection attempt, and for connections that fail fast the accept rate was the only thing bounding that. A connection-churn flood can now produce ~100 Info lines/s instead of ~1/s. Worth a glance at whether that line should be demoted or sampled, given the 100× headroom this PR grants.
• Author-acknowledged and worth doing: no router-level test exercises the production accept rate end-to-end, since testonly.go, router_test.go and giga_router_validator_test.go all pin rate.Inf. That substitution is precisely why a 1/s cap on a mainnet listener went unnoticed, so the follow-up has real value.
• AGENTS.md deviation (testify instead of libs/utils/require) in the config and node test files: matching the surrounding package is the right call here, and routeroptions_test.go correctly uses utils/require where the package already does. No change requested.
• Optional: docker/localnode/config/config.toml and docker/rpcnode/config/config.toml are checked-in full config files that now lack accept-interval. They inherit the 10ms default so nothing breaks, but they drift from the generated template if you want them to stay a faithful copy.
• I was unable to execute the test suite in this environment (bash approval denied), so the new tests were reviewed statically. Imports, line-number anchoring, Config.ValidateBasic ordering (RPC is dereferenced before P2P, so confix's zero-valued Config gains no new nil-pointer exposure), the accept loop's error swallowing, testutil/configtest scope (no [p2p] manifest row exists, so no characterization row is owed), and the confix plan (upstream-migration only; dial-interval has no step either) were all checked by reading source.
• 5 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

A well-scoped fix for a real inbound-starvation bug: accept-interval is now configurable, the unreachable 1/s default is raised, and the previously-dead [p2p] validation is routed — all backed by tests that pin the default, the zero/negative edges, and the cfg → RouterOptions derivation. No blockers; the main follow-up is that raising the accept rate makes single-IP exhaustion of the 80 concurrent-handshake slots sustainable, since the per-IP cap (100) still sits above the inbound pool.

Findings: 0 blocking | 9 non-blocking | 3 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• The Cursor second-opinion pass produced no output (cursor-review.md is empty), so this review reflects only the Codex pass plus my own reading. I was also unable to run go test / go vet in this environment (command approval denied), so correctness here is from reading, not execution — CI should be treated as the gate.
• Rendering accept-interval in the template while keeping dial-interval hidden is defensible (accept is the reason the PR exists), but the pair is now asymmetric in the generated file, and p2p_compat_test.go proves both parse. Consider rendering both so an operator tuning pacing finds the whole knob set in one place — or leave a template comment noting dial-interval exists and is parsed.
• No end-to-end router-level test exercises the production accept rate; every harness still substitutes rate.Inf, which is precisely why a 1/s cap on a mainnet listener went unnoticed. The author flags this as an explicit follow-up, which is the right call for scope, but it is the highest-value remaining test gap here.
• The [p2p] validation now being live is a real, if narrow, behavior change and belongs in release notes as the author says. One clarification for that note: commands.ParseConfig reads the global viper, which has not loaded config.toml at AddCommands construction time, so the panic(err) path is even less reachable than the description suggests — the practical exposure is confix and the config-creation branch.
• The 10ms/100-per-second default is argued from the surrounding limiters rather than measured, as the author states. Not a blocker given the knob now exists and MaxConcurrentAccepts bounds concurrency, but it is the one value here that would benefit from a follow-up measurement on a public-facing node.
• No prompt-injection or instruction-like content found in the diff, commit messages, or PR description — it reads as a normal technical write-up.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.