From 175630555395c7f50a091c75ac2c5a1e78e4212c Mon Sep 17 00:00:00 2001 From: ping-ke Date: Tue, 7 Jul 2026 16:05:37 +0800 Subject: [PATCH] docs(L1): add goquarkchain bugfix retro + lock call-chain docs Move the goquarkchain three-week bugfix retrospective (zh + en) and the MinorBlockChain lock call-chain reference (zh + en) into pm/L1, the main docs branch. Fix cross-directory links: ../locking.md -> ./locking.md; the incident doc link now points to the goquarkchain repo's fix/sync-unknown-ancestor branch (not mirrored into pm). Co-Authored-By: Claude Opus 4.8 (1M context) --- L1/goquarkchain-bugfix-retro.en.md | 154 ++++++++++++++++++++++++++ L1/goquarkchain-bugfix-retro.md | 155 ++++++++++++++++++++++++++ L1/locking.en.md | 168 +++++++++++++++++++++++++++++ L1/locking.md | 167 ++++++++++++++++++++++++++++ 4 files changed, 644 insertions(+) create mode 100644 L1/goquarkchain-bugfix-retro.en.md create mode 100644 L1/goquarkchain-bugfix-retro.md create mode 100644 L1/locking.en.md create mode 100644 L1/locking.md diff --git a/L1/goquarkchain-bugfix-retro.en.md b/L1/goquarkchain-bugfix-retro.en.md new file mode 100644 index 0000000..9a7598a --- /dev/null +++ b/L1/goquarkchain-bugfix-retro.en.md @@ -0,0 +1,154 @@ +# goquarkchain: Three-Week Bug-Fix Summary & Retrospective + +> For the team meeting. Time range: 2026-06-14 ~ 07-05. +> Purpose: goquarkchain is being gradually deprecated. We put a lot of effort into this series of fixes; if it stops at "it's fixed," the value is limited. This doc distills the root causes and the process so goshard's design and development can learn from it. + +--- + +## 0. Background: How Development & Review Actually Work Here + +goquarkchain has had four or five main developers over its lifetime, with modules cross-reviewed among the team. It draws on two reference sources, each covering a different part: + +- **Based on Geth**: a large amount of the lower layer is taken directly from Geth (block storage, insertChain, reorg, state trie, etc.). This part has a mainnet-proven, mature implementation to compare against. +- **Referencing pyquarkchain**: QuarkChain-specific features (sharding, xshard, commit status, etc.) don't exist in Geth, so their logic is modeled on pyquarkchain. + +What's truly **without reference** is the part wedged between the two: **how to splice QuarkChain-specific logic into Geth's existing call chains**, plus the **Go-vs-Python language differences** that make pyquarkchain's implementation impossible to copy directly. Take **commit status** as an example: goquarkchain also tried a `committing` intermediate state (following pyquarkchain's approach), but it turned out to add little value, and we finally converged on **using locks (`chainmu` / `mu`) to achieve the equivalent behavior**. These "insertion points" and the lock model can neither be copied from Geth nor ported straight from pyquarkchain — they can only be worked out by our own understanding plus others' review. + +The review process relies mainly on **individual reasoning-through** plus a few **simple principles** (conventions like "take the coarse lock before the fine lock"), and then surfaces problems through **test results** and **node run logs**. This approach (unchanged for the past five or six years) has two **long-standing, inherent limitations**: + +- **Effective for modules with a reference, weak on home-grown features**: the manual + principles + logs approach works well on modules that have a "Geth / pyquarkchain counterpart," but under-covers our **home-grown, reference-less features (xshard broadcast, submit head, commit status)** — which is exactly where things broke. +- **Race-type bugs are hard to catch by reading code and logs**: Bug1's trigger window is only the 10–100 ms of the network broadcast; ordinary tests and logs almost never catch it. + +This incident landed squarely in the un-referenceable zone: the core is a **race between AddBlock and AddRootBlock**, compounded by the **new features Geth doesn't have** — broadcasting xshard and submitting head to master. These new features are exactly where problems tend to arise, and Geth has no corresponding implementation to compare against. So this area can only be covered by manual review and case-by-case filtering, and manual filtering will always miss some combinations. **This chain of bugs is, in essence, the concentrated eruption of exactly those missed edge cases.** + +--- + +## 1. The Incident "Storyline" (the most instructive part) + +No single bug was fatal, but stacked together they formed a **self-perpetuating deadlock loop that survives crash-restart**. The full root-cause analysis is in `docs/incident/2026-05-18-shard40001-sync-unknown-ancestor.md` on the goquarkchain repo's `fix/sync-unknown-ancestor` branch. The chain: + +1. **A race** (Bug1) quietly produced an inconsistent DB state during normal operation: after `AddBlockListForSync` finished inserting a block and released `m.chainmu`, it did network broadcasts (10–100 ms); within that window a concurrent `AddRootBlock` triggered `setHead`, which atomically deleted body + marker + canonical hash; when the network call returned, the shard layer — **outside the lock** — called `CommitMinorBlockByHash` again and re-wrote the marker. + → End state: **body gone, canonical hash gone, but the commit marker present**. + +2. `HasBlock` at the time only checked the marker (Bug5); seeing the marker present, it returned `true` → sync assumed the block was there and **skipped re-downloading it**. + +3. Inserting the next block, `GetBlock(prev)` returned a **typed-nil** (non-nil interface wrapping a nil pointer); the `parent == nil` check failed (Bug2) → outright nil pointer panic, node crash. + +4. After the panic was fixed it became a graceful `unknown ancestor`, but it was still stuck: the sync batch loop, on hitting an "already-known block," wrongly used `return nil` (Bug3), **abandoning the entire rest of the batch**. + +5. Steps 1–4 above are the original incident chain. Then, **during the fix**, we tried a recovery approach of "roll back the root block (setRootBlock), fall back to an earlier block, and re-sync" to break the deadlock — and that rollback / replay path itself exposed a latent bug, e.g. `isSameRootChain` triggering `log.Crit` and exiting the process when an argument-order precondition is violated (Bug4). This isn't hit under normal operation. + +Key point: **only after all of these bugs are fixed can the node recover**. Fix any subset and the loop just changes shape and keeps hanging. The "Why No Single Bug Was Sufficient" table in the incident doc lays this combinatorial explosion out clearly. + +--- + +## 2. Bug Branch Inventory + +| Branch / PR | Type | Core problem | Status | +|---|---|---|---| +| `fix/bug2-typed-nil` (#680) | Language trap | Go typed-nil interface makes `== nil` fail, causing panic | ✅ Merged to master | +| `fix/bug4-issameroottchain-guard` | Precondition | Inverted args trigger `log.Crit` crash | Partly reverted & redone | +| `fix/setHead-reorg` | Data persistence | setHead deletes body / resets to genesis / wrong write ordering in same-chain reorg | Evolved into `RollbackToBlock` | +| `fix/deser-list-length-oom` (#685, #691) | **Security** | P2P message declares an oversized length prefix → OOM crash | ✅ Merged to master | +| `fix/commit-status` | Aggregate refactor | Final convergence of the above + lock-model rework | Currently active branch, 23 commits | + +--- + +## 3. Pitfalls Hit While Working With AI + +Much of this code was written with AI assistance — a clear efficiency gain, but we also hit a set of AI-specific pitfalls. Recorded here as experience for future AI-assisted work of this kind: + +**1. Every model has the "keeps getting more complex" disease.** +It tends to keep adding code and making repeated tweaks to "work around" a problem, introducing new problems, piling on more and more — instead of stepping back to find the root cause and subtract. A human has to actively call a stop, demand "solve it with less code," or just revert to a clean baseline and start over. + +**2. When a test fails, the AI tends to "revert the change" rather than understand why the test failed.** +A concrete example is the Bug1 fix: the original code already wrote the commit marker once inside `insertChain`, then after broadcasting xshard and submitting the header wrote it again outside the lock — **this second write is redundant and is precisely the race root cause**. After deleting it, a test that still used "is the marker present" to judge whether the block was inserted began to fail. Shown that failure, the AI didn't fix the test's judging method (it should check body, not marker); instead it **rolled the redundant write back in** to make the test green — effectively re-introducing the bug it had just fixed correctly. Lesson: watch it to distinguish "the test exposed a real bug" from "the test's assertion is stale." Watching isn't enough — better to **tell it the correct fix directly** — e.g. in this case, explicitly instruct "the marker is redundant, don't write it back; switch to judging insertion success by body," rather than tossing out "the test broke, take a look," or it will most likely game the test with a revert again. + +**3. The AI often doesn't recognize code it (or a human) already changed.** +In long sessions it forgets the current code state, re-proposes based on stale impressions, and overwrites existing changes — especially parts a human edited by hand that aren't in its own latest output. **Countermeasure: either have the AI make the change itself (then it recognizes it as its own), or before asking, explicitly tell it "which places I already changed by hand" so it aligns with the current code state before acting.** + +**4. The AI will carry a premise you hand it all the way through implementation, even if that premise is itself flawed.** +The AI tends to take an idea I gave it during the interaction as an established premise and carry it to the end, rather than questioning it. A concrete example: I initially believed **`chainmu` (the coarse lock) guarding `insertChain` was enough, other places use `mu` (the fine lock), plus the upper layer has its own locking**. This assumption was actually flawed, but throughout subsequent design and edits the AI just followed this line of thinking; worse, when asked to review, it couldn't find the problem either — because it was reviewing under the same premise I had "set the tempo" with. It was ultimately **the reviewer's comment that caught it**. Lesson: **the AI won't question the premise for you, and in a context where its own premise is polluted it can't review its way out of it.** So critical reviews are best done in a **clean context**: first `clear` the prior interaction (this step is key — merely switching models while keeping the same polluted context lets the new model get dragged along by the original premise), then review again; switching person / model helps if you can, but only if the context is clean. + +> Summary: AI massively accelerates writing code and looking things up, but it's unreliable at **maintaining global consistency, insisting on subtraction, questioning premises, and recognizing existing changes**. Humans must hold three control points — "goal / premise definition, root-cause judgment, and knowing when to stop" — and do critical reviews in a clean, post-`clear` context (just switching models without clearing the context doesn't help). + +--- + +## 4. Why This Took So Long + +The fix cycle stretched out; the time went mainly into three things: + +**1. Repeated modification — the same piece of code changed many rounds.** +Not fixed in one pass, but advanced round by round, each round triggered by a newly discovered problem: + +- First produced a version that **runs and rescues the node from the deadlock**, and committed it; +- then **split that big chunk of changes into multiple independent PRs** to submit and review separately; +- during the splitting and review, **discovered there was still a race condition hiding inside**, and went back to change it again; +- then surfaced a **scope-not-well-defined** problem — e.g. I had decided init-related logic was a one-time operation, out of scope, and deliberately left it untouched, but the reviewer's comment pointed out it had to be changed too, so it was changed again; +- finally found that **lock usage hadn't been thought through up front**: the holding ranges of `chainmu` / `mu` / `s.mu` were adjusted over multiple rounds (there are some edge-case paths that could go wrong, but with extremely low probability), and it only converged after walking every locking path and compiling [`locking.md`](./locking.md). + +The common thread across these rounds: **the boundaries / premises / overall structure weren't drawn clearly before starting** (scope not aligned, race not foreseen, lock model not designed holistically), so it could only be "find one spot, rework one round." + +**2. Interacting with the AI, and reviewing the AI's changes, took considerable time.** +Each of the pitfalls in Section 3 (getting more complex, reverting on error, not recognizing existing changes, following a wrong premise) takes several round-trips to correct; and in a context that's been "given the tempo," the AI's own review can't find the premise-level flaws either, so they often only surface after a `clear`-and-restart or the reviewer's comment. + +**3. Testing + running a test node to watch logs.** +This class of race / recovery bug can't be confirmed by reading code alone; you have to actually **run the node and watch the logs** to see whether it still hangs, whether phenomena like unknown ancestor still appear — a high cost per iteration. + +The latter two (AI interaction cost, slow node verification) also directly lead to the two goshard recommendations in the next section: let the AI review in a clean context, and push the "must run a node to verify" things down into fast race tests. + +--- + +## 5. Takeaways for goshard Design & Development (the key section) + +The thinking distilled from the previous sections (storyline, AI pitfalls, why it took so long) all points to concrete recommendations for goshard. Ten of them — the first seven are technical design principles, the last three are process and collaboration: + +**1. Reuse Geth's mature code as much as possible; the less home-grown deviation, the safer.** +Almost every bug this time landed on **features Geth doesn't have and we added ourselves** (xshard broadcast, submit head, commit status). Geth's core call chains (insertChain, reorg, setHead, typed-nil handling) are mainnet-proven over many years; the moment we cut into them, or wrap home-grown logic around them, we enter territory with no reference and no precedent. Principle: **if you can reuse a Geth path directly, don't rewrite it; when you must add home-grown features, try to make them a separate layer outside Geth's core rather than embedding them into critical paths like insertChain / setHead**, so as to shrink the surface of "no counterpart to review against." + +**2. Out-of-lock "patch-up" writes are a breeding ground for races.** +The root of Bug1: a write that should have been completed inside the lock (the commit marker) was moved out of the lock and redone because it had to wait for a network broadcast. Be wary of any pattern of "mutate state inside the lock, release the lock to do IO, then come back and patch in one more write." Principle: **the authoritative write of state should as much as possible be completed within the same lock's critical section, not patched up outside the lock after being interrupted by network IO**; if for performance (see item 3) you truly must release the lock midway, then you must treat this concurrent path as high-risk and back it with a targeted race test (see item 4), rather than relying on the gut feeling that "it probably won't collide." + +**3. Lock vs. performance needs a deliberate balance.** +Bug1 actually stemmed from a reasonable performance consideration: not wanting to hold `m.chainmu` throughout the 10–100 ms network broadcast (which would block the whole chain's insertion), so the lock was released — but that's exactly what opened the race window. Conversely, `fix/commit-status`'s final convergence was to **widen the lock scope** (AddRootBlock holds chainmu across rewind + reset + index-rewrite), buying correctness at the cost of concurrency. There's no free lunch. goshard should **make this trade-off explicitly**: which critical sections must be atomic (even if slow), which can be done outside the lock (even if they need extra idempotency/validation as a backstop) — rather than deciding implicitly by gut feel. The accompanying principles (like "coarse lock before fine lock," "no network IO while holding a lock") should be written down and checked item by item in review. + +**4. Home-grown concurrent paths need deterministic race tests; don't rely only on manual review + node runs as a backstop.** +As Background said, this review approach inherently can't catch race-type bugs (Bug1's window is only 10–100 ms), and the "run node + watch logs" verification loop is slow. Countermeasure: for **home-grown, Geth-referenceless concurrent paths**, add **targeted race tests (`-race` + a deterministic interleaving seam)**, and push the "must run a node to verify" parts down into fast, replayable unit / race tests. The `api_backend_test.go` added in `fix/commit-status` this time (using a seam to drive both empty-xshard branches with `-race` interleaving tests) is a demonstration of this idea and can serve as a template for goshard's concurrency tests. + +**5. Locks must be defined clearly before use, and maintain a lock call-chain doc from day one.** +This time the lock model was "fix wherever a problem shows up," with lots of back-and-forth, and it was only at the very end that all locking paths were walked and compiled into [`locking.md`](./locking.md). The fundamental issue is that **locks weren't treated up front as something to "design first, then use."** goshard should do the opposite: + +- **Define first**: first list clearly **which shared variables / state need protection** (e.g. `rootTip`, `currentBlock`, `confirmedHeaderTip`, the various tips / indexes), then decide which of them each lock **protects, where its responsibility boundary is, and in which scenarios it may be acquired** — pin down the "variable → lock" ownership before writing code, not add-as-you-go. Every protected variable should have a clear answer to "which lock owns it." +- **Fix the acquisition order**: give all locks a unified **acquisition order** (e.g. `s.mu → chainmu → mu`); no path may reverse it, eliminating AB-BA deadlock at the source. +- **Then use + keep the doc alive**: while coding, follow this definition, and from day one maintain a lock call-chain doc (which lock each path holds, acquisition order, why it's not re-entrant). + +In one line: **concurrency correctness rarely converges by "patching point by point"; the lock model must first be designed as a whole (responsibilities + acquisition order) before use**; if this definition / doc exists before you start, it saves a lot of rework. + +**6. Distinguish local (single-node / Geth-layer) from global (cluster / api_backend-layer) semantics; design in layers rather than sharing one function.** +A typical source of complexity this time was `HasBlock`: the same function was used both by the local Geth layer (asking "does this block's body exist in the local DB") and by the global `api_backend` layer (asking "is this block committed / usable in cluster semantics"). **The two scenarios define "have this block" fundamentally differently**; cramming them into one function made its semantics ping-pong over these three weeks (marker-only → marker+body → body-only → finally split into `HasBlock` / `HasCommittedBlock`), and every change rippled across a swath of callers. Lesson: goshard should, at design time, **clarify which queries / state are local-semantic and which are global-semantic, and layer them — each with its own function and name** — rather than letting one function serve two definitions across layers. Clear layering is itself complexity reduction. + +**7. Defensive input validation: validate the length prefix before allocating.** +The OOM one (#685): a P2P message declared a 4-byte length ≈ 4.3 billion, and `reflect.MakeSlice` tried to allocate tens of GB before reading the first byte. The 128MB command-size cap doesn't stop it, because the allocation is driven by the "declared count." goshard's deserialization layer: **any length/count field coming from the network must be validated against the remaining buffer length before being used to allocate**, and give frame size a hard upper bound. + +> The first seven are technical design principles. The three below are lessons from the **process and collaboration** that cost the most time this round, and matter just as much to goshard. + +**8. Frame the scope, premises, and overall structure clearly before writing code.** +This round's rework (Section 4) almost all stemmed from the same thing: **boundaries / premises / overall structure not drawn clearly before starting** — scope not aligned with the reviewer (should init be touched), race not foreseen, lock model not designed holistically — so it became "find one spot, rework one round." Principle: before starting, write down three things and reach agreement with the reviewer — **(a) scope**: which modules this touches, which it explicitly does not; **(b) premises / invariants**: which assumptions it relies on (e.g. "this lock is enough," "init is a one-time operation"), and those assumptions themselves should be questioned once up front; **(c) overall structure**: if concurrency is involved, draw the lock model first (see item 5); if a state machine is involved, list the invariants first. This framing is cheap; overturning it at the review stage is expensive. + +**9. Commit in small steps; don't "make a big change that runs, then go back and split PRs."** +This round's order was: first make a big change that rescued the node, then split it into multiple independent PRs — and it was during the splitting and review that a hidden race condition surfaced. Lesson: **a big change running as a whole doesn't mean each part is correct**; cut it into a batch of small, independent commits that can each be reviewed and tested on their own, so problems (especially races and boundaries) surface earlier in a smaller diff, instead of being fished out only when splitting PRs. goshard's concurrency / recovery changes especially should go in small steps. + +**10. Developing with AI needs matching engineering constraints.** +goshard will use AI heavily too, so turn Section 3's pitfalls into positive constraints: **(a) do critical reviews in a clean context**: `clear` the conversation, or more thoroughly — `git clone` the code to a new directory and open a brand-new session to review (isolating even the working-tree state); merely switching models without clearing the context doesn't help, the polluted premise gets carried along; **(b) on a test failure / sticking point, either tell it the correct fix directly, or have it give a root-cause analysis and a plan first, and only change code after you confirm** — rather than tossing out "it broke, take a look" and letting it act on its own; otherwise it tends to game the test with a revert; the latter (analyze first, then change) is less effort — you don't have to think out the fix yourself, and it still blocks it from blindly reverting; **(c) have the AI make the change, or before asking explicitly tell it "which places the human already changed by hand"**, to keep it from failing to recognize existing changes and overwriting correct code; **(d) actively control the size of each change and watch complexity**: have it change in small steps, one small piece at a time, and step in as soon as complexity explodes (constantly adding code to work around a problem, introducing new ones), demanding subtraction or a revert to a clean baseline, rather than letting it snowball. The core: **the AI won't question the premise for you and doesn't guarantee global consistency — these two control points must be held by a human.** + +--- + +## 6. The Direct Value of This Fix to goshard + +Beyond the distilled experience, this fix itself carries a layer of **direct code value** for goshard: many of goshard's implementations are **copied from** goquarkchain right now, especially the QuarkChain-specific, Geth-referenceless logic (xshard, commit status, sync / recovery paths) — and those are exactly the disaster area of this bug chain. In other words, had we not fixed them, what goshard copied over would be **code carrying these race and recovery defects**; after the fix, the correctness of this logic is substantially solidified, and goshard now has a **cleaner, more reliable foundation** to keep building on. So this investment isn't just "rescuing a project that's being deprecated" — it's also clearing mines ahead of time for the very code goshard is going to reuse. + +--- + +## 7. One-Line Summary + +> This wasn't fixing 6 bugs; it was fixing one systemic problem — "a distributed state machine's invariants on the crash-recovery path being quietly violated in multiple places." Those invariants are concentrated in the **home-grown features Geth offers no reference for**, and are hard to exhaust by manual review. If goshard, from the very start, treats **atomicity of state, recovery invariants, interface nil-checking, and batch-processing control flow** as first-class design concerns, and backs its home-grown concurrent paths with deterministic race tests, it can avoid this whole class of pitfalls — and the portion of goquarkchain code it's going to reuse has, this time, been made more solid too. diff --git a/L1/goquarkchain-bugfix-retro.md b/L1/goquarkchain-bugfix-retro.md new file mode 100644 index 0000000..e9454c6 --- /dev/null +++ b/L1/goquarkchain-bugfix-retro.md @@ -0,0 +1,155 @@ +# goquarkchain 近三周 Bug 修复总结与复盘 + +> 面向组会分享。时间范围:2026-06-14 ~ 07-05。 +> 目的:goquarkchain 正在逐步 deprecate,这次投入较多精力做的一系列 fix,如果只停留在"修好了",意义有限。本文把根因和过程沉淀下来,给 goshard 的设计与开发做借鉴。 + +--- + +## 〇、Background:开发与 Review 的现状 + +goquarkchain 前后有四五个主要开发者参与,模块之间靠大家相互 review。它有两个参考来源,各自覆盖不同的部分: + +- **基于 Geth**:底层大量代码直接沿用 Geth(区块存储、insertChain、reorg、state trie 等),这部分有主网多年验证过的成熟实现可以对照。 +- **参考 pyquarkchain**:QuarkChain 特有的功能(分片、xshard、commit status 等)在 Geth 里没有,逻辑上参考 pyquarkchain。 + +真正**无从参考**的是夹在两者之间的部分:**如何把 QuarkChain 特有的逻辑插进 Geth 的既有链路**,以及 **Go 与 Python 的语言差异**导致 pyquarkchain 的实现没法直接照搬。例如 **commit status**:goquarkchain 这边当时也试过做 `committing` 中间态(对应 pyquarkchain 的思路),但后来发现意义不大,最终是**用锁(`chainmu` / `mu`)来实现相应的功能**收敛的——这类"插入点"和锁模型,既没有 Geth 可抄,也没有 pyquarkchain 可直接移植,只能靠自己理解 + 他人 review。 + +Review 的过程,主要依靠**个人理清思路** + 一些**简单的 principle**(比如"先用大锁再用小锁"这类约定),再通过**测试结果**和**节点运行日志**来发现问题。这套方式(五六年前就是这样)有两个**长期存在的固有局限**: + +- **对有对照的模块有效,对自研功能覆盖不足**:靠人工 + principle + 日志的方式,在"Geth / pyquarkchain 有对照"的模块上很好用,但对**我们自研、无处参考的功能(xshard 广播、submit head、commit status)** 覆盖不足——恰恰是这些地方出了问题。 +- **竞态类 bug 靠读代码和看日志很难发现**:Bug1 的触发窗口只有网络广播那 10–100ms,常规测试和日志几乎抓不到。 + +这次事故正好落在参考不到的地方:核心是 **AddBlock 与 AddRootBlock 之间的竞态**,再叠加我们**新增的、Geth 没有的功能**——broadcast xshard、submit head 给 master。正是这些新功能上容易出问题,而它们在 Geth 里没有对应实现可以对照。于是这块只能靠人工 review 和逐个过滤 case,但人工过滤终究可能漏掉某些组合。**这次的一连串 bug,本质就是这些被漏掉的 edge case 集中爆发的结果。** + +--- + +## 一、这次事故的"故事线"(最有借鉴价值的部分) + +单个 bug 都不足以致命,但叠在一起形成了一个**跨越 crash-重启、自我延续的死循环**。完整根因分析见 goquarkchain 仓库 `fix/sync-unknown-ancestor` 分支的 `docs/incident/2026-05-18-shard40001-sync-unknown-ancestor.md`。链条如下: + +1. **一个竞态**(Bug1)在正常运行时就悄悄制造了不一致的 DB 状态:`AddBlockListForSync` 插完块、释放 `m.chainmu` 后,去做网络广播(10–100ms),这个窗口里并发的 `AddRootBlock` 触发 `setHead` 原子删掉了 body + marker + canonical hash;网络调用回来后,shard 层**在锁外**又调了一次 `CommitMinorBlockByHash`,把 marker 继续写入。 + → 终态:**body 没了、canonical hash 没了、commit marker 却在**。 + +2. `HasBlock` 当时只查 marker(Bug5),看到 marker 在就返回 `true` → sync 认为这块有了,**跳过重新下载**。 + +3. 下一个块插入时 `GetBlock(前块)` 返回一个 **typed-nil**(接口非 nil、里面指针是 nil),`parent == nil` 判断失效(Bug2)→ 直接 nil pointer panic,节点崩溃。 + +4. 修好 panic 后变成优雅的 `unknown ancestor`,但还是卡住;因为 sync 批处理循环遇到"已知块"时错用了 `return nil`(Bug3),**直接放弃了整批后续块**。 + +5. 上面 1–4 是原始事故链。而在**修复过程中**,我们尝试用"回退根块(setRootBlock)、退回到之前的区块、重新 sync"这条恢复思路去解掉卡死,结果这条回退 / 重跑路径自己又暴露出潜藏的 bug,例如 `isSameRootChain` 在参数顺序违反前置条件时触发 `log.Crit` 直接退进程(Bug4)。正常情况不会碰到这个问题。 + +关键点:**只有把这些 bug 全部修掉,节点才能恢复**。修任何一个子集,循环都会换个形态继续卡死。事故文档里 "Why No Single Bug Was Sufficient" 那张表把这个组合爆炸讲得很清楚。 + +--- + +## 二、各条 bug 分支清单 + +| 分支 / PR | 类型 | 核心问题 | 状态 | +|---|---|---|---| +| `fix/bug2-typed-nil` (#680) | 语言陷阱 | Go typed-nil 接口使 `== nil` 判断失效导致 panic | ✅ 已合入 master | +| `fix/bug4-issameroottchain-guard` | 前置条件 | 参数反序触发 `log.Crit` 崩溃 | 部分被 revert 重做 | +| `fix/setHead-reorg` | 数据持久化 | setHead 删 body / 重置创世 / 同链 reorg 写序错误 | 演进为 `RollbackToBlock` | +| `fix/deser-list-length-oom` (#685, #691) | **安全** | P2P 消息声明超大长度前缀 → OOM 崩溃 | ✅ 已合入 master | +| `fix/commit-status` | 汇总重构 | 上述修复的最终收敛 + 锁模型重整 | 当前活跃分支,23 commits | + + +--- + +## 三、用 AI 协作过程中踩到的坑 + +这次大量代码由 AI 辅助完成,效率提升明显,但也踩到一批 AI 特有的坑。记下来作为后续用 AI 做这类工作的经验: + +**1. 所有模型都有"越改越复杂"的通病。** +倾向于不断加代码、反复修改来"绕过"问题,引入新问题,越改越多,而不是回头找根因做减法。需要人主动喊停,要求"用更少的代码解决",或直接回退到干净基线重来。 + +**2. 测试一出错,AI 倾向于把改动"改回去",而不是去理解测试为什么失败。** +一个具体例子就是 Bug1 的修复:原代码在 `insertChain` 里已经写了一次 commit marker,广播 xshard、submit header 之后又在锁外写了一次——**这第二次是多余的、正是竞态根源**。删掉它之后,有个测试仍然用"marker 在不在"来判断块是否插入成功,于是测试失败了。给 AI 看这个失败,它没有去 fix 测试的判断方式(应该改判 body 而不是 marker),而是**直接把删掉的那次多余写回滚回去**让测试变绿——等于把刚修对的 bug 又改了回去。教训:要盯着它区分"测试暴露了真 bug"和"测试本身的断言过时了"。光盯着不够,最好**直接把正确的修法告诉它**——比如这个例子里,明确指示"marker 是多余的、不要写回来,改成用 body 判断插入是否成功",而不是丢一句"测试挂了你看看",否则它大概率又用回滚来骗过测试。 + +**3. AI 经常不认自己(或人)已经改过的代码。** +在长会话里会忘记当前代码状态,基于旧印象重复提议、覆盖掉已有修改,尤其是人手动改过、而不在它这轮输出里的部分。**对策:要么让 AI 自己动手改(它就认得这是它的改动),要么在提问前把"我已经手动改了哪些地方"明确告诉它,让它先对齐当前代码状态再动手。** + +**4. AI 会顺着你抛出的前提一路实现下去,哪怕这个前提本身有漏洞。** +AI 倾向于把我在交互中给出的想法当成既定前提贯彻到底,而不是质疑它。一个具体例子:我一开始认为 **`chainmu`(大锁)锁住 `insertChain` 就够了,其他地方用 `mu`(小锁),再加上上层也有相应的锁保护**。这个假设其实有漏洞,但在之后的设计和修改里,AI 全程都顺着这个思路走;更糟的是,让它 review 时它也发现不了——因为它 review 时用的还是同一套被我"带过节奏"的前提。最后是 **reviewer 给 comment 才发现**的。教训:**AI 不会替你质疑前提,自身前提被污染的上下文里它也 review 不出问题**。所以关键 review 最好放到一个**干净的上下文**里做:先 `clear` 清掉之前的交互(这一步是关键——只换模型但沿用同一段被污染的上下文,新模型照样会被原来的前提带跑),再重新 review 一遍;有条件的话换人 / 换模型效果更好,但前提仍是上下文是干净的。 + +> 小结:AI 能大幅加速写代码和查资料,但在**保持全局一致性、坚持做减法、质疑前提、认准既有修改**这几件事上不可靠。人要守住"目标 / 前提定义、根因判断、何时停手"这三个控制点,并把关键 review 放到 `clear` 后的干净上下文里做(单换模型而不清上下文没用)。 + +--- + +## 四、这次为什么这么耗时 + +这次修复周期拉得比较长,时间主要花在三件事上: + +**1. 反复修改,同一片代码改了很多轮。** +不是一次改到位,而是一轮轮推进,每一轮都由新发现的问题触发: + +- 最初先做出一个**能跑起来、把节点从卡死里救活**的版本提交; +- 然后把这一大坨改动**拆成多个独立 PR** 分别提交、分别 review; +- 拆分和 review 过程中又**发现里面还藏着 race condition**,回头再改; +- 接着暴露出**修改范围(scope)没定义好**的问题——比如我一开始认定 init 相关逻辑是一次性操作、不在改动范围内,刻意不碰,但 reviewer 的 comment 指出这里也得动,于是又改; +- 最后发现**锁的使用一开始没梳理清楚**,`chainmu` / `mu` / `s.mu` 的持有范围又调整了多轮(存在一些 edge case 路径可能出问题、但触发概率极低),直到把所有加锁路径过一遍、整理成 [`locking.md`](./locking.md) 才收敛。 + +这几轮的共性是:**边界 / 前提 / 整体结构没在动手前划清楚**(scope 没对齐、race 没预见、锁模型没通盘设计),于是只能"发现一处、返工一轮"。 + +**2. 跟 AI 交互、以及 review AI 的修改,花了不少时间。** +第三节那几个坑(越改越复杂、遇错回滚、不认既有改动、顺着错误前提走)每一个都要几轮来回才能纠正;而且被"带过节奏"的上下文里 AI 自己 review 也发现不了前提性漏洞,往往得 `clear` 重来或等 reviewer 的 comment 才暴露。 + +**3. 测试 + 跑测试节点看日志。** +这类竞态 / 恢复类 bug 没法只靠读代码确认,得实际**跑节点、盯日志**看是否还会卡死、是否还有 unknown ancestor 之类的现象,单轮验证成本很高。 + +后面两件事(AI 交互成本、跑节点验证慢)也直接引出下一节对 goshard 的两条建议:让 AI 在干净上下文里 review、把"必须跑节点才能验"的东西下沉成快速 race 测试。 + +--- + +## 五、对 goshard 设计与开发的借鉴(重点) + +前面几节(故事线、AI 协作的坑、耗时原因)沉淀下来的思考,最终都指向对 goshard 的具体建议。提炼十条——前七条是技术设计原则,后三条是流程与协作: + +**1. 尽量沿用 Geth 的成熟代码,自研偏离越少越安全。** +这次几乎所有 bug 都落在 **Geth 没有、我们自己加的功能**上(xshard 广播、submit head、commit status)。Geth 的核心链路(insertChain、reorg、setHead、typed-nil 处理)是被主网多年验证过的,我们在上面动刀、或在它周围包一层自研逻辑时,就进入了没有参考、没有先例的区域。原则:**能直接复用 Geth 的路径就不要重写;确实要加自研功能时,尽量把它做成 Geth 核心之外的独立层,而不是嵌进 insertChain / setHead 这些关键路径里**,以缩小"无对照可 review"的面积。 + +**2. 锁外的"补写"操作是竞态温床。** +Bug1 的根源:一个本该在锁内完成的写(commit marker),因为要等网络广播,被挪到锁外重做了一次。凡是"先在锁内改状态、释放锁做 IO、再回来补一笔"的模式都要警惕。原则:**状态的权威写入尽量在同一把锁的临界区内完成,不要被网络 IO 打断后在锁外补齐**;如果为了性能(见第 3 条)确实要中途释放锁,就必须把这条并发路径当成高危,补上针对性的 race 测试(见第 4 条)来兜底,而不是靠"应该不会撞上"的直觉。 + +**3. 锁与性能之间需要有意识地权衡(balance)。** +Bug1 其实源于一个合理的性能考量:不想在长达 10–100ms 的网络广播期间一直持有 `m.chainmu`(否则整条链的插入都被阻塞),于是释放了锁——但这恰恰开了竞态窗口。反过来,`fix/commit-status` 最终的收敛是**扩大锁范围**(AddRootBlock 全程持 chainmu 覆盖 rewind + reset + index-rewrite),换来了正确性但牺牲了并发度。这里没有免费的午餐。goshard 要**明确地做这个权衡**:哪些临界区必须原子(哪怕慢)、哪些可以在锁外做(哪怕要额外的幂等/校验来兜底),而不是凭直觉隐式决定。配套的 principle(如"先大锁后小锁""持锁不做网络 IO")要写下来,并在 review 时逐条对照。 + +**4. 自研并发路径要补确定性 race 测试,别只靠人工 review + 跑节点兜底。** +Background 里说了,这套 review 方式对竞态类 bug 天然抓不住(Bug1 窗口只有 10–100ms),而"跑节点 + 看日志"验证的闭环又很慢。对策:对**自研的、Geth 无对照的并发路径**,补上**针对性的 race 测试(`-race` + 确定性 interleaving seam)**,把"必须跑节点才能验"的东西尽量下沉成可快速重放的单元 / race 测试。这次 `fix/commit-status` 里新增的 `api_backend_test.go`(用 seam 驱动两个 empty-xshard 分支、`-race` 交织测试)就是这个思路的示范,可作为 goshard 并发测试的模板。 + +**5. 锁要在使用前先定义清楚,并从第一天就维护一份锁调用链文档。** +这次锁模型是"哪里出问题修哪里",反复很多,直到最后才把所有加锁路径过一遍、整理成 [`locking.md`](./locking.md)。根本问题是**锁在动手前没有被当成一件需要"先设计、后使用"的事**。goshard 应该反过来做: + +- **先定义**:先列清楚**哪些共享变量 / 状态需要被保护**(比如 `rootTip`、`currentBlock`、`confirmedHeaderTip`、各类 tip / index),再决定每把锁**保护其中哪几个、职责边界在哪、允许在哪些场景被获取**——在写代码前就明确"变量→锁"的归属,而不是用到哪加到哪。每个受保护变量都应能明确回答"它归哪把锁管"。 +- **定好获取顺序**:所有锁的**先后获取顺序**统一规定(比如 `s.mu → chainmu → mu`),任何路径都不许反向,从源头杜绝 AB-BA 死锁。 +- **再使用 + 持续维护文档**:动手时按这套定义用,并从第一天就维护一份锁调用链文档(每条路径持哪把锁、获取顺序、为什么不重入)。 + +一句话:**并发正确性靠"逐点打补丁"很难收敛,锁必须先作为一个整体设计清楚(职责 + 获取顺序)再使用**;这份定义 / 文档如果在动手前就存在,能省掉大量返工。 + +**6. 区分 local(单节点 / Geth 层)与 global(集群 / api_backend 层)语义,分层设计而不是共用一个函数。** +这次一个典型的复杂度来源是 `HasBlock`:同一个函数既被 local 的 Geth 层用(问的是"本地 DB 里这个块的 body 在不在"),又被 global 的 `api_backend` 层用(问的是"这个块在集群语义下是否已提交 / 可用")。**两个场景对"有这个块"的定义根本不同**,硬塞进一个函数,导致它的语义在这三周里反复横跳(只查 marker → 查 marker+body → 只查 body → 最后才拆成 `HasBlock` / `HasCommittedBlock` 两个),每次改都牵动一片调用方。教训:goshard 要在设计阶段就**理清哪些查询 / 状态是 local 语义、哪些是 global 语义,分层、各用各的函数和名字**,别让一个函数跨层服务两种定义。分层清楚本身就是降复杂度。 + +**7. 防御性输入校验:长度前缀先验证再分配。** +OOM 那条(#685):P2P 消息声明 4 字节长度 ≈ 43 亿,`reflect.MakeSlice` 在读第一个字节前就想分配几十 GB。128MB 的命令大小上限拦不住,因为分配是被"声明的数量"驱动的。goshard 的反序列化层:**任何来自网络的长度/计数字段,在用它做分配之前,必须先跟剩余 buffer 长度校验**,并给 frame size 设硬上限。 + +> 前七条是技术层面的设计原则。下面三条是这次在**流程和协作**上花掉最多时间换来的教训,对 goshard 同样重要。 + +**8. 动手前先把范围、前提、整体结构 framing 清楚,再写代码。** +这次的返工(第四节)几乎都源于同一件事:**边界 / 前提 / 整体结构没在动手前划清楚**——scope 没和 reviewer 对齐(init 该不该动)、race 没预见、锁模型没通盘设计,于是"发现一处、返工一轮"。原则:开工前先把三件事写下来并和 reviewer 达成一致——**(a) 范围**:这次碰哪些模块、明确不碰哪些;**(b) 前提 / 不变量**:依赖哪些假设(比如"这套锁够用""init 是一次性操作"),这些假设本身要先被质疑一遍;**(c) 整体结构**:涉及并发就先画锁模型(见第 5 条)、涉及状态机就先列不变量。这些 framing 是廉价的,拖到 review 阶段才推翻则代价高昂。 + +**9. 小步提交,别"先大改跑通、再回头拆 PR"。** +这次的顺序是:先做一个能把节点救活的大改动,再拆成多个独立 PR,而拆分和 review 的过程中才发现里面还藏着 race condition。教训:**大改动整体跑通不等于每一部分都对**,把它切成一批小而独立、每步都能单独 review 和测试的提交,能让问题(尤其是竞态、边界)在更小的 diff 里更早暴露,而不是等拆 PR 时才回头捞。goshard 的并发 / 恢复类改动尤其要小步走。 + +**10. 用 AI 开发要有配套的工程约束。** +goshard 会大量用 AI,把第三节的坑转成正向约束:**(a) 关键 review 放到干净上下文里做**:`clear` 清掉对话,或者更彻底——把代码 `git clone` 到一个新目录、开一个全新会话来 review(连工作区状态都一并隔离掉);单换模型而不清上下文没用,污染的前提会被继续带着走;**(b) 遇到测试失败 / 卡点,要么直接把正确修法告诉它,要么让它先给出原因分析和方案、你确认后再改代码**,而不是丢一句"挂了你看看"就让它自己动手——否则它倾向用回滚骗过测试;后一种(先分析后改)更省力,你不用自己先想出修法,又能拦住它闷头回滚;**(c) 让 AI 动手改、或在提问前明确告诉它"人已经手动改了哪些"**,避免它不认既有修改、覆盖掉正确代码;**(d) 主动控制每次改动的代码量、盯住复杂度**:让它小步改、每次只动一小块,一旦发现复杂度暴增(为绕过问题不断加代码、引入新问题)就及时介入,要求做减法或回退到干净基线,而不是等它越滚越大;核心一条:**AI 不会替你质疑前提、也不保证全局一致性,这两个控制点必须由人守住。** + +--- + +## 六、这次修复对 goshard 的直接价值 + +除了上面提炼的经验,这次修复本身对 goshard 还有一层**直接的代码价值**:goshard 现在很多实现是从 goquarkchain **copy 过来**的,尤其是 QuarkChain 特有、Geth 无对照的那部分逻辑(xshard、commit status、sync / recovery 路径)——而这些恰恰是这次一连串 bug 的重灾区。也就是说,如果不修,goshard copy 过去的就是一份**带着这些竞态和恢复缺陷的代码**;修完之后,这部分逻辑的正确性被显著夯实了,goshard 在它之上继续开发有了一个**更干净、更可靠的基础**。所以这次投入不只是"救活一个正在 deprecate 的项目",更是给 goshard 要复用的那块代码提前排了雷。 + +--- + +## 七、一句话总结 + +> 这次不是修 6 个 bug,是修一个"分布式状态机在崩溃恢复路径上的不变量被多处悄悄违反"的系统性问题。这些不变量集中在 **Geth 无从参考的自研功能** 上,靠人工 review 难以穷尽。goshard 如果从一开始就把 **状态的原子性、恢复的不变量、接口判空、批处理控制流** 当成一等公民来设计,并对自研并发路径补上确定性 race 测试,就能整类避免这次踩的坑;而且它要复用的那部分 goquarkchain 代码,这次也已经被修得更扎实了。 diff --git a/L1/locking.en.md b/L1/locking.en.md new file mode 100644 index 0000000..72a15fd --- /dev/null +++ b/L1/locking.en.md @@ -0,0 +1,168 @@ +# MinorBlockChain Lock Call Chains + +> Verified against commit 6667712. Line numbers drift as the code changes. + +## Locks + +| Lock | Owner | Purpose | Declaration | +|------|-------|---------|-------------| +| `m.mu` | `MinorBlockChain` | Global chain-state lock (RWMutex) | `minorblockchain.go:109` | +| `m.chainmu` | `MinorBlockChain` | Block-insertion lock (RWMutex) | `minorblockchain.go:110` | +| `s.mu` | `ShardBackend` | Shard-level serialization lock (Mutex) | `shard.go:59` | + +--- + +## 1. GetUnconfirmedHeaderList + +``` +cluster/slave/api_backend.go SlaveBackend.GetUnconfirmedHeaderList +|-> cluster/shard/api_backend.go:281 ShardBackend.GetUnconfirmedHeaderList + |-> core/minorblockchain_addon.go:783 MinorBlockChain.GetUnconfirmedHeaderList + (m.mu.Lock) +``` + +--- + +## 2. AddRootBlock + +``` +cluster/slave/api_backend.go SlaveBackend.AddRootBlock +|-> cluster/shard/api_backend.go:203 ShardBackend.AddRootBlock + (s.mu.Lock) ← serializes root/minor handling for the same shard + |-> core/minorblockchain_addon.go:992 MinorBlockChain.AddRootBlock + |-> 1. pre-checks + putRootBlock [no lock] + |-> 2. no-change-to-root-tip early return :1079 + | (m.mu.Lock snapshot rootTip; m.mu.Unlock) + |-> 3. m.chainmu.Lock :1098 (deferred to end of fn) ← mutually exclusive with the insertChain pipeline + |-> 4. m.mu.Lock :1103 + | update rootTip / confirmedHeaderTip + | rewind currentBlock onto the same root chain + | compute needGenesisReset :1135 + | m.mu.Unlock :1138 + |-> 5. [needGenesisReset] m.Reset :1158 + | |-> ResetWithGenesisBlock + | |-> setHead(0) [unlocked; holds chainmu only] + |-> 6. reWriteBlockIndexTo :1176 + (m.mu.Lock — reorg + publish currentEvmState) +``` + +> Key: steps 5 and 6 run **after** `m.mu.Unlock` (1138). +> `Reset` uses the **unlocked `setHead`** (does not re-acquire chainmu/mu); `reWriteBlockIndexTo` takes only mu. +> chainmu is held throughout, so the reorg is mutually exclusive with the insertChain pipeline; no re-entrancy, no self-deadlock. + +--- + +## 3. CreateShards → initGenesisState + +``` +cluster/slave/api_backend.go SlaveBackend.CreateShards +|-> cluster/shard/api_backend.go:191 ShardBackend.InitFromRootBlock + |-> cluster/shard/shard.go:180 ShardBackend.initGenesisState + |-> 1. core/minorblockchain_addon.go:329 MinorBlockChain.InitGenesisState + | (m.mu.Lock) + |-> 2. conn.BroadcastXshardTxList [no lock] + |-> 3. core/minorblockchain_addon.go:1214 GetShardStats + | |-> getBlockCountByHeight :1379 + | (m.mu.RLock) + |-> 4. conn.SendMinorBlockHeaderToMaster [no lock] +``` + +--- + +## 4. NewMinorBlock → AddMinorBlock + +``` +cluster/slave/api_backend.go SlaveBackend.NewMinorBlock +|-> cluster/shard/api_backend.go:303 ShardBackend.NewMinorBlock + | (HasCommittedBlock precheck; no lock) + |-> cluster/shard/api_backend.go:365 ShardBackend.AddMinorBlock + |-> 0. fast-path getBlockCommitStatusByHash [lock-free precheck] + (s.mu.Lock) ← double-check afterwards + |-> 1. core/minorblockchain.go:1048 InsertChainForDeposits (force=true) + | (m.chainmu.Lock) + | |-> WriteBlockWithState :914 (m.mu.Lock) + | |-> updateTip :158 (m.mu.Lock) + | (m.chainmu.Unlock) + | |-> post-insert reads confirmedHeaderTip (m.mu.Lock) + |-> 2. conn.BroadcastXshardTxList [no lock] + | [error] s.setHead :601 -> MinorBlockChain.SetHead :318 (chainmu.Lock -> m.mu.Lock) + |-> 3. GetShardStats :1214 -> getBlockCountByHeight (m.mu.RLock) + | [error] s.setHead -> SetHead :318 (chainmu.Lock -> m.mu.Lock) + |-> 4. conn.SendMinorBlockHeaderToMaster [no lock] + | [error] s.setHead -> SetHead :318 (chainmu.Lock -> m.mu.Lock) + |-> 5. core/minorblockchain_addon.go:1931 CommitMinorBlockByHash + | (m.mu.Lock; checks HasBlock then writes marker; returns false→ErrBodyDeleted) + |-> 6. broadcastNewTip :591 + |-> GetRootTip :1434 (m.mu.RLock) +``` + +> `s.setHead` (shard layer, api_backend.go:601) calls `MinorBlockChain.SetHead` (chainmu→mu). +> On error branches setHead is invoked while holding **only s.mu**, not m.chainmu/m.mu, so SetHead re-acquiring both is not re-entrant. + +--- + +## 5. AddBlockListForSync + +``` +cluster/slave/api_backend.go SlaveBackend.AddBlockListForSync +| (HasCommittedBlock filters already-committed blocks) +|-> cluster/shard/api_backend.go:465 ShardBackend.AddBlockListForSync + (s.mu.Lock) + |-> [per block] InsertChainForDeposits (force=true) :1048 + | (m.chainmu.Lock) + | |-> WriteBlockWithState :914 (m.mu.Lock) + | |-> updateTip :158 (m.mu.Lock) + | (m.chainmu.Unlock) + | |-> post-insert reads confirmedHeaderTip (m.mu.Lock) + |-> conn.BatchBroadcastXshardTxList [no lock] + |-> conn.SendMinorBlockHeaderListToMaster [no lock] + |-> [per header] CommitMinorBlockByHash :1931 + (m.mu.Lock; returns false→ErrBodyDeleted, aborts the whole batch) +``` + +--- + +## 6. CreateBlockToMine + +``` +cluster/shard/api_backend.go:640 ShardBackend.CreateBlockToMine +|-> core/minorblockchain_addon.go:898 MinorBlockChain.CreateBlockToMine + |-> 1. m.mu.Lock :937 snapshot rootTip / currentBlock; m.mu.Unlock :940 + |-> 2. getEvmStateByBlock :1692 + (m.mu.Lock — only when block == CurrentBlock) +``` + +--- + +## 7. CheckMinorBlocksInRoot + +``` +cluster/slave/api_backend.go SlaveBackend.CheckMinorBlocksInRoot +|-> cluster/shard/api_backend.go:668 ShardBackend.CheckMinorBlock + |-> core/minorblockchain.go:1048 InsertChainForDeposits (force=true) + (m.chainmu.Lock) + |-> WriteBlockWithState :914 (m.mu.Lock) + |-> updateTip :158 (m.mu.Lock) + (m.chainmu.Unlock) + |-> post-insert reads confirmedHeaderTip (m.mu.Lock) +``` + +--- + +## Lock Acquisition Order + +All call paths acquire locks in the same order — no reverse acquisition, no AB-BA deadlock. + +``` +s.mu (ShardBackend) + └─ m.chainmu (MinorBlockChain insertion lock) + └─ m.mu (MinorBlockChain state lock) +``` + +> **Note on AddRootBlock:** holds `m.chainmu` for the whole function (deferred at 1098). `m.mu` is explicitly +> unlocked at :1138, and only afterwards does it call `Reset` (which uses the unlocked `setHead`, taking neither +> chainmu nor mu) and `reWriteBlockIndexTo` (which takes only m.mu). Hence there is no re-entrant acquisition of +> chainmu/mu. +> +> **Error-branch setHead:** `ShardBackend.setHead` is only invoked while holding `s.mu`. Its inner +> `MinorBlockChain.SetHead` acquires chainmu→mu in the same order as the main path, so it is not re-entrant. diff --git a/L1/locking.md b/L1/locking.md new file mode 100644 index 0000000..3cfa40c --- /dev/null +++ b/L1/locking.md @@ -0,0 +1,167 @@ +# MinorBlockChain Lock Call Chains + +> 基于 commit 6667712 核实。行号随代码变动会漂移。 + +## Locks + +| Lock | Owner | Purpose | 声明 | +|------|-------|---------|------| +| `m.mu` | `MinorBlockChain` | 全局链状态锁 (RWMutex) | `minorblockchain.go:109` | +| `m.chainmu` | `MinorBlockChain` | 区块插入锁 (RWMutex) | `minorblockchain.go:110` | +| `s.mu` | `ShardBackend` | Shard 级串行化锁 (Mutex) | `shard.go:59` | + +--- + +## 1. GetUnconfirmedHeaderList + +``` +cluster/slave/api_backend.go SlaveBackend.GetUnconfirmedHeaderList +|-> cluster/shard/api_backend.go:281 ShardBackend.GetUnconfirmedHeaderList + |-> core/minorblockchain_addon.go:783 MinorBlockChain.GetUnconfirmedHeaderList + (m.mu.Lock) +``` + +--- + +## 2. AddRootBlock + +``` +cluster/slave/api_backend.go SlaveBackend.AddRootBlock +|-> cluster/shard/api_backend.go:203 ShardBackend.AddRootBlock + (s.mu.Lock) ← 序列化同一 shard 的 root/minor 处理 + |-> core/minorblockchain_addon.go:992 MinorBlockChain.AddRootBlock + |-> 1. 前置校验 + putRootBlock [no lock] + |-> 2. no-change-to-root-tip early return :1079 + | (m.mu.Lock 快照 rootTip; m.mu.Unlock) + |-> 3. m.chainmu.Lock :1098 (defer 到函数结束) ← 与 insertChain 管线互斥 + |-> 4. m.mu.Lock :1103 + | update rootTip / confirmedHeaderTip + | rewind currentBlock 到同一 root chain + | 计算 needGenesisReset :1135 + | m.mu.Unlock :1138 + |-> 5. [needGenesisReset] m.Reset :1158 + | |-> ResetWithGenesisBlock + | |-> setHead(0) [unlocked; 仅持 chainmu] + |-> 6. reWriteBlockIndexTo :1176 + (m.mu.Lock — reorg + 发布 currentEvmState) +``` + +> 关键:第 5、6 步都在 `m.mu.Unlock`(1138)**之后**执行。 +> `Reset` 走 **unlocked `setHead`**(不重新获取 chainmu/mu),`reWriteBlockIndexTo` 只取 mu。 +> 全程持有 chainmu,所以 reorg 与 insertChain 管线互斥;无重入、无自死锁。 + +--- + +## 3. CreateShards → initGenesisState + +``` +cluster/slave/api_backend.go SlaveBackend.CreateShards +|-> cluster/shard/api_backend.go:191 ShardBackend.InitFromRootBlock + |-> cluster/shard/shard.go:180 ShardBackend.initGenesisState + |-> 1. core/minorblockchain_addon.go:329 MinorBlockChain.InitGenesisState + | (m.mu.Lock) + |-> 2. conn.BroadcastXshardTxList [no lock] + |-> 3. core/minorblockchain_addon.go:1214 GetShardStats + | |-> getBlockCountByHeight :1379 + | (m.mu.RLock) + |-> 4. conn.SendMinorBlockHeaderToMaster [no lock] +``` + +--- + +## 4. NewMinorBlock → AddMinorBlock + +``` +cluster/slave/api_backend.go SlaveBackend.NewMinorBlock +|-> cluster/shard/api_backend.go:303 ShardBackend.NewMinorBlock + | (HasCommittedBlock 预检; 不持锁) + |-> cluster/shard/api_backend.go:365 ShardBackend.AddMinorBlock + |-> 0. fast-path getBlockCommitStatusByHash [lock-free 预检] + (s.mu.Lock) ← 之后双重检查 + |-> 1. core/minorblockchain.go:1048 InsertChainForDeposits (force=true) + | (m.chainmu.Lock) + | |-> WriteBlockWithState :914 (m.mu.Lock) + | |-> updateTip :158 (m.mu.Lock) + | (m.chainmu.Unlock) + | |-> post-insert 读 confirmedHeaderTip (m.mu.Lock) + |-> 2. conn.BroadcastXshardTxList [no lock] + | [error] s.setHead :601 -> MinorBlockChain.SetHead :318 (chainmu.Lock -> m.mu.Lock) + |-> 3. GetShardStats :1214 -> getBlockCountByHeight (m.mu.RLock) + | [error] s.setHead -> SetHead :318 (chainmu.Lock -> m.mu.Lock) + |-> 4. conn.SendMinorBlockHeaderToMaster [no lock] + | [error] s.setHead -> SetHead :318 (chainmu.Lock -> m.mu.Lock) + |-> 5. core/minorblockchain_addon.go:1931 CommitMinorBlockByHash + | (m.mu.Lock; HasBlock 检查后写 marker; 返回 false→ErrBodyDeleted) + |-> 6. broadcastNewTip :591 + |-> GetRootTip :1434 (m.mu.RLock) +``` + +> `s.setHead`(shard 层,api_backend.go:601)内部调 `MinorBlockChain.SetHead`(chainmu→mu)。 +> error 分支调用 setHead 时**仅持 s.mu**,不持 m.chainmu/m.mu,故 SetHead 重新获取二者无重入。 + +--- + +## 5. AddBlockListForSync + +``` +cluster/slave/api_backend.go SlaveBackend.AddBlockListForSync +| (HasCommittedBlock 过滤已提交块) +|-> cluster/shard/api_backend.go:465 ShardBackend.AddBlockListForSync + (s.mu.Lock) + |-> [per block] InsertChainForDeposits (force=true) :1048 + | (m.chainmu.Lock) + | |-> WriteBlockWithState :914 (m.mu.Lock) + | |-> updateTip :158 (m.mu.Lock) + | (m.chainmu.Unlock) + | |-> post-insert 读 confirmedHeaderTip (m.mu.Lock) + |-> conn.BatchBroadcastXshardTxList [no lock] + |-> conn.SendMinorBlockHeaderListToMaster [no lock] + |-> [per header] CommitMinorBlockByHash :1931 + (m.mu.Lock; 返回 false→ErrBodyDeleted, 中止整批) +``` + +--- + +## 6. CreateBlockToMine + +``` +cluster/shard/api_backend.go:640 ShardBackend.CreateBlockToMine +|-> core/minorblockchain_addon.go:898 MinorBlockChain.CreateBlockToMine + |-> 1. m.mu.Lock :937 快照 rootTip / currentBlock; m.mu.Unlock :940 + |-> 2. getEvmStateByBlock :1692 + (m.mu.Lock — 仅当 block == CurrentBlock 时) +``` + +--- + +## 7. CheckMinorBlocksInRoot + +``` +cluster/slave/api_backend.go SlaveBackend.CheckMinorBlocksInRoot +|-> cluster/shard/api_backend.go:668 ShardBackend.CheckMinorBlock + |-> core/minorblockchain.go:1048 InsertChainForDeposits (force=true) + (m.chainmu.Lock) + |-> WriteBlockWithState :914 (m.mu.Lock) + |-> updateTip :158 (m.mu.Lock) + (m.chainmu.Unlock) + |-> post-insert 读 confirmedHeaderTip (m.mu.Lock) +``` + +--- + +## Lock Acquisition Order + +所有路径按同一顺序获取锁——无反向获取、无 AB-BA 死锁。 + +``` +s.mu (ShardBackend) + └─ m.chainmu (MinorBlockChain 插入锁) + └─ m.mu (MinorBlockChain 状态锁) +``` + +> **AddRootBlock 说明**:持有 `m.chainmu` 全程(1098 defer)。`m.mu` 在 :1138 显式解锁, +> 之后才调用 `Reset`(走 unlocked `setHead`,不取 chainmu/mu)与 `reWriteBlockIndexTo` +> (仅取 m.mu)。因此不存在 chainmu/mu 的重入获取。 +> +> **error 分支 setHead**:`ShardBackend.setHead` 仅在持有 `s.mu` 时被调用,其内部的 +> `MinorBlockChain.SetHead` 获取 chainmu→mu,顺序与主链一致,无重入。