[Storehouse] 007 Payloadless Checkpoint (v7) - #8578
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // Read the V6 checkpoint fully — the V6 reader already reads the 16 subtrie | ||
| // part files concurrently. The resulting tries share sub-tries via Go pointer | ||
| // identity, which lets FromV6Tries memoize and avoid redundant conversion. | ||
| v6Tries, err := LoadCheckpoint(v6Header, logger) |
There was a problem hiding this comment.
This loads the entire checkpoint in memory, which requires a lot of memory. An improvement would be streaming the file, iterate each node, if it's a leaf node, then convert into payload less. This could save lots of memory. Skipped for now, because that requires additional code, can be implemented later.
| return nil, nil | ||
| } | ||
| if index > totalSubTrieNodeCount { | ||
| nodePos := index - totalSubTrieNodeCount |
There was a problem hiding this comment.
This part is same logic as v6 (getNodeByIndex) but slightly different in style, where v6 extracted it into getTopNodeByIndex, which adds little value. Instead, the logic here for getPayloadlessTopNodeByIndex is inlined.
| @@ -0,0 +1,421 @@ | |||
| package wal | |||
There was a problem hiding this comment.
vimdiff ledger/complete/wal/checkpoint_v6_reader.go ledger/complete/wal/checkpoint_v7_reader.go
This file is similar to v6_reader, except some minimal coding style changes. Some read functions are not copied over but reused, such as readCheckpointHeader, allPartFileExist, readSubTriesFooter etc
| @@ -0,0 +1,484 @@ | |||
| package wal | |||
There was a problem hiding this comment.
vimdiff ledger/complete/wal/checkpoint_v6_writer.go ledger/complete/wal/checkpoint_v7_writer.go
v7 is similar to v6 except some comments changes.
| @@ -0,0 +1,372 @@ | |||
| package payloadless | |||
There was a problem hiding this comment.
mirrored from ledger/complete/mtrie/flattener/encoding.go and ledger/complete/mtrie/flattener/iterator.go
| encNodeIndexSize = 8 | ||
| encRegCountSize = 8 | ||
|
|
||
| encLeafHashFlagSize = 1 |
There was a problem hiding this comment.
encRegSizeSize and encPayloadLengthSize in v6 checkpoint node (see ledger/complete/mtrie/flattener/encoding.go) is replaced by encLeafHashFlagSize in payloadless
| return nil, fmt.Errorf("cannot read leaf hash flag: %w", err) | ||
| } | ||
|
|
||
| flag := scratch[0] |
There was a problem hiding this comment.
this part is different from readPayloadFromReader
|
|
||
| type EncodedTrie struct { | ||
| RootIndex uint64 | ||
| RegCount uint64 |
| encLeafHashFlagSize + | ||
| encLeafHashSize |
There was a problem hiding this comment.
encPayloadLengthSize + encPayloadSize is replaced by encLeafHashFlagSize + encLeafHashSize in payloadless
| copy(buf[pos:], path[:]) | ||
| pos += encPathSize | ||
|
|
||
| // Encode leaf hash flag (1 byte) and optional leaf hash (0 or 32 bytes) |
There was a problem hiding this comment.
this part is different from full trie node's encodeLeafNode
7b14692 to
2625248
Compare
| } | ||
|
|
||
| lastCheckpointNum, err := c.checkpointer.LatestCheckpoint() | ||
| lastCheckpointNum, err := c.checkpointer.LatestCheckpointV6() |
There was a problem hiding this comment.
The compactor will eventually need to support both V6 and V7, but this PR only supports V6. To avoid ambiguity, I renamed the function to LatestCheckpointV6.
d7dea94 to
fcc22e5
Compare
c25cf48 to
267d209
Compare
fcc22e5 to
f138980
Compare
267d209 to
8d341b5
Compare
f138980 to
fc173be
Compare
1ea0be7 to
ea43efc
Compare
fc173be to
ff4a401
Compare
ea43efc to
a835d1f
Compare
ff4a401 to
b825a6a
Compare
a835d1f to
8b9576b
Compare
| // HasRootCheckpointV7 guard keeps a re-entry after an interrupted | ||
| // bootstrap from hitting ConvertCheckpointV6ToV7's "output exists" check. | ||
| // | ||
| // TODO: ConvertCheckpointV6ToV7 reads the entire V6 forest into memory | ||
| // before emitting V7, a memory/time spike at first boot for mainnet-scale | ||
| // root checkpoints. A future optimization is to convert subtrie-by-subtrie | ||
| // without loading the whole forest. | ||
| if exeNode.exeConf.payloadless { | ||
| triedir := exeNode.exeConf.triedir | ||
| hasV7Root, err := wal.HasRootCheckpointV7(triedir) |
There was a problem hiding this comment.
If the node dies mid-conversion (realistic here: this is the memory-heavy step, so an OOM kill), the part files are left on disk but the header file is not, it is written last. On the next boot HasRootCheckpointV7 returns false (it only checks the header file), so this branch converts again, but ConvertCheckpointV6ToV7 refuses because the leftover part files trip its output-already-exists check. The node then cannot boot until someone deletes the partial files by hand (StoreCheckpointV7 only cleans up after itself on error returns, not on process death).
Suggestion: when hasV7Root is false, first delete any leftover partial output, e.g. deleteCheckpointFiles(triedir, modelbootstrap.FilenameWALRootCheckpoint+wal.V7FileSuffix). The V6 source is untouched, so retrying the conversion from scratch is always safe and the manual util keeps its strict no-clobber behavior.
| hashes, err := wal.ReadTriesRootHash(logger, dir, fileName) | ||
| var hashes []ledger.RootHash | ||
| var err error | ||
| if strings.HasSuffix(fileName, wal.V7FileSuffix) { |
There was a problem hiding this comment.
This branch is currently unreachable through its only caller chain: GenerateProtocolSnapshotForCheckpointWithHeights → findLatestCheckpointFilePath (line 141) picks last from wal.ListCheckpoints, which since this PR includes V7 numbers, but always renders wal.NumberToFilename(last), the V6 name. On a payloadless triedir whose newest checkpoint is V7-only, that produces a path to a nonexistent file, and it can never produce the .v7 path this dispatch handles. findLatestCheckpointFilePath should use ListCheckpointsWithInfo and render NumberToFilenameV7 when the latest checkpoint is V7.
| // If both failed, return combined error | ||
| if v6Err != nil && v7Err != nil { | ||
| return fmt.Errorf("failed to remove checkpoint %d: v6 error: %w, v7 error: %v", checkpoint, v6Err, v7Err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
deleteCheckpointFiles succeeds when its glob matches nothing, so the common failure case, V6 files exist but deletion fails, no V7 files present, returns nil here. (%v also drops the v7 error from the wrap chain.) Since both compactors now use the version-specific variants and no production caller of this method remains, consider errors.Join(v6Err, v7Err) (wrapped with the checkpoint number) or deprecating this method like LoadCheckpoint.
| switch flag { | ||
| case leafHashAbsent: | ||
| return nil, nil | ||
| case leafHashPresent: | ||
| if len(scratch) < encHashSize { | ||
| scratch = make([]byte, encHashSize) | ||
| } | ||
| _, err := io.ReadFull(reader, scratch[:encHashSize]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot read leaf hash: %w", err) | ||
| } | ||
| leafHash, err := hash.ToHash(scratch[:encHashSize]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode leaf hash: %w", err) | ||
| } | ||
| return &leafHash, nil | ||
| default: | ||
| return nil, fmt.Errorf("invalid leaf hash flag: %d", flag) |
There was a problem hiding this comment.
The leaf-hash flag is the one new on-disk encoding mechanism vs V6, and no test executes the leafHashAbsent path or the invalid-flag branch: there is no flattener_test.go, and every checkpoint test fixture uses non-empty values.
|
|
||
| // Version 7 includes these changes: | ||
| // - payloadless mode: leaf nodes store payload hashes (32 bytes) instead of full payloads | ||
| // - used for verification nodes that don't need actual payload values |
There was a problem hiding this comment.
nit: V7 checkpoints are read and written by payloadless execution nodes (this PR wires them into the EN builder); verification-node use is future/speculative.
| // - used for verification nodes that don't need actual payload values | |
| // - used by payloadless execution nodes, which read register values from the storehouse |
| // For now config.LedgerServiceAddr is ignored. | ||
| // | ||
| // Expected error returns during normal operation: | ||
| // - error if config.Triedir is empty |
There was a problem hiding this comment.
nit: the missing-V7-checkpoint refusal (both the "V6 exists, run checkpoint-convert-v7" return and the plain "no V7 found" return) is equally expected during normal operation.
| "github.com/onflow/flow-go/utils/unittest" | ||
| ) | ||
|
|
||
| func TestVersionV7(t *testing.T) { |
There was a problem hiding this comment.
nit: three V6 unit tests have no V7 analog: TestEncodeSubTrie, TestGetNodesByIndex, and TestCannotStoreTwice.
| Msg("payloadless ledger: could not also list V6 checkpoints while reporting missing V7") | ||
| } | ||
| if latestV6 >= 0 { | ||
| logger.Warn(). |
There was a problem hiding this comment.
Should this be Error level?
| v6Numbers, latestV6, v6ListErr := wal.ListV6Checkpoints(config.Triedir) | ||
| if v6ListErr != nil { | ||
| logger.Warn().Err(v6ListErr). | ||
| Str("triedir", config.Triedir). |
There was a problem hiding this comment.
All loggers contain Str("triedir", config.Triedir). you could create a new logger with that already included.
| type PayloadlessLedgerWithCompactor struct { | ||
| *PayloadlessLedger | ||
| compactor *PayloadlessCompactor | ||
| logger zerolog.Logger |
There was a problem hiding this comment.
*PayloadlessLedger already has a logger. Can we reuse that one?
payloadless compactor
8b9576b to
d77747c
Compare
The previous PR #8577 has integrated the payloadless feature to the execution node behind a feature flag, but the payloadless feature only works for empty state. To support non-empty state, which practically to support restart, we need to support a v7 payloadless checkpoint.