Skip to content

feat(seeds): ship Sei Labs seeds as the default bootstrap-peers - #3885

Open
monty-sei wants to merge 2 commits into
mainfrom
monty/plt-816-default-bootstrap-peers
Open

feat(seeds): ship Sei Labs seeds as the default bootstrap-peers#3885
monty-sei wants to merge 2 commits into
mainfrom
monty/plt-816-default-bootstrap-peers

Conversation

@monty-sei

@monty-sei monty-sei commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

What

A fresh seid init --chain-id pacific-1 (or atlantic-2) now writes a config.toml with bootstrap-peers already populated with the Sei Labs seed nodes, so a node bootstraps peer discovery with no other config set. Today the field defaults to "" and operators have to source a peer list out of band.

How

Adds app/seeds, mirroring the existing app/genesis pattern for well-known chain data, and calls applyDefaultBootstrapPeers in InitCmd after the chain-id is resolved and before WriteConfigFile.

Decisions worth reviewing

  • bootstrap-peers, not persistent-peers. Seeds populate the address book via PEX and may then be dropped. Holding operator connections open against our seeds indefinitely is wrong for them and a load multiplier for us.
  • arctic-1 is deliberately excluded. It is a devnet, it has no Cosmos chain-registry entry, and a devnet is the most likely network to be reset or re-keyed — the worst case for an address baked permanently into a release. It stays well-known for genesis; it just ships no seeds. A test asserts both halves of that.
  • Init-time only, no runtime injection. Writing config at start-up would mutate a file the operator owns.

Behaviour on existing configs — please read before release notes

An earlier revision of this description said "the operator value always wins". That was imprecise, and the review was right to flag it. Precisely:

  • seid init builds its config from tmcfg.DefaultConfig() and exposes no flag for bootstrap-peers, so the field is always empty at that point. The empty check in applyDefaultBootstrapPeers is defensive, not an operator-precedence mechanism — it keeps the behaviour correct for any future caller that pre-populates the field.
  • Without --overwrite, init refuses to touch an existing config at all, so a hand-edited bootstrap-peers is safe.
  • With --overwrite, config.toml is rewritten wholesale, so a hand-edited bootstrap-peers is now replaced by the Sei seeds instead of by "". Not a regression, and arguably an improvement, but it is a behaviour change and belongs in the release notes.

Verified empirically: hand-edit bootstrap-peers, run init --overwrite, and the seeds replace it; run init without --overwrite and it errors out leaving the file untouched.

Nodes that already ran init do not retroactively get seeds — they are covered by the docs update and a separate chain-registry submission.

Permanence

These strings ship inside released binaries and operators pin them; the secret-connection handshake verifies the NodeID, so a changed ID is a rejected dial rather than a degraded one, and a release in the wild cannot be recalled. The inputs are final: the DNS pattern is settled, all node keys are pinned in encrypted secrets, and every instance-target NLB port is pinned in the infrastructure repo. Retiring an address means keeping it dialable until every release carrying it is out of use — noted in the package doc.

Drive-by

Corrects the --chain-id flag help, which claimed "if left blank will use sei" while the code panics on an empty value.

The other half of that mismatch — panic() printing a Go stack trace for what is plain operator error, where RunE returns errors for the neighbouring invalid-mode case — is left for a separate PR, per the review. It is tracked.

Testing

Wiring coverage (new). TestInitCmdWritesDefaultBootstrapPeers executes the real InitCmd against a temp home and asserts the written config.toml. This closes a gap the review identified: previously nothing exercised RunE, so deleting the wiring left the entire suite green. Confirmed by mutation — removing the call now fails this test.

Data coverage. app/seeds tests hold each NodeID against types.NodeID.Validate rather than a local regex (so the check cannot drift from CometBFT's definition), require the :26656 port, and assert NodeID and host uniqueness across the whole table — a per-chain check would miss a pacific-1 entry pasted into the atlantic-2 block. A further test asserts every seeded chain is well-known per genesis.IsWellKnown, catching a typo'd chain-id that would otherwise be a silent no-op.

End to end with a locally built binary:

--chain-id bootstrap-peers
pacific-1 3 entries
atlantic-2 3 entries
arctic-1 ""
my-private-chain ""

Existing cmd/seid/cmd tests pass unchanged.

Related

The operator docs update is held as a draft until this ships in a release, since it documents the defaulted behaviour.

@cursor

cursor Bot commented Aug 10, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes default P2P bootstrap config for new inits and bakes long-lived seed endpoints into releases; behavior is guarded by tests and skips unknown chains and pre-set values.

Overview
Fresh seid init on pacific-1 or atlantic-2 now writes bootstrap-peers in config.toml with three Sei Labs seed nodes per network, so operators no longer need an out-of-band peer list. A new app/seeds package (same idea as app/genesis) holds the embedded NodeID@host:26656 addresses; applyDefaultBootstrapPeers runs during init and only fills an empty field—unknown or private chain IDs stay unchanged, and arctic-1 deliberately gets no seeds.

init --overwrite now replaces a hand-edited bootstrap-peers with the defaults (previously it became empty). The --chain-id flag help text is corrected to match the required-chain-id behavior.

Reviewed by Cursor Bugbot for commit f75f72e. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedAug 10, 2026, 1:20 AM

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.77%. Comparing base (a115971) to head (f75f72e).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3885      +/-   ##
==========================================
- Coverage   61.61%   60.77%   -0.85%     
==========================================
  Files        2348     2255      -93     
  Lines      200852   190366   -10486     
==========================================
- Hits       123755   115687    -8068     
+ Misses      66044    64452    -1592     
+ Partials    11053    10227     -826     
Flag Coverage Δ
sei-chain-pr 52.13% <100.00%> (?)
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 Δ
app/seeds/seeds.go 100.00% <100.00%> (ø)
cmd/seid/cmd/init.go 66.01% <100.00%> (+30.87%) ⬆️

... and 94 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.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, well-documented change that defaults p2p.bootstrap-peers to the Sei Labs seeds at seid init; the data package and its tests are solid and the wiring is correctly scoped (empty-value only, exact chain-id match, no runtime mutation). No blockers — the notes are about the untested production wiring, an inline comment that AGENTS.md would have as a named step, and two small test/API cleanups.

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

Blockers

  • None at the file/PR level.

Non-blocking

  • No test exercises InitCmd's RunE, so nothing covers the new wiring (shared finding with Codex). cmd/seid/cmd/init_test.go re-simulates config construction (tmcfg.DefaultConfig() + SetTendermintConfigByMode + WriteConfigFile) rather than running the command, so deleting init.go:128-130 leaves every test in the tree passing. The app/seeds tests only cover the data. The manual verification table in the PR description is the only evidence the feature works; extracting the step into a named helper (see the inline note) makes it assertable without standing up the whole command.
  • The Cursor second-opinion pass produced no output — cursor-review.md is empty. Only the Codex pass contributed findings (its single P2 item is folded in above).
  • The PR description says "Operator value always wins," which is true for the value but worth stating precisely in release notes: seid init builds tmConfig from tmcfg.DefaultConfig(), so there is no way for an operator to supply bootstrap-peers to init at all, and init --overwrite rewrites config.toml wholesale. The behavior change there is that an operator's hand-edited bootstrap-peers is now replaced by the Sei seeds instead of by "" — not a regression, but not "the operator value survives" either.
  • Verified against REVIEW_GUIDELINES.md and testutil/configtest: no configtest row is owed here. The suite pins configuration reads, and this change is an init-time write of a default; p2p.bootstrap-peers is not in the tendermint precedence manifest (only p2p.persistent-peers is), so nothing in that suite fails or needs updating.
  • 5 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread cmd/seid/cmd/init.go Outdated
panic("chain-id is required, please set using --chain-id")
}

// Public networks ship with the Sei Labs seeds pre-populated so a fresh

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] AGENTS.md, "Structural corrections": "The step name carries the what, and the doc comment carries the why. A long comment sitting inline in a flow means the step was never named. Extract the step and move the rationale to its doc comment." This is a five-line rationale sitting inline in a ~110-line RunE. Extracting it — e.g. applyDefaultBootstrapPeers(tmConfig, chainID) in this package, with this comment as its doc comment — makes RunE read as a sequence of named steps and, more usefully, makes the wiring directly testable without executing the whole command. That closes the coverage gap Codex raised: today nothing asserts that init populates the field at all.

Comment thread app/seeds/seeds_test.go Outdated
if len(addrs) == 0 {
t.Errorf("%s: no seeds configured", chainID)
}
seen := make(map[string]bool, len(addrs))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] seen is scoped per chain, so the duplicate check can't catch the most likely copy/paste error in this table: a pacific-1 NodeID pasted into the atlantic-2 block. Hoisting seen outside the range chainSeeds loop makes it global across chains. Same argument for keying on hostPort as well as id — two entries with distinct NodeIDs pointing at one host currently pass, and that's the other half of a bad paste.

Comment thread app/seeds/seeds_test.go Outdated

// addrRe matches CometBFT's `NodeID@host:port`, where NodeID is the 20-byte
// address as 40 lowercase hex characters.
var addrRe = regexp.MustCompile(`^[0-9a-f]{40}@[a-zA-Z0-9.-]+:[0-9]{1,5}$`)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] addrRe re-encodes CometBFT's NodeID rule locally, and the port group [0-9]{1,5} accepts :99999 (harmless here only because the :26656 suffix check below catches it). sei-tendermint/types is importable from this package — init.go already imports it — so types.NodeID(id).Validate() would hold the ID against the real definition instead of a copy that can drift from it. (Note the actual consumer, p2p.ParseNodeAddress, is behind the sei-tendermint/internal boundary and genuinely not reachable from app/seeds, so a regex or types.NodeID is the right call here.)

Comment thread app/seeds/seeds.go Outdated
}

// Chains returns the chain-ids that ship with seeds. Order is unspecified.
func Chains() []string {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Chains() has no production caller — its only use is TestSeedChainsAreWellKnown, and that test is in package seeds (not seeds_test), so it can range over chainSeeds directly the way TestSeedAddressesAreWellFormed already does. Consider dropping it rather than shipping exported API that exists only for a same-package test.

Comment thread cmd/seid/cmd/init.go
cmd.Flags().BoolP(FlagOverwrite, "o", false, "overwrite the genesis.json and existing config files (config.toml, app.toml)")
cmd.Flags().Bool(FlagRecover, false, "provide seed phrase to recover existing key instead of creating")
cmd.Flags().String(flags.FlagChainID, "", "genesis file chain-id, if left blank will use sei")
cmd.Flags().String(flags.FlagChainID, "", "chain-id to initialise for (required), e.g. pacific-1 or atlantic-2")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Good fix to the help text. Since the drive-by is specifically about the mismatch between "if left blank will use sei" and the code's behavior, the other half is still there: line 120 panic("chain-id is required, ...") prints a Go stack trace for what is plain operator error. RunE already returns errors for the invalid-mode case a few lines up — return fmt.Errorf("chain-id is required, please set using --chain-id") would match that and give a clean message. Fine to leave for a separate PR.

A fresh `seid init --chain-id pacific-1` (or atlantic-2) now writes a
config.toml with bootstrap-peers already populated, so a node bootstraps
peer discovery with no operator configuration. Previously the field
defaulted to "" and operators had to source a peer list out of band.

Adds app/seeds, mirroring the existing app/genesis pattern for well-known
chain data, and consults it in InitCmd after the chain-id is resolved.

- An operator-supplied bootstrap-peers always wins; we only fill an empty
  value. An unrecognised chain-id is a no-op, so private and local chains
  are unaffected.
- Seeds go in bootstrap-peers rather than persistent-peers: they seed the
  address book via PEX and may then be dropped, and pinning operators to
  long-lived connections against them is wrong for them and a load
  multiplier for us.
- arctic-1 is deliberately excluded. It is a devnet with no Cosmos
  chain-registry entry, and a devnet is the most likely network to be
  reset or re-keyed — the worst case for an address baked permanently
  into a release.

Also corrects the --chain-id flag help, which claimed "if left blank will
use sei" while the code panics on an empty value.

Verified end to end: init on pacific-1 and atlantic-2 yields three seeds
each, arctic-1 and unknown chain-ids yield "".
@monty-sei
monty-sei force-pushed the monty/plt-816-default-bootstrap-peers branch from ec0fff5 to 4c49b2b Compare August 10, 2026 01:02

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small, well-scoped change that pre-populates bootstrap-peers at seid init for pacific-1/atlantic-2, with a genuinely thoughtful package doc and decent unit tests. No correctness or security blockers — the pinned NodeIDs mean a hijacked DNS name yields a rejected dial rather than a MITM — but the wiring in InitCmd is untested, and a couple of comments describe invariants the code doesn't actually establish.

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

Blockers

  • None at the file/PR level.

Non-blocking

  • No test drives InitCmd's RunE, so the new wiring is uncovered (shared with Codex). TestInitModeConfiguration re-implements init's steps (tmcfg.DefaultConfig()SetTendermintConfigByModeWriteConfigFile) rather than invoking the command, so deleting the seeds block at cmd/seid/cmd/init.go:123-130 would not fail a single test. Given this repo's characterization-test culture for configuration (see testutil/configtest/AGENTS.md), a test that runs the command for pacific-1 / atlantic-2 / arctic-1 / a private chain-id and asserts p2p.bootstrap-peers in the written config.toml would pin exactly the four rows the PR description verified by hand.
  • Security posture is sound and worth recording: the seed list is the only new attack surface, and each address is bound to a pinned NodeID verified by the secret-connection handshake, so DNS compromise of *.platform.sei.io produces a failed dial rather than a MITM; seeds only populate the address book via PEX and gate nothing in consensus.
  • The Cursor second-opinion file (./cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's and Codex's findings.
  • Optional, adjacent to the drive-by flag-help fix: cmd/seid/cmd/init.go:120 still panics on an empty chain-id inside a RunE that returns error. Now that the help text advertises the flag as required, returning an error (or marking the flag required via cobra) would give operators a clean message instead of a stack trace. Out of scope if you'd rather keep the diff tight.
  • 5 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread cmd/seid/cmd/init.go Outdated
// operator-supplied value always wins, and an unrecognised chain-id is a
// no-op. Seeds go in bootstrap-peers, not persistent-peers: they seed the
// address book via PEX and may then be dropped.
if tmConfig.P2P.BootstrapPeers == "" {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The guard is always true as written, and the comment claims an invariant nothing establishes.

tmConfig is a fresh tmcfg.DefaultConfig() (line 106); DefaultP2PConfig() sets no BootstrapPeers, and params.SetTendermintConfigByMode never touches the field. Between line 106 and here nothing reads viper, a flag, or an existing config.toml into tmConfigseid init exposes no --p2p.bootstrap-peers flag at all. So there is no path by which an operator value reaches this check.

That makes "An operator-supplied value always wins" misleading in the one place a reader will look. It's actively load-bearing because of what --overwrite does: checkConfigOverwrite rejects the run unless --overwrite is passed, and with it WriteConfigFile regenerates the whole file from DefaultConfig() — so an operator re-running init --overwrite on a configured node loses their custom bootstrap-peers (along with every other customisation) regardless of this branch. Pre-existing behaviour, not introduced here, but the comment as phrased implies the opposite.

Keeping the == "" check as cheap insurance against a future default is fine; suggest re-wording to what's true, e.g. that init always builds config from defaults so seeds are only ever filling an empty field, and that operator overrides are expressed by editing config.toml after init.

Comment thread app/seeds/seeds_test.go Outdated

// addrRe matches CometBFT's `NodeID@host:port`, where NodeID is the 20-byte
// address as 40 lowercase hex characters.
var addrRe = regexp.MustCompile(`^[0-9a-f]{40}@[a-zA-Z0-9.-]+:[0-9]{1,5}$`)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Consider validating the NodeID with the real validator instead of a hand-rolled regex: types.NodeID(id).Validate() from github.com/sei-protocol/sei-chain/sei-tendermint/types enforces exactly the 40-lowercase-hex rule (node_id.go:52-65) that the node applies at runtime, so the test tracks the parser rather than a duplicate of it.

For the address as a whole, p2p.ParseNodeAddress would be the ideal thing to assert against — but it lives in sei-tendermint/internal/p2p, so it is not importable from app/seeds. types.NodeID is the importable half, and init.go already depends on that package.

Comment thread app/seeds/seeds_test.go
// address as 40 lowercase hex characters.
var addrRe = regexp.MustCompile(`^[0-9a-f]{40}@[a-zA-Z0-9.-]+:[0-9]{1,5}$`)

// A malformed NodeID is rejected at the secret-connection handshake, so the

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The stated failure model is inverted, which undersells the test.

A malformed address is not silent: sei-tendermint/node/setup.go:528-533 runs p2p.ParseNodeAddress over every comma-separated bootstrap-peers entry and returns invalid peer address %q on failure, aborting node setup. So a typo in chainSeeds doesn't quietly fail to connect — it bricks seid start for every freshly-initialised mainnet node, which is a strictly better reason for this test to exist.

The silent-never-connects case is the well-formed but wrong NodeID (right shape, wrong key): that one is rejected at the secret-connection handshake with no error surfaced, and no unit test can catch it. Both failures are worth naming, and per AGENTS.md the doc comment is where the why belongs — worth getting right since it's the rationale a future editor of this table will read.

Comment thread app/seeds/seeds_test.go Outdated
if len(addrs) == 0 {
t.Errorf("%s: no seeds configured", chainID)
}
seen := make(map[string]bool, len(addrs))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] seen is re-created per chain, so duplicate NodeIDs are only detected within a network. The realistic copy-paste error when adding a third network is lifting a key from an existing block — e.g. a pacific-1 NodeID pasted under atlantic-2 — and that passes today. Hoisting seen out of the chainSeeds loop (keyed by NodeID, and optionally a second map for hostnames) closes it for free.

Comment thread app/seeds/seeds.go Outdated
}

// Chains returns the chain-ids that ship with seeds. Order is unspecified.
func Chains() []string {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Chains() has no non-test caller, and its only consumer (TestSeedChainsAreWellKnown) is in package seeds — it can range over chainSeeds directly, exactly as TestSeedAddressesAreWellFormed two tests above it already does. Exporting a function solely to serve a same-package test widens the public surface of a package whose whole doc comment is about how permanent its contents are. Unexport it or drop it unless an external caller is planned.

Extracts the inline bootstrap-peers block into applyDefaultBootstrapPeers,
so RunE reads as a sequence of named steps and the rationale lives in a
doc comment rather than mid-flow.

The extraction is what makes the wiring testable. TestInitCmdWrites-
DefaultBootstrapPeers now executes InitCmd against a temp home and asserts
the written config.toml; previously the wiring had no coverage at all —
deleting the call left the whole suite green.

Test hardening in app/seeds:
- Hold NodeIDs against types.NodeID.Validate instead of a local regex, so
  the check cannot drift from CometBFT's definition (the old pattern also
  accepted a five-digit port).
- Check NodeID and host uniqueness across the whole table rather than per
  chain. The likeliest copy/paste error is a pacific-1 entry pasted into
  the atlantic-2 block, which a per-chain check could not catch.

Drops the exported Chains(), whose only caller was a same-package test
that can range over chainSeeds directly.

Also documents the actual --overwrite behaviour on the helper: init has no
bootstrap-peers flag, so the empty check is defensive rather than an
operator-precedence mechanism, and `init --overwrite` replaces a
hand-edited value with the seeds (previously with "").

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, well-tested change: app/seeds ships the Sei Labs seed list and applyDefaultBootstrapPeers wires it into seid init, with tests covering both the data and the call site. No blocking issues found; remaining notes are operational (verifying the baked-in addresses, release-notes coverage for init --overwrite) plus two nits.

Findings: 0 blocking | 7 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); Codex reported no material issues. This review is therefore effectively a single-reviewer pass plus Codex.
  • init --overwrite now replaces a hand-edited bootstrap-peers with the Sei seeds instead of "". The PR description calls this out correctly and nothing in the repo documents it — worth making sure it actually lands in the release notes, since the only in-tree record is the doc comment on applyDefaultBootstrapPeers.
  • The --chain-id help text now says (required) while the code still panic()s on an empty value, printing a Go stack trace for plain operator error next to a RunE-returned error for the invalid-mode case. The author explicitly deferred the panicreturn fmt.Errorf(...) change to a tracked follow-up; noting only so it isn't lost, not asking for it here.
  • Verification note rather than a defect: the correctness of the six seed addresses cannot be checked from this repo — the tests validate form (NodeID validity, :26656, uniqueness) but a well-formed wrong host or ID passes. Given the stated one-way permanence, a dial check against all six from outside the Sei network before the release is cut would be the thing that catches it.
  • 3 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread app/seeds/seeds.go
// sei-protocol/platform (the SeiNode's externalAddress plus its NodeID).
var chainSeeds = map[string][]string{
"pacific-1": {
"0cd5f57c249b5aca815710338e1fe7a14797585d@seed-0-p2p.pacific-1.prod.platform.sei.io:26656",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Naming asymmetry worth a second look: seed-1/seed-2 carry an explicit cell suffix (prod-euw1, prod-use2) while seed-0 is bare prod, so the doc comment's "one per cell (eu-central-1, eu-west-1, us-east-2)" is only readable as three cells if you already know prod == eu-central-1. Not something the tests can catch (host uniqueness passes either way), and these strings are one-way once released — worth confirming against clusters/<cell>/<chain>/seeds/ that the unsuffixed name really is the eu-central-1 cell for both chains.

Comment thread app/seeds/seeds.go
if !ok {
return nil
}
out := make([]string, len(s))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] slices.Clone(s) expresses the copy in one line and is the idiom the neighbouring app/genesis package already uses (it imports slices). Behaviour is identical, including nil in, nil out — though that branch is unreachable here.

Comment thread cmd/seid/cmd/init.go
// with no operator configuration. An unrecognised chain-id leaves the field
// untouched, so private and local chains are unaffected.
//
// Seeds belong in bootstrap-peers rather than persistent-peers: they seed the

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This paragraph restates the bootstrap-peers-vs-persistent-peers rationale verbatim from the app/seeds package doc. Two copies of a rationale drift independently; a pointer ("see the app/seeds package doc for why bootstrap-peers rather than persistent-peers") keeps one source of truth. The --overwrite paragraph below it is genuinely local to this call site and belongs here.

@masih masih left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integration tests would be awesome to have where we assert those endpoints are connectable and are usable as seed, i.e. new peers are discovered from them. Those tests should be disabled by default and only run on CI since they would be making remote calls.

Excited to get this moving! 🙌

Comment thread app/seeds/seeds.go
return nil
}
out := make([]string, len(s))
copy(out, s)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use built-in Go SDK slices to copy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and there is no need to special-case nil - slices.Clone already does that

Comment thread app/seeds/seeds.go
// BootstrapPeers returns the seeds for a chain as the comma-separated value
// CometBFT's `bootstrap-peers` expects, or "" when the chain is unrecognised.
func BootstrapPeers(chainID string) string {
return strings.Join(ForChain(chainID), ",")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocker: This is an anti-pattern: over refactored exported API that is only used once.

Why export ForChain at all. Why refactor it to its own function that is only used once?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, if ForChain was unexported, then even cloning the slice makes no sense.

Comment thread app/seeds/seeds_test.go

for chainID, addrs := range chainSeeds {
if len(addrs) == 0 {
t.Errorf("%s: no seeds configured", chainID)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use testify utilities to reduce verbosity.

Comment thread app/seeds/seeds_test.go
// across the whole table, not per chain: the likeliest copy/paste error is a
// pacific-1 entry pasted into the atlantic-2 block, which a per-chain check
// would miss.
func TestSeedAddressesAreWellFormed(t *testing.T) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a tendermint address parser we can use instead of string slicing? that would make a stronger test and avoid the tests here diverging from what actually matters: tendermint p2p understands the addresses.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p2p.ParseNodeAddress is internal to sei-tendermint, but I'd recommend to reexport it from sei-tendermit/config.

Comment thread app/seeds/seeds_test.go
t.Errorf("%s: seed %q has an invalid NodeID: %v", chainID, a, err)
continue
}
if !strings.HasSuffix(hostPort, ":26656") {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should one cate about the port? That is unclear from the tests alone. Technically, that port can be any valid port. no?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Comment thread app/seeds/seeds_test.go
func TestBootstrapPeers(t *testing.T) {
got := BootstrapPeers("pacific-1")
if n := len(strings.Split(got, ",")); n != 3 {
t.Errorf("expected 3 comma-separated entries, got %d (%q)", n, got)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is magical about the length 3 other than the fact that it so happens to be the number of seed nodes we have added?

I would encourage tests to capture the essence of business logic and rationale, more than mirroring what so happens to be an incarnation of it.

Unless I have missed something?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, this is a change detector. At best you can assert that the list is non-empty. The other problem of this test is that it dupes TestForChain

Comment thread app/seeds/seeds_test.go
}
}

func TestForChain(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these test are not useful as soon as you make ForChain private.

Comment thread app/seeds/seeds_test.go
}

func TestArcticIsDeliberatelyExcluded(t *testing.T) {
if got := ForChain("arctic-1"); got != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil check is too strong, just assert it is empty

Comment thread app/seeds/seeds_test.go
t.Fatalf("arctic-1 is a devnet and must not ship seeds, got %v", got)
}
// Guard the premise of the exclusion: arctic-1 is still initialisable.
if !genesis.IsWellKnown("arctic-1") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make the chain names constants instead of copying them all over the place

Comment thread app/seeds/seeds_test.go
if n := len(strings.Split(got, ",")); n != 3 {
t.Errorf("expected 3 comma-separated entries, got %d (%q)", n, got)
}
if strings.Contains(got, " ") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the config parser really complain about spaces?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants