(Autobahn) Prove commit-committee identity on giga handshake and learn live dial addresses (CON-358) - #4108
(Autobahn) Prove commit-committee identity on giga handshake and learn live dial addresses (CON-358)#4108wen-coding wants to merge 10 commits into
Conversation
…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>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
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:
dialAndRunConnnow hard-fails witherrMissingGigaClaimagainst 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.UnmarshalTextnow rejects any non-http(s)evmrpcin an existing autobahn config file, andNewGigaValidatorRouternow refuses to start if the local validator is missing fromValidatorAddrsor its node key does not match. - [suggestion]
p2p.protonewly capsself_addrand eachpex_addrsentry at 256 bytes. ANodeAddressstring 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 failsprotoutils.Scanand aborts the handshake instead of merely being ignored. Consider sizing these to the actualNodeAddressmaximum. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
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>
|
@seidroid re-review please |
|
Claude finished @wen-coding's task in 6m 17s —— View job Re-review —
|
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>
|
@seidroid re-review please |
|
Claude finished @wen-coding's task in 5m 22s —— View job Re-review —
|
…to what (CON-358) Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid re-review please |
|
Claude finished @wen-coding's task in 3m 3s —— View job Re-review —
|
… guard (CON-358) Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid re-review please |
|
Claude finished @wen-coding's task in 1m 27s —— View job Re-review —
|
wen-coding
changed the title
wen-coding
changed the title
shemnon
left a comment
shemnon
left a comment
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Good to know 3.141592653589793238462643383279502884197169399375105820974944592.eu can host a sei validator!
(no action needed)
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Are we limiting to 100 peers total or just per handshake?
There was a problem hiding this comment.
This number is copied from MaxPexAddrs
This is per handshake limit.
| require.NoError(t, err) | ||
| require.NoError(t, protoutils.Scan[*pb.Handshake](raw)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we test rejecting partial claim sets (less than all of authkey, authsig, and evmrpc)?
| 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) |
There was a problem hiding this comment.
I like the functional decomposition here, easier to read.
Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryHigh Risk Overview 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 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 Reviewed by Cursor Bugbot for commit 17ccbca. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ 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.
| if !ok { | ||
| return utils.None[atypes.PublicKey]() | ||
| } | ||
| if !r.nextCommitEpoch.Load().Committee().HasReplica(claim.Validator) { |
There was a problem hiding this comment.
how do we manage the state machine for a node that joins initially as a fullnode then is upgraded to a validator? i don't think we transition it to validator later on, based on comments here
There was a problem hiding this comment.
also another issue is - does this account if validator A leaves the set? i.e. in epoch E -1 (Anchor) validator A is still part of committee, but we don't connect to it because it is not a validator in ecpoh E
There was a problem hiding this comment.
I originally wrote the PR to allow one connection being upgraded from fullnode to validator and vice versa when committee changes. But it introduced a lot of corner cases bypassing the fullnode limit we set on each validator. For example, if we limit each validator to accept 10 fullnodes, now one validator already has 10 fullnodes, then a peer validator is downgraded to fullnode, which fullnode do you kick out.
4 participants
Summary
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.ValidatorAddrsbook 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.DialInterval.utils.CheckHTTPURL(config, decode, local offer). Loopback, link-local, and unspecified hosts are refused on the overlay. The local book address must surviveNodeAddress.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)TestGigaRouterCommon_RunInboundConnLearnsMemberAndClosesOnLeavelearns the overlay row;validatorAddris book-first)RunInboundConnLearnsMemberAndClosesOnLeave,RunPerCommitteeMemberKeepsLeaversWhileAnchorLags)TestHandshakeAcceptsFullnodeWithoutClaim;acceptInboundreturns None when there is no current-committee claim)0.0.0.0EVM RPC is not learned, and a local:0address refuses to start (AcceptInboundRefusesUnroutableEvmRPC,RejectsInvalidSelfAddr)