(Autobahn) dial peers from the live commit-epoch committee (CON-358) - #4086
(Autobahn) dial peers from the live commit-epoch committee (CON-358)#4086wen-coding wants to merge 3 commits into
Conversation
…tic address book. Inbound giga role is fixed at accept: a membership change closes the connection so the peer's dialer reconnects into the role it now has. Skip republishing an unchanged next-commit epoch so watchers do not thrash every QC. Co-authored-by: Cursor <cursoragent@cursor.com>
wen-coding
changed the title
|
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 #4086 +/- ##
==========================================
- Coverage 61.30% 60.26% -1.05%
==========================================
Files 2178 2070 -108
Lines 190788 178403 -12385
==========================================
- Hits 116968 107512 -9456
+ Misses 62796 60908 -1888
+ Partials 11024 9983 -1041
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Moving outbound giga/EVM dialing onto the live commit-epoch committee is a solid restructuring with good unit coverage of runPerCommitteeMember / runUntilMembershipChange. However, the inbound half of the design does not actually work: on a membership change RunInboundConn stops serving but never closes the connection, because server.Run is a main task in the same scope that is waiting for it.
Findings: 1 blocking | 2 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| return nil | ||
| } | ||
| changed, err := r.runUntilMembershipChange(ctx, v, isCommittee, func(ctx context.Context) error { |
There was a problem hiding this comment.
[blocker] A membership change does not close the inbound connection, so the documented mechanism ("a membership change ends it, and the peer's dialer reconnects into the role it now has") does not happen.
server.Run is spawned a few lines above with s.Spawn, i.e. as a main task of this scope. scope.Run does s.Spawn(main); s.main.Wait(); s.cancel() — the scope context is only cancelled after every main task returns. So when runUntilMembershipChange cancels its own (inner) scope and RunServer returns, this main function returns nil and then scope.Run blocks in main.Wait() on server.Run, which itself only returns when the mux ctx is cancelled or the socket dies. The inner scope's s.Cancel(nil) is on a descendant context and cannot reach it.
Concretely: validator V (in the address book) is accepted as a committee peer; V leaves the committee. We log "inbound giga peer changed committee membership; closing", tear down all Serve loops, and then sit on a live TCP connection that serves nothing. router.go applies no timeout to RunInboundConn. The peer only notices via clientPing's 10s interval + 5s timeout, then redials, and only that redial (evicting the entry from poolIn) unblocks us. For the reverse case — a fullnode-slot peer that joins the committee — the stale connection also keeps holding its inboundFullnodeCap slot (defer r.inboundFullnodeCount.Add(-1) runs only once InsertAndRun returns) while the replacement connection takes a second one.
The outbound path does not have this problem because there the cancellation always arrives from an ancestor context.
Minimal fix: spawn the mux as a background task —
s.SpawnBg(func() error { return server.Run(ctx, hConn.conn) })SpawnBg still calls s.Cancel(err) on failure, so the existing "connection died" path is unchanged, but background tasks are cancelled once the main tasks return, which closes the conn immediately on a membership change. Worth a test that asserts the inbound conn actually terminates after the committee flips — the current tests cover runPerCommitteeMember/runUntilMembershipChange in isolation but not RunInboundConn.
| } | ||
| taskCtx, cancel := context.WithCancel(ctx) | ||
| done := make(chan struct{}) | ||
| live[validator] = &memberSession{cancel: cancel, done: done} |
There was a problem hiding this comment.
[suggestion] Entries are only removed from live by stopDepartingMembers, i.e. when the validator leaves the committee. If a committeeMemberTask ever returns on its own (all tasks in the member scope returning ends the session), done closes but the entry stays in live, so that member is treated as running forever and is never restarted while it remains in the committee — a silent loss of the connection and EVM proxy for that peer.
Today's two tasks (runCommitteePeer, runEvmProxy) only return context errors, so this is latent, but committeeMemberTask is a general extension point whose godoc doesn't state the contract. Either say explicitly on committeeMemberTask that a task must run until its context is cancelled, or reap finished sessions (e.g. drop entries whose done is already closed at the top of each iteration).
| }) | ||
| } | ||
|
|
||
| // runCommitteePeer maintains an outbound connection to a committee member. |
There was a problem hiding this comment.
[suggestion] The rationale that used to sit next to this code — why self disables GetBlock, and why the comparison is against r.key.Public() (a NodePublicKey) rather than validatorKey — is still parked as an inline block above s.SpawnNamed("committeeMembers", ...) in Run, several lines away from the getBlock := addr.Key != r.key.Public() it explains. Per AGENTS.md ("the step name carries the what, the doc comment carries the why"; extracting a step means moving its rationale to that step's doc comment), that paragraph belongs on runCommitteePeer, leaving Run reading as a plain list of named steps.
…for the peer. Spawn the mux as a background task so the inbound scope can cancel it; document that committee-member tasks run until cancel. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid re-review please |
|
Claude finished @wen-coding's task in 7m 31s —— View job Re-review — all three findings are addressed
Verdict: the blocker is genuinely resolved. Two small non-blocking items below; nothing that should hold the PR. Blocker — resolved, traced end to end
The "connection died" path is unchanged: The two suggestions are addressed as well: I also re-checked the parts I had not chased on the first pass and found them sound:
Non-blocking1. The contract godoc states the mechanism backwards —
It's the opposite: an early return leaves the entry in 2. The fixed half is still the untested half. The new tests cover One design consequence worth trackingThe Not verified: |
| if err := utils.Sleep(ctx, r.cfg.DialInterval); err != nil { | ||
| return err | ||
| for { | ||
| ep := r.nextCommitEpoch.Load() |
There was a problem hiding this comment.
what happens if this node stops and restarts when the committee view has changed, and all it knows are stale validators? i.e.
- epoch 1, committee {A, B}
- node stops
- epoch 2, committee {C}
- node starts up, only will try committee {A, B}
(ffti if this edge case is addressed in future work)
There was a problem hiding this comment.
Yes, right now if epoch 1 and epoch 2 have no overlap in committee, this validator will get stuck. I don't know whether we need to handle this case though, because that should be very rare in reality.
As long as there is one member in both epoch 1 and epoch 2 which can serve QCs, this guy will eventually reach epoch 2 and talk with the correct guys.
| }() | ||
| return r.nextCommitEpoch.Iter(ctx, func(_ context.Context, epoch *atypes.Epoch) error { | ||
| committee := epoch.Committee() | ||
| if err := stopDepartingMembers(ctx, live, committee); err != nil { |
There was a problem hiding this comment.
will this cause issues if enough validators change at epoch boundary? specifically because of the lag between consensus and execution, i think we run into an issue if 33%+ validator state changes between epochs, because we immediately disconnect from those 33%+ validators but we still need them to reach AppQC. specifically
- epoch 1, committee A,B,C, we know that next commitee is C,D,E
- (disconnect from A,B, but still finalizing App state, we can't reach quorum because we need AppVotes form A,B)
- (unreachable) epoch 2, committee C,D,E
There was a problem hiding this comment.
Nice catch, I should disconnect members in epoch 1 only when AppQC has been received, changed
| } | ||
| isCommittee := false | ||
| if v, ok := validator.Get(); ok { | ||
| isCommittee = r.nextCommitEpoch.Load().Committee().HasReplica(v) |
There was a problem hiding this comment.
would this mean if the val is not caught up then inbound will also be refused? (might be okay but want to check if it's intentional)
There was a problem hiding this comment.
Yes. If we can't confirm you are in the committee we won't accept your inbound connections as a validator, just to be safe. If there is fullnode capacity you can still stream blocks/QCs as a fullnode; that leaves a small door so joiners can catch up.
…commit. Dropping N-1 members as soon as nextCommitEpoch advances cuts AppVote paths that are still owed. Dial and prune the union of the commit and Anchor committees, and only prune once Anchor is present and at most one epoch behind. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid re-review please |
|
Claude finished @wen-coding's task in 8m 7s —— View job Re-review — the Anchor gate holds up
Verdict: no blockers. The What I verifiedThe gate is exhaustive, not just conservative. Anchor's semantics actually support the claim.
Session lifecycle is sound. Sessions are main tasks on the outer scope, and the The new tests are the right ones: Non-blocking1. The inbound half is now asymmetric with the fix, and the mismatch is silent rather than an error — The new godoc is honest that inbound role follows
Concretely, the mirror image of the scenario this commit fixes: on N→N+1, X keeps its outbound to departing V (✓ X gets V's AppVotes), but V's outbound to X is X's inbound, which X closes and re-accepts as a fullnode. Symmetrically for every other member of Same mechanism answers @codchen's question more completely than "refused": when the local node is merely behind and misclassifies a genuine committee member, that peer's consensus streams park silently until the local node catches up and The symmetric fix is small — compute 2. A fullnode whose address book misses the whole committee stops syncing silently —
3. The
The entry stays in Smaller notes
Not verified: |
3 participants
Summary
NextCommitEpochwhen the epoch index is unchanged so watchers do not thrash on every QC.Linear: CON-358
Test plan
cd sei-tendermint && GOWORK=off go test ./internal/p2p/ -count=1 -run 'TestGigaRouter'cd sei-tendermint && GOWORK=off go test ./internal/p2p/... -count=1Made with Cursor