Refactor separating AppQC from CommitQC - #3849
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![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.
Large, well-structured redesign of the AppQC flow, but it contains two node-fatal regressions: PushAppHash rejects the app-tip re-push that runExecute performs on every restart (node cannot restart when its last executed block is the final block of a persisted CommitQC range, and also after upgrading a DB with no AppProposal records), and fullnodes now dial validators with the full validator client set on a block-sync-only Service, which panics in validatorState(). Several recovery tests were also inverted to assert the opposite of what their names claim rather than re-pinning the new behaviour.
Findings: 5 blocking | 18 non-blocking | 15 posted inline
Blockers
- No test covers the fullnode dial path —
giga_router_fullnode_test.goconstructs the router but never callsRun/runFullnodeSubscriber, which is why theRunClient-on-block-sync-service panic is invisible to CI. Please add a test that actually runs a fullnode router against a validator server (or at least a unit test assertingRunFullNodeClientis what fullnodes use). - No test covers restart when the app tip is the last block of a persisted CommitQC/AppProposal range.
TestPushAppQCPersistsAndRecoverspinsinner.nextAppProposal == gr1.Nextafter recovery andTestExecutionpins thatPushAppHash(gr.Next-1)errors in that state — together they encode the restart failure, but nothing exercises the two in sequence the wayrunExecutedoes. - 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review is the merge of Claude + Codex only. PushQCcapacity is now gated oninner.first + blocksCacheSize, andfirstadvances only when an AppQC is persisted. Block/QC ingestion liveness is therefore coupled to quorum AppQC progress with onlyblocksCacheSizeof slack (previously the gate used the node-localnextAppProposal). This looks intentional per the PR description, but it is a meaningful change in failure modes and deserves a comment naming the intended slack.- Widespread stale doc comments after the renames:
// RunBlockSyncServer spawns only...aboveRunFullNodeServerand// RunBlockSyncClient...aboveRunFullNodeClient(giga/service.go:92,104); theServicetype doc still says block-sync-only serves "StreamFullCommitQCs + GetBlock" (now also StreamAppQCs);BlockPersister.PruneAndPersistandCommitQCPersister.PruneAndPersistdocs still describe the removedanchor/afterEachparameters;persisters.closedoc still claims fields are nil-checked. - Test names and comments in
data/state_recovery_test.gono longer match behaviour:TestRecoveryStartsAtLastExecutedBlockno longer starts at LastExecutedBlock,TestRecoveryRejectsEmptyBlockDBAfterFirstCommittedBlocknow assertsNoError, andTestPruningDiscardsnow asserts nothing is discarded. Per AGENTS.md, the review prompt here is to record the new behaviour under an accurate name rather than widen the assertion until it passes. AppVotedoc comment has a typo:nextAppProposa(data/state.go:630).CommitQCPersister.Next()(renamed fromLoadNext) appears to have no remaining callers — worth removing if it is now dead.- 12 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
Large, mostly well-structured redesign of the AppQC flow (AppQCs/AppProposals now first-class BlockDB records, BlockDBIterator replaced by ReadSuffix, avail's prune anchor sourced from data.State). One blocking correctness bug: the PushAppHash durability wait uses n <= persisted.NextAppProposal, which is already satisfied on entry for single-block CommitQC ranges — so the AppHash can be lost on crash and the app tip can drift past the persisted cursor, producing an unrecoverable startup error on restart.
Findings: 2 blocking | 14 non-blocking | 9 posted inline
Blockers
- Cursor's second-opinion file (
cursor-review.md) is empty — that review pass produced no output, so its coverage is missing from this synthesis. Codex's pass produced two findings, both merged below. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Test coverage gap around the new durability contract: no test pins the
PushAppHashdurability wait for a single-block CommitQC range — which is exactly the case the current condition misses.pushAppHashes*helpers and the recovery tests all use multi-blockTestCommitQCranges, so the blocker above is invisible to the suite. A test that writes a 1-block CommitQC and assertsPushAppHashdoes not return untilStatus().NextAppProposal > nwould have caught it. - Test coverage gap in the BlockDB contract:
testWriteAppProposalOrderRejected/testWriteAppQCOrderRejectedonly ever write QC-aligned ranges (appProposalForBatch(rng, b0)), so the documented rejection of mid-QC starts / ranges spanning two CommitQCs is never exercised on either backend. This is the same gap as the missing range check noted inline. ReadSuffixreplacesBlockDBIteratorand materializes the whole suffix in memory. In the state where no AppQC has ever been written, littblock'sReadSuffixloop condition (!gotAppQC || ...) never short-circuits, so it scans and decodes the entire table and returns every block and QC — andPruneBeforeis simultaneously a no-op in that state. Benign at genesis, but worth an explicit bound or a log line, since nothing signals that the store is in the unbounded-growth regime.- Design consequence worth surfacing operationally:
inner.firstnow advances only when an AppQC is persisted, so a stall in AppQC collection freezes BlockDB pruning and (afterblocksCacheSize= 4000 blocks) blocksPushQCentirely. This matches the PR description, but there is no metric or log when that backpressure engages — and withObserveAppQCnow dead (see inline), there is also no AppQC progress gauge left to diagnose it from. - Stale doc on
giga.Service(service.go:18-24): it still says handlers "reach it viavalidatorState()and panic if invoked outside RunServer / RunClient" —validatorState()was deleted in this PR — and still describes the fullnode subset as "StreamFullCommitQCs + GetBlock", which now also includesPingandStreamAppQCs. Per AGENTS.md ("the doc comment carries the why"), these load-bearing comments should be updated with the split intovalidatorService. - No prompt-injection or instruction-like content was found in the diff, commit messages, or PR description.
- 8 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 779b058. Configure here.
There was a problem hiding this comment.
Large, mostly coherent redesign of the AppQC flow (AppQC/AppProposal now first-class BlockDB records, Iterator replaced by ReadSuffix, avail's prune-anchor WAL replaced by a data.State anchor). One peer-triggerable indefinite stall in avail.State.PushVote should be fixed before merge, plus a removed defence-in-depth check on AppQC↔CommitQC alignment, unenforced BlockDB write contracts, a sizeable avail test-coverage regression, and a batch of doc comments left describing removed parameters/semantics.
Findings: 1 blocking | 20 non-blocking | 11 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Test-coverage regression in
avail:inner_test.go+state_test.golose ~1120 lines / 13 tests and gain 2. Much of that targeted removed code (prune-anchor persistence), but live error paths lost their pins:newInner'snon-contiguous persisted commitQCs,parent hash mismatch at block %d, andloaded %d blocks exceeds capacity %dare now unexercised, andTestSubscribeAppVotesJumpsToDataFloorwas deleted even thoughAppVotesRecv.Recv's ErrPruned branch changed in this PR (max(r.next+1, ...)->max(r.next, data.NextAppQC())). I traced that the new form still makes progress (nextAppQC > inner.firstwhenever ErrPruned fires), but that is exactly the kind of invariant the deleted test was pinning. - No test for the new node-fatal AppHash divergence check in
data.State.runPersist(AppHash divergence detected at block %v). This is the most safety-critical behaviour the PR adds — it halts the node — and nothing exercises it. Worth a test that pushes an AppQC whose AppProposal AppHash differs from the locally computed one and assertsRunreturns that error. WriteAppProposal/WriteAppQC's coverage guards (gr.Next > status.NextBlock/gr.Next > status.NextAppProposal) are never hit byblock_db_test.go:testWriteAppProposalOrderRejectedandtestWriteAppQCOrderRejectedonly reach the contiguity branch. Adding a case that writes an AppProposal for a QC whose blocks are not yet written would pin the documented write order.sei-tendermint/internal/p2p/giga_router_common.go:358— the comment "BuildDataState caps recovery at BlockDB's durable block tip, so a crash after app.Commit but before the BlockDB flush resumes by syncing the missing suffix. If retention instead passed the app tip, GlobalBlock returns ErrPruned here" no longer describes the code:Config.LastExecutedBlockis gone andBuildDataStateno longer readsApp.Info().LastBlockHeight. Recovery now starts from the BlockDB AppQC anchor, and the oldtypes.ErrNotFoundguard for an app tip beyond the crash window (TestRecoveryRejectsAppTipBeyondCrashWindow) is deleted. The new invariant that makes this safe —PushAppHashblocks on the AppProposal fsync, so the app tip can lead BlockDB by at most one CommitQC range, and the pre-loopPushAppHash(last, ...)covers that range — deserves to be written down where the old one was.- Retention semantics changed observably:
data.State.PruneBefore(n)is now clamped to the AppQC tip (min(n, status.First)), so it is a no-op until the first AppQC lands and thereafter trails it.TestPruningDiscardswas inverted fromrequire.ErrorIs(ErrPruned)torequire.NoErrorto match. That is intentional per the new BlockDB contract, but it means the app'sRetainHeightno longer bounds BlockDB growth on a node that is not receiving AppQCs — worth calling out in the PR description for operators. TestLittblockRefusesToOpenWithStrandedBlocksdroppedrequire.ErrorContains(t, err, "no surviving QC")and now accepts any error, so it no longer distinguishes the corruption guard from an unrelated open failure.recoverWatermark's new message is "corrupt store: no QC in non-empty store" — asserting on that would keep the test honest.PushAppHashnow blocks on a BlockDB flush of the AppProposal before returning (theCRITICALcomment atdata/state.go:624), putting an fsync on the block-execution hot path once per CommitQC range. The TODO acknowledges this as temporary; flagging so it is not forgotten, sinceexecuteBlockcalls it synchronously afterapp.Commit.avail.State.PushVote's second wait (h.BlockNumber() < q.first+BlocksPerLane) is also unbounded on an attacker-supplied block number. That shape predates this PR (it waspersistedBlockStart+BlocksPerLane), so not a regression — but with the epoch guard above it now also blocking rather than erroring,PushVotehas no bounded rejection path left at all.- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review merges only my findings with Codex's. - Process note: I reviewed statically —
go build/go testwere not runnable in this environment, so none of the findings above are backed by a failing build or test run. - 10 suggestion(s)/nit(s) flagged inline on specific lines.
| gr.First, status.NextAppProposal, types.ErrAppProposalNonContiguous) | ||
| } | ||
| if gr.Next > status.NextBlock { | ||
| return fmt.Errorf("AppProposal [%d,%d) is not covered by written blocks: %w", gr.First, gr.Next, types.ErrAppProposalMissingQC) |
There was a problem hiding this comment.
[suggestion] Two issues here, and the same two in memblock/mem_block_db.go:119.
-
The interface doc promises "A matching CommitQC must already be written: the CommitQC covering GlobalRange.First must have the same GlobalRange", and
types.BlockDB's type doc says the ordering contract is "enforced (not merely expected)". Neither implementation checks range equality — onlyNextAppProposal == gr.Firstandgr.Next <= NextBlock. An AppProposal spanning two CommitQCs is accepted and persisted, and thendata.State.insertAppProposalrejects it on the next open (appProposal.Verifyrequires exactGlobalRangeequality), so the store is written into a state it cannot be recovered from. Production callers always build proposals from a single QC'sProposal(), so this isn't reachable today — which is precisely why the guard belongs at this choke point rather than in the caller.WriteAppQChas the mirror gap against the persisted AppProposal. -
This error is misclassified: the failure is "blocks not written yet", but it wraps
ErrAppProposalMissingQC, which the errors.go doc defines as "no previously written CommitQC exactly matches the AppProposal's GlobalRange". A caller doingerrors.Iscannot tell the two apart. Either add a distinct sentinel or reword the existing one to cover both.
| } | ||
| } | ||
| return true, nil | ||
| if i.roads.Len() == 0 { |
There was a problem hiding this comment.
[suggestion] This contradicts the comment 20 lines below in newInner: "anchor.CommitQC is NOT stored in avail.State. We need it to get persisted before we update persistedCommitQC." On the newInner path roads is freshly constructed, so Len() == 0 always holds here and persistedCommitQC is set to anchor.CommitQC — a QC that is not in the avail CommitQC WAL and never will be (collectPersistBatch derives its cursor from persistedCommitQC, so it starts at idx+1).
I believe the behaviour is actually sound — the anchor is durable in BlockDB, which is a stronger guarantee than the avail WAL — and a later restart stays contiguous because prune re-fast-forwards roads.first to idx+1. But one of the two has to change: either drop the store here and let markCommitQCsPersisted publish it, or fix the comment to say the anchor's BlockDB durability is what licenses publishing it. As written a reader can't tell which is intended.
| // LoadNext returns the road index of the first CommitQC that has not been | ||
| // persisted (exclusive upper bound of what's on disk). | ||
| func (cp *CommitQCPersister) LoadNext() types.RoadIndex { | ||
| func (cp *CommitQCPersister) Next() types.RoadIndex { |
There was a problem hiding this comment.
[nit] Doc comment still starts "LoadNext returns..." after the rename to Next.
| commitQCs []*types.CommitQC, | ||
| afterEach utils.Option[func(*types.CommitQC)], | ||
| ) error { | ||
| // afterEach, when present, is called after each successful append. It is |
There was a problem hiding this comment.
[nit] afterEach was removed from the signature in this PR, so these two lines document a parameter that no longer exists. The four bullets above (lines 145-151) likewise describe anchor set / anchor empty combinations, but the parameter is now an unconditional deleteBefore types.RoadIndex. Same for NewCommitQCPersister's doc: "The caller MUST use PruneAndPersist with the prune CommitQC in Anchor to re-establish the cursor and re-persist the anchor's CommitQC" — deleteBefore no longer re-persists anything.
Per AGENTS.md the doc comment is where the why lives, so these are the comments most worth keeping accurate.
| } | ||
|
|
||
| // MaybePruneAndPersistLane optionally truncates the lane's WAL and/or appends | ||
| // PruneAndPersist optionally truncates the lane's WAL and/or appends |
There was a problem hiding this comment.
[nit] Same as commitqcs.go: the bullets below still describe an anchor utils.Option[*types.CommitQC] parameter and an afterEach callback, both replaced by a plain first types.BlockNumber that is now applied unconditionally (truncateForAnchor(lane, first) on every call rather than only when an anchor was present). laneWAL.persist also still calls the method truncateForAnchor although there is no longer an anchor.
| // ErrBlockGap is returned when the persisted blocks are not contiguous, | ||
| // surfaced by BlockDBIterator.Next during a scan. WriteBlock rejects gapped | ||
| // writes, so a gap on disk indicates store corruption. | ||
| // ErrBlockGap is returned when persisted blocks are not contiguous. WriteBlock |
There was a problem hiding this comment.
[nit] With BlockDBIterator gone, ErrBlockGap is no longer returned or checked anywhere in the tree — the only remaining references are this declaration and its doc. unused won't catch an exported var, so it will linger. Either delete it or note what is expected to start returning it.
| suffix, err := db.ReadSuffix() | ||
| if err != nil { | ||
| return 0, 0, fmt.Errorf("failed to open ledger iterator: %w", err) | ||
| return 0, 0, fmt.Errorf("failed to read suffix ledger data: %w", err) |
There was a problem hiding this comment.
[nit] countExistingState now returns the size of the recovery suffix, not the whole ledger, but the doc comment above still says "scans the ledger to count the persisted blocks and QCs, exercising the replay path at startup". Since blockCount/qcCount are reported to the operator at line 106, the numbers now mean something different from what the benchmark previously printed — worth either renaming to countSuffixState or updating the comment so the output isn't misread.
pompon0
added
the
ai: skip-review
4 participants
Redesign of the AppQC flow: