(Autobahn) Share vote aggregation (CON-423) - #4152
Conversation
The three phases ran the same latest-vote-per-key, weighted-bucket, monotonic-QC loop in triplicate. Fold that into one aggregator so quorum and replacement are tested once, and each phase only keeps the properties that are actually its own. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4152 +/- ##
==========================================
- Coverage 66.56% 65.42% -1.15%
==========================================
Files 2196 2081 -115
Lines 169150 157378 -11772
==========================================
- Hits 112602 102964 -9638
+ Misses 56407 54273 -2134
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The three phase-vote aggregators are correctly folded into one generic voteAggregator plus a phaseVotes adapter — the latest-vote-per-key pruning, bucket-weight accounting, quorum check, and QC-monotonicity guard all preserve main's semantics, and the pushVote → pushVerifiedVote rename matches what state.go actually guarantees. Two non-blocking notes: the bucket slice is now materialized before the publication guard, and the "QC advances to a newer view" branch lost its test coverage when timeout_votes_test.go was replaced.
Findings: 0 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| return utils.None[[]V]() | ||
| } | ||
|
|
||
| votes := make([]V, 0, len(set.votes)) |
There was a problem hiding this comment.
[suggestion] The bucket copy now happens before the publication guard, which is an ordering change from main. Previously each phase checked weight >= quorum and the QC-view guard (!old.View().Less(view)) before building the votes slice, so votes arriving after a QC was published at that view cost O(1). Now the aggregator materializes the whole bucket on every such vote and pushVerifiedVote (phase_votes.go:52) immediately discards it. For an equal-weight committee that receives all n votes for a view, the copying work under the phase mutex goes from O(n) to O(n²).
Correctness is unaffected, but the fix is local: have pushVote return the bucket (e.g. utils.Option[*voteSet[V]]) or a bool, and let pushVerifiedVote materialize the slice only on the path where it actually calls newQC.
| return | ||
| } | ||
| // Construct a QC from the votes. | ||
| if old, ok := p.qc.Load().Get(); ok && !ph.qcView(old).Less(ph.view(vote)) { |
There was a problem hiding this comment.
[suggestion] The branch where this guard passes — an already-published QC being replaced by one at a later view — is no longer covered. TestTimeoutVotes_ReplacesOlderVoteAndAdvancesQC exercised exactly that (QC at view0, quorum migrates to view1, published QC becomes view1) and was deleted with timeout_votes_test.go. Its replacement, TestVoteAggregator_ReplacesOlderVotesAndEmitsAtNewView, stops at the aggregator and never reaches phaseVotes; every phaseVotes test either publishes the first QC from None or asserts the same-view no-op.
That leaves a liveness-critical property untested: a mis-wired qcView (returning a view that never compares as less) would stall the published QC at its first view and still pass this suite. Restoring one phase-level test that pushes a quorum at view0, then a quorum at view0.Next(), and asserts the published QC advances would close it — and would let the PR's "existing tests pass unchanged" claim hold for the QC-publication logic, not just for the aggregator.
The aggregator already covered replacement; this hits qc.Load() so a stuck qcView cannot stall the published QC at its first view. Co-authored-by: Cursor <cursoragent@cursor.com>
shemnon
left a comment
shemnon
left a comment
There was a problem hiding this comment.
LGTM
Non-blocking design question, why a struct of function pointers and not an interface with three impls? Is this for performance or code size?
@shemnon neither. The three vote types run the same loop; they just pull key/view/bucket off the message differently (prepare/commit bucket by hash, timeout by view). A struct of those extractors was the smallest way to share the loop without three new types. |
3 participants
Summary
qc.Load(), thenNew*QC, thenStore).Quorum wiring
PrepareQuorum,CommitQuorum, andTimeoutQuorumare already the same function on main (totalWeight - Faulty(); prepare/timeout/app returnCommitQuorum()). The phase tests size the committee withCommitQuorum()and do not pin which getter is wired into each adapter.If those thresholds diverged later, swapping the wrong getter would not always fail this suite (a too-strict aggregator in particular would still form a QC). We are not adding two-sided per-phase quorum tests for that: we do not expect the getters to diverge in the near future.
QC.Verifywould still catch the too-loose direction.Test plan
GOWORK=off go test github.com/sei-protocol/sei-chain/sei-tendermint/internal/autobahn/consensus -count=1make fmtcheckMade with Cursor