From d4ce8468f36e7c4f67847cfb7f0fb34660bffb57 Mon Sep 17 00:00:00 2001 From: ping-ke Date: Mon, 13 Jul 2026 11:12:21 +0800 Subject: [PATCH 1/4] docs(L1): add minor block commit-status fix design & review doc Design/review doc for the goquarkchain minor-block commit-status fix, split into 3 stacked PRs (head locking / commit-status semantics / sidechain body anchor). Chinese and English versions. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../minor-commit-status-review.en.md | 288 ++++++++++++++++++ .../minor-commit-status-review.md | 224 ++++++++++++++ 2 files changed, 512 insertions(+) create mode 100644 L1/quarkchain-fix/minor-commit-status-review.en.md create mode 100644 L1/quarkchain-fix/minor-commit-status-review.md diff --git a/L1/quarkchain-fix/minor-commit-status-review.en.md b/L1/quarkchain-fix/minor-commit-status-review.en.md new file mode 100644 index 0000000..a4d86b5 --- /dev/null +++ b/L1/quarkchain-fix/minor-commit-status-review.en.md @@ -0,0 +1,288 @@ +# Minor Block Commit Status — Design & Review Doc + +> In one line: split a minor block's "data is local" state from its "commit is +> done" state, and tighten the concurrency between head rewind / root reorg / +> commit along the way. The implementation is split into 3 stacked PRs. + +This doc gives the whole picture of the change set: first the problem and the +overall approach, then what each of the 3 PRs does, where to focus your review, +and how tests cover it. Read it in the order **this doc → PR1 → PR2 → PR3**: +the PRs have a strict dependency chain (concurrency first, then semantics, then +the feature), explained below. + +## Background: a block actually has several independent "states" + +Building the model from scratch. On a local node, a minor block can be in any +combination of these independent states: + +1. **body present**: the block's content is written to the local DB (you can read the block). +2. **state present**: the block's state trie is local, so you can execute on top of it / query balances. +3. **broadcast / report done**: the x-shard tx list has been broadcast to neighbor shards, and the block header has been reported to master. +4. **commit marker present**: an explicit marker meaning "this block's shard-side commit flow (step 3) is fully done." + +The core problem: **the old code conflated 1 and 4.** + +In the old logic `HasBlock` was effectively equivalent to "commit marker present", +because `WriteBlockWithState` wrote the marker right after writing body/state. +So "a body was just persisted locally" was treated as "this block's outward +commit is done." + +That looks fine under normal sequential execution, but it breaks on these paths: + +- **Crash / restart recovery**: can't tell whether a block is "committed" or "just has a body on disk, broadcast/report not sent yet." +- **Head rewind (`SetHead`)**: rewind deletes the body, but if another goroutine is writing the marker for the same block at that moment, you can end up with a **marker pointing at an already-deleted body** — a state that can never be consistent. +- **Root reorg**: `currentBlock` / `rootTip` / `currentEvmState` are published in several steps, so a reader can observe mutually inconsistent combinations mid-flight. +- **Sidechain replay / sync retry**: can't distinguish "should skip" / "should replay" / "should re-commit." + +## Overall approach + +Three steps, one per PR, and the order matters: + +1. **Fix the concurrency first** (PR1), via two means: (a) add / widen locking on + rewind / root reorg / import so paths that were not mutually exclusive now + serialize under the same locks; (b) publish currentBlock / rootTip / + currentEvmState / canonical index inside one critical section, so readers + never see a half-updated intermediate state. +2. **Then split the state semantics** (PR2). Make "body present" ≠ "committed"; + the marker is written explicitly by the shard layer only after broadcast / + report succeed. +3. **Finally build the feature on the new semantics** (PR3). A body doesn't mean + committed, but it can still serve as a local sync anchor — avoiding re-download + of history bodies already in the DB — and recover history blocks that "have a + body but miss the marker." + +After the split the semantics become three clear predicates: + +| Predicate | Meaning | Typical use | +| --- | --- | --- | +| `HasBlock(hash)` | body present | whether the body needs to be persisted again | +| `HasCommittedBlock(hash)` | body **and** commit marker both present | sync deciding "is this block really done" | +| `HasBodyWithoutState(hash)` | body present but state not local | anchor for a pruned sidechain | + +## How the code is split + +All three PRs are carved out of the original large branch, stacked +(PR3 depends on PR2, PR2 depends on PR1). + +### PR 1 · `fix/minor-chain-head-locking` + +**Theme**: serialize minor head rewind and root reorg, and make head publication +atomic. This PR does not touch commit marker semantics. + +Commits: `f1f0f50` (core-layer locks and head publication), `f40beb8` +(shard-layer `AddRootBlock` serialization). + +Files: `core/minorblockchain.go`, `core/minorblockchain_addon.go`, +`cluster/shard/api_backend.go`. + +Key points: + +- Make `SetHead` / `Reset` / `ResetWithGenesisBlock` hold both `chainmu` + `mu`, + so rewind and import are mutually exclusive (old code: rewind held only + `chainmu`, import held only `mu` — they were not mutually exclusive). +- When taking both locks, follow the existing order **`s.mu (shard) → chainmu → + mu (core)`**, consistent with the insert pipeline (`InsertChainForDeposits` + holds `chainmu`, `WriteBlockWithState` takes `mu`), to avoid introducing a new deadlock. +- `setHead` now **validates the target state is usable first, then deletes bodies + above the target, and finally publishes** `currentEvmState` + DB head hash + + `currentBlock` in one shot. I.e. the irreversible delete happens only after the + state check passes: either it fails and returns an error with no body deleted + (safe to retry), or it deletes all the way through. The old code did "delete + bodies first, then discover the target state is missing," which could only fall + all the way back to the genesis block. +- Root reorg keeps the target block in a local variable and publishes the head in + one shot only after canonical index / rootTip / confirmedHeaderTip / EVM state are all ready. +- The no-head-publish path of root reorg only rewrites canonical hashes and + **does not delete sidechain bodies**, so a later root reorg can switch back. +- `ShardBackend.AddRootBlock` takes `s.mu`, serializing root block handling with + the shard-side broadcast / report of `AddMinorBlock` / `AddBlockListForSync`. + +Reviewer focus: + +- Is the lock order always `s.mu → chainmu → mu`? Any path acquiring in reverse that could deadlock? +- Are `currentBlock` / `currentEvmState` / rootTip / canonical index published within the same critical section? +- On a rewind error, is it guaranteed that "bodies are never deleted before returning the error"? + +### PR 2 · `fix/minor-block-commit-status` + +**Theme**: split "body present" from "committed." This is the semantic core of the change set. + +Commits: `3ea16ad` (semantic core + call sites across layers + race tests), +`79d7f18` (commit-recovery invariant tests). + +Files: core (`minorblockchain*.go`, `rootblockchain.go`), shard (`api_backend.go`, +`shard.go`), slave (`api_backend.go`), sync (`minor_task.go`, `root_task.go`, +`sync.go`), and their tests. + +Key points: + +- `WriteBlockWithState` / `insertSidechain` **no longer auto-write the commit + marker**; the marker is written explicitly by the shard layer only after + x-shard broadcast + master header report succeed. +- `HasBlock` is narrowed to "body present"; a new `HasCommittedBlock` means body + marker. +- `CommitMinorBlockByHash` checks under `m.mu` whether the body still exists + before writing the marker; if the body was deleted by a rewind, it returns + `false`. This is the last gate for the "marker ⟹ body" invariant. +- sync / slave decide "is it done" via `HasCommittedBlock`, no longer mistaking a body-only block for committed. +- A block with body/state but missing marker can be retried: re-send + broadcast / report, then write the marker. This relies on **force insert** (next point). +- **force insert**: shard-layer imports (`AddMinorBlock` / `AddBlockListForSync`) + all call `InsertChainForDepositsWithBlocks` with `force=true`. Its purpose is to + make the validator **skip the `ErrKnownBlock` short-circuit** — under the old + behavior a block that "already has body/state" was rejected as a known block, + so a block missing its marker could never get one written. With `force=true`: + if a block already has body+state, it is **re-executed only to recompute the + x-shard list** (head untouched, body not rewritten), then handed to the shard + layer to re-broadcast / re-report and write the marker. This is the mechanism + behind both "retry recovery" and how sidechain replay obtains the x-shard list. +- Remove the deprecated `BLOCK_COMMITTING` placeholder state. + +Reviewer focus: + +- For every "skip a block we already have" check, confirm one by one whether it should use `HasBlock` or `HasCommittedBlock` (this is the easiest place to get wrong). +- Before writing the marker, are the x-shard broadcast and master header report actually complete? +- On a marker-write failure, is it guaranteed to never leave a "marker pointing at a missing body"? +- Does `DeleteMinorBlock` delete the body and marker in the same batch (so the rollback side also holds the invariant)? +- In the `force=true` "block already has body+state" branch, does it **only + recompute the x-shard list, without changing the head / rewriting the body** — + so force can't overwrite already-correct data or wrongly advance the head? + +### PR 3 · `fix/minor-sidechain-body-anchor` + +**Theme**: let sidechain / pruned bodies serve as a sync anchor, and recover +history blocks missing their marker. Built on PR2's new semantics. + +> A pruned body is a block whose body is still present but whose state trie has +> been pruned away — i.e. the `HasBodyWithoutState` case above. + +Commits: `d887d70`, `fad5e1b` (comment clarification of sidechain replay retry behavior). + +Files: `cluster/shard/api_backend.go`, `cluster/sync/minor_task.go`, +`cluster/sync/sync.go`, `core/minorblockchain.go`, `core/rootblockchain.go`, and their tests. + +Background: what insert sidechain / sidechain replay is (following go-ethereum's mechanism) + +- When importing a batch, if the fork point is too old and the parent's + **state trie has been pruned** (`ErrPrunedAncestor`), it enters sidechain + import: those blocks **only write the body — no state, no canonical index, no + commit marker**. Since this fork may never be adopted, just storing the body + is enough and cheap. +- Only when this fork's **cumulative height / difficulty exceeds the current main + chain**, and we actually switch to it, do we start from "the nearest ancestor + that still has state," **replay** (re-execute) each body-only block to produce + state, then reorg the main chain onto this fork. Only this step produces the + x-shard list, and only then do broadcast / report / write-marker run. +- So for this change set: **body present = usable as a sync anchor (no re-download); + but body present ≠ committed**. During replay + reorg, one pass may replay + multiple history blocks, and each must complete broadcast / report / write-marker + in order to count as committed. + +Key points: + +- sync `findAncestor` can treat a body-only / body-without-state block as a local + anchor, avoiding re-download of history bodies already in the DB; returns an + explicit error when no common ancestor is found (no more nil-panic). +- Sidechain replay can return the x-shard list of **multiple history blocks**, + broadcast / report / commit each in chain order. +- `AddBlockListForSync` (the batch sync-import entry) now **imports by "contiguous + segment"** instead of the old block-by-block import: + - Iterate the batch, first skipping already-committed blocks and duplicate blocks within the batch (dedup); + - accumulate blocks that are number-contiguous and parent-linked into a segment; + once a gap is hit **within the same batch** (number not contiguous or parent + mismatch), import + commit the accumulated segment first, then keep + accumulating the next segment after the break, still within the same batch + (gap split). Here "commit" is the full PR2 flow (insert body/state → broadcast + x-shard → report master → write commit marker), with the marker as the final + step; "next segment" means the contiguous blocks after the break within the + same batch, still inside the same `AddBlockListForSync` call — not a new sync round; + - why: sidechain replay needs **a contiguous run of blocks** to replay from the + ancestor that has state; block-by-block insert cannot trigger a correct replay. +- When `NewMinorBlock` hits a parent that "has body/state but misses the marker," + it first tries to commit the uncommitted ancestors, then handles the current + block; when the parent is a body-only sidechain, it persists the child body + directly and lets a later replay rebuild the state. +- The commit marker's meaning is unchanged: the marker is written only after + broadcast / report are sent; if the marker write fails the block stays + uncommitted and is re-sent by sync retry. + +Reviewer focus: + +- Does the body-only anchor **only widen the "we already have this data locally" check**, never mistaking a block for "commit done"? +- Is the commit order of multi-block replay consistent with chain order? +- After a replay / commit failure, does it rely on "staying uncommitted" so sync retries later, rather than leaving a half-committed state? + +## Suggested review order + +Read **PR1 → PR2 → PR3**; this order matches the dependencies, and reordering loses context: + +- PR1 first makes concurrency and head publication consistent (so splitting semantics later won't hit concurrency windows). +- PR2 then changes commit marker semantics (relying on PR1's locks to hold the invariant). +- PR3 finally uses the new semantics for the sidechain body anchor and retry recovery. + +## Test plan + +Idea: every PR's core risk has a test backing it; the most critical "marker ⟹ +body" invariant is pressed directly with race tests. + +Below is a "risk → test" mapping; reviewers can check each by test name. + +**core layer** (PR1 / PR2) + +- Don't write marker when body is absent; write only when body is present → + `TestCommitMinorBlockByHash_SkipsWhenBodyAbsent` / `TestCommitMinorBlockByHash_WritesWhenBodyPresent` +- `HasBlock` means body present only (returns false when marker present but body absent) → `TestHasBlock_FalseWhenBodyAbsentButMarkerPresent` +- `InsertChain` does not auto-write the marker after writing body/state → `TestInsertChain_BodyWrittenWithoutCommitMarker` +- After head rewind / genesis reset, `currentBlock` and `currentEvmState` are + consistent → reuse existing reorg tests (`TestMinorLargeReorgTrieGC`, + `TestMinorChainTxReorgs`, etc.) + review; no dedicated test added. + +**shard layer** (PR2 / PR3) + +- `AddMinorBlock` / `AddBlockListForSync` explicitly write the marker on success → + `TestAddMinorBlock_CommitMarkerPresentAfterBlock` / `TestAddBlockListForSync_CommitMarkerPresentAfterSync` +- A block with body/state but missing marker can be recovered on retry → + `TestAddBlockListForSync_RecoversXShardListOnRetry` / `TestNewMinorBlock_RecoversUncommittedBodyOnRetry` (both single-block retry) +- Skip an already-committed block in the batch and keep processing the rest → `TestAddBlockListForSync_ContinuesPastKnownBlock` +- **marker ⟹ body (invariant under concurrency)** → `TestAddMinorBlockMarkerBodyConsistencyUnderRace` / + `TestAddBlockListForSyncMarkerBodyConsistencyUnderRace` (`-race`, concurrent with `SetHead`) +- TODO: **multi-block commit in chain order** and **gap split / in-batch dedup** + have no dedicated tests yet; currently covered indirectly by + `ContinuesPastKnownBlock` + single-block retry above — dedicated cases recommended. + +**sync layer** (PR2 / PR3) + +- minor sync finds the ancestor via committed / body-only anchor → `TestMinorFindAncestorUsesCommittedOrBodyWithoutState` +- root / minor task mocks explicitly distinguish `HasBlock` from `HasCommittedBlock` (test scaffolding, not a standalone assertion). + +Suggested local runs: + +```bash +go test ./core/... ./cluster/sync/... ./cluster/shard/... +go test -race ./cluster/shard -run 'TestAddBlockListForSyncMarkerBodyConsistencyUnderRace|TestAddMinorBlockMarkerBodyConsistencyUnderRace' +``` + +## Non-goals + +- **Read-side atomicity**: PR1 only converged the write-side publication order. + On the read side, `CurrentBlock()` / `GetBlockByNumber()` don't take `m.mu`, so + during the window where a root reorg has rewritten the canonical index but the + new head isn't published yet, they can still read an inconsistent snapshot of + "old head + new canonical index." Making read tip / state observe the same lock + is follow-up work, tracked in [#693](https://github.com/QuarkChain/goquarkchain/issues/693). +- **No strong "succeed-once" guarantee; converge by retry**: we don't change the + resend/dedup mechanism of x-shard broadcast / master report (the same block may + be resent multiple times; the receiver dedups by block hash and won't process + it twice), nor do we wrap history recovery in a single transaction; on failure a + block stays uncommitted and is filled in on a later retry via the existing + "dedup by block hash + sync retry." + +## PR overview + +- PR 1: `fix/minor-chain-head-locking` (commits `f1f0f50`, `f40beb8`) — serialize concurrency, atomic head publication +- PR 2: `fix/minor-block-commit-status` (commits `3ea16ad`, `79d7f18`) — split "body present" from "committed" +- PR 3: `fix/minor-sidechain-body-anchor` (commits `d887d70`, `fad5e1b`) — sidechain body anchor and retry recovery + +The consensus we want after reading: the difference between the three states +(body / state / commit marker); why locking must come first, then the marker +semantics split, then the sidechain body anchor; and what to focus on in each PR +and which risks the tests cover. diff --git a/L1/quarkchain-fix/minor-commit-status-review.md b/L1/quarkchain-fix/minor-commit-status-review.md new file mode 100644 index 0000000..9b7d96a --- /dev/null +++ b/L1/quarkchain-fix/minor-commit-status-review.md @@ -0,0 +1,224 @@ +# Minor Block Commit Status —— Design & Review Doc + +> 一句话：把 minor block 的「本地有数据」和「对外提交完成」两个状态拆开， +> 顺带收紧 head rewind / root reorg / commit 之间的并发关系。实现拆成 3 个 stacked PR。 + +这份文档给出整组改动的 whole picture：先讲清楚问题、整体思路，再分别说明 3 个 PR 各自在做什么、 +每个 PR 该重点看哪里、测试怎么覆盖。建议按 **本文档 → PR1 → PR2 → PR3** 的顺序看： +三个 PR 有严格的依赖关系（先并发，再语义，最后功能），下面会说明为什么。 + +## 背景：一个块其实有好几种「状态」 + +给完全没有上下文的人先建立模型。一个 minor block 在本地节点上，可能处于以下几种彼此独立的状态： + +1. **body 存在**：block 的内容已经写进本地 DB（可以读到这个块）。 +2. **state 存在**：这个块对应的 state trie 在本地，可以在它之上执行交易 / 查询余额。 +3. **对外广播 / 上报完成**：x-shard tx list 已经广播给相邻 shard，block header 已经上报给 master。 +4. **commit marker 存在**：一个显式标记，代表「这个块的 shard 侧提交流程（第 3 步）已经彻底完成」。 + +关键问题：**旧代码把 1 和 4 混成了一件事。** + +旧逻辑里 `HasBlock` 实际等价于「commit marker 存在」，因为 `WriteBlockWithState` 一写完 body/state 就顺手写了 marker。 +于是「本地刚落盘一个 body」被当成了「这个块已经对外提交完成」。 + +这在正常顺序执行时看不出问题，但在这几条路径上就会出事： + +- **崩溃 / 重启恢复**：分不清一个块是「提交完了」还是「只是 body 落了盘、广播 / 上报还没发」。 +- **head rewind（`SetHead`）**：回滚会删掉 body，但如果此时另一个 goroutine 正在给同一个块写 marker， + 就可能出现 **marker 指向一个已经被删掉的 body**——一个永远无法自洽的状态。 +- **root reorg**：`currentBlock` / `rootTip` / `currentEvmState` 分几步发布，中间时刻读者会看到互相不一致的组合。 +- **sidechain replay / sync retry**：无法区分「该跳过」「该重放」「该重新提交」。 + +## 整体思路 + +三步走，对应三个 PR，顺序不能乱： + +1. **先解决并发问题**（PR1），两个手段：一是给 rewind / root reorg / import 增加 / 扩大加锁范围， + 让原来彼此不互斥的路径都在同一组锁下串行；二是把 currentBlock / rootTip / currentEvmState / + canonical index 放进同一个临界区一起发布，避免读者看到半更新的中间状态。 +2. **再把状态语义拆开**（PR2）。让 body 存在 ≠ 已提交，marker 由 shard 层在广播 / 上报成功后显式写入。 +3. **最后用新语义做功能**（PR3）。body 虽然不代表已提交，但仍可作为 sync 的本地 anchor， + 避免重复下载已有历史 body，并恢复那些「有 body 缺 marker」的历史块。 + +拆开后语义变成三个清晰的谓词： + +| 谓词 | 含义 | 典型用途 | +| --- | --- | --- | +| `HasBlock(hash)` | body 存在 | 是否需要重新落盘 body | +| `HasCommittedBlock(hash)` | body **且** commit marker 都存在 | sync 判断「这个块是否真的完成了」 | +| `HasBodyWithoutState(hash)` | body 存在但 state 不在本地 | 作为 pruned sidechain 的 anchor | + +## 代码怎么拆 + +三个 PR 都从原来的大分支拆出来，彼此 stacked（PR3 依赖 PR2，PR2 依赖 PR1）。 + +### PR 1 · `fix/minor-chain-head-locking` + +**主题**：串行化 minor head rewind 和 root reorg，保证 head 原子发布。本 PR 不碰 commit marker 语义。 + +对应 commit：`f1f0f50`（core 层锁与 head 发布）、`f40beb8`（shard 层 `AddRootBlock` 串行化）。 + +改动范围：`core/minorblockchain.go`、`core/minorblockchain_addon.go`、`cluster/shard/api_backend.go`。 + +关键点： + +- 让 `SetHead` / `Reset` / `ResetWithGenesisBlock` 都同时持有 `chainmu` + `mu`，使 rewind 与 import 互斥 + （原来 rewind 只拿 `chainmu`、import 只拿 `mu`，两者并不互斥）。 +- 同时拿两把锁时沿用既有的 **`s.mu (shard) → chainmu → mu (core)`** 顺序，和 insert pipeline 一致 + （`InsertChainForDeposits` 持 `chainmu`，`WriteBlockWithState` 取 `mu`），避免引入新的死锁。 +- `setHead` 改为**先校验目标 state 可用，再删除高于目标的 body，最后一次性发布** `currentEvmState` + DB head hash + `currentBlock`； + 也就是把不可逆的删除放到状态校验通过之后：要么校验失败、一个 body 都没删就返回错误（可安全重试）， + 要么删到底。旧代码是「先删 body，删完才发现目标 state 不在」，只能一路退回创世块。 +- root reorg 把目标块保持为局部变量，等 canonical index / rootTip / confirmedHeaderTip / EVM state 都就绪后再一次性发布 head。 +- root reorg 的 no-head-publish 路径只重写 canonical hash，**不删 sidechain body**，方便之后 root reorg 再切回。 +- `ShardBackend.AddRootBlock` 加 `s.mu`，让 root block 处理与 `AddMinorBlock` / `AddBlockListForSync` 的 shard 侧广播 / 上报串行。 + +Reviewer 重点： + +- 锁顺序是否始终是 `s.mu → chainmu → mu`，有没有反向获取导致死锁的路径。 +- `currentBlock` / `currentEvmState` / rootTip / canonical index 是否在同一临界区内一起发布。 +- rewind 出错时，是否保证「不会先删 body 再返回错误」。 + +### PR 2 · `fix/minor-block-commit-status` + +**主题**：拆分「body 存在」和「已提交」两个状态。这是整组改动的语义核心。 + +对应 commit：`3ea16ad`（语义主体 + 各层调用点 + race test）、`79d7f18`（commit recovery 不变量测试）。 + +改动范围：core（`minorblockchain*.go`、`rootblockchain.go`）、shard（`api_backend.go`、`shard.go`）、 +slave（`api_backend.go`）、sync（`minor_task.go`、`root_task.go`、`sync.go`）及对应测试。 + +关键点： + +- `WriteBlockWithState` / `insertSidechain` **不再自动写 commit marker**；marker 改由只在 shard 层在 + x-shard broadcast + master header 上报成功之后显式写入。 +- `HasBlock` 语义收窄为「body 存在」；新增 `HasCommittedBlock`（body + marker）。 +- `CommitMinorBlockByHash` 在 `m.mu` 下先检查 body 是否还在，再写 marker；body 已被 rewind 删除则返回 `false`。 + 这是「marker ⟹ body」不变量的最后一道闸。 +- sync / slave 判断「是否已完成」统一改用 `HasCommittedBlock`，不再把只有 body 的块误当已提交。 +- 有 body/state 但缺 marker 的块可被 retry：重发广播 / 上报后补写 marker。这依赖 **force insert**（下条）。 +- **force insert**：shard 层导入（`AddMinorBlock` / `AddBlockListForSync`）统一用 `force=true` 调 + `InsertChainForDepositsWithBlocks`。作用是让 validator **跳过 `ErrKnownBlock` 短路**——旧行为下一个 + 「已有 body/state」的块会被当成 known block 直接拒掉，缺 marker 的块就永远补不上 marker。`force=true` + 时改为：block 已有 body+state 就**只重新执行一遍、把 x-shard list 重算出来**（不动 head、不重写 body）， + 再交给 shard 层重发广播 / 上报、补写 marker。这就是「retry 恢复」和 sidechain replay 能拿到 x-shard list 的机制。 +- 删除已经废弃的 `BLOCK_COMMITTING` 占位状态。 + +Reviewer 重点： + +- 所有「跳过已拥有块」的判断，逐个确认该用 `HasBlock` 还是 `HasCommittedBlock`（这是最容易看错的地方）。 +- marker 写入前，x-shard 广播与 master header 上报是否确实已经完成。 +- marker 写失败时，是否绝不会留下「marker 指向缺失 body」。 +- `DeleteMinorBlock` 是否把 body 和 marker 放在同一个 batch 里删（保证回滚侧也满足不变量）。 +- `force=true` 下「块已有 body+state」的分支是否**只重算 x-shard list、不改 head / 不重写 body**， + 不会因为 force 而把已有正确数据覆盖或误推进 head。 + +### PR 3 · `fix/minor-sidechain-body-anchor` + +**主题**：让 sidechain / pruned body 作为 sync anchor，并恢复缺 marker 的历史块。建立在 PR2 的新语义之上。 + +> pruned body 指 body 还在、但对应 state trie 已被裁剪掉的块，即前面的 `HasBodyWithoutState`。 + +对应 commit：`d887d70`、`fad5e1b`（sidechain replay retry 行为的注释澄清）。 + +改动范围：`cluster/shard/api_backend.go`、`cluster/sync/minor_task.go`、`cluster/sync/sync.go`、 +`core/minorblockchain.go`、`core/rootblockchain.go` 及对应测试。 + +背景：什么是 insert sidechain / sidechain replay（沿用 go-ethereum 的机制） + +- 导入一批块时，如果发现分叉点太老、父块的 **state trie 已经被裁剪掉**（`ErrPrunedAncestor`）， + 就进入 sidechain 导入：这些块**只写 body，不算 state、不写 canonical index、不写 commit marker**。 + 因为这条分叉可能根本不会被采纳，先把 body 存下来即可，代价很低。 +- 只有当这条分叉的**累积高度 / 难度超过当前主链**、真的要切过去时，才从「最近一个还留着 state 的祖先」 + 开始，把中间这些只有 body 的块**逐个 replay**（重新执行）出 state，然后 reorg 把主链切到这条分叉。 + 这一步才产生 x-shard list，也才需要走广播 / 上报 / 写 marker。 +- 所以对本组改动来说：**body 在 = 可以作为 sync anchor（不用重下）；但 body 在 ≠ committed**。 + replay + reorg 时，一次可能重放出多个历史块，它们各自都要按顺序完成广播 / 上报 / 写 marker 才算 committed。 + +关键点： + +- sync `findAncestor` 可以把 body-only / body-without-state 的块当作本地 anchor， + 避免重复下载已经在 DB 里的历史 body；找不到共同祖先时返回显式错误（不再 nil-panic）。 +- sidechain replay 可返回**多个历史块**的 x-shard list，按 chain order 逐个 broadcast / report / commit。 +- `AddBlockListForSync`（sync 批量导入入口）改为**按「连续段」导入**，而不是旧代码的逐块导入： + - 遍历 batch，把编号连续、parent 相接的块攒成一个 segment；一旦在**同一批里**遇到断档（编号不连续或 parent 对不上）， + 就把已攒的 segment 先导入并提交，再从断点之后继续攒同一批里的下一段（gap split）。这里的「commit」是 + PR2 那套完整流程（insert 落 body/state → 广播 x-shard → 上报 master → 写 commit marker）， + marker 是收尾那一步；「下一段」指同一批里断点之后的连续块，仍在同一次 `AddBlockListForSync` 调用内， + 不是新的 sync round； + - 这样做的原因：sidechain replay 需要**一段连续的块**才能从有 state 的祖先逐块重放； + 逐块单独 insert 无法触发正确的 replay。 +- `NewMinorBlock` 遇到 parent「有 body/state 缺 marker」时，先尝试补交未提交祖先，再处理当前块； + parent 是 body-only sidechain 时，直接把子块 body 落盘，等后续 replay 重建 state。 +- commit marker 含义保持不变：只有广播 / 上报发完才写 marker；marker 写失败则块保持 uncommitted，由 sync retry 重发。 + +Reviewer 重点： + +- body-only anchor 是否**只扩大「本地已有数据」的判断**，绝不把块误认为「已完成提交」。 +- multi-block replay 的提交顺序是否与 chain order 一致。 +- replay / commit 失败后，是否依赖「保持 uncommitted」让 sync 后续自然重试，而不是留下半提交状态。 + +## 建议的 review 顺序 + +按 **PR1 → PR2 → PR3** 看，这个顺序对应依赖关系，打乱会缺上下文： + +- PR1 先保证并发与 head 发布一致（后面拆语义才不会踩到并发窗口）。 +- PR2 再改 commit marker 语义（依赖 PR1 的锁保证不变量）。 +- PR3 最后用新语义做 sidechain body anchor 与 retry recovery。 + +## 测试方案 + +思路：每个 PR 的核心风险都要有对应测试兜底，最关键的「marker ⟹ body」不变量用 race test 直接压。 + +下面「风险 → 测试」一一对应，reviewer 可直接按测试名核对。 + +**core 层**（PR1 / PR2） + +- body 缺失时不写 marker、body 在才写 → `TestCommitMinorBlockByHash_SkipsWhenBodyAbsent` / + `TestCommitMinorBlockByHash_WritesWhenBodyPresent` +- `HasBlock` 只代表 body 存在（有 marker 无 body 时返回 false）→ `TestHasBlock_FalseWhenBodyAbsentButMarkerPresent` +- `InsertChain` 写完 body/state 后不自动写 marker → `TestInsertChain_BodyWrittenWithoutCommitMarker` +- head rewind / genesis reset 后 `currentBlock` 与 `currentEvmState` 一致 → 复用现有 reorg 测试 + （`TestMinorLargeReorgTrieGC`、`TestMinorChainTxReorgs` 等）+ review，无新增专项测试。 + +**shard 层**（PR2 / PR3） + +- `AddMinorBlock` / `AddBlockListForSync` 成功后显式写 marker → `TestAddMinorBlock_CommitMarkerPresentAfterBlock` / + `TestAddBlockListForSync_CommitMarkerPresentAfterSync` +- 有 body/state 缺 marker 的块可 retry 恢复提交 → `TestAddBlockListForSync_RecoversXShardListOnRetry` / + `TestNewMinorBlock_RecoversUncommittedBodyOnRetry`（均为单块 retry） +- batch 里遇到已 committed 的块时跳过、继续处理后续块 → `TestAddBlockListForSync_ContinuesPastKnownBlock` +- **marker ⟹ body（并发下不变量）** → `TestAddMinorBlockMarkerBodyConsistencyUnderRace` / + `TestAddBlockListForSyncMarkerBodyConsistencyUnderRace`（`-race`，与 `SetHead` 并发） +- 待补：**多历史块按 chain order 顺序提交**、**gap split / batch 内重复去重** 目前无专项测试， + 现由上面 `ContinuesPastKnownBlock` + 单块 retry 间接覆盖，建议补独立用例。 + +**sync 层**（PR2 / PR3） + +- minor sync 用 committed / body-only anchor 找 ancestor → `TestMinorFindAncestorUsesCommittedOrBodyWithoutState` +- root / minor task mock 显式区分 `HasBlock` 与 `HasCommittedBlock`（测试支撑，非独立断言）。 + +建议本地运行： + +```bash +go test ./core/... ./cluster/sync/... ./cluster/shard/... +go test -race ./cluster/shard -run 'TestAddBlockListForSyncMarkerBodyConsistencyUnderRace|TestAddMinorBlockMarkerBodyConsistencyUnderRace' +``` + +## 非目标 + +- **读侧原子性**：PR1 只收敛了写侧发布顺序。读侧的 `CurrentBlock()` / `GetBlockByNumber()` 不走 `m.mu`， + 在 root reorg 重写 canonical 索引、还没发布新 head 的窗口里，仍可能读到「旧 head + 新 canonical 索引」的 + 不一致快照。让 read tip / state 走同一把锁是后续工作，留到 [#693](https://github.com/QuarkChain/goquarkchain/issues/693)。 +- **不追求"一次成功"，靠重试收敛**：不改 x-shard broadcast / master report 的重发去重机制 + （同一个块重发多次，接收方按 block hash 去重，不会重复处理），也不把历史 recovery 包成单个事务； + 失败后块保持 uncommitted，靠现有「按 block hash 去重 + sync retry」在后续重试里自然补上。 + +## PR 一览 + +- PR 1：`fix/minor-chain-head-locking`（commit `f1f0f50`、`f40beb8`）—— 串行化并发、head 原子发布 +- PR 2：`fix/minor-block-commit-status`（commit `3ea16ad`、`79d7f18`）—— 拆分 body 存在与已提交语义 +- PR 3：`fix/minor-sidechain-body-anchor`（commit `d887d70`、`fad5e1b`）—— sidechain body anchor 与 retry recovery + +看完希望达成的共识：body / state / commit marker 三个状态的区别，为什么必须先做 locking、 +再拆 marker 语义、最后做 sidechain body anchor，以及每个 PR 的重点和测试覆盖的风险。 From 440699eae362c77e009cca3f5ba4b34d43356668 Mon Sep 17 00:00:00 2001 From: ping-ke Date: Tue, 14 Jul 2026 01:07:12 +0800 Subject: [PATCH 2/4] resolve comments --- .../minor-commit-status-review.en.md | 434 +++++++----------- .../minor-commit-status-review.md | 276 +++++------ 2 files changed, 304 insertions(+), 406 deletions(-) diff --git a/L1/quarkchain-fix/minor-commit-status-review.en.md b/L1/quarkchain-fix/minor-commit-status-review.en.md index a4d86b5..81dcc97 100644 --- a/L1/quarkchain-fix/minor-commit-status-review.en.md +++ b/L1/quarkchain-fix/minor-commit-status-review.en.md @@ -1,288 +1,208 @@ -# Minor Block Commit Status — Design & Review Doc +# Minor Block Commit Status - Review Doc -> In one line: split a minor block's "data is local" state from its "commit is -> done" state, and tighten the concurrency between head rewind / root reorg / -> commit along the way. The implementation is split into 3 stacked PRs. +> This set of PRs fixes one main issue: "the minor block body is in the DB" should not mean +> "broadcast / report is already finished". It also serializes head rewind, root reorg, and commit, +> because those paths can otherwise step on each other. -This doc gives the whole picture of the change set: first the problem and the -overall approach, then what each of the 3 PRs does, where to focus your review, -and how tests cover it. Read it in the order **this doc → PR1 → PR2 → PR3**: -the PRs have a strict dependency chain (concurrency first, then semantics, then -the feature), explained below. +Please read in this order: **this doc -> PR1 -> PR2 -> PR3**. -## Background: a block actually has several independent "states" +## Background -Building the model from scratch. On a local node, a minor block can be in any -combination of these independent states: +On a local node, a minor block is not just "present / absent": -1. **body present**: the block's content is written to the local DB (you can read the block). -2. **state present**: the block's state trie is local, so you can execute on top of it / query balances. -3. **broadcast / report done**: the x-shard tx list has been broadcast to neighbor shards, and the block header has been reported to master. -4. **commit marker present**: an explicit marker meaning "this block's shard-side commit flow (step 3) is fully done." +1. **body present**: the block can be read from the DB. +2. **state present**: the local node still has this block's state trie in DB or memory, so it can execute the next block or query state. +3. **broadcast / report done**: the x-shard tx list has been broadcast, and the minor header has been reported to master. +4. **commit marker present**: step 3 is done, and CommitStatus has been saved to the DB. -The core problem: **the old code conflated 1 and 4.** +The issue is: **the old code mixed 1 and 4 together.** `HasBlock` was effectively the same as +"commit marker present", because `WriteBlockWithState` wrote the marker right after writing body/state. +So "the DB can now read the body" was treated as "broadcast / report is also done." -In the old logic `HasBlock` was effectively equivalent to "commit marker present", -because `WriteBlockWithState` wrote the marker right after writing body/state. -So "a body was just persisted locally" was treated as "this block's outward -commit is done." +This affects several failure and recovery paths: -That looks fine under normal sequential execution, but it breaks on these paths: +- After crash / restart, the node cannot tell "committed" from "only the local body was written". +- Head rewind can delete a body while another path writes the marker, leaving a marker whose body is gone. +- Root reorg updates head/rootTip/currentEvmState in multiple steps, so other code can observe mismatched state. -- **Crash / restart recovery**: can't tell whether a block is "committed" or "just has a body on disk, broadcast/report not sent yet." -- **Head rewind (`SetHead`)**: rewind deletes the body, but if another goroutine is writing the marker for the same block at that moment, you can end up with a **marker pointing at an already-deleted body** — a state that can never be consistent. -- **Root reorg**: `currentBlock` / `rootTip` / `currentEvmState` are published in several steps, so a reader can observe mutually inconsistent combinations mid-flight. -- **Sidechain replay / sync retry**: can't distinguish "should skip" / "should replay" / "should re-commit." +## Split Order -## Overall approach +The code is split into 3 dependent PRs: -Three steps, one per PR, and the order matters: +1. **Fix concurrency first** (PR1). Later PRs move commit marker writes from right after saving the body to + after x-shard broadcast / report. That makes the time between "body is in the DB" and "marker is written" longer. + Before changing marker semantics, rewind / root reorg must not conflict with import / commit during + that period. PR1 serializes those paths under the same locks, and publishes currentBlock / rootTip / + currentEvmState / canonical index under the same lock acquisition. +2. **Then split the state semantics** (PR2). With PR1's locking and publish order in place, PR2 can safely + make body present != committed, and make the shard layer write the marker only after broadcast / report + succeeds. `CommitMinorBlockByHash` can then check that the body still exists before writing the marker, + preserving `marker => body`. +3. **Then update sync / replay behavior** (PR3). PR3 depends on the new PR2 API meaning: `HasBlock` means body, + and `HasCommittedBlock` means commit is done. For ancestor lookup, sync needs a more precise rule: + normal body/state blocks still require `HasCommittedBlock`; only pruned sidechain blocks can use + `HasBodyWithoutState` as local data. This avoids re-downloading historical bodies and recovers + historical blocks that have body but miss the marker. -1. **Fix the concurrency first** (PR1), via two means: (a) add / widen locking on - rewind / root reorg / import so paths that were not mutually exclusive now - serialize under the same locks; (b) publish currentBlock / rootTip / - currentEvmState / canonical index inside one critical section, so readers - never see a half-updated intermediate state. -2. **Then split the state semantics** (PR2). Make "body present" ≠ "committed"; - the marker is written explicitly by the shard layer only after broadcast / - report succeed. -3. **Finally build the feature on the new semantics** (PR3). A body doesn't mean - committed, but it can still serve as a local sync anchor — avoiding re-download - of history bodies already in the DB — and recover history blocks that "have a - body but miss the marker." +After the split, each interface has one job: -After the split the semantics become three clear predicates: - -| Predicate | Meaning | Typical use | +| Interface | Meaning | Typical use | | --- | --- | --- | -| `HasBlock(hash)` | body present | whether the body needs to be persisted again | -| `HasCommittedBlock(hash)` | body **and** commit marker both present | sync deciding "is this block really done" | -| `HasBodyWithoutState(hash)` | body present but state not local | anchor for a pruned sidechain | +| `HasBlock(hash)` | body present | whether the body needs to be written again | +| `HasCommittedBlock(hash)` | body **and** commit marker both present | sync deciding whether this block is really done | +| `HasBodyWithoutState(hash)` | body present but state not local | pruned sidechain body is already local | -## How the code is split +## PR Split -All three PRs are carved out of the original large branch, stacked -(PR3 depends on PR2, PR2 depends on PR1). +All three PRs are split out from the original large branch and are stacked: PR3 depends on PR2, PR2 depends on PR1. ### PR 1 · `fix/minor-chain-head-locking` -**Theme**: serialize minor head rewind and root reorg, and make head publication -atomic. This PR does not touch commit marker semantics. - -Commits: `f1f0f50` (core-layer locks and head publication), `f40beb8` -(shard-layer `AddRootBlock` serialization). - -Files: `core/minorblockchain.go`, `core/minorblockchain_addon.go`, -`cluster/shard/api_backend.go`. - -Key points: - -- Make `SetHead` / `Reset` / `ResetWithGenesisBlock` hold both `chainmu` + `mu`, - so rewind and import are mutually exclusive (old code: rewind held only - `chainmu`, import held only `mu` — they were not mutually exclusive). -- When taking both locks, follow the existing order **`s.mu (shard) → chainmu → - mu (core)`**, consistent with the insert pipeline (`InsertChainForDeposits` - holds `chainmu`, `WriteBlockWithState` takes `mu`), to avoid introducing a new deadlock. -- `setHead` now **validates the target state is usable first, then deletes bodies - above the target, and finally publishes** `currentEvmState` + DB head hash + - `currentBlock` in one shot. I.e. the irreversible delete happens only after the - state check passes: either it fails and returns an error with no body deleted - (safe to retry), or it deletes all the way through. The old code did "delete - bodies first, then discover the target state is missing," which could only fall - all the way back to the genesis block. -- Root reorg keeps the target block in a local variable and publishes the head in - one shot only after canonical index / rootTip / confirmedHeaderTip / EVM state are all ready. -- The no-head-publish path of root reorg only rewrites canonical hashes and - **does not delete sidechain bodies**, so a later root reorg can switch back. -- `ShardBackend.AddRootBlock` takes `s.mu`, serializing root block handling with - the shard-side broadcast / report of `AddMinorBlock` / `AddBlockListForSync`. - -Reviewer focus: - -- Is the lock order always `s.mu → chainmu → mu`? Any path acquiring in reverse that could deadlock? -- Are `currentBlock` / `currentEvmState` / rootTip / canonical index published within the same critical section? -- On a rewind error, is it guaranteed that "bodies are never deleted before returning the error"? +**Theme**: serialize minor head rewind and root reorg, and publish head-related fields together. +This PR does not change commit marker semantics. + +Issue / impact: + +- `SetHead` / root reorg can conflict with minor block import / commit, so a body may be deleted while still being committed or referenced. +- During root reorg, head, root tip, EVM state, and canonical index are published in separate steps; other code can observe inconsistent state. + +Root cause: + +- rewind / root reorg / import were not fully serialized by the same set of locks. +- head-related fields were written step by step instead of computing the target first and publishing together. + +Fix: + +- Use the lock order `s.mu -> chainmu -> mu`, so rewind / root reorg / import are mutually exclusive. +- `setHead` validates the target state first, then deletes bodies, then publishes head/state together. +- root reorg publishes root tip, confirmed tip, EVM state, and head together at the end; the no-head-publish path only rewrites canonical hash and does not delete sidechain bodies. + +Review focus: + +- Any reverse lock order or re-entrant deadlock? +- Are head, root tip, EVM state, and canonical index published under the same lock acquisition? +- If rewind returns an error, can it return after deleting bodies? ### PR 2 · `fix/minor-block-commit-status` -**Theme**: split "body present" from "committed." This is the semantic core of the change set. - -Commits: `3ea16ad` (semantic core + call sites across layers + race tests), -`79d7f18` (commit-recovery invariant tests). - -Files: core (`minorblockchain*.go`, `rootblockchain.go`), shard (`api_backend.go`, -`shard.go`), slave (`api_backend.go`), sync (`minor_task.go`, `root_task.go`, -`sync.go`), and their tests. - -Key points: - -- `WriteBlockWithState` / `insertSidechain` **no longer auto-write the commit - marker**; the marker is written explicitly by the shard layer only after - x-shard broadcast + master header report succeed. -- `HasBlock` is narrowed to "body present"; a new `HasCommittedBlock` means body + marker. -- `CommitMinorBlockByHash` checks under `m.mu` whether the body still exists - before writing the marker; if the body was deleted by a rewind, it returns - `false`. This is the last gate for the "marker ⟹ body" invariant. -- sync / slave decide "is it done" via `HasCommittedBlock`, no longer mistaking a body-only block for committed. -- A block with body/state but missing marker can be retried: re-send - broadcast / report, then write the marker. This relies on **force insert** (next point). -- **force insert**: shard-layer imports (`AddMinorBlock` / `AddBlockListForSync`) - all call `InsertChainForDepositsWithBlocks` with `force=true`. Its purpose is to - make the validator **skip the `ErrKnownBlock` short-circuit** — under the old - behavior a block that "already has body/state" was rejected as a known block, - so a block missing its marker could never get one written. With `force=true`: - if a block already has body+state, it is **re-executed only to recompute the - x-shard list** (head untouched, body not rewritten), then handed to the shard - layer to re-broadcast / re-report and write the marker. This is the mechanism - behind both "retry recovery" and how sidechain replay obtains the x-shard list. -- Remove the deprecated `BLOCK_COMMITTING` placeholder state. - -Reviewer focus: - -- For every "skip a block we already have" check, confirm one by one whether it should use `HasBlock` or `HasCommittedBlock` (this is the easiest place to get wrong). -- Before writing the marker, are the x-shard broadcast and master header report actually complete? -- On a marker-write failure, is it guaranteed to never leave a "marker pointing at a missing body"? -- Does `DeleteMinorBlock` delete the body and marker in the same batch (so the rollback side also holds the invariant)? -- In the `force=true` "block already has body+state" branch, does it **only - recompute the x-shard list, without changing the head / rewriting the body** — - so force can't overwrite already-correct data or wrongly advance the head? +**Theme**: split "body present" from "committed". + +Issue / impact: + +- Body/state in the DB does not mean x-shard broadcast and master header report are done. +- The old code mixed body present with committed, so after crash / retry it could not tell which blocks still needed commit side effects. +- During concurrent rewind, a marker could be written for a block whose body was already deleted. + +Root cause: + +- `WriteBlockWithState` wrote the commit marker while writing body/state, marking the block committed too early. +- `HasBlock` meant both "does the body exist" and "is commit done". +- Marker write was not an atomic check-and-write with body existence. + +Fix: + +- `WriteBlockWithState` / `insertSidechain` no longer auto-write the commit marker. The marker is written only by the shard layer after x-shard broadcast + master header report succeed. +- `HasBlock` now only means "body present"; add `HasCommittedBlock` for body + marker. +- `CommitMinorBlockByHash` checks under `m.mu` that the body still exists before writing the marker. If rewind already deleted the body, it returns `false`. +- sync / slave use `HasCommittedBlock` when deciding whether a block is complete, so body-only blocks are not treated as committed. +- Blocks with body/state but missing marker can be re-executed to recompute the x-shard list, then broadcast / reported / marked. +- Remove the obsolete `BLOCK_COMMITTING` placeholder state. + +Review focus: + +- Should each call site use `HasBlock` or `HasCommittedBlock`? +- Is the marker written only after broadcast / report succeeds? +- Does `marker => body` hold on both commit and delete paths? ### PR 3 · `fix/minor-sidechain-body-anchor` -**Theme**: let sidechain / pruned bodies serve as a sync anchor, and recover -history blocks missing their marker. Built on PR2's new semantics. - -> A pruned body is a block whose body is still present but whose state trie has -> been pruned away — i.e. the `HasBodyWithoutState` case above. - -Commits: `d887d70`, `fad5e1b` (comment clarification of sidechain replay retry behavior). - -Files: `cluster/shard/api_backend.go`, `cluster/sync/minor_task.go`, -`cluster/sync/sync.go`, `core/minorblockchain.go`, `core/rootblockchain.go`, and their tests. - -Background: what insert sidechain / sidechain replay is (following go-ethereum's mechanism) - -- When importing a batch, if the fork point is too old and the parent's - **state trie has been pruned** (`ErrPrunedAncestor`), it enters sidechain - import: those blocks **only write the body — no state, no canonical index, no - commit marker**. Since this fork may never be adopted, just storing the body - is enough and cheap. -- Only when this fork's **cumulative height / difficulty exceeds the current main - chain**, and we actually switch to it, do we start from "the nearest ancestor - that still has state," **replay** (re-execute) each body-only block to produce - state, then reorg the main chain onto this fork. Only this step produces the - x-shard list, and only then do broadcast / report / write-marker run. -- So for this change set: **body present = usable as a sync anchor (no re-download); - but body present ≠ committed**. During replay + reorg, one pass may replay - multiple history blocks, and each must complete broadcast / report / write-marker - in order to count as committed. - -Key points: - -- sync `findAncestor` can treat a body-only / body-without-state block as a local - anchor, avoiding re-download of history bodies already in the DB; returns an - explicit error when no common ancestor is found (no more nil-panic). -- Sidechain replay can return the x-shard list of **multiple history blocks**, - broadcast / report / commit each in chain order. -- `AddBlockListForSync` (the batch sync-import entry) now **imports by "contiguous - segment"** instead of the old block-by-block import: - - Iterate the batch, first skipping already-committed blocks and duplicate blocks within the batch (dedup); - - accumulate blocks that are number-contiguous and parent-linked into a segment; - once a gap is hit **within the same batch** (number not contiguous or parent - mismatch), import + commit the accumulated segment first, then keep - accumulating the next segment after the break, still within the same batch - (gap split). Here "commit" is the full PR2 flow (insert body/state → broadcast - x-shard → report master → write commit marker), with the marker as the final - step; "next segment" means the contiguous blocks after the break within the - same batch, still inside the same `AddBlockListForSync` call — not a new sync round; - - why: sidechain replay needs **a contiguous run of blocks** to replay from the - ancestor that has state; block-by-block insert cannot trigger a correct replay. -- When `NewMinorBlock` hits a parent that "has body/state but misses the marker," - it first tries to commit the uncommitted ancestors, then handles the current - block; when the parent is a body-only sidechain, it persists the child body - directly and lets a later replay rebuild the state. -- The commit marker's meaning is unchanged: the marker is written only after - broadcast / report are sent; if the marker write fails the block stays - uncommitted and is re-sent by sync retry. - -Reviewer focus: - -- Does the body-only anchor **only widen the "we already have this data locally" check**, never mistaking a block for "commit done"? -- Is the commit order of multi-block replay consistent with chain order? -- After a replay / commit failure, does it rely on "staying uncommitted" so sync retries later, rather than leaving a half-committed state? - -## Suggested review order - -Read **PR1 → PR2 → PR3**; this order matches the dependencies, and reordering loses context: - -- PR1 first makes concurrency and head publication consistent (so splitting semantics later won't hit concurrency windows). -- PR2 then changes commit marker semantics (relying on PR1's locks to hold the invariant). -- PR3 finally uses the new semantics for the sidechain body anchor and retry recovery. - -## Test plan - -Idea: every PR's core risk has a test backing it; the most critical "marker ⟹ -body" invariant is pressed directly with race tests. - -Below is a "risk → test" mapping; reviewers can check each by test name. - -**core layer** (PR1 / PR2) - -- Don't write marker when body is absent; write only when body is present → - `TestCommitMinorBlockByHash_SkipsWhenBodyAbsent` / `TestCommitMinorBlockByHash_WritesWhenBodyPresent` -- `HasBlock` means body present only (returns false when marker present but body absent) → `TestHasBlock_FalseWhenBodyAbsentButMarkerPresent` -- `InsertChain` does not auto-write the marker after writing body/state → `TestInsertChain_BodyWrittenWithoutCommitMarker` -- After head rewind / genesis reset, `currentBlock` and `currentEvmState` are - consistent → reuse existing reorg tests (`TestMinorLargeReorgTrieGC`, - `TestMinorChainTxReorgs`, etc.) + review; no dedicated test added. - -**shard layer** (PR2 / PR3) - -- `AddMinorBlock` / `AddBlockListForSync` explicitly write the marker on success → - `TestAddMinorBlock_CommitMarkerPresentAfterBlock` / `TestAddBlockListForSync_CommitMarkerPresentAfterSync` -- A block with body/state but missing marker can be recovered on retry → - `TestAddBlockListForSync_RecoversXShardListOnRetry` / `TestNewMinorBlock_RecoversUncommittedBodyOnRetry` (both single-block retry) -- Skip an already-committed block in the batch and keep processing the rest → `TestAddBlockListForSync_ContinuesPastKnownBlock` -- **marker ⟹ body (invariant under concurrency)** → `TestAddMinorBlockMarkerBodyConsistencyUnderRace` / - `TestAddBlockListForSyncMarkerBodyConsistencyUnderRace` (`-race`, concurrent with `SetHead`) -- TODO: **multi-block commit in chain order** and **gap split / in-batch dedup** - have no dedicated tests yet; currently covered indirectly by - `ContinuesPastKnownBlock` + single-block retry above — dedicated cases recommended. - -**sync layer** (PR2 / PR3) - -- minor sync finds the ancestor via committed / body-only anchor → `TestMinorFindAncestorUsesCommittedOrBodyWithoutState` -- root / minor task mocks explicitly distinguish `HasBlock` from `HasCommittedBlock` (test scaffolding, not a standalone assertion). - -Suggested local runs: +**Theme**: handle the sync / sidechain replay code that has to change after PR2 splits marker semantics. + +> A pruned body means the body is still present but the corresponding state trie was pruned, i.e. the +> `HasBodyWithoutState` case above. + +First, what insert sidechain / sidechain replay means (following the go-ethereum mechanism) + +- When importing a batch, if the fork point is too old and the parent's **state trie has been pruned** + (`ErrPrunedAncestor`), the chain enters sidechain import: those blocks **only write body, not state, + not canonical index, and not commit marker**. The fork may never be adopted, so keeping only the body is cheap. +- Only when this fork's **cumulative height / difficulty exceeds the current main chain** and the node really + switches to it does replay start from the nearest ancestor that still has state. The body-only blocks are + replayed one by one to rebuild state, and reorg moves the canonical chain to that fork. This is the step + that produces x-shard lists, so this is also when broadcast / report / marker writes are needed. +- For this change set: **normal body/state blocks still need `HasCommittedBlock` to count as ancestors; + only pruned sidechain blocks can use `HasBodyWithoutState` as local data to avoid re-download**. + That still does not mean committed. During replay + reorg, one pass can replay multiple historical blocks, + and each of them must complete broadcast / report / marker write in order. + +Issue / impact: + +- **Existing pruned sidechain bodies may be downloaded again**: after PR2, body-only no longer means committed. + Normal body/state blocks still use `HasCommittedBlock`; but for `HasBodyWithoutState` pruned sidechain blocks, + the body is already in the DB. If sync does not treat those blocks as local, `findAncestor` can walk past + them and re-download historical bodies from peers. +- **Sidechain replay may miss the commit step for historical blocks**: when a pruned sidechain becomes + canonical, replay can execute multiple historical blocks and produce an x-shard list for each block. + The old shard commit path was mostly written for "the single block or blocks currently requested", not for + "commit multiple replayed historical blocks in order". +- **A parent with body/state but missing marker can block live propagation**: if the node crashes after + body/state write but before marker write, then after restart a child block sees the parent as not committed. + The node needs to finish broadcast / report / marker for the parent and earlier ancestors before processing the child. + +Root cause: + +- The old sync logic only had a committed / not-committed check, so it did not separately handle pruned sidechain bodies already in the DB. +- The old shard commit path was single-block oriented and did not cover sidechain replay recovering multiple historical blocks at once. +- `NewMinorBlock` assumed the current block could be handled directly, without first checking whether its parent chain had local blocks missing markers. + +Fix: + +- sync `findAncestor` uses `HasCommittedBlock || HasBodyWithoutState` for ancestor lookup: + normal blocks must be committed, while pruned sidechain blocks only need their body in the DB. This avoids + re-downloading historical bodies already in the DB; if no common ancestor is found, it returns a normal error instead of nil-panic. +- sidechain replay can return x-shard lists for **multiple historical blocks**, and they are broadcast / reported / committed in chain order. +- `AddBlockListForSync` imports by contiguous segment. If the batch is not contiguous, it commits the previous + segment first; the next segment still goes through normal parent/state validation, so a missing parent fails + and waits for a later sync retry. +- `NewMinorBlock` first tries to finish uncommitted ancestors when the parent has body/state but misses marker; if the parent is a body-only sidechain block, it writes the child body to the DB and lets later replay rebuild state. +- Commit marker meaning stays the same: write marker only after broadcast / report is sent; if marker write fails, the block stays uncommitted and sync retry sends it again. + +Review focus: + +- Is `HasBodyWithoutState` used only as local sync data, never mistaken for committed? +- Does multi-block sidechain replay commit in chain order? +- After replay / commit failure, does the block stay uncommitted so sync retry can recover it? + +## Test Plan + +**Unit test** + +- Cover core semantics: distinguish `HasBlock` / `HasCommittedBlock`, write commit marker only when body exists, and keep `InsertChain` from writing marker automatically. +- Cover shard commit flow: write marker after broadcast / report succeeds, and recover blocks that have body/state but miss marker through retry. +- Cover sync semantics: `HasBodyWithoutState` can serve as ancestor local data, but cannot be treated as committed. ```bash go test ./core/... ./cluster/sync/... ./cluster/shard/... +``` + +**Race test** + +- Focus on the key concurrent invariant: `commit marker present => body present`. +- Cover `AddMinorBlock` / `AddBlockListForSync` racing with `SetHead`. + +```bash go test -race ./cluster/shard -run 'TestAddBlockListForSyncMarkerBodyConsistencyUnderRace|TestAddMinorBlockMarkerBodyConsistencyUnderRace' ``` -## Non-goals - -- **Read-side atomicity**: PR1 only converged the write-side publication order. - On the read side, `CurrentBlock()` / `GetBlockByNumber()` don't take `m.mu`, so - during the window where a root reorg has rewritten the canonical index but the - new head isn't published yet, they can still read an inconsistent snapshot of - "old head + new canonical index." Making read tip / state observe the same lock - is follow-up work, tracked in [#693](https://github.com/QuarkChain/goquarkchain/issues/693). -- **No strong "succeed-once" guarantee; converge by retry**: we don't change the - resend/dedup mechanism of x-shard broadcast / master report (the same block may - be resent multiple times; the receiver dedups by block hash and won't process - it twice), nor do we wrap history recovery in a single transaction; on failure a - block stays uncommitted and is filled in on a later retry via the existing - "dedup by block hash + sync retry." - -## PR overview - -- PR 1: `fix/minor-chain-head-locking` (commits `f1f0f50`, `f40beb8`) — serialize concurrency, atomic head publication -- PR 2: `fix/minor-block-commit-status` (commits `3ea16ad`, `79d7f18`) — split "body present" from "committed" -- PR 3: `fix/minor-sidechain-body-anchor` (commits `d887d70`, `fad5e1b`) — sidechain body anchor and retry recovery - -The consensus we want after reading: the difference between the three states -(body / state / commit marker); why locking must come first, then the marker -semantics split, then the sidechain body anchor; and what to focus on in each PR -and which risks the tests cover. +**Run nodes** + +- Start local cluster nodes and confirm they can start, sync, and keep advancing root blocks. +- Keep the nodes running for a while and watch for `ErrBodyDeleted`, missing parent, missing state, or commit marker related errors. If they appear, they should recover through sync retry instead of stopping sync. +- Restart the cluster on existing chain data and confirm it restores head from the DB, then continues syncing and mining. + +## Non-Goals + +- **Read-side atomicity**: PR1 only changes write-side publication order. Read-side `CurrentBlock()` / + `GetBlockByNumber()` does not take `m.mu`, so while root reorg has rewritten canonical index but not yet + published the new head, readers may still observe "old head + new canonical index". Making read tip / state + use the same lock is follow-up work, tracked in [#693](https://github.com/QuarkChain/goquarkchain/issues/693). diff --git a/L1/quarkchain-fix/minor-commit-status-review.md b/L1/quarkchain-fix/minor-commit-status-review.md index 9b7d96a..bba54af 100644 --- a/L1/quarkchain-fix/minor-commit-status-review.md +++ b/L1/quarkchain-fix/minor-commit-status-review.md @@ -1,130 +1,126 @@ -# Minor Block Commit Status —— Design & Review Doc +# Minor Block Commit Status —— Review Doc -> 一句话：把 minor block 的「本地有数据」和「对外提交完成」两个状态拆开， -> 顺带收紧 head rewind / root reorg / commit 之间的并发关系。实现拆成 3 个 stacked PR。 +> 这组 PR 主要解决一件事：不要再把「DB 里有 minor block body」等同于「这个块已经完成广播 / 上报」。 +> 同时把 head rewind、root reorg、commit 这几条会互相影响的路径串行起来，避免并发时互相冲突。 -这份文档给出整组改动的 whole picture：先讲清楚问题、整体思路，再分别说明 3 个 PR 各自在做什么、 -每个 PR 该重点看哪里、测试怎么覆盖。建议按 **本文档 → PR1 → PR2 → PR3** 的顺序看： -三个 PR 有严格的依赖关系（先并发，再语义，最后功能），下面会说明为什么。 +建议按 **本文档 → PR1 → PR2 → PR3** 的顺序看。 -## 背景：一个块其实有好几种「状态」 +## 背景 -给完全没有上下文的人先建立模型。一个 minor block 在本地节点上，可能处于以下几种彼此独立的状态： +一个 minor block 在本地节点上，不只有“有 / 没有”两种状态： -1. **body 存在**：block 的内容已经写进本地 DB（可以读到这个块）。 -2. **state 存在**：这个块对应的 state trie 在本地，可以在它之上执行交易 / 查询余额。 -3. **对外广播 / 上报完成**：x-shard tx list 已经广播给相邻 shard，block header 已经上报给 master。 -4. **commit marker 存在**：一个显式标记，代表「这个块的 shard 侧提交流程（第 3 步）已经彻底完成」。 +1. **body 存在**：DB 里已经能读到这个 block。 +2. **state 存在**：本地（ DB 或内存中）还保留这个 block 的 state trie ，可以继续执行下一个块或查询状态。 +3. **对外广播 / 上报完成**：x-shard tx list 已广播，minor header 已上报 master。 +4. **commit marker 存在**：表示第 3 步已经完成并将 CommitStatus 保存到DB。 -关键问题：**旧代码把 1 和 4 混成了一件事。** +问题在于：**旧代码把 1 和 4 混成了一件事。** `HasBlock` 实际等价于 commit marker 存在，因为 `WriteBlockWithState` +写完 body/state 后会同时写 marker。于是「DB 里刚能读到 body」会被当成「广播 / 上报也已经完成」。 -旧逻辑里 `HasBlock` 实际等价于「commit marker 存在」，因为 `WriteBlockWithState` 一写完 body/state 就顺手写了 marker。 -于是「本地刚落盘一个 body」被当成了「这个块已经对外提交完成」。 +这会影响几条异常和恢复路径： -这在正常顺序执行时看不出问题，但在这几条路径上就会出事： +- crash / restart 后分不清「已提交」还是「只写入了本地 body」。 +- head rewind 可能删 body，同时另一条路径写 marker，最后 DB 里有 marker，但对应 body 已经没了。 +- root reorg 分几步更新 head/rootTip/currentEvmState，其他代码可能看到互相对不上的状态。 -- **崩溃 / 重启恢复**：分不清一个块是「提交完了」还是「只是 body 落了盘、广播 / 上报还没发」。 -- **head rewind（`SetHead`）**：回滚会删掉 body，但如果此时另一个 goroutine 正在给同一个块写 marker， - 就可能出现 **marker 指向一个已经被删掉的 body**——一个永远无法自洽的状态。 -- **root reorg**：`currentBlock` / `rootTip` / `currentEvmState` 分几步发布，中间时刻读者会看到互相不一致的组合。 -- **sidechain replay / sync retry**：无法区分「该跳过」「该重放」「该重新提交」。 +## 拆分顺序 -## 整体思路 +代码分成 3 个有依赖关系的 PR： -三步走，对应三个 PR，顺序不能乱： +1. **先解决并发问题**（PR1）。后面会把 commit marker 的写入从 body 保存之后延后到 xshard 广播 / 上报之后， + 这会让“body 已经写入 DB、marker 还没写”的时间变长，冲突可能性增大。因此在改 marker 语义前，必须先保证 + rewind / root reorg 不会在这段时间里和 import / commit 冲突：PR1 把这些路径放到同一组锁下串行， + 并在同一次加锁里一起发布 currentBlock / rootTip / currentEvmState / canonical index。 +2. **再把状态语义拆开**（PR2）。有了 PR1 的锁和发布顺序，PR2 才能安全地让 + body 存在 ≠ 已提交，并把 marker 改为由 shard 层在广播 / 上报成功后主动写入。这样 + `CommitMinorBlockByHash` 才能先确认 body 还在，再写 marker，保证 `marker => body`。 +3. **最后处理 sync / replay 行为**（PR3）。在 `HasBlock` 表示 body、`HasCommittedBlock` + 表示提交完成之后，sync 查共同祖先时要更精确：普通有 body/state 的块仍然要看 `HasCommittedBlock`， + 只有 pruned sidechain 这种 `HasBodyWithoutState` 才能只靠 body 当作本地已有。基于这个语义， + PR3 才能避免重复下载已有历史 body，并恢复那些「有 body 缺 marker」的历史块。 -1. **先解决并发问题**（PR1），两个手段：一是给 rewind / root reorg / import 增加 / 扩大加锁范围， - 让原来彼此不互斥的路径都在同一组锁下串行；二是把 currentBlock / rootTip / currentEvmState / - canonical index 放进同一个临界区一起发布，避免读者看到半更新的中间状态。 -2. **再把状态语义拆开**（PR2）。让 body 存在 ≠ 已提交，marker 由 shard 层在广播 / 上报成功后显式写入。 -3. **最后用新语义做功能**（PR3）。body 虽然不代表已提交，但仍可作为 sync 的本地 anchor， - 避免重复下载已有历史 body，并恢复那些「有 body 缺 marker」的历史块。 +这样拆以后，每个 PR 的 review 范围都比较单纯：PR1 只看锁和 head 发布，PR2 只看 body/marker 语义和 +`marker => body`，PR3 只看 sidechain 已有 body 复用 / replay 恢复；reviewer 不需要在一个巨大 diff +里同时验证三类问题。 -拆开后语义变成三个清晰的谓词： +拆完以后，这几个接口各管一件事： -| 谓词 | 含义 | 典型用途 | +| 接口 | 含义 | 典型用途 | | --- | --- | --- | -| `HasBlock(hash)` | body 存在 | 是否需要重新落盘 body | +| `HasBlock(hash)` | body 存在 | 是否需要重新写入 body | | `HasCommittedBlock(hash)` | body **且** commit marker 都存在 | sync 判断「这个块是否真的完成了」 | -| `HasBodyWithoutState(hash)` | body 存在但 state 不在本地 | 作为 pruned sidechain 的 anchor | +| `HasBodyWithoutState(hash)` | body 存在但 state 不在本地 | 表示 pruned sidechain 的 body 已在本地 | -## 代码怎么拆 +## PR 拆分 三个 PR 都从原来的大分支拆出来，彼此 stacked（PR3 依赖 PR2，PR2 依赖 PR1）。 ### PR 1 · `fix/minor-chain-head-locking` -**主题**：串行化 minor head rewind 和 root reorg，保证 head 原子发布。本 PR 不碰 commit marker 语义。 +**主题**：串行化 minor head rewind 和 root reorg，保证 head 相关字段一起发布。本 PR 不碰 commit marker 语义。 -对应 commit：`f1f0f50`（core 层锁与 head 发布）、`f40beb8`（shard 层 `AddRootBlock` 串行化）。 +Issue / impact： -改动范围：`core/minorblockchain.go`、`core/minorblockchain_addon.go`、`cluster/shard/api_backend.go`。 +- `SetHead` / root reorg 可能和 minor block import / commit 并发冲突，导致 body 被删后仍被提交或引用。 +- root reorg 过程中 head、root tip、EVM state、canonical index 分步发布，其他代码可能看到不一致状态。 -关键点： +Root cause： -- 让 `SetHead` / `Reset` / `ResetWithGenesisBlock` 都同时持有 `chainmu` + `mu`，使 rewind 与 import 互斥 - （原来 rewind 只拿 `chainmu`、import 只拿 `mu`，两者并不互斥）。 -- 同时拿两把锁时沿用既有的 **`s.mu (shard) → chainmu → mu (core)`** 顺序，和 insert pipeline 一致 - （`InsertChainForDeposits` 持 `chainmu`，`WriteBlockWithState` 取 `mu`），避免引入新的死锁。 -- `setHead` 改为**先校验目标 state 可用，再删除高于目标的 body，最后一次性发布** `currentEvmState` + DB head hash + `currentBlock`； - 也就是把不可逆的删除放到状态校验通过之后：要么校验失败、一个 body 都没删就返回错误（可安全重试）， - 要么删到底。旧代码是「先删 body，删完才发现目标 state 不在」，只能一路退回创世块。 -- root reorg 把目标块保持为局部变量，等 canonical index / rootTip / confirmedHeaderTip / EVM state 都就绪后再一次性发布 head。 -- root reorg 的 no-head-publish 路径只重写 canonical hash，**不删 sidechain body**，方便之后 root reorg 再切回。 -- `ShardBackend.AddRootBlock` 加 `s.mu`，让 root block 处理与 `AddMinorBlock` / `AddBlockListForSync` 的 shard 侧广播 / 上报串行。 +- rewind / root reorg / import 没有被同一组锁完整串行化。 +- head 相关字段逐步写入，而不是先算出目标，再一起发布。 -Reviewer 重点： +Fix： -- 锁顺序是否始终是 `s.mu → chainmu → mu`，有没有反向获取导致死锁的路径。 -- `currentBlock` / `currentEvmState` / rootTip / canonical index 是否在同一临界区内一起发布。 -- rewind 出错时，是否保证「不会先删 body 再返回错误」。 +- 统一锁顺序为 `s.mu -> chainmu -> mu`，让 rewind / root reorg / import 互斥。 +- `setHead` 先校验目标 state，再删除 body，最后一次性发布 head/state。 +- root reorg 只在末尾一起发布 root tip、confirmed tip、EVM state 和 head；no-head-publish 路径只改 canonical hash，不删 sidechain body。 + +Review focus： + +- 是否存在反向锁顺序或重入死锁。 +- head、root tip、EVM state、canonical index 是否在同一次加锁里发布。 +- rewind 出错时是否不会先删 body 再返回。 ### PR 2 · `fix/minor-block-commit-status` -**主题**：拆分「body 存在」和「已提交」两个状态。这是整组改动的语义核心。 +**主题**：拆分「body 存在」和「已提交」两个状态。 + +Issue / impact： -对应 commit：`3ea16ad`（语义主体 + 各层调用点 + race test）、`79d7f18`（commit recovery 不变量测试）。 +- DB 里已经有 body/state，不代表 x-shard broadcast 和 master header 上报已经完成。 +- 旧代码把 body 存在和已提交混在一起，crash / 重试后无法判断哪些块需要补上提交。 +- 并发 rewind 时，marker 可能写到已经被删除 body 的块上。 -改动范围：core（`minorblockchain*.go`、`rootblockchain.go`）、shard（`api_backend.go`、`shard.go`）、 -slave（`api_backend.go`）、sync（`minor_task.go`、`root_task.go`、`sync.go`）及对应测试。 +Root cause： -关键点： +- `WriteBlockWithState` 在写 body/state 的同时写 commit marker，过早标记为“提交完成”。 +- `HasBlock` 同时承担“body 是否存在”和“是否已提交完成”两种含义。 +- commit marker 写入没有和 body-present 检查绑定成原子操作。 -- `WriteBlockWithState` / `insertSidechain` **不再自动写 commit marker**；marker 改由只在 shard 层在 - x-shard broadcast + master header 上报成功之后显式写入。 -- `HasBlock` 语义收窄为「body 存在」；新增 `HasCommittedBlock`（body + marker）。 +Fix： + +- `WriteBlockWithState` / `insertSidechain` **不再自动写 commit marker**；marker 改为只在 shard 层 + x-shard broadcast + master header 上报成功之后写入。 +- `HasBlock` 现在只表示「body 存在」；新增 `HasCommittedBlock`（body + marker）。 - `CommitMinorBlockByHash` 在 `m.mu` 下先检查 body 是否还在，再写 marker；body 已被 rewind 删除则返回 `false`。 - 这是「marker ⟹ body」不变量的最后一道闸。 + 这样可以保证「有 marker 就一定还有 body」。 - sync / slave 判断「是否已完成」统一改用 `HasCommittedBlock`，不再把只有 body 的块误当已提交。 -- 有 body/state 但缺 marker 的块可被 retry：重发广播 / 上报后补写 marker。这依赖 **force insert**（下条）。 -- **force insert**：shard 层导入（`AddMinorBlock` / `AddBlockListForSync`）统一用 `force=true` 调 - `InsertChainForDepositsWithBlocks`。作用是让 validator **跳过 `ErrKnownBlock` 短路**——旧行为下一个 - 「已有 body/state」的块会被当成 known block 直接拒掉，缺 marker 的块就永远补不上 marker。`force=true` - 时改为：block 已有 body+state 就**只重新执行一遍、把 x-shard list 重算出来**（不动 head、不重写 body）， - 再交给 shard 层重发广播 / 上报、补写 marker。这就是「retry 恢复」和 sidechain replay 能拿到 x-shard list 的机制。 +- 有 body/state 但缺 marker 的块可重新执行并重算 x-shard list，再补广播 / 上报 / marker。 - 删除已经废弃的 `BLOCK_COMMITTING` 占位状态。 -Reviewer 重点： +Review focus： -- 所有「跳过已拥有块」的判断，逐个确认该用 `HasBlock` 还是 `HasCommittedBlock`（这是最容易看错的地方）。 -- marker 写入前，x-shard 广播与 master header 上报是否确实已经完成。 -- marker 写失败时，是否绝不会留下「marker 指向缺失 body」。 -- `DeleteMinorBlock` 是否把 body 和 marker 放在同一个 batch 里删（保证回滚侧也满足不变量）。 -- `force=true` 下「块已有 body+state」的分支是否**只重算 x-shard list、不改 head / 不重写 body**， - 不会因为 force 而把已有正确数据覆盖或误推进 head。 +- 每个调用点该用 `HasBlock` 还是 `HasCommittedBlock`。 +- marker 是否只在广播 / 上报成功后写入。 +- `marker => body` 不变量是否在提交和删除两侧都成立。 ### PR 3 · `fix/minor-sidechain-body-anchor` -**主题**：让 sidechain / pruned body 作为 sync anchor，并恢复缺 marker 的历史块。建立在 PR2 的新语义之上。 +**主题**：处理 PR2 拆开 marker 语义后，sync / sidechain replay 需要跟着改的地方。 > pruned body 指 body 还在、但对应 state trie 已被裁剪掉的块，即前面的 `HasBodyWithoutState`。 -对应 commit：`d887d70`、`fad5e1b`（sidechain replay retry 行为的注释澄清）。 - -改动范围：`cluster/shard/api_backend.go`、`cluster/sync/minor_task.go`、`cluster/sync/sync.go`、 -`core/minorblockchain.go`、`core/rootblockchain.go` 及对应测试。 - -背景：什么是 insert sidechain / sidechain replay（沿用 go-ethereum 的机制） +先说明一下 insert sidechain / sidechain replay（沿用 go-ethereum 的机制） - 导入一批块时，如果发现分叉点太老、父块的 **state trie 已经被裁剪掉**（`ErrPrunedAncestor`）， 就进入 sidechain 导入：这些块**只写 body，不算 state、不写 canonical index、不写 commit marker**。 @@ -132,93 +128,75 @@ Reviewer 重点： - 只有当这条分叉的**累积高度 / 难度超过当前主链**、真的要切过去时，才从「最近一个还留着 state 的祖先」 开始，把中间这些只有 body 的块**逐个 replay**（重新执行）出 state，然后 reorg 把主链切到这条分叉。 这一步才产生 x-shard list，也才需要走广播 / 上报 / 写 marker。 -- 所以对本组改动来说：**body 在 = 可以作为 sync anchor（不用重下）；但 body 在 ≠ committed**。 - replay + reorg 时，一次可能重放出多个历史块，它们各自都要按顺序完成广播 / 上报 / 写 marker 才算 committed。 - -关键点： - -- sync `findAncestor` 可以把 body-only / body-without-state 的块当作本地 anchor， - 避免重复下载已经在 DB 里的历史 body；找不到共同祖先时返回显式错误（不再 nil-panic）。 -- sidechain replay 可返回**多个历史块**的 x-shard list，按 chain order 逐个 broadcast / report / commit。 -- `AddBlockListForSync`（sync 批量导入入口）改为**按「连续段」导入**，而不是旧代码的逐块导入： - - 遍历 batch，把编号连续、parent 相接的块攒成一个 segment；一旦在**同一批里**遇到断档（编号不连续或 parent 对不上）， - 就把已攒的 segment 先导入并提交，再从断点之后继续攒同一批里的下一段（gap split）。这里的「commit」是 - PR2 那套完整流程（insert 落 body/state → 广播 x-shard → 上报 master → 写 commit marker）， - marker 是收尾那一步；「下一段」指同一批里断点之后的连续块，仍在同一次 `AddBlockListForSync` 调用内， - 不是新的 sync round； - - 这样做的原因：sidechain replay 需要**一段连续的块**才能从有 state 的祖先逐块重放； - 逐块单独 insert 无法触发正确的 replay。 -- `NewMinorBlock` 遇到 parent「有 body/state 缺 marker」时，先尝试补交未提交祖先，再处理当前块； - parent 是 body-only sidechain 时，直接把子块 body 落盘，等后续 replay 重建 state。 -- commit marker 含义保持不变：只有广播 / 上报发完才写 marker；marker 写失败则块保持 uncommitted，由 sync retry 重发。 - -Reviewer 重点： +- 所以对本组改动来说：**普通有 body/state 的块，sync 仍然要用 `HasCommittedBlock` 判断是否可作为共同祖先； + 只有 pruned sidechain 的 `HasBodyWithoutState` 可以靠 body 表示本地已有，避免重新下载**。 + replay + reorg 时，一次可能重新执行多个历史块，它们各自都要按顺序完成广播 / 上报 / 写 marker 才算已提交。 -- body-only anchor 是否**只扩大「本地已有数据」的判断**，绝不把块误认为「已完成提交」。 -- multi-block replay 的提交顺序是否与 chain order 一致。 -- replay / commit 失败后，是否依赖「保持 uncommitted」让 sync 后续自然重试，而不是留下半提交状态。 +Issue / impact： -## 建议的 review 顺序 +- **已有 pruned sidechain body 可能被重复下载**：PR2 之后，只有 body 不再等于已提交。普通有 body/state 的块 + 仍然要用 `HasCommittedBlock` 判断；但对 `HasBodyWithoutState` 的 pruned sidechain 来说，body 已经在 DB 里。 + 如果 sync 不把这类块当作本地已有，`findAncestor` 会继续往前找，甚至从 peer 重新下载这些历史 body。 +- **sidechain replay 可能漏掉历史块的提交步骤**：一条 pruned sidechain 变成 canonical 时， + replay 可能一次重新执行多个历史块，并为每个块产生 x-shard list。旧 shard 提交流程主要围绕 + 「当前请求的单个或多个块」写，缺少“按顺序提交多个 replay 出来的历史块”的路径。 +- **有 body/state 但缺 marker 的父块会影响实时传播**：如果节点在 body/state 写入后、marker 写入前崩溃， + 重启后收到子块时，父块不是已提交状态。需要先补上父块以及更早祖先的广播 / 上报 / marker，再继续处理当前块。 -按 **PR1 → PR2 → PR3** 看，这个顺序对应依赖关系，打乱会缺上下文： +Root cause： -- PR1 先保证并发与 head 发布一致（后面拆语义才不会踩到并发窗口）。 -- PR2 再改 commit marker 语义（依赖 PR1 的锁保证不变量）。 -- PR3 最后用新语义做 sidechain body anchor 与 retry recovery。 +- 原来的 sync 逻辑只有“已提交 / 未提交”这一层判断，没有单独处理 pruned sidechain body 已经在本地的情况。 +- shard 层原来的提交路径偏向单块处理，没有覆盖 sidechain replay 一次恢复多个历史块的场景。 +- `NewMinorBlock` 默认当前块可以直接处理，没有先检查父链上是否还有缺 marker 的本地块。 -## 测试方案 +Fix： -思路：每个 PR 的核心风险都要有对应测试兜底，最关键的「marker ⟹ body」不变量用 race test 直接压。 +- sync `findAncestor` 用 `HasCommittedBlock || HasBodyWithoutState` 判断共同祖先： + 普通块必须已提交，pruned sidechain 块只要 body 还在即可，避免重复下载已经在 DB 里的历史 body； + 找不到共同祖先时返回正常错误（不再 nil-panic）。 +- sidechain replay 可返回**多个历史块**的 x-shard list，按 chain order 逐个 broadcast / report / commit。 +- `AddBlockListForSync` 按连续 segment 导入；如果 batch 内出现不连续，先提交前一段，后一段仍然要按正常 + parent/state 校验，缺 parent 时会失败并等待后续同步重试。 +- `NewMinorBlock` 遇到 parent「有 body/state 缺 marker」时，先尝试补上未提交祖先的提交流程，再处理当前块； + parent 是只有 body 的 sidechain 时，直接把子块 body 写入本地 DB，等后续 replay 重建 state。 +- commit marker 含义保持不变：只有广播 / 上报发完才写 marker；marker 写失败则块保持未提交，由同步重试重发。 -下面「风险 → 测试」一一对应，reviewer 可直接按测试名核对。 +Review focus： -**core 层**（PR1 / PR2） +- `HasBodyWithoutState` 是否只作为 sync 的本地已有 body，不被误判为已提交。 +- multi-block sidechain replay 是否按 chain order 提交。 +- replay / commit 失败后是否保持未提交，让同步重试恢复。 -- body 缺失时不写 marker、body 在才写 → `TestCommitMinorBlockByHash_SkipsWhenBodyAbsent` / - `TestCommitMinorBlockByHash_WritesWhenBodyPresent` -- `HasBlock` 只代表 body 存在（有 marker 无 body 时返回 false）→ `TestHasBlock_FalseWhenBodyAbsentButMarkerPresent` -- `InsertChain` 写完 body/state 后不自动写 marker → `TestInsertChain_BodyWrittenWithoutCommitMarker` -- head rewind / genesis reset 后 `currentBlock` 与 `currentEvmState` 一致 → 复用现有 reorg 测试 - （`TestMinorLargeReorgTrieGC`、`TestMinorChainTxReorgs` 等）+ review，无新增专项测试。 +## 测试方案 -**shard 层**（PR2 / PR3） +**Unit test** -- `AddMinorBlock` / `AddBlockListForSync` 成功后显式写 marker → `TestAddMinorBlock_CommitMarkerPresentAfterBlock` / - `TestAddBlockListForSync_CommitMarkerPresentAfterSync` -- 有 body/state 缺 marker 的块可 retry 恢复提交 → `TestAddBlockListForSync_RecoversXShardListOnRetry` / - `TestNewMinorBlock_RecoversUncommittedBodyOnRetry`（均为单块 retry） -- batch 里遇到已 committed 的块时跳过、继续处理后续块 → `TestAddBlockListForSync_ContinuesPastKnownBlock` -- **marker ⟹ body（并发下不变量）** → `TestAddMinorBlockMarkerBodyConsistencyUnderRace` / - `TestAddBlockListForSyncMarkerBodyConsistencyUnderRace`（`-race`，与 `SetHead` 并发） -- 待补：**多历史块按 chain order 顺序提交**、**gap split / batch 内重复去重** 目前无专项测试， - 现由上面 `ContinuesPastKnownBlock` + 单块 retry 间接覆盖，建议补独立用例。 +- 覆盖 core 语义：`HasBlock` / `HasCommittedBlock` 区分、commit marker 只在 body 存在时写入、`InsertChain` 不再自动写 marker。 +- 覆盖 shard 提交流程：广播 / 上报成功后写 marker，已有 body/state 但缺 marker 的块可通过重试恢复。 +- 覆盖 sync 语义：`HasBodyWithoutState` 可作为 ancestor 的本地已有 body，但不能被当作已提交。 -**sync 层**（PR2 / PR3） +```bash +go test ./core/... ./cluster/sync/... ./cluster/shard/... +``` -- minor sync 用 committed / body-only anchor 找 ancestor → `TestMinorFindAncestorUsesCommittedOrBodyWithoutState` -- root / minor task mock 显式区分 `HasBlock` 与 `HasCommittedBlock`（测试支撑，非独立断言）。 +**Race test** -建议本地运行： +- 重点验证并发下最重要的不变量：`commit marker present => body present`。 +- 场景覆盖 `AddMinorBlock` / `AddBlockListForSync` 与 `SetHead` 并发。 ```bash -go test ./core/... ./cluster/sync/... ./cluster/shard/... go test -race ./cluster/shard -run 'TestAddBlockListForSyncMarkerBodyConsistencyUnderRace|TestAddMinorBlockMarkerBodyConsistencyUnderRace' ``` -## 非目标 +**跑节点** -- **读侧原子性**：PR1 只收敛了写侧发布顺序。读侧的 `CurrentBlock()` / `GetBlockByNumber()` 不走 `m.mu`， - 在 root reorg 重写 canonical 索引、还没发布新 head 的窗口里，仍可能读到「旧 head + 新 canonical 索引」的 - 不一致快照。让 read tip / state 走同一把锁是后续工作，留到 [#693](https://github.com/QuarkChain/goquarkchain/issues/693)。 -- **不追求"一次成功"，靠重试收敛**：不改 x-shard broadcast / master report 的重发去重机制 - （同一个块重发多次，接收方按 block hash 去重，不会重复处理），也不把历史 recovery 包成单个事务； - 失败后块保持 uncommitted，靠现有「按 block hash 去重 + sync retry」在后续重试里自然补上。 +- 启动本地 cluster 节点，确认节点能正常启动、同步、root block 持续推进。 +- 保持节点运行一段时间，观察是否出现 `ErrBodyDeleted`、missing parent、missing state、commit marker 相关错误； + 这些错误如果出现，应该能通过同步重试恢复，而不是让节点停止同步。 +- 做一次节点重启验证：在已有链数据上重启 cluster，确认重启后能从 DB 恢复 head、继续同步和出块。 -## PR 一览 - -- PR 1：`fix/minor-chain-head-locking`（commit `f1f0f50`、`f40beb8`）—— 串行化并发、head 原子发布 -- PR 2：`fix/minor-block-commit-status`（commit `3ea16ad`、`79d7f18`）—— 拆分 body 存在与已提交语义 -- PR 3：`fix/minor-sidechain-body-anchor`（commit `d887d70`、`fad5e1b`）—— sidechain body anchor 与 retry recovery +## 非目标 -看完希望达成的共识：body / state / commit marker 三个状态的区别，为什么必须先做 locking、 -再拆 marker 语义、最后做 sidechain body anchor，以及每个 PR 的重点和测试覆盖的风险。 +- **读侧原子性**：PR1 只调整了写侧发布顺序。读侧的 `CurrentBlock()` / `GetBlockByNumber()` 不走 `m.mu`， + 在 root reorg 重写 canonical 索引、还没发布新 head 的过程中，仍可能读到「旧 head + 新 canonical 索引」的 + 不一致状态。让 read tip / state 走同一把锁是后续工作，留到 [#693](https://github.com/QuarkChain/goquarkchain/issues/693)。 From 148b21435e298a3d4ec383dc9ec9516971032335 Mon Sep 17 00:00:00 2001 From: ping-ke Date: Wed, 15 Jul 2026 10:59:31 +0800 Subject: [PATCH 3/4] update --- L1/quarkchain-fix/minor-commit-status-review.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/L1/quarkchain-fix/minor-commit-status-review.md b/L1/quarkchain-fix/minor-commit-status-review.md index bba54af..c7e9b82 100644 --- a/L1/quarkchain-fix/minor-commit-status-review.md +++ b/L1/quarkchain-fix/minor-commit-status-review.md @@ -63,6 +63,7 @@ Issue / impact： - `SetHead` / root reorg 可能和 minor block import / commit 并发冲突，导致 body 被删后仍被提交或引用。 - root reorg 过程中 head、root tip、EVM state、canonical index 分步发布，其他代码可能看到不一致状态。 +- 并发 rewind 时，marker 可能写到已经被删除 body 的块上。 Root cause： @@ -79,7 +80,7 @@ Review focus： - 是否存在反向锁顺序或重入死锁。 - head、root tip、EVM state、canonical index 是否在同一次加锁里发布。 -- rewind 出错时是否不会先删 body 再返回。 +- rewind 出错时是否不会删除部分数据出现不可逆的中间态。 ### PR 2 · `fix/minor-block-commit-status` @@ -87,9 +88,7 @@ Review focus： Issue / impact： -- DB 里已经有 body/state，不代表 x-shard broadcast 和 master header 上报已经完成。 - 旧代码把 body 存在和已提交混在一起，crash / 重试后无法判断哪些块需要补上提交。 -- 并发 rewind 时，marker 可能写到已经被删除 body 的块上。 Root cause： From 028f5dfbb83c85f835ca36cc566e38f81cbf789b Mon Sep 17 00:00:00 2001 From: ping-ke Date: Wed, 15 Jul 2026 19:11:19 +0800 Subject: [PATCH 4/4] update --- .../minor-commit-status-review.en.md | 14 ++++------- .../minor-commit-status-review.md | 23 ++++++++----------- 2 files changed, 14 insertions(+), 23 deletions(-) diff --git a/L1/quarkchain-fix/minor-commit-status-review.en.md b/L1/quarkchain-fix/minor-commit-status-review.en.md index 81dcc97..63d3b16 100644 --- a/L1/quarkchain-fix/minor-commit-status-review.en.md +++ b/L1/quarkchain-fix/minor-commit-status-review.en.md @@ -58,29 +58,25 @@ All three PRs are split out from the original large branch and are stacked: PR3 ### PR 1 · `fix/minor-chain-head-locking` -**Theme**: serialize minor head rewind and root reorg, and publish head-related fields together. +**Theme**: serialize minor head rewind and root reorg, and avoid conflicts with import / commit. This PR does not change commit marker semantics. Issue / impact: - `SetHead` / root reorg can conflict with minor block import / commit, so a body may be deleted while still being committed or referenced. -- During root reorg, head, root tip, EVM state, and canonical index are published in separate steps; other code can observe inconsistent state. Root cause: - rewind / root reorg / import were not fully serialized by the same set of locks. -- head-related fields were written step by step instead of computing the target first and publishing together. Fix: - Use the lock order `s.mu -> chainmu -> mu`, so rewind / root reorg / import are mutually exclusive. - `setHead` validates the target state first, then deletes bodies, then publishes head/state together. -- root reorg publishes root tip, confirmed tip, EVM state, and head together at the end; the no-head-publish path only rewrites canonical hash and does not delete sidechain bodies. Review focus: - Any reverse lock order or re-entrant deadlock? -- Are head, root tip, EVM state, and canonical index published under the same lock acquisition? - If rewind returns an error, can it return after deleting bodies? ### PR 2 · `fix/minor-block-commit-status` @@ -202,7 +198,7 @@ go test -race ./cluster/shard -run 'TestAddBlockListForSyncMarkerBodyConsistency ## Non-Goals -- **Read-side atomicity**: PR1 only changes write-side publication order. Read-side `CurrentBlock()` / - `GetBlockByNumber()` does not take `m.mu`, so while root reorg has rewritten canonical index but not yet - published the new head, readers may still observe "old head + new canonical index". Making read tip / state - use the same lock is follow-up work, tracked in [#693](https://github.com/QuarkChain/goquarkchain/issues/693). +- **Read-side atomicity**: this is not part of PR1; it is the follow-up tracked in + [#693](https://github.com/QuarkChain/goquarkchain/issues/693). The issue is that during root reorg / genesis reset, + `CurrentBlock()`, `GetBlockByNumber()`, and `State()` can observe different head / canonical-index / state snapshots. + That mixed-snapshot problem needs a separate fix. diff --git a/L1/quarkchain-fix/minor-commit-status-review.md b/L1/quarkchain-fix/minor-commit-status-review.md index c7e9b82..cceb541 100644 --- a/L1/quarkchain-fix/minor-commit-status-review.md +++ b/L1/quarkchain-fix/minor-commit-status-review.md @@ -21,7 +21,6 @@ - crash / restart 后分不清「已提交」还是「只写入了本地 body」。 - head rewind 可能删 body，同时另一条路径写 marker，最后 DB 里有 marker，但对应 body 已经没了。 -- root reorg 分几步更新 head/rootTip/currentEvmState，其他代码可能看到互相对不上的状态。 ## 拆分顺序 @@ -29,8 +28,7 @@ 1. **先解决并发问题**（PR1）。后面会把 commit marker 的写入从 body 保存之后延后到 xshard 广播 / 上报之后， 这会让“body 已经写入 DB、marker 还没写”的时间变长，冲突可能性增大。因此在改 marker 语义前，必须先保证 - rewind / root reorg 不会在这段时间里和 import / commit 冲突：PR1 把这些路径放到同一组锁下串行， - 并在同一次加锁里一起发布 currentBlock / rootTip / currentEvmState / canonical index。 + rewind / root reorg 不会在这段时间里和 import / commit 冲突：PR1 把这些路径放到同一组锁下串行。 2. **再把状态语义拆开**（PR2）。有了 PR1 的锁和发布顺序，PR2 才能安全地让 body 存在 ≠ 已提交，并把 marker 改为由 shard 层在广播 / 上报成功后主动写入。这样 `CommitMinorBlockByHash` 才能先确认 body 还在，再写 marker，保证 `marker => body`。 @@ -39,7 +37,7 @@ 只有 pruned sidechain 这种 `HasBodyWithoutState` 才能只靠 body 当作本地已有。基于这个语义， PR3 才能避免重复下载已有历史 body，并恢复那些「有 body 缺 marker」的历史块。 -这样拆以后，每个 PR 的 review 范围都比较单纯：PR1 只看锁和 head 发布，PR2 只看 body/marker 语义和 +这样拆以后，每个 PR 的 review 范围都比较单纯：PR1 只看锁，PR2 只看 body/marker 语义和 `marker => body`，PR3 只看 sidechain 已有 body 复用 / replay 恢复；reviewer 不需要在一个巨大 diff 里同时验证三类问题。 @@ -57,30 +55,25 @@ ### PR 1 · `fix/minor-chain-head-locking` -**主题**：串行化 minor head rewind 和 root reorg，保证 head 相关字段一起发布。本 PR 不碰 commit marker 语义。 +**主题**：串行化 minor head rewind 和 root reorg，避免它们和 import / commit 互相冲突。本 PR 不碰 commit marker 语义。 Issue / impact： - `SetHead` / root reorg 可能和 minor block import / commit 并发冲突，导致 body 被删后仍被提交或引用。 -- root reorg 过程中 head、root tip、EVM state、canonical index 分步发布，其他代码可能看到不一致状态。 -- 并发 rewind 时，marker 可能写到已经被删除 body 的块上。 Root cause： - rewind / root reorg / import 没有被同一组锁完整串行化。 -- head 相关字段逐步写入，而不是先算出目标，再一起发布。 Fix： - 统一锁顺序为 `s.mu -> chainmu -> mu`，让 rewind / root reorg / import 互斥。 - `setHead` 先校验目标 state，再删除 body，最后一次性发布 head/state。 -- root reorg 只在末尾一起发布 root tip、confirmed tip、EVM state 和 head；no-head-publish 路径只改 canonical hash，不删 sidechain body。 Review focus： - 是否存在反向锁顺序或重入死锁。 -- head、root tip、EVM state、canonical index 是否在同一次加锁里发布。 -- rewind 出错时是否不会删除部分数据出现不可逆的中间态。 +- rewind 出错时是否不会先删 body 再返回。 ### PR 2 · `fix/minor-block-commit-status` @@ -88,7 +81,9 @@ Review focus： Issue / impact： +- DB 里已经有 body/state，不代表 x-shard broadcast 和 master header 上报已经完成。 - 旧代码把 body 存在和已提交混在一起，crash / 重试后无法判断哪些块需要补上提交。 +- 并发 rewind 时，marker 可能写到已经被删除 body 的块上。 Root cause： @@ -196,6 +191,6 @@ go test -race ./cluster/shard -run 'TestAddBlockListForSyncMarkerBodyConsistency ## 非目标 -- **读侧原子性**：PR1 只调整了写侧发布顺序。读侧的 `CurrentBlock()` / `GetBlockByNumber()` 不走 `m.mu`， - 在 root reorg 重写 canonical 索引、还没发布新 head 的过程中，仍可能读到「旧 head + 新 canonical 索引」的 - 不一致状态。让 read tip / state 走同一把锁是后续工作，留到 [#693](https://github.com/QuarkChain/goquarkchain/issues/693)。 +- **读侧原子性**：这不是 PR1 要修的内容，而是 issue [#693](https://github.com/QuarkChain/goquarkchain/issues/693) 要跟的后续问题。 + 具体是 root reorg / genesis reset 时，`CurrentBlock()`、`GetBlockByNumber()`、`State()` 可能看到不同步的 head / canonical index / state。 + 这类“混合快照”需要单独处理，不在本 PR 范围内。