Skip to content

(Autobahn) Prove commit-committee identity on giga handshake and learn live dial addresses (CON-358) - #4108

Open
wen-coding wants to merge 10 commits into
mainfrom
wen/giga-handshake-live-addrs
Open

(Autobahn) Prove commit-committee identity on giga handshake and learn live dial addresses (CON-358)#4108
wen-coding wants to merge 10 commits into
mainfrom
wen/giga-handshake-live-addrs

Conversation

@wen-coding

@wen-coding wen-coding commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Validators sign a tagged giga handshake claim (validator key, Autobahn SelfAddr, EVM RPC). No claim, or a claim outside the current commit committee, is served as a fullnode; a verified current member is served as that validator for the life of the socket.
  • Inbound advertisements of current members are learned on a live overlay. The configured ValidatorAddrs book wins on dial; the overlay only covers members it omits. An unroutable EVM RPC means we learn none of the advertisement. Overlay rows and leaver sessions stay until Anchor is within one epoch of commit.
  • Join does not upgrade a block-sync socket. A book-absent member who inbounds too early is recovered by their own redial after DialInterval.
  • HTTP(S) EVM RPC is checked by utils.CheckHTTPURL (config, decode, local offer). Loopback, link-local, and unspecified hosts are refused on the overlay. The local book address must survive NodeAddress.Validate() or the validator will not start.

Linear: CON-358

Test plan

  • GOWORK=off go test ./sei-tendermint/internal/p2p/ ./sei-tendermint/libs/utils/ ./sei-tendermint/config/ -count=1 -run 'TestHandshake|TestGigaRouter|TestDecodeGigaClaim|TestCheckHTTPURL|TestIsLoopbackOrLinkLocalURL|TestURLUnmarshal' (32 passed)
  • Confirm a book-absent current member who inbounds is dialed back at the advertised Autobahn address (TestGigaRouterCommon_RunInboundConnLearnsMemberAndClosesOnLeave learns the overlay row; validatorAddr is book-first)
  • Confirm a leaver’s inbound closes, and overlay dials stop only after Anchor catches up (RunInboundConnLearnsMemberAndClosesOnLeave, RunPerCommitteeMemberKeepsLeaversWhileAnchorLags)
  • Confirm a giga inbound without a claim is capped block-sync and is not learned as a validator (TestHandshakeAcceptsFullnodeWithoutClaim; acceptInbound returns None when there is no current-committee claim)
  • Confirm localhost / 0.0.0.0 EVM RPC is not learned, and a local :0 address refuses to start (AcceptInboundRefusesUnroutableEvmRPC, RejectsInvalidSelfAddr)

…ses (CON-358)

Validators sign a tagged claim (validator key, SelfAddr, EVM RPC) on the
giga handshake so inbound role follows the current commit committee, not
the static book. Joiners not in ValidatorAddrs become dialable via a live
overlay; leave closes that inbound and drops the overlay row.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 9, 2026, 8:21 PM

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.58779% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.98%. Comparing base (f80f6e7) to head (17ccbca).

Files with missing lines Patch % Lines
sei-tendermint/internal/p2p/giga_router_common.go 80.37% 16 Missing and 5 partials ⚠️
sei-tendermint/internal/p2p/conv.go 75.00% 4 Missing and 4 partials ⚠️
...i-tendermint/internal/p2p/giga_router_validator.go 82.85% 3 Missing and 3 partials ⚠️
sei-tendermint/config/autobahn.go 66.66% 1 Missing and 1 partial ⚠️
...ei-tendermint/internal/p2p/giga_router_fullnode.go 66.66% 2 Missing ⚠️
sei-tendermint/internal/p2p/handshake.go 94.59% 1 Missing and 1 partial ⚠️
sei-tendermint/internal/p2p/pb/p2p.wireguard.go 81.81% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4108      +/-   ##
==========================================
- Coverage   60.99%   59.98%   -1.01%     
==========================================
  Files        2204     2088     -116     
  Lines      193005   179295   -13710     
==========================================
- Hits       117727   107558   -10169     
+ Misses      63846    61587    -2259     
+ Partials    11432    10150    -1282     
Flag Coverage Δ
sei-chain-pr 70.75% <83.58%> (?)
sei-db 69.80% <ø> (ø)
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/autobahn/types/msg.go 89.69% <100.00%> (+0.17%) ⬆️
sei-tendermint/internal/p2p/giga_router.go 100.00% <ø> (ø)
sei-tendermint/internal/p2p/router.go 90.76% <100.00%> (+0.11%) ⬆️
sei-tendermint/libs/utils/url.go 100.00% <100.00%> (ø)
sei-tendermint/node/setup.go 58.38% <100.00%> (ø)
sei-tendermint/config/autobahn.go 50.00% <66.66%> (ø)
...ei-tendermint/internal/p2p/giga_router_fullnode.go 35.48% <66.66%> (+3.28%) ⬆️
sei-tendermint/internal/p2p/handshake.go 72.52% <94.59%> (+17.17%) ⬆️
sei-tendermint/internal/p2p/pb/p2p.wireguard.go 93.93% <81.81%> (-6.07%) ⬇️
...i-tendermint/internal/p2p/giga_router_validator.go 87.65% <82.85%> (-2.67%) ⬇️
... and 2 more

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

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread sei-tendermint/internal/p2p/giga_router_common.go
seidroid[bot]
seidroid Bot previously requested changes Sep 9, 2026

@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.

The tagged giga handshake claim is well constructed (challenge + node key + length-prefixed address/URL binding, domain-separated tag, partial-claim rejection, wireguard size caps), but the accompanying rework of session lifetime silently drops the Anchor-lag retention guard — and deletes the tests that pinned it — and the new live-address overlay shadows operator configuration more broadly than the PR describes.

Findings: 1 blocking | 6 non-blocking | 5 posted inline

Blockers

  • None at the file/PR level.
  • 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

  • [suggestion] Mixed-version operation is not possible: dialAndRunConn now hard-fails with errMissingGigaClaim against any committee peer that omits a claim, and an old binary never sends one, so a new node and an old node cannot form a committee link in either direction (the old node's connection is demoted to the block-sync subset and also consumes an inbound-fullnode slot). There is no negotiation or grace path, so the whole committee has to restart together. Two smaller startup-compat changes ride along and deserve the same release note: config.URL.UnmarshalText now rejects any non-http(s) evmrpc in an existing autobahn config file, and NewGigaValidatorRouter now refuses to start if the local validator is missing from ValidatorAddrs or its node key does not match.
  • [suggestion] p2p.proto newly caps self_addr and each pex_addrs entry at 256 bytes. A NodeAddress string is <40-hex node id>@<hostname>:<port>, which reaches ~300 bytes for a maximal (253-char) DNS name, so a legitimately long advertised address now fails protoutils.Scan and aborts the handshake instead of merely being ignored. Consider sizing these to the actual NodeAddress maximum.
  • 4 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread sei-tendermint/internal/p2p/giga_router_common.go
Comment thread sei-tendermint/internal/p2p/giga_router_common.go
Comment thread sei-tendermint/internal/p2p/giga_router_common.go
Comment thread sei-tendermint/internal/p2p/giga_router_common.go Outdated
Comment thread sei-tendermint/internal/p2p/giga_router_common.go
Keep departing members until Anchor is within one epoch of commit, prefer
ValidatorAddrs over the live overlay, and discard an advertisement whose
EVMRPC is loopback or link-local. Also silence handshake gosec and raise
the self_addr/pex_addrs wire cap to 320.

Co-authored-by: Cursor <cursoragent@cursor.com>

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread sei-tendermint/internal/p2p/giga_router_common.go
@wen-coding

Copy link
Copy Markdown
Contributor Author

@seidroid re-review please

@seidroid

seidroid Bot commented Sep 9, 2026

Copy link
Copy Markdown

Claude finished @wen-coding's task in 6m 17s —— View job


Re-review — e25769d

  • Read the full PR diff
  • Verify the Anchor-lag retention guard is restored with test coverage
  • Verify book-first validatorAddr ordering
  • Check the EVM RPC URL validation (loopback/link-local, unspecified addrs)
  • Re-check previously-flagged items
  • Post findings

The second commit closes the blocker cleanly, and the two narrowing changes (book-first lookup, unroutable-EVM-RPC refusal) also shrink most of what I flagged last round. One new guard is bypassable; the rest is cleanup and release-note material.

Resolved since the last review

  • Anchor-lag retention is back, and correctly: settled gates only the departure branch, so an address change still cancels a session while Anchor catches up (giga_router_common.go:643-676). Both deleted subtests (no AppQC yet, AppQC still in the prior epoch) and …DialsBothCommitteesUntilStable are restored. The overlay drop is gated on settled too, so retention is symmetric between sessions and overlay rows.
  • validatorAddr is book-first (:577) with …ValidatorAddrPrefersConfiguredBook pinning it. That also confines address-change churn to book-absent members, since a book-present member's lookup can never differ from session.addr.
  • self_addr / pex_addrs caps raised to 320 with the derivation in the proto comment. Checks out: NodeAddress.String() is mconn:// + 40 + @ + 253 + : + 5 = 308 bytes worst case, and max_count = 100 matches MaxPexAddrs.
  • Local validator's own EVMRPC is validated at construction, so a node can't advertise something it would itself reject.

Also re-checked and clean: the claim is bound to the HKDF-derived secret-connection challenge, so it can't be replayed onto another connection or relayed by a MITM; the two liveAddrVersion bumps are both inside liveAddrs.Lock(), so no lost update; range-over-func return inside Lock() still releases the mutex.

And a correction to my last review: the poolIn cap-slot leak I predicted does not materialize. When a joiner is served the block-sync subset, its own validatorService.RunClient streams have no server handler, so the peer's dialAndRunConn fails and it closes the socket — which frees the poolIn slot.

Blocker

1. The new unroutable-EVM-RPC guard misses unspecified addressessei-tendermint/libs/utils/url.go:30

IsLoopbackOrLinkLocalURL covers localhost, IsLoopback, and both link-local predicates, but not IsUnspecified. http://0.0.0.0:8545 and http://[::]:8545 are learned, and ethrpc.DialContext on them connects to the local host — which on a Sei validator is its own EVM RPC. Every peer that learns that row then submits EVM transactions addressed to the shard owner into its own node instead, where it is not the shard owner. Same class as the loopback case the guard was added for, reached by one misconfigured or malicious current committee member.

return ip != nil && (ip.IsLoopback() || ip.IsUnspecified() ||
    ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast())

url_test.go:TestIsLoopbackOrLinkLocalURL already has the table to extend (http://0.0.0.0:8545, http://[::]:8545). Fix this →

Non-blocking

2. [suggestion] runUntilMembershipChange's isCommittee parameter is now deadgiga_router_common.go:742

All three call sites (:800, giga_router_fullnode.go:113, and the tests) pass true. The HasReplica(validator) != isCommittee generality has no live caller, and the log lines already say "left the committee". Fold the parameter away and rename/re-doc the step as what it now is — run until the validator leaves the committee. That also makes the removal of the joins subtest the correct consequence of a narrowed contract rather than lost coverage, which is how I'd read it now.

3. [suggestion] The inbound fullnode path still has no membership watchgiga_router_common.go:778

A verified claim from a member that is not yet in our nextCommitEpoch view is served the block-sync subset for the life of the socket, and its address is not learned. Recovery is real but indirect: the peer's runCommitteePeer loop notices its client streams failing and redials every DialInterval, and acceptInbound re-evaluates on the fresh handshake. So the cost is one DialInterval of delay per stale-view join rather than a stall — but it is now load-bearing and undocumented, where the removed runUntilMembershipChange(…, false, …) made it immediate. Either restore the watch or say in RunInboundConn's doc that a join is recovered by the peer's redial.

4. [suggestion] fillInboundHandshake still overrides SelfAddress for non-giga peersgiga_router_common.go:67

It runs for every inbound connection in acceptPeersRoutine before the peer's SeiGigaConnection flag is known, so a validator advertises its autobahn committee hostname to plain p2p peers, which store and gossip it via PEX. The two values are configured independently (ValidatorAddrs[self].HostPort vs p2p.external_address), and an operator who splits them — internal name for committee traffic, public name for p2p — poisons PEX with an address public peers cannot dial. Since the offer must be written before the peer's flag is known, the cheap fix is a startup check that the two agree; NewGigaValidatorRouter already validates the self row, so it is the natural place. (Fullnodes are unaffected: selfAddr is None there.)

5. [suggestion] Fold the map write and the version bump into one methodgiga_router_common.go:626, :657

Store(Load() + 1) is a read-modify-write that is correct only because both call sites happen to sit inside liveAddrs.Lock(). Nothing states that invariant, so a third writer added later loses updates silently and the symptom is a member that is never re-dialed. A learnAddr / forgetDeparted pair holding the lock and bumping the version is the choke point.

6. [nit] The advertised HostPort gets none of the checking the advertised EVMRPC now getsgiga_router_common.go:611

acceptInbound refuses an unroutable EVMRPC but takes SelfAddr's hostname as given, so a member can point peers' consensus dials at 127.0.0.1. The damage is bounded — dialAndRunConn's peerKey != expectedNodeKey check fails a self-connect, so it is redial churn rather than misrouting — but if the advertisement is untrusted enough to justify the URL check, the address deserves the same one.

7. [nit] Overlay churn is unbounded but now cheap. Each distinct advertised address bumps liveAddrVersion, and stopStaleSessions cancels and waits on that member's session before re-dialing, restarting its consensus streams and EVM proxy on every peer. Book-first lookup confines this to book-absent members, so the blast radius is much smaller than last round — worth a sentence saying that is what bounds it.

8. [suggestion] Release note for the rollout. Unchanged from last round and still worth writing down, since three separate things now refuse to start or connect: dialAndRunConn hard-fails with errMissingGigaClaim against a peer that omits a claim (an old binary never sends one, so the committee restarts together); config.URL.UnmarshalText rejects any non-http(s) evmrpc in an existing autobahn config file; and NewGigaValidatorRouter refuses to start when the local validator is absent from ValidatorAddrs, its node key mismatches, or its own evmrpc is not http(s).


I could not execute the test plan — go build / go test were not permitted in this environment, so the findings above are from reading the code. Worth running GOWORK=off go test ./sei-tendermint/internal/p2p/ ./sei-tendermint/libs/utils/ ./sei-tendermint/config/ -count=1 and make fmtcheck before merge.
· branch wen/giga-handshake-live-addrs

wen-coding and others added 3 commits September 8, 2026 20:20
0.0.0.0 and :: dial the local node the same way loopback does, so treat
them as unroutable when deciding whether to learn an advertisement.

Co-authored-by: Cursor <cursoragent@cursor.com>
A join while nextCommitEpoch is stale is recovered by the peer's redial,
not by watching membership on the leftover fullnode socket.

Co-authored-by: Cursor <cursoragent@cursor.com>
Every caller already waited for a leave; the parameter only suggested
a join-watch that inbound fullnode no longer has.

Co-authored-by: Cursor <cursoragent@cursor.com>
@wen-coding
wen-coding dismissed seidroid[bot]’s stale review September 9, 2026 03:57

Fixed, please re-review

@wen-coding

Copy link
Copy Markdown
Contributor Author

@seidroid re-review please

@seidroid

seidroid Bot commented Sep 9, 2026

Copy link
Copy Markdown

Claude finished @wen-coding's task in 5m 22s —— View job


Re-review — 5ef388f

  • Read the three new commits since e25769d
  • Verify the unspecified-address blocker is closed
  • Verify the isCommittee removal and the inbound-role doc
  • Re-check the remaining open items
  • Post findings

All three of last round's actionable items are closed. One new finding, from tracing the claim's signed bytes through their round trip.

Resolved since the last review

  • Unspecified addresses are refused (a9b6f71). IsLoopbackOrLinkLocalURL now includes ip.IsUnspecified(), and both url_test.go and TestGigaRouterCommon_AcceptInboundRefusesUnroutableEvmRPC gained http://0.0.0.0:8545 / http://[::]:8545. The blocker is gone.
  • isCommittee is folded away (5ef388f). runUntilMembershipChange is now !epoch.Committee().HasReplica(validator), the doc says what it does, and all three call sites are updated. Reads as one step now.
  • The join gap is documented (037ef1e). RunInboundConn's doc names the recovery path — the peer's own dialer redials after DialInterval — so the load-bearing behaviour is written down. Fine as the resolution.

Also re-verified on this head, and clean: the read cap change from a hard-coded 1 MiB to (&pb.Handshake{}).MaxSize() is safe — the generated MaxSize() is a constant (34878), not a per-instance size, so it does not shrink with an empty receiver; max_count = 100 matches MaxPexAddrs = 100; pb.Handshake field 5 (pex_addrs) was previously unregistered in the wireguard schema and now has a bound; and Service.RunServer(ctx, server, true) degrades safely on a fullnode, because x.state is None there, which is exactly what the new GigaRouter doc claims. EvmProxy's re-key from poolOut.Get(target.Key) to poolOut.Get(validator) lines up with poolOut now being keyed by atypes.PublicKey and with proxies always having been keyed that way.

A correction to my last round on one point: I said the overlay is validator-only. gigaFullnodeRouter.Run also calls runPerCommitteeMember(ctx, r.runEvmProxy), so fullnodes do consult validatorAddr and do run stopStaleSessions. It makes no practical difference — validators only dial committee members, so a fullnode's liveAddrs stays empty — but the overlay code is live on both roles.

New finding

1. [suggestion] The claim's signed self_addr round-trips lossily, and the local address is never validatedgiga_router_validator.go:71

The signer signs selfAddr.String() (handshake.go:94), which is the exact wire string conv.go:114 sends. The verifier re-derives it from the parsed struct (handshake.go:133 passes msg.SelfAddr, whose String() is called again inside gigaClaimSignBytes). That is only sound while ParseNodeAddress ∘ String is the identity, and it is not:

// address.go:69 — ParseNodeAddress
if address.Port == 0 { address.Port = defaultPort }   // defaultPort = 26657
// address.go:100 — String()
if a.Port > 0 { u.Host = net.JoinHostPort(...) } else { u.Host = a.Hostname }

NewGigaValidatorRouter builds selfAddr straight from cfg.ValidatorAddrs[self].HostPort with no Validate(), and AutobahnFileConfig.Validate checks evmrpc on every row but never checks address. So "address": "validator1.example.com:0" passes config validation, signs mconn://<id>@validator1.example.com, and the peer verifies against mconn://<id>@validator1.example.com:26657 — different bytes, signature fails. The node then cannot form a single giga connection, and the only diagnostic is on the peer, reading handshakeMsg.GigaClaim: <sig error>, which points at the wrong node. "address": ":26656" (empty hostname) is the same class, failing one step earlier in ParseNodeAddress's Validate() as a decode error.

Before this PR a zero port was a visible local dial failure. Two fixes, ideally both:

  • Add selfAddr.Validate() to the startup checks you already added at the top of NewGigaValidatorRouter — it catches both Port == 0 and Hostname == "", and it belongs next to the node-key and evmrpc checks that are there for exactly this reason.
  • Verify against the raw wire string rather than a re-serialization, so no future normalization in ParseNodeAddress can silently invalidate claims.

Fix this →

Still open from last round

2. [suggestion] fillInboundHandshake overrides SelfAddress for non-giga peersgiga_router_common.go:67

Sharpened now that I've traced both directions: acceptPeersRoutine (router.go:240) applies the override to every inbound, before SeiGigaConnection is known, while dialPeersRoutine (router.go:302) still sends the un-overridden r.options.SelfAddress. So a giga validator advertises two different self-addresses depending on who dialed whom, and the inbound one — the autobahn book hostname — is what plain p2p peers store and gossip via PEX. Usually the same socket, since giga rides the p2p listener; the split bites an operator who uses an internal name in the committee book and a public one in p2p.external_address. A startup check that the two agree, in NewGigaValidatorRouter alongside the checks above, is the cheap fix.

3. [suggestion] Fold the map write and the version bump into one methodgiga_router_common.go:626, :657

r.liveAddrVersion.Store(r.liveAddrVersion.Load() + 1) is a read-modify-write that is correct only because both sites sit inside liveAddrs.Lock(). Nothing states that invariant, so a third writer loses updates silently and the symptom is a member that is never re-dialed. A learnAddr / forgetDeparted pair holding the lock and bumping the version is the choke point — the same shape the AGENTS.md guard-at-the-choke-point rule asks for.

4. [nit] The advertised HostPort gets none of the checking the advertised EVMRPC getsgiga_router_common.go:611. Related to finding 1: acceptInbound refuses an unroutable EVMRPC but takes SelfAddr's hostname as given. Damage is bounded by dialAndRunConn's peerKey != expectedNodeKey check, so it is redial churn rather than misrouting, but if the advertisement is untrusted enough for the URL check it deserves the same one.

5. [nit] Overlay churn is unbounded but cheap. Book-first lookup confines it to book-absent members. Worth a sentence saying that is what bounds it.

6. [suggestion] Release note. Three things now refuse to start or connect: dialAndRunConn hard-fails with errMissingGigaClaim against a peer that omits a claim (an old binary never sends one, so the committee restarts together); config.URL.UnmarshalText rejects any non-http(s) evmrpc in an existing autobahn config file; and NewGigaValidatorRouter refuses to start when the local validator is absent from ValidatorAddrs, its node key mismatches, or its own evmrpc is not http(s).


Findings are from reading the code — go build, go test, and gofmt were all refused in this environment. Please run GOWORK=off go test ./sei-tendermint/internal/p2p/ ./sei-tendermint/libs/utils/ ./sei-tendermint/config/ -count=1 and make fmtcheck before merge.
· branch wen/giga-handshake-live-addrs

…to what (CON-358)

Co-authored-by: Cursor <cursoragent@cursor.com>
@wen-coding

Copy link
Copy Markdown
Contributor Author

@seidroid re-review please

@seidroid

seidroid Bot commented Sep 9, 2026

Copy link
Copy Markdown

Claude finished @wen-coding's task in 3m 3s —— View job


Re-review — 7cffdce

  • Read the new commit since 5ef388f
  • Verify the selfAddr validation finding is closed
  • Audit the comment trims for deleted invariants
  • Re-check the remaining open items
  • Post findings

Last round's finding is closed correctly. The comment trim that rode along with it removed two load-bearing notes, one of which was the entire resolution of a finding from the round before. No new correctness issues.

Resolved since the last review

  • selfAddr is validated at the constructor. NewGigaValidatorRouter now builds the NodeAddress up front and calls Validate() (giga_router_validator.go:44-51), before consensus.NewState, which is why TestGigaRouter_RejectsInvalidSelfAddr can pass a nil dataState. Both cases are covered (zero port, empty hostname), and it sits next to the node-key and evmrpc checks — right place. With Port != 0 and Hostname != "" guaranteed locally, ParseNodeAddress ∘ String is now the identity for ordinary hostnames and IP literals, so the signature mismatch that motivated this is gone.
  • CheckHTTPURL's TODO: need more complete checks is replaced by a statement of what it does not cover. That is the better shape — a reader now knows the boundary instead of knowing someone once meant to widen it.

Re-verified and still clean on this head: decodeGigaClaim already url.Parses and CheckHTTPURLs evm_rpc, so acceptInbound's OrPanic1(url.Parse(claim.evmRPC)) cannot fire; handshake pins selfAddr.NodeID == NodeAuth.Key().NodeID(), so the advertised address cannot name a different node; self_addr / pex_addrs at 320 with max_count = 100 still match MaxPexAddrs; the settled gate still covers both the session-departure branch and the overlay drop.

New findings

1. [suggestion] 7cffdce deletes the doc that resolved the stale-view join gapgiga_router_common.go:759

Commit 037ef1e added exactly this to RunInboundConn, and I accepted it as the resolution of both my finding #3 and Cursor Bugbot's high-severity "Stale view skips overlay learning":

// Join is not watched here: a book-absent member who inbounds before they
// appear in nextCommitEpoch stays undialable until they reconnect, which
// their outbound dialer does after DialInterval.

7cffdce removes it. Behaviour is unchanged and the recovery path still works, but the only written record of it is gone — and it is load-bearing precisely because it is not visible in the code: nothing at this call site hints that a rejected-for-staleness peer is recovered elsewhere, by the peer's own timer.

The trim was right that this does not belong in a godoc — it is a "why". AGENTS.md says where it goes instead: rationale belongs "in an inline comment at the line that needs them, or nowhere," and "Relocating a load-bearing invariant is the move, never deleting one to tidy up." Put it inline at the acceptInbound branch:

	if member, ok := r.acceptInbound(hConn).Get(); ok {
		return r.runInboundValidator(ctx, hConn, member)
	}
	// A member who inbounds before they appear in our nextCommitEpoch view is
	// served as a fullnode for this socket's life; their own dialer redials
	// after DialInterval and re-handshakes into the validator role.
	return r.runInboundFullnode(ctx, hConn)

Fix this →

2. [suggestion] Same commit deletes the reason the Anchor-lag guard existsgiga_router_common.go:629

Dropped from stopStaleSessions' godoc: "until then the validators of the epochs in between are in neither endpoint committee, and the AppQCs for those epochs cannot form without them." What remains — "Departures are only acted on once Anchor is at most one epoch behind commitEpoch" — restates the code without saying what breaks if you remove it.

That sentence is not decoration here. This PR dropped that exact guard in b5fe65d and had to restore it in e25769d after review. Deleting the only note explaining why it is load-bearing sets up the same regression for the next person who reads settled as redundant with keepReplicas. Same fix as #1 — move it to an inline comment at the guard:

	// Until Anchor is within one epoch, validators of the epochs in between are
	// in neither endpoint committee, and the AppQCs for those epochs cannot form
	// without them.
	settled := hasAnchor && commitEpoch.EpochIndex() <= a.Epoch.EpochIndex()+1

(The handshake_test.go comment trim in the same commit is fine — that one really was decoration.)

Still open from prior rounds

3. [nit] The verifier still re-serializes rather than verifying the wire stringhandshake.go:133. The blocker half of this is fixed by the constructor check; what is left is narrow. An IPv6 zone ID (fe80::1%eth0) passes NodeAddress.Validate() locally — non-empty hostname, non-zero port — but net.JoinHostPort + url.Parse rejects the % on the peer, so the peer fails with SelfAddr: invalid node address and the diagnostic again lands on the wrong node. Link-local, so it is a nit. Hashing *p.SelfAddr instead of msg.SelfAddr.String() closes the class for good.

4. [suggestion] fillInboundHandshake overrides SelfAddress for non-giga peersgiga_router_common.go:67. acceptPeersRoutine applies it to every inbound before SeiGigaConnection is known, while dialPeersRoutine sends the un-overridden r.options.SelfAddress — so the address a validator advertises depends on who dialed whom, and the inbound one is what plain p2p peers gossip via PEX. A startup check that the two agree belongs with the checks you just added.

5. [suggestion] Fold the map write and the version bump into one methodgiga_router_common.go:623, :652. Store(Load() + 1) is correct only because both sites sit inside liveAddrs.Lock(), and nothing states that. A third writer loses updates silently; the symptom is a member that is never re-dialed. learnAddr / forgetDeparted holding the lock and bumping the version is the choke point AGENTS.md asks for.

6. [nit] The overlay learns rows it will never read. acceptInbound records an address for every verified current member, but validatorAddr is book-first, so a book-present member's row is dead — it only bumps liveAddrVersion and wakes runPerCommitteeMember for a no-op recompute. Skipping the learn when cfg.ValidatorAddrs has a row would make the code match validatorAddr's doc ("the live overlay only covers members it omits") and bound churn to book-absent members explicitly rather than by accident.

7. [nit] The advertised HostPort gets none of the checking the advertised EVMRPC getsgiga_router_common.go:613. Bounded by dialAndRunConn's peerKey != expectedNodeKey check, so redial churn rather than misrouting.

8. [suggestion] Release note. Three things now refuse to start or connect: dialAndRunConn hard-fails with errMissingGigaClaim against a peer that omits a claim (an old binary never sends one, so the committee restarts together); config.URL.UnmarshalText rejects any non-http(s) evmrpc in an existing autobahn config file; and NewGigaValidatorRouter now refuses to start when the local validator is absent from ValidatorAddrs, its node key mismatches, its evmrpc is not http(s), or its address has no port or no hostname — that last one is new in 7cffdce and is the most likely to bite an existing config.


Findings are from reading the code — go build, go vet, and go test were all refused in this environment, so TestGigaRouter_RejectsInvalidSelfAddr is unverified by execution. Please run GOWORK=off go test ./sei-tendermint/internal/p2p/ ./sei-tendermint/libs/utils/ ./sei-tendermint/config/ -count=1 and make fmtcheck before merge.
· branch wen/giga-handshake-live-addrs

… guard (CON-358)

Co-authored-by: Cursor <cursoragent@cursor.com>
@wen-coding

Copy link
Copy Markdown
Contributor Author

@seidroid re-review please

@seidroid

seidroid Bot commented Sep 9, 2026

Copy link
Copy Markdown

Claude finished @wen-coding's task in 1m 27s —— View job


Re-review — 1e69b25

  • Read the new commit since 7cffdce
  • Verify the two relocated comments landed at the lines they guard
  • Re-check the remaining open items
  • Post findings

1e69b25 is comment-only (+6 lines, one file) and closes both of last round's findings exactly as suggested. No new findings. From my side the correctness work on this PR is done; what remains is the cleanup list below, none of it blocking.

Resolved since the last review

  • The Anchor-lag rationale is back, at the guard (giga_router_common.go:640-643). It sits directly above settled := hasAnchor && commitEpoch.EpochIndex() <= a.Epoch.EpochIndex()+1, and the split reads correctly against the AGENTS.md rule: the godoc says what (Departures are only acted on once Anchor is at most one epoch behind commitEpoch) and the inline comment says why (the AppQCs for those epochs cannot form without them). The next reader who suspects settled is redundant with keepReplicas now has the counterexample in front of them, which is what this PR's own b5fe65de25769d round-trip showed was needed.
  • The stale-view join recovery is documented at the branch (:772-774). On runInboundFullnode, where nothing in the code hints that the peer's own DialInterval timer is what recovers it. That also stands as the written resolution of Cursor Bugbot's "Stale view skips overlay learning".

Re-verified on this head and unchanged: settled still gates both the overlay drop (:645) and the session-departure branch (:663) and nothing else, so an address change still cancels a session while Anchor lags; validatorAddr is still book-first (:578); IsLoopbackOrLinkLocalURL still covers localhost + IsLoopback + IsUnspecified + both link-local predicates (url.go:35); NewGigaValidatorRouter's four startup checks (book row, node key, evmrpc, selfAddr.Validate()) are all still ahead of consensus.NewState; the claim is still bound to the HKDF-derived secret-connection challenge and to msg.NodeAuth.Key(), so it cannot be replayed or relayed.

Still open — all non-blocking

1. [suggestion] fillInboundHandshake overrides SelfAddress for non-giga peersgiga_router_common.go:67

acceptPeersRoutine applies the override to every inbound before SeiGigaConnection is known, while dialPeersRoutine sends the un-overridden r.options.SelfAddress — so the address a validator advertises depends on who dialed whom, and the inbound one (the autobahn book hostname) is what plain p2p peers store and gossip via PEX. Bites an operator who uses an internal name in the committee book and a public one in p2p.external_address. A startup check that the two agree belongs with the four checks now at giga_router_validator.go:34-52.

2. [suggestion] Fold the map write and the version bump into one methodgiga_router_common.go:623, :655

r.liveAddrVersion.Store(r.liveAddrVersion.Load() + 1) is a read-modify-write that is correct only because both sites sit inside liveAddrs.Lock(). Nothing states that invariant, so a third writer added later loses updates silently and the symptom is a member that is never re-dialed. A learnAddr / forgetDeparted pair holding the lock and bumping the version is the choke point AGENTS.md asks for.

3. [nit] The overlay learns rows it will never readgiga_router_common.go:618. acceptInbound records an address for every verified current member, but validatorAddr is book-first, so a book-present member's row is dead: it only bumps liveAddrVersion and wakes runPerCommitteeMember for a no-op recompute. Skipping the learn when cfg.ValidatorAddrs has a row would make the code match validatorAddr's doc ("the live overlay only covers members it omits") and bound churn to book-absent members explicitly rather than by accident.

4. [nit] The verifier re-serializes rather than verifying the wire stringhandshake.go:133. The blocker half is fixed by selfAddr.Validate(); what's left is narrow. An IPv6 zone ID (fe80::1%eth0) passes NodeAddress.Validate() locally but net.JoinHostPort + url.Parse rejects the % on the peer, so the peer fails with SelfAddr: invalid node address and the diagnostic lands on the wrong node. Link-local, so a nit. Hashing the received bytes rather than msg.SelfAddr.String() closes the class.

5. [nit] The advertised HostPort gets none of the checking the advertised EVMRPC getsgiga_router_common.go:613. Bounded by dialAndRunConn's peerKey != expectedNodeKey check, so redial churn rather than misrouting.

6. [suggestion] Release note. Four things now refuse to start or connect: dialAndRunConn hard-fails with errMissingGigaClaim against a peer that omits a claim (an old binary never sends one, so the committee restarts together); config.URL.UnmarshalText rejects any non-http(s) evmrpc in an existing autobahn config file; NewGigaValidatorRouter refuses to start when the local validator is absent from ValidatorAddrs or its node key mismatches; and it also refuses when the local address has no port or no hostname — that last one is the most likely to bite an existing config.


Findings are from reading the code: go vet, go test, and gofmt were all refused in this environment. Please run GOWORK=off go test ./sei-tendermint/internal/p2p/ ./sei-tendermint/libs/utils/ ./sei-tendermint/config/ -count=1 and make fmtcheck before merge. (1e69b25 adds only tab-indented comment lines inside existing blocks, so it is unlikely to move the formatters.)
· branch wen/giga-handshake-live-addrs

@wen-coding wen-coding changed the title Prove giga committee identity on handshake and learn live dial addresses (CON-358) (autobahn) Allow epoch members verify identity via secure handshake (CON-358) Sep 9, 2026
@wen-coding wen-coding changed the title (autobahn) Allow epoch members verify identity via secure handshake (CON-358) (Autobahn) Prove commit-committee identity on giga handshake and learn live dial addresses (CON-358) Sep 9, 2026

@shemnon shemnon left a comment

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.

LGTM modulo sedroid and cursor comments.

// NodeAddress that this peer can be dialed at.
optional string self_addr = 4;
// 320 fits the longest NodeAddress: "mconn://" + 40-hex node ID + "@" + a
// 253-char DNS name + ":" + a 5-digit port.

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.

Good to know 3.141592653589793238462643383279502884197169399375105820974944592.eu can host a sei validator!
(no action needed)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

lol, the limits we put in for network addr and url are pretty basic. I think when Greg comes back we can talk about what protection we actually want.

// (it is at full capacity).
repeated string pex_addrs = 5;
repeated string pex_addrs = 5 [
(wireguard.max_count) = 100,

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.

Are we limiting to 100 peers total or just per handshake?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This number is copied from MaxPexAddrs

const MaxPexAddrs = 100

This is per handshake limit.

require.NoError(t, err)
require.NoError(t, protoutils.Scan[*pb.Handshake](raw))
}

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.

Should we test rejecting partial claim sets (less than all of authkey, authsig, and evmrpc)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added

Comment on lines +769 to +775
if member, ok := r.acceptInbound(hConn).Get(); ok {
return r.runInboundValidator(ctx, hConn, member)
}
// A member who inbounds before they appear in our nextCommitEpoch view is
// served as a fullnode for this socket's life; their own dialer redials
// after DialInterval and re-handshakes into the validator role.
return r.runInboundFullnode(ctx, hConn)

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.

I like the functional decomposition here, easier to read.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cursor

cursor Bot commented Sep 9, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes authenticated giga P2P identity, committee routing, and how validators are dialed/proxied—core networking and EVM proxy behavior with epoch/membership edge cases.

Overview
Validators now advertise commit-committee identity on the giga P2P handshake via a tagged Ed25519 claim (SEI_GIGA_VALIDATOR_HANDSHAKE_V1) binding the session challenge, node key, SelfAddr, and EVM RPC URL. Peers verify the claim on giga connections; outbound dials to committee members require a matching claim and register clients by validator key (not node key).

Inbound routing treats a verified current-committee claim as a validator connection for the life of the socket; everyone else (no claim, wrong committee, or early joiner) stays block-sync fullnode until reconnect. Validators publish a handshake offer at startup with strict local checks (book entry, node key, HTTP(S) EVM RPC, valid SelfAddr).

Live address overlay: routable inbound advertisements update a learned map when absent from the configured book; the book still wins on dial. Loopback/link-local EVM RPC is not stored. Overlay rows and outbound sessions refresh when addresses change or members leave, but leaver cleanup waits until Anchor is within one epoch of commit.

Hardening: Autobahn/config EVM RPC URLs must pass CheckHTTPURL; handshake protobuf fields get wireguard size caps (including max 2048-byte evm_rpc).

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

@cursor cursor 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.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit da93e47. Configure here.

Comment thread sei-tendermint/internal/p2p/giga_router_common.go
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.

3 participants