diff --git a/.omo/plans/fix-c1-crc32c.md b/.omo/plans/fix-c1-crc32c.md new file mode 100644 index 0000000..b6e1a57 --- /dev/null +++ b/.omo/plans/fix-c1-crc32c.md @@ -0,0 +1,330 @@ +# C1 修复方案:CRC 多项式从 IEEE 改为 crc32c + +## TL;DR + +> **目标**:把 WAL 所有 CRC 计算从 `crc32.ChecksumIEEE`(IEEE 802.3,多项式 0xEDB88320)改为 `crc32.Checksum(data, crc32cTable)`(Castagnoli,多项式 0x82F63B78)。设计文档明确要求 crc32c,当前代码自洽但和规范脱钩。 +> +> **交付**: +> - 新建包级 `crc32cTable` 变量(`crc32.MakeTable(crc32.Castagnoli)`) +> - 5 处 `ChecksumIEEE` 调用替换为 `Checksum(data, crc32cTable)` +> - 1 个固定向量测试(防回归关键) +> - 单次 commit +> +> **预估工时**:1 小时 +> **风险**:低。改动局限在 5 处函数调用。**Breaking change**:旧 WAL 文件无法读,但 Phase 1 未 release + +--- + +## Context + +### Bug 摘要 + +5 处 CRC 计算用错多项式: + +| 文件 | 行 | 用途 | +|------|------|------| +| `wal/header.go:44` | encode WAL header CRC | +| `wal/header.go:83` | decode verify WAL header CRC | +| `wal/record.go:30` | encode physical record CRC | +| `wal/record.go:55` | decode verify physical record CRC | +| `wal/block_writer_test.go:349` | test helper | + +当前都用 `crc32.ChecksumIEEE`,但设计要求 `crc32c`。 + +### 设计依据 + +`docs/design.md` §3.2 line 359(Physical Record 字段说明): + +> | crc32c | 校验 `length + type + payload`,用于识别 torn write、partial write 和数据损坏 | + +WAL 格式中所有 CRC 都是 crc32c(Castagnoli),headerCRC 也应该一致。 + +### 为什么这是 bug(即使当前能跑) + +1. **不符合设计**:文档代码不一致 +2. **失去硬件加速**:Intel SSE4.2 的 `CRC32` 指令只支持 Castagnoli 多项式 +3. **未来兼容性**:跨实现 / 跨工具读写需要正确的 CRC +4. **行业标准**:ext4 / SQLite / RocksDB / LevelDB 都用 crc32c + +### Phase 1 修复成本最低 + +- Phase 1 未 release,没有真实数据需要迁移 +- 越晚改,迁移成本越高(要维护 v1/v2 两套 CRC) + +--- + +## 执行计划 + +### Phase A:代码改动(15 分钟) + +#### A.1 新建 `wal/crc32c.go` + +```go +package wal + +import "hash/crc32" + +// crc32cTable is the CRC-32 table using the Castagnoli polynomial (0x82F63B78 +// reflected). Required by design §3.2 line 359 for all WAL CRC computations. +// +// Distinct from crc32.IEEE (Ethernet/PNG polynomial 0xEDB88320) — the two +// produce unrelated checksums for the same input. SSE4.2 native CRC32 +// instruction only supports Castagnoli, so this table also enables hardware +// acceleration via Go's standard library internals. +var crc32cTable = crc32.MakeTable(crc32.Castagnoli) +``` + +> **必要 docstring**:解释为什么单独抽这个变量(C1 bug 防回归),引用设计行号。属于 security-related 必要注释。 + +#### A.2 替换 5 处 `ChecksumIEEE` + +`wal/header.go:44`: + +```go +// 改前: +h.HeaderCRC = crc32.ChecksumIEEE(buf[0:28]) +// 改后: +h.HeaderCRC = crc32.Checksum(buf[0:28], crc32cTable) +``` + +`wal/header.go:83`: + +```go +// 改前: +gotCRC := crc32.ChecksumIEEE(data[0:28]) +// 改后: +gotCRC := crc32.Checksum(data[0:28], crc32cTable) +``` + +`wal/record.go:30`: + +```go +// 改前: +crc := crc32.ChecksumIEEE(buf[4:]) +// 改后: +crc := crc32.Checksum(buf[4:], crc32cTable) +``` + +`wal/record.go:55`: + +```go +// 改前: +expectedCRC := crc32.ChecksumIEEE(data[4 : 7+length]) +// 改后: +expectedCRC := crc32.Checksum(data[4 : 7+length], crc32cTable) +``` + +`wal/block_writer_test.go:349`: + +```go +// 改前: +computedCRC := crc32.ChecksumIEEE(crcData) +// 改后: +computedCRC := crc32.Checksum(crcData, crc32cTable) +``` + +**注释更新**:`wal/header.go:43` 注释 `// CRC32 IEEE over bytes 0–27` 改成 `// CRC32C over bytes 0–27 (per design §3.2 line 341, 359)`。 + +> **Oracle 修订(bg_0323e6b9)**:原计划只引用 line 359,但 line 359 描述的是 physical record CRC,header CRC 应该引用 line 341(headerCRC 字段定义)。改成同时引用两条,或写"per design §3.2 CRC field spec"。 + +### Phase B:测试(30 分钟) + +#### B.1 固定向量测试(关键防回归) + +新增 `wal/crc32c_test.go`: + +```go +package wal + +import ( + "hash/crc32" + "testing" +) + +// Regression guard for C1: verify all WAL CRC uses Castagnoli polynomial +// (crc32c), not IEEE. Per design §3.2 line 359. +// +// The standard CRC-32C test vector from RFC 3720 Appendix B is the 9-byte +// ASCII string "123456789": +// - crc32c: 0xE3069283 +// - crc32 IEEE: 0xCBF43926 +// +// If anyone changes crc32cTable back to IEEE, this test fails immediately. +func TestCRC32CStandardVector(t *testing.T) { + got := crc32.Checksum([]byte("123456789"), crc32cTable) + const want = uint32(0xE3069283) + if got != want { + t.Errorf("crc32c('123456789') = 0x%X, want 0x%X (Castagnoli)", got, want) + } +} + +// Negative regression: confirm IEEE would produce a DIFFERENT value. This +// catches the case where someone "fixes" crc32cTable to use IEEE by mistake. +func TestCRC32CEdistinctFromIEEE(t *testing.T) { + data := []byte("123456789") + ieee := crc32.ChecksumIEEE(data) + castagnoli := crc32.Checksum(data, crc32cTable) + if ieee == castagnoli { + t.Errorf("IEEE and Castagnoli produced the same CRC (impossible unless table is wrong); both = 0x%X", ieee) + } +} +``` + +#### B.2 直接断言 header CRC 用的是 crc32c(Oracle 新增) + +> **Oracle 修订(bg_0323e6b9)NICE-TO-HAVE**:现有 `TestHeaderRoundtrip` 不能抓"encode + decode 同时改回 IEEE"(自洽)。需要直接断言存储的 CRC 值。 + +新增到 `wal/header_test.go`(或 `crc32c_test.go`): + +```go +// Regression guard for C1: header CRC must be computed with crc32c, not +// IEEE. TestHeaderRoundtrip is self-consistent (encode + decode use same +// polynomial), so a paired reversion to IEEE would pass it. This test +// directly asserts the stored CRC matches Castagnoli. +func TestHeaderCRCUsesCastagnoli(t *testing.T) { + hdr := &WalFileHeader{ + BlockSize: 32 * 1024, + SegmentID: 42, + StartSequence: 100, + } + encoded := EncodeWalHeader(hdr) + want := crc32.Checksum(encoded[0:28], crc32cTable) + got := encoded[28] | uint32(encoded[29])<<8 | uint32(encoded[30])<<16 | uint32(encoded[31])<<24 + if got != want { + t.Errorf("stored headerCRC = 0x%X, want crc32c value 0x%X", got, want) + } + // Sanity: confirm IEEE would produce a different value (catches paired reversion). + ieeeValue := crc32.ChecksumIEEE(encoded[0:28]) + if got == ieeeValue { + t.Errorf("stored headerCRC = 0x%X matches IEEE value (C1 regression)", got) + } +} +``` + +同理 physical record 也加一个(可选,但便宜): + +```go +// Regression guard for C1: physical record CRC must be Castagnoli. +func TestPhysicalRecordCRCUsesCastagnoli(t *testing.T) { + payload := []byte("test-payload") + encoded := EncodePhysicalRecord(RecFull, payload) + // Record format: [crc u32][length u16][type u8][payload] + want := crc32.Checksum(encoded[4:7+len(payload)], crc32cTable) + got := encoded[0] | uint32(encoded[1])<<8 | uint32(encoded[2])<<16 | uint32(encoded[3])<<24 + if got != want { + t.Errorf("stored record CRC = 0x%X, want crc32c value 0x%X", got, want) + } + ieeeValue := crc32.ChecksumIEEE(encoded[4 : 7+len(payload)]) + if got == ieeeValue { + t.Errorf("stored record CRC = 0x%X matches IEEE value (C1 regression)", got) + } +} +``` + +#### B.3 现有测试自动通过(无需改动) + +所有 round-trip 测试(encode → decode → verify)自洽:encode 和 decode 用同一个多项式。改成 crc32c 后两端都用 crc32c,仍然自洽。 + +需要确认: +- `wal/header_test.go` round-trip 测试通过 +- `wal/record_test.go` round-trip 测试通过 +- `wal/block_writer_test.go` round-trip 测试通过 +- `wal/recover_test.go` 等端到端 recovery 测试通过 +- `db_test.go` / `db_e2e_test.go` 通过 + +### Phase C:验证(15 分钟) + +```bash +# 1. 编译 +go build ./... + +# 2. 重点测试(C1 新增) +go test ./wal -run 'TestCRC32C' -count=1 -v + +# 3. wal 包全量 +go test ./wal/... -count=1 + +# 4. 全仓 +go test ./... -count=1 + +# 5. race +go test -race ./... -count=1 + +# 6. vet +go vet ./... +``` + +### Phase D:Commit message draft + +``` +fix: use CRC-32C (Castagnoli) instead of IEEE for WAL (C1) + +Per design §3.2 line 359, all WAL CRC computations must use crc32c +(Castagnoli polynomial 0x82F63B78). The previous code used crc32.IEEE +(Ethernet/PNG polynomial 0xEDB88320) in 5 places. While the system was +self-consistent (encode + decode both used IEEE), it diverged from the +design spec and lost SSE4.2 hardware acceleration (the native CRC32 +instruction only supports Castagnoli). + +Changes: +- wal/crc32c.go (new): package-level crc32cTable = crc32.MakeTable( + crc32.Castagnoli). Central definition prevents future drift. +- wal/header.go: 2 ChecksumIEEE calls replaced with crc32.Checksum( + data, crc32cTable). Comment updated to reference design §3.2 line 359. +- wal/record.go: 2 ChecksumIEEE calls replaced. +- wal/block_writer_test.go: 1 ChecksumIEEE call in test helper replaced. +- wal/crc32c_test.go (new): standard CRC-32C test vector (RFC 3720: + crc32c("123456789") = 0xE3069283) + negative test confirming IEEE + produces different value. These guards catch any future regression + to IEEE. + +BREAKING CHANGE: WAL files written before this fix (with IEEE CRC) +cannot be read after this fix (expects crc32c). Phase 1 has not been +released, so no real data migration is needed. Production users +post-release would need to drain + re-create the database. + +Developers pulling this change should delete any local Phase-1 WAL +directories (`rm -rf /segment-*.wal`) before running the code; +old IEEE-encoded WALs will fail recovery on local dev machines. + +Verified: all existing round-trip tests pass (encode + decode both use +crc32c, still self-consistent). Full suite green including +go test -race ./... . + +Audit context: docs/audit-3.2.md C1. +``` + +--- + +## 验收清单 + +- [ ] Phase A.1:`wal/crc32c.go` 存在,定义 `crc32cTable` +- [ ] Phase A.2:5 处 `ChecksumIEEE` 全部替换为 `Checksum(data, crc32cTable)` +- [ ] Phase A.2:`wal/header.go:43` 注释更新引用设计行号 +- [ ] Phase B.1:`TestCRC32CStandardVector` + `TestCRC32CEdistinctFromIEEE` 存在 +- [ ] `go test ./wal/... -count=1` 全绿 +- [ ] `go test ./... -count=1` 全绿 +- [ ] `go test -race ./... -count=1` 全绿 +- [ ] `go vet ./...` 无新增警告 +- [ ] 单次 commit,message 引用 audit C1 + 标注 BREAKING CHANGE + +--- + +## 不在本次范围内(后续 issue) + +| 编号 | 为什么不放进来 | +|------|---------------| +| C7 | Put/Close 竞态,独立 | +| H1-H7 | 其他 High,独立 | +| WAL 格式版本号 | Phase 1 未 release,不需要 v2 迁移机制 | + +--- + +## 修订记录 + +- **v1(原始)**:C1 修复方案初稿,送 Momus 审 +- **v1.0(Momus 审核 bg_5a6543ac)**:[OKAY],无 blocking。验证全仓只有 5 处 `ChecksumIEEE`,无遗漏 +- **v1.1(Oracle 审核 bg_0323e6b9)**:**approve**(无 blocking)。3 个 nice-to-have 已采纳: + - 加直接断言测试(header / physical record CRC 用的是 crc32c 值,不是 IEEE)—— `TestHeaderRoundtrip` 抓不到"encode + decode 同时改回 IEEE" + - header.go 注释引用设计行号改成 `line 341, 359`(line 359 是 physical record,header CRC 应引 line 341) + - commit message 加开发者提醒:删本地 Phase-1 WAL 目录 diff --git a/wal/block_writer_test.go b/wal/block_writer_test.go index 38ad3c8..7fc87a6 100644 --- a/wal/block_writer_test.go +++ b/wal/block_writer_test.go @@ -346,7 +346,7 @@ func TestBlockWriterPhysicalRecordCRC(t *testing.T) { // Verify CRC over [length, type, payload]. crcData := data[4 : 7+length] - computedCRC := crc32.ChecksumIEEE(crcData) + computedCRC := crc32.Checksum(crcData, crc32cTable) if storedCRC != computedCRC { t.Errorf("CRC mismatch: stored %d, computed %d", storedCRC, computedCRC) } diff --git a/wal/crc32c.go b/wal/crc32c.go new file mode 100644 index 0000000..cbc5392 --- /dev/null +++ b/wal/crc32c.go @@ -0,0 +1,12 @@ +package wal + +import "hash/crc32" + +// crc32cTable is the CRC-32 table using the Castagnoli polynomial (0x82F63B78 +// reflected). Required by design §3.2 line 359 for all WAL CRC computations. +// +// Distinct from crc32.IEEE (Ethernet/PNG polynomial 0xEDB88320) — the two +// produce unrelated checksums for the same input. SSE4.2 native CRC32 +// instruction only supports Castagnoli, so this table also enables hardware +// acceleration via Go's standard library internals. +var crc32cTable = crc32.MakeTable(crc32.Castagnoli) diff --git a/wal/crc32c_test.go b/wal/crc32c_test.go new file mode 100644 index 0000000..0ac495e --- /dev/null +++ b/wal/crc32c_test.go @@ -0,0 +1,76 @@ +package wal + +import ( + "encoding/binary" + "hash/crc32" + "testing" +) + +// Regression guard for C1: verify all WAL CRC uses Castagnoli polynomial +// (crc32c), not IEEE. Per design §3.2 line 359. +// +// The standard CRC-32C test vector from RFC 3720 Appendix B is the 9-byte +// ASCII string "123456789": +// - crc32c: 0xE3069283 +// - crc32 IEEE: 0xCBF43926 +// +// If anyone changes crc32cTable back to IEEE, this test fails immediately. +func TestCRC32CStandardVector(t *testing.T) { + got := crc32.Checksum([]byte("123456789"), crc32cTable) + const want = uint32(0xE3069283) + if got != want { + t.Errorf("crc32c('123456789') = 0x%X, want 0x%X (Castagnoli)", got, want) + } +} + +// Negative regression: confirm IEEE would produce a DIFFERENT value. This +// catches the case where someone "fixes" crc32cTable to use IEEE by mistake. +func TestCRC32CEdistinctFromIEEE(t *testing.T) { + data := []byte("123456789") + ieee := crc32.ChecksumIEEE(data) + castagnoli := crc32.Checksum(data, crc32cTable) + if ieee == castagnoli { + t.Errorf("IEEE and Castagnoli produced the same CRC (impossible unless table is wrong); both = 0x%X", ieee) + } +} + +// Regression guard for C1: header CRC must be computed with crc32c, not IEEE. +// TestHeaderRoundtrip is self-consistent (encode + decode use same polynomial), +// so a paired reversion to IEEE would pass it. This test directly asserts +// the stored CRC matches Castagnoli. +func TestHeaderCRCUsesCastagnoli(t *testing.T) { + hdr := &WalFileHeader{ + BlockSize: 32 * 1024, + SegmentID: 42, + StartSequence: 100, + } + encoded := EncodeWalHeader(hdr) + want := crc32.Checksum(encoded[0:28], crc32cTable) + got := binary.LittleEndian.Uint32(encoded[28:32]) + if got != want { + t.Errorf("stored headerCRC = 0x%X, want crc32c value 0x%X", got, want) + } + // Sanity: confirm IEEE would produce a different value (catches paired reversion). + ieeeValue := crc32.ChecksumIEEE(encoded[0:28]) + if got == ieeeValue { + t.Errorf("stored headerCRC = 0x%X matches IEEE value (C1 regression)", got) + } +} + +// Regression guard for C1: physical record CRC must be Castagnoli. +// Same rationale as TestHeaderCRCUsesCastagnoli — direct value assertion +// catches paired encode/decode reversion. +func TestPhysicalRecordCRCUsesCastagnoli(t *testing.T) { + payload := []byte("test-payload") + encoded := EncodePhysicalRecord(RecFull, payload) + // Record format: [crc u32][length u16][type u8][payload] + want := crc32.Checksum(encoded[4:7+len(payload)], crc32cTable) + got := binary.LittleEndian.Uint32(encoded[0:4]) + if got != want { + t.Errorf("stored record CRC = 0x%X, want crc32c value 0x%X", got, want) + } + ieeeValue := crc32.ChecksumIEEE(encoded[4 : 7+len(payload)]) + if got == ieeeValue { + t.Errorf("stored record CRC = 0x%X matches IEEE value (C1 regression)", got) + } +} diff --git a/wal/header.go b/wal/header.go index b511387..9d84c7b 100644 --- a/wal/header.go +++ b/wal/header.go @@ -40,8 +40,8 @@ func EncodeWalHeader(h *WalFileHeader) [walFileHeaderSize]byte { le.PutUint64(buf[12:20], h.SegmentID) le.PutUint64(buf[20:28], h.StartSequence) - // CRC32 IEEE over bytes 0–27 (excludes the CRC field itself) - h.HeaderCRC = crc32.ChecksumIEEE(buf[0:28]) + // CRC32C over bytes 0–27 (per design §3.2 line 341, 359) + h.HeaderCRC = crc32.Checksum(buf[0:28], crc32cTable) le.PutUint32(buf[28:32], h.HeaderCRC) return buf @@ -80,7 +80,7 @@ func DecodeWalHeader(data []byte) (*WalFileHeader, error) { } // Verify CRC before trusting any other fields - gotCRC := crc32.ChecksumIEEE(data[0:28]) + gotCRC := crc32.Checksum(data[0:28], crc32cTable) storedCRC := le.Uint32(data[28:32]) if gotCRC != storedCRC { return nil, errCRCMismatch diff --git a/wal/record.go b/wal/record.go index c4bf994..fbd4240 100644 --- a/wal/record.go +++ b/wal/record.go @@ -26,8 +26,9 @@ func EncodePhysicalRecord(recType uint8, payload []byte) []byte { buf[6] = recType copy(buf[7:], payload) - // CRC covers bytes [4:] = length + type + payload. - crc := crc32.ChecksumIEEE(buf[4:]) + // CRC covers bytes [4:] = length + type + payload. Castagnoli polynomial + // per design §3.2 line 359. + crc := crc32.Checksum(buf[4:], crc32cTable) binary.LittleEndian.PutUint32(buf[0:4], crc) return buf @@ -52,7 +53,7 @@ func DecodePhysicalRecord(data []byte) (rec *PhysicalRecord, consumed int, err e copy(payload, data[7:7+length]) // Verify CRC: covers length + type + payload. - expectedCRC := crc32.ChecksumIEEE(data[4 : 7+length]) + expectedCRC := crc32.Checksum(data[4 : 7+length], crc32cTable) if crc != expectedCRC { return nil, 0, errors.New("record: CRC mismatch") }