Derive Autobahn epoch committees from execution stake (CON-358) - #4046
Derive Autobahn epoch committees from execution stake (CON-358)#4046wen-coding wants to merge 6 commits into
Conversation
Stage C_{E+2} from the bonded set after Commit at LastRoad(E) and publish it only after a matching AppQC, so consensus no longer fills later epochs from genesis placeholders.
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 #4046 +/- ##
==========================================
- Coverage 61.24% 60.27% -0.98%
==========================================
Files 2153 2055 -98
Lines 188379 176920 -11459
==========================================
- Hits 115379 106644 -8735
+ Misses 62266 60491 -1775
+ Partials 10734 9785 -949
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.
Replaces genesis-seeded epoch placeholders with an execution-derived stage/activate registry (persisted as an A/B snapshot) and gates activation on a matching AppQC; the staging/activation state machine, the divergence check move into runPersist, and the test rewrites all look coherent. No blocking defects found — the notes below cover a per-block cost on the execution path, a consensus-critical read taken from the CheckTx cache state, a snapshot/restore asymmetry, and a new governance-parameter coupling that can halt the chain.
Findings: 0 blocking | 6 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] No test covers the two persisted halves together:
TestNewRegistry_RestoresSnapshotPending/TestNewRegistry_RestoresPrunedSnapshotexercise the registry snapshot alone, andTestNewState_ActivatesStagedEpochFromPersistedAppQCstages in memory. A round-trip throughBuildDataStatewith a realPersistentStateDir(registry snapshot on disk + BlockStore holding the AppQC forLastRoad(E), restarted) would pin the interaction that matters in production, including the case where the snapshot is ahead of the BlockStore after a crash. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil, fmt.Errorf("app.Commit(): %w", err) | ||
| } | ||
| if err := r.data.PushAppHash(ctx, b.GlobalNumber, resp.AppHash); err != nil { | ||
| weights, err := committeeWeights(app.GetValidators()) |
There was a problem hiding this comment.
[suggestion] committeeWeights(app.GetValidators()) runs on every block, but the result is only consumed when n closes LastRoad(E) — once per 108,000 roads. App.GetValidators() is StakingKeeper.GetBondedValidators(...), i.e. a reverse-prefix iteration over the power index plus a mustGetValidator KV read and an ABCIValidatorUpdate (pubkey marshal) per bonded validator; committeeWeights then does PubKeyFromProto + PublicKeyFromBytes per validator on top. executeBlock already pays for one GetValidators() call at line 197 for proposerAddress, so this doubles it.
Consider making the weights lazy — e.g. change the parameter to func() (map[types.PublicKey]uint64, error) and have PushAppHash invoke it only on the branch where endEpoch is Some. The same thunk works for the runExecute recovery call site.
| } | ||
|
|
||
| // committeeWeights maps the bonded validator set after Commit to voting power. | ||
| func committeeWeights(vals []abci.ValidatorUpdate) (map[atypes.PublicKey]uint64, error) { |
There was a problem hiding this comment.
[suggestion] The stake feeding the committee derivation now comes from App.GetValidators(), which is StakingKeeper.GetBondedValidators(app.GetCheckCtx()) — the CheckTx cache state, not the committed/deliver state.
Calling it right after app.Commit is what makes this work today: Commit calls setCheckState(header), which rebuilds checkState as a fresh cache over the committed store. But that state is node-local and is written concurrently by CheckTx ante execution (msCache.Write() runs for check mode too), so the read is neither race-free nor structurally pinned to the committed set. Before this PR the value only picked a proposerAddress to suppress app log noise; now a per-node divergence in it produces a different C_{E+2} on that node and forks the epoch.
Worth sourcing this from a committed context (or a deliver-state snapshot captured inside Commit) rather than the CheckTx cache, so the consensus-critical read does not depend on setCheckState ordering and mempool quiescence.
| if err != nil { | ||
| return fmt.Errorf("pending: %w", err) | ||
| } | ||
| if idx != s.live.Next { |
There was a problem hiding this comment.
[suggestion] snapshot() encodes Pending at index s.live.Next (line 76), but restore() derives live.Next only from snapshot.Live — when Live is empty it stays at the constructor default of 2. So a state with all live epochs pruned plus a staged committee serializes to a snapshot that restore() then rejects with pending epoch N, want 2, and NewRegistry fails: the node cannot start.
That state is exactly what TestPruneBefore_KeepsPending builds in memory (live = {6,6}, pending at 6), so snapshot()/restore() are not a total round trip. It is not reachable from the one production caller today — runPersist passes the anchor's epoch index, which is always registered, so keep <= live.Next-1 and Live stays non-empty — but the keep = min(keep, s.live.Next) clamp deliberately permits it.
Either persist live.First/live.Next explicitly so restore() reconstructs the window independently of the Live list, or refuse to prune past the epoch a pending committee derives from.
| if s.dropped(next) { | ||
| next++ | ||
| continue | ||
| committee, err := prev.Committee().DeriveNext(weights, target) |
There was a problem hiding this comment.
[suggestion] DeriveNext → newCommittee rejects len(weights) > MaxValidators (100, autobahn/types/committee.go:36), and GetBondedValidatorsByPower returns up to the staking module's MaxValidators param (default 35, governance-settable).
This PR newly couples the two: if that param is ever raised above 100, then at the next epoch boundary StageEpoch fails with too many validators, PushAppHash propagates the error, and executeBlock/runExecute halt the node. Because the failure is deterministic on re-execution of the same block, a restart hits it again — a permanent chain halt requiring a param or code change to clear. Previously the committee was genesis-fixed, so the staking param could not reach consensus this way.
Worth either validating the bonded-set size against types.MaxValidators at a place where it can be surfaced as an operator alarm ahead of the boundary, or truncating to the top-100 by power rather than failing.
| // counters and diverge from production. ToReqBeginBlock skips the per- | ||
| // validator loop when Signatures is empty, so empty Votes flow into | ||
| // distribution/slashing on both paths. | ||
| // LastCommit is non-nil with empty Signatures. |
There was a problem hiding this comment.
[suggestion] Only one clause of the deleted comment went stale — "the committee is fixed by genesis (no validator-set updates)" — and this PR is what makes it stale. The rest was the load-bearing reason for the empty Signatures: surfacing N absent-signature entries here would make trace replay's BeginBlock bump missed-block counters and diverge from production, and ToReqBeginBlock skips the per-validator loop when Signatures is empty so empty Votes flow into distribution/slashing on both paths. That invariant still holds and now has no record anywhere.
Per AGENTS.md ("Relocating a load-bearing invariant is the move, never deleting one to tidy up"), rewrite the rationale with the stale clause dropped rather than reducing it to a restatement of the code.
The codegen diff check regenerates these files and compares, so they must match the generator byte for byte rather than the repo's goimports grouping. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2ccf666. Configure here.
gosec flags the uint64 to int conversion. The value is only a capacity hint, so clamping is harmless. Co-authored-by: Cursor <cursoragent@cursor.com>
The genesis-fixed committee sentence was stale; the missed-block / empty Votes invariant is not, and belongs on LastCommit itself. Co-authored-by: Cursor <cursoragent@cursor.com>
shemnon
left a comment
shemnon
left a comment
There was a problem hiding this comment.
I wish github would have allowed file ordering, so I could start with the committee protobufs instead of end with them.
LGTM, but I'm the new hire who hasn't been indoctrinated into the ways of authbahn;
| b := GenPublicKey(rng) | ||
| c1 := utils.OrPanic1(NewCommittee(map[PublicKey]uint64{a: 3, b: 1})) | ||
| c2 := utils.OrPanic1(NewCommittee(map[PublicKey]uint64{b: 1, a: 3, GenPublicKey(rng): 0})) | ||
| require.True(t, c1.Equal(c2)) |
There was a problem hiding this comment.
These are two relatively disjoint tests. Shoudln't they be two different tests?
| rng := utils.TestRng() | ||
| a := GenPublicKey(rng) | ||
| b := GenPublicKey(rng) | ||
| src := utils.OrPanic1(NewCommittee(map[PublicKey]uint64{a: 1, b: 1})) |
There was a problem hiding this comment.
Can we add a c case that is unchanged? as well as a D case that is never set?, and make the be case not set in src but is added by derive?
| return n < i.nextAppQC && | ||
| bytes.Equal(i.appProposals[n].AppHash(), i.appQCs[n].Proposal().AppHash()) | ||
| } | ||
| n = max(n+1, p.GlobalRange().Next) |
There was a problem hiding this comment.
is there ever a case where p.GlobalRange().Next is less than n+1? Is it only populated for gaps?
There was a problem hiding this comment.
That's a good point, actually this shouldn't happen, changed.
…urvives. Split committee equality tests, drop the unused AppQC height max, and retain live.Next-1 so snapshot restore can still activate a staged committee. Co-authored-by: Cursor <cursoragent@cursor.com>
A failed fsync leaves the live registry unchanged so a retry actually writes, and restart at an epoch-1 tip keeps the LastRoad pending committee. Co-authored-by: Cursor <cursoragent@cursor.com>
| } | ||
| } | ||
|
|
||
| func TestCommitteeEqual_IgnoresZeroWeightsAndMapOrder(t *testing.T) { |
There was a problem hiding this comment.
nit - test is a bit misleading i don't think we're testing ignore zero weights, and zero weights, maybe we should rename remove "ZeroWeights"
| // An error if it conflicts, if the target is not live.Next, or if endEpoch+1 | ||
| // is not live. | ||
| func (r *Registry) StageEpoch(endEpoch types.EpochIndex, weights map[types.PublicKey]uint64) error { | ||
| target := endEpoch + 2 |
There was a problem hiding this comment.
the reason to do epoch C{e+2} is because C{e+1} cannot wait for epoch e to finish right (given async execution). open question - but do we have enough guarantees that we will always finish execution epoch e by the time we need to calculate C{e+2}?
There was a problem hiding this comment.
i.e. - do we have backpressure in place?
| appQC := inner.appQCs[status.NextAppQC] | ||
| n := status.NextAppQC | ||
| appQC := inner.appQCs[n] | ||
| if got, want := inner.appProposals[n].AppHash(), appQC.Proposal().AppHash(); !bytes.Equal(got, want) { |
There was a problem hiding this comment.
just making sure that there's no edge case where we might compare bytes.Equal.(nil, []byte{})? i think this isn't possible but want to be extremely sure
4 participants
Summary
C_{E+2}from the bonded validator set afterCommitat LastRoad(E); keep it pending until a matching AppQC (or eviction past that road) publishes it.SetupInitialEpochs/AdvanceIfNeeded).Made with Cursor