Repository navigation
fix(blockchain): refuse a rollback target below the archived block bodies - #1405
Conversation
…dies The chunk archiver deletes block bodies at and below (maxDeletedChunkNum+1)*ChunkblockNum-1. Rollback loads every block above its target, so a target below that floor fails part way down the deletion loop -- and that failure cannot be recovered from: the tip pointer has already been moved to a height whose body is gone, so the database can no longer be started. On a node that had archived up to 578999, rolling back to 546819 removed 321190 blocks and then panicked on 578999 with ErrHashNotExist; that datadir had to be replaced, even though everything above the floor was still intact. Check the floor in NeedRollback, where the other "this rollback cannot work" conditions already live, and refuse with an error naming the target, the floor and how it was derived. GetMaxDeletedChunkNum reports -1 when nothing has been archived, in which case no body has been deleted and no floor applies. Rollbackblock now exits non-zero when it did not roll back, so a caller cannot mistake a refusal for a completed rollback -- it used to exit 0 either way. The -rollback flag help now says the target cannot go below the archived bodies and that the operation cannot be undone. Fixes 33cn#1404. Co-authored-by: Claude <[email protected]>
复查:下界少算了一个"启动窗口",放行的目标仍会让节点起不来方向是对的, 代码路径
实测(PR head 72b0ff5,调用真实
|
The floor was one chunk too low: it only kept the deletion loop able to read every body above the target, and missed that the tip then *stands* at the target, where the next start reads the bodies above it. InitCache loads [tip-DefCacheSize, tip] and [tip-HighAllowPackHeight-LowAllowPackHeight+1, tip] and panics on the first block it cannot load, so a target at the floor deletes happily and then leaves a datadir that cannot start -- 800 blocks of margin short with the default 128 / 600+200 windows (measured: with the bodies at or below 999 gone, tip 999 and tip 1000 panic in InitCache, tip 1799 starts). - Take the floor from the chunk records, not from ChunkblockNum times the cursor: chunkShardHandle writes each record with the chunk size in effect at the time and DeleteBlockBody deletes exactly the range it names, so lowering ChunkblockNum between two starts would otherwise compute a floor that is too low. The configured size remains the fallback when no record is stored. - Add the startup window when TxHeight is enabled, reading both windows from the configuration rather than hardcoding the defaults. - Read the cursor with its error. It reported -1 both for "nothing archived yet" and for a failed read, and -1 means "no floor applies" -- a safety check must not fail open on a read failure. - Exit 0 when the target is at or above the tip: that is a no-op, and a service manager would otherwise report a failure for it. Refusals stay non-zero. Tests: TestRollbackTargetMustLeaveTheStartupWindowIntact archives a chunk for real (record plus deleted bodies), asserts the guard refuses the height one below the accepted one, and starts the chain at both -- the accepted target does not panic and the refused one does. Rollbackblock's exit codes and the flag help follow the same rule. Co-authored-by: Claude <[email protected]>
|
感谢复查,这条抓得很准 —— 我的下界确实少了整个启动窗口。已按建议 1 改完( 主问题:下界少了 800 块先在代码上复核了你的路径: 改法与你给的伪代码一致,但两个窗口都从配置读、不写死: 边界单测按新阈值改了,并补上你要的那条集成用例 下界改成取 chunk 记录(你的第 3 条次要项)你说得对, 次要项
没有做的:建议 2(让
|
archivedBodyFloor read the deletion cursor as if it were the end of the archive, but the cursor only says where the previous run stopped. CheckDeleteBlockBody keeps going in this process -- SetQueueClient starts the routine before the rollback runs, and it fires every minute -- and it stops DelRollbackChunkNum chunks short of the newest archived chunk, so a node with ten chunks of backlog will delete bodies the guard had already cleared. That is the same failure the guard exists to prevent: the deletion loop panics on the first missing body, or the next start panics in InitCache. The floor now comes from the chunk at max(maxDeletedChunk, maxSerialChunkNum - DelRollbackChunkNum - 1) -- the last chunk the archiver will ever delete -- read from that chunk's own record, and the record is still what decides the height. The record read also failed open. Any error, a real read failure included, fell back to (chunk+1) * ChunkblockNum, and that formula uses the current configuration, which is too low whenever ChunkblockNum was reduced after the bodies were dropped -- GetKey reports a failed read as types.ErrNotFound, so the two cases were one. Only db.ErrNotFoundInDb, a datadir from before the records were written, falls back now; anything else refuses the rollback and logs the chunk and the error. Tests, against the pre-change function: - TestRollbackFloorFollowsTheArchiverNotTheCursor: cursor on chunk 0, chunk 1 dropped with a different chunk size on record, archiver twelve chunks ahead. The floor has to be 1500 (chunk 1's record), not 1000; the old function returned 1000 and cleared a target below the real floor. Fails before, passes after. - TestRollbackFloorRefusesAnUnreadableRecord: an undecodable record must reach the caller instead of the fallback floor. Fails before, passes after. - TestRollbackFloorFallsBackToTheConfiguredChunkSize, TestRollbackFloorWithoutTxHeight: cover the two branches that had none, the fallback and the config bityuan runs (TxHeight=false, where InitCache reads nothing and no window applies). These pass before as well -- they pin behaviour, they do not discriminate. go test ./blockchain/ -count=1 passes, go vet and gofmt are clean. Co-authored-by: Claude <[email protected]>
|
自查后又补了一个提交( 下界原来读的是"删除游标",而归档协程还在跑游标( 现在下界取 记录读失败原来是 fail-open
用例(都对着改动前的函数跑过)
|
|
🎉 This PR is included in version 1.72.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fixes #1404.
-rollbackhas no guard against the chunk-archive floor, and a target below it leaves a datadir that cannot be started. On a node whose bodies had been archived up to 578999,-rollback 546819removed 321190 blocks, then panicked on 578999 withErrHashNotExist, and the datadir had to be replaced even though everything above the floor was still intact.The boundary is the startup window, not the deletion loop
The archiver deletes block bodies, and rollback loads every block above its target, so the target has to clear the lowest stored body for the deletion loop to work at all. That is not sufficient: the tip then stands at the target, and the next start reads the bodies above it.
InitCacheloads[tip-DefCacheSize, tip](128 by default) and[tip-HighAllowPackHeight-LowAllowPackHeight+1, tip](600+200-1 = 799 by default,types/const.go) and panics on the first block it cannot load, so the tip has to clear the archived range by the larger of the two windows.Measured against a real
InitCache, with the bodies at or below 999 deleted:So the lowest target that restarts is the floor plus 799 with the default configuration, which is 800 blocks shallower than the floor alone. Both windows are read from the configuration (
DefCacheSize,types.HighAllowPackHeight/LowAllowPackHeight, whichchain.gooverrides from the module config), and the window only applies whenTxHeightis enabled —InitCachereturns early without reading anything otherwise.The floor comes from the chunk records, at the archiver's frontier
Not from
ChunkblockNumtimes the cursor:chunkShardHandlewrites each record with the chunk size in effect at the time, andDeleteBlockBodydeletes exactly the range that record names. A node whoseChunkblockNumwas lowered between two starts would compute a floor that is too low and reopen the gap. The configured size remains the fallback for a datadir that has a cursor but no record.Nor from the cursor alone. The cursor says where the previous run stopped, and the archiver keeps going in the same process:
SetQueueClientstartschunkDeleteRoutinebeforeRollbackblockruns (util/cli/chain33.go), it fires every minute, and the loop stopsDelRollbackChunkNum(10,chunkshard.go) chunks short of the newest archived chunk. A node that never drained its backlog therefore has a cursor well behind the last chunk it is about to delete, and a target the guard had cleared could still be deleted into by the next tick — the same death, one tick later. The floor is taken from the chunk atmax(maxDeletedChunk, maxSerialChunkNum - DelRollbackChunkNum - 1), the last chunk the archiver will ever delete, read from that chunk's own record.A record that cannot be read is not "nothing was archived".
BlockStore.GetKeyreports a failed read astypes.ErrNotFound, so a read failure and a missing record were the same case, and both fell back to the configured chunk size — the bound that is too low exactly when it matters, sinceChunkblockNummay have been reduced after the bodies were dropped. Onlydb.ErrNotFoundInDbfalls back now; anything else refuses the rollback and logs the chunk and the error.The cursor itself still cannot make the floor too low: it advances even when a chunk has no record or a height inside it is skipped, and both cases only raise the derived floor.
Also in here
Rollbackblockexits 0 when the target is at or above the tip. That is a no-op, not a refusal; a service manager withRestart=alwayswould otherwise report a failure for it. Refusals stay non-zero, so a refusal still cannot be mistaken for a completed rollback.GetMaxDeletedChunkNumreported-1both for "nothing archived yet" and for a failed read, and-1means "no floor applies" — a safety check must not fail open on a read failure.GetMaxDeletedChunkNumWithErris added for that caller; the existing method keeps its behaviour for the archiver, which uses the value as "where to resume".Not done here: making
InitCachetolerate a missing body instead of panicking. It is the more robust fix, since any cause that brings the tip near the archive would stop locking the datadir, but it changes startup behaviour for every cause rather than this one and deserves its own change and discussion.Tests
TestRollbackTargetMustLeaveTheStartupWindowIntactbuilds the archived datadir for real (chunk record written, bodies deleted, cursor set), asserts the guard refuses the height one below the accepted one, and then starts the chain at both heights: the accepted target does not panic inInitCache, the refused one does.TestRollbackFloorFollowsTheArchiverNotTheCursorbuilds a datadir with two chunks dropped, the cursor still on the first and ten chunks of backlog, and asserts the floor is the second chunk's record (1500), not the cursor's (1000). The pre-change function returned1000and cleared a target below the real floor. Fails before, passes after.TestRollbackFloorRefusesAnUnreadableRecord: an undecodable record has to reach the caller instead of the fallback floor. Fails before, passes after.TestRollbackFloorFallsBackToTheConfiguredChunkSizeandTestRollbackFloorWithoutTxHeightcover the two branches that had none: the fallback, andTxHeight=false— bityuan's configuration, whereInitCachereads nothing and no window applies. These pass before as well; they pin behaviour rather than discriminate.TestNeedRollbackArchiveFloorfollows the new boundary instead of the old one.Rollbackblock's exit codes and the-rollbackflag help follow the same rule.go test ./blockchain/ -count=1passes;gofmtandgo vetare clean.🤖 Generated with Claude Code