Track number of blocks/transactions when verifying#8534
Open
Track number of blocks/transactions when verifying#8534
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds aggregated block verification statistics returned from verifier functions; concurrent verifier workers accumulate matched/mismatched chunk and transaction counts; CLI captures and logs these totals after verification completes. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Verify CLI
participant Verifier as Verifier (concurrent)
participant Storage as Block Storage
participant Logger as Logger
CLI->>Verifier: Call VerifyRange / VerifyLastKHeight
Verifier->>Storage: Read block headers & execution results (per-height, per-chunk)
Storage-->>Verifier: Return data or ErrNotFound
Verifier->>Verifier: Worker verifyHeight -> return BlockVerificationStats
Verifier->>Verifier: Aggregate BlockVerificationStats (mutex)
Verifier-->>CLI: Return total BlockVerificationStats + error
CLI->>Logger: Log "finished verifying..." and summary counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
engine/verification/verifier/verifiers.go (1)
396-423: Collection ID will be empty for system chunks.When
chunkDataPack.Collectionis nil (which occurs for system chunks per the ChunkDataPack definition),collectionIDremains as the zero-valueflow.Identifier. The error/warning messages will display this as an empty/zero ID, which could be confusing.Consider using a more explicit label for system chunks:
♻️ Suggested improvement for clearer logging
if err != nil { - var collectionID flow.Identifier - if chunkDataPack.Collection != nil { - collectionID = chunkDataPack.Collection.ID() - } + collectionIDStr := "system-chunk" + if chunkDataPack.Collection != nil { + collectionIDStr = chunkDataPack.Collection.ID().String() + } if stopOnMismatch { return BlockVerificationStats{}, fmt.Errorf( - "could not verify chunk (index: %v, ID: %v) at block %v (%v): %w", + "could not verify chunk (index: %v, collection: %v) at block %v (%v): %w", i, - collectionID, + collectionIDStr, height, blockID, err, ) } if vcd.IsSystemChunk { log.Warn().Err(err).Msgf( - "could not verify system chunk (index: %v, ID: %v) at block %v (%v)", - i, collectionID, height, blockID, + "could not verify system chunk (index: %v) at block %v (%v)", + i, height, blockID, ) } else { log.Error().Err(err).Msgf( - "could not verify chunk (index: %v, ID: %v) at block %v (%v)", - i, collectionID, height, blockID, + "could not verify chunk (index: %v, collection: %v) at block %v (%v)", + i, collectionIDStr, height, blockID, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/verification/verifier/verifiers.go` around lines 396 - 423, The logs use the zero-value flow.Identifier when chunkDataPack.Collection is nil, making system-chunk messages show an empty ID; update the branch around collectionID in the chunk verification loop to render a clear label instead of the zero-value: when chunkDataPack.Collection is nil (e.g., for system chunks) set a descriptive string like "system" or "no-collection" and use that value in the log/Error fmt arguments (the code that computes collectionID and the subsequent log.Warn()/log.Error() calls and the vcd.IsSystemChunk branch should be updated to reference the formatted label rather than the raw zero-value flow.Identifier).cmd/util/cmd/verify_execution_result/cmd.go (1)
137-142: Consider using structured logging fields for better queryability.The summary stats are currently formatted as a string. Using structured fields would make the log output easier to parse and query.
♻️ Suggested improvement
- lg.Info().Msgf("matching chunks: %d/%d. matching transactions: %d/%d", - totalStats.MatchedChunkCount, - totalStats.MatchedChunkCount+totalStats.MismatchedChunkCount, - totalStats.MatchedTransactionCount, - totalStats.MatchedTransactionCount+totalStats.MismatchedTransactionCount, - ) + lg.Info(). + Uint64("matched_chunks", totalStats.MatchedChunkCount). + Uint64("total_chunks", totalStats.MatchedChunkCount+totalStats.MismatchedChunkCount). + Uint64("matched_transactions", totalStats.MatchedTransactionCount). + Uint64("total_transactions", totalStats.MatchedTransactionCount+totalStats.MismatchedTransactionCount). + Msg("verification complete")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/util/cmd/verify_execution_result/cmd.go` around lines 137 - 142, Replace the formatted Msgf string with structured logging fields on the existing logger (lg) using the numeric fields from totalStats (MatchedChunkCount, MismatchedChunkCount, MatchedTransactionCount, MismatchedTransactionCount); e.g., call lg.Info().Int("matched_chunks", totalStats.MatchedChunkCount).Int("total_chunks", totalStats.MatchedChunkCount+totalStats.MismatchedChunkCount).Int("matched_transactions", totalStats.MatchedTransactionCount).Int("total_transactions", totalStats.MatchedTransactionCount+totalStats.MismatchedTransactionCount).Msg("verification summary") so logs are emitted as queryable fields rather than a single formatted string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/verification/verifier/verifiers.go`:
- Around line 173-182: Update the test mock `mockVerifyHeight` to match the new
`verifyHeight` callback signature used by `verifyConcurrently`: change its
return type to `(BlockVerificationStats, error)` and return a
`BlockVerificationStats` zero value along with the existing `error` (or a
meaningful `BlockVerificationStats` when simulating success). Ensure the mock's
declaration and all callers in `verifiers_test.go` are updated so the mock
returns a `(BlockVerificationStats, error)` tuple instead of just `error`.
---
Nitpick comments:
In `@cmd/util/cmd/verify_execution_result/cmd.go`:
- Around line 137-142: Replace the formatted Msgf string with structured logging
fields on the existing logger (lg) using the numeric fields from totalStats
(MatchedChunkCount, MismatchedChunkCount, MatchedTransactionCount,
MismatchedTransactionCount); e.g., call lg.Info().Int("matched_chunks",
totalStats.MatchedChunkCount).Int("total_chunks",
totalStats.MatchedChunkCount+totalStats.MismatchedChunkCount).Int("matched_transactions",
totalStats.MatchedTransactionCount).Int("total_transactions",
totalStats.MatchedTransactionCount+totalStats.MismatchedTransactionCount).Msg("verification
summary") so logs are emitted as queryable fields rather than a single formatted
string.
In `@engine/verification/verifier/verifiers.go`:
- Around line 396-423: The logs use the zero-value flow.Identifier when
chunkDataPack.Collection is nil, making system-chunk messages show an empty ID;
update the branch around collectionID in the chunk verification loop to render a
clear label instead of the zero-value: when chunkDataPack.Collection is nil
(e.g., for system chunks) set a descriptive string like "system" or
"no-collection" and use that value in the log/Error fmt arguments (the code that
computes collectionID and the subsequent log.Warn()/log.Error() calls and the
vcd.IsSystemChunk branch should be updated to reference the formatted label
rather than the raw zero-value flow.Identifier).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e0b679a-bdb4-445d-88dc-7ad369001dea
📒 Files selected for processing (2)
cmd/util/cmd/verify_execution_result/cmd.goengine/verification/verifier/verifiers.go
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/verification/verifier/verifiers_test.go`:
- Around line 63-67: The tests in verifiers_test.go currently use a mock that
always returns zero BlockVerificationStats and discard the returned totals (they
do "_, err := ..."), so they don't verify the new aggregation behavior; update
the mock (the closure returning (BlockVerificationStats, error) that checks
tt.errors[height]) to return realistic non-zero BlockVerificationStats for
different heights (matched/mismatched chunks/transactions), change the test call
to capture totals (e.g., totals, err := VerifyRange...), and add assertions that
totals equal the expected aggregated BlockVerificationStats for the whole range;
apply the same changes to the other similar test block (lines referenced as
76-87) so both tests validate aggregation rather than only error propagation.
- Around line 76-90: The test's expected-error branch logic is inverted: inside
the verifyConcurrently test block check that an error occurred and that it
matches the expected error using errors.Is; replace the current condition (which
fails when errors.Is is true) with one that fails when err is nil or when
!errors.Is(err, tt.expectedErr). Specifically update the verifyConcurrently test
assertion for tt.expectedErr to require err != nil and errors.Is(err,
tt.expectedErr) (using errors.Is to correctly handle wrapped fmt.Errorf values)
so the test only fails when there is no error or the error does not match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7423b93-6953-4c01-bbfc-6fbd18037da1
📒 Files selected for processing (1)
engine/verification/verifier/verifiers_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
engine/verification/verifier/verifiers_test.go (1)
71-75:⚠️ Potential issue | 🟠 MajorStats aggregation behavior is still not validated in this test.
mockVerifyHeightalways returns empty stats, and Line 84 discards the returned totals. This misses the main behavior added by the PR (aggregating matched/mismatched chunk/transaction counts).Suggested test update
func TestVerifyConcurrently(t *testing.T) { @@ tests := []struct { name string from uint64 to uint64 nWorker uint errors map[uint64]error // Map of heights to errors + stats map[uint64]BlockVerificationStats + expected BlockVerificationStats expectedErr error }{ { name: "All heights verified successfully", from: 1, to: 5, nWorker: 3, errors: nil, + stats: map[uint64]BlockVerificationStats{ + 1: {MatchedChunkCount: 1, MatchedTransactionCount: 2}, + 2: {MatchedChunkCount: 1, MismatchedChunkCount: 1, MismatchedTransactionCount: 3}, + 3: {MatchedChunkCount: 2, MatchedTransactionCount: 5}, + 4: {MismatchedChunkCount: 1, MismatchedTransactionCount: 1}, + 5: {MatchedChunkCount: 1, MatchedTransactionCount: 4}, + }, + expected: BlockVerificationStats{ + MatchedChunkCount: 5, + MismatchedChunkCount: 2, + MatchedTransactionCount: 11, + MismatchedTransactionCount: 4, + }, expectedErr: nil, }, @@ mockVerifyHeight := func( @@ ) (BlockVerificationStats, error) { if err, ok := tt.errors[height]; ok { return BlockVerificationStats{}, err } - return BlockVerificationStats{}, nil + return tt.stats[height], nil } @@ - _, err := verifyConcurrently( + totalStats, err := verifyConcurrently( @@ if tt.expectedErr != nil { if err == nil || !errors.Is(err, tt.expectedErr) { t.Fatalf("expected error: %v, got: %v", tt.expectedErr, err) } } else if err != nil { t.Fatalf("expected no error, got: %v", err) + } else if totalStats != tt.expected { + t.Fatalf("expected stats: %+v, got: %+v", tt.expected, totalStats) }Also applies to: 84-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/verification/verifier/verifiers_test.go` around lines 71 - 75, The test currently uses mockVerifyHeight which always returns empty BlockVerificationStats and the test ignores the aggregated totals; update mockVerifyHeight to return distinct, non-zero BlockVerificationStats for different heights (e.g., set MatchedChunks, MismatchedChunks, MatchedTxns, MismatchedTxns) and modify the test to capture and assert the aggregated totals (the variable currently discarded around Line 84) against the expected sums across heights so the new aggregation behavior in the verifier is validated; reference mockVerifyHeight and BlockVerificationStats when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@engine/verification/verifier/verifiers_test.go`:
- Around line 71-75: The test currently uses mockVerifyHeight which always
returns empty BlockVerificationStats and the test ignores the aggregated totals;
update mockVerifyHeight to return distinct, non-zero BlockVerificationStats for
different heights (e.g., set MatchedChunks, MismatchedChunks, MatchedTxns,
MismatchedTxns) and modify the test to capture and assert the aggregated totals
(the variable currently discarded around Line 84) against the expected sums
across heights so the new aggregation behavior in the verifier is validated;
reference mockVerifyHeight and BlockVerificationStats when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b398821c-edaf-4a47-9232-f13daa3f069a
📒 Files selected for processing (1)
engine/verification/verifier/verifiers_test.go
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
engine/verification/verifier/verifiers_test.go (1)
347-363: Assert the wrapped sentinel here too.
require.Errorwill still pass if this path starts returning the wrong error. Add anerrors.Is/assert.ErrorIscheck forstorage.ErrNotFoundso the test covers the classification contract, not just “some error.”As per coding guidelines "Treat all inputs as potentially byzantine and classify errors in a context-dependent manner".Suggested assertion
require.Error(t, err) + assert.ErrorIs(t, err, storage.ErrNotFound) assert.Equal(t, BlockVerificationStats{}, stats)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/verification/verifier/verifiers_test.go` around lines 347 - 363, Update TestVerifyHeight_HeaderNotFound to assert the specific sentinel error: after calling verifyHeight (and keeping the existing require.Error check), add an errors.Is or assert.ErrorIs assertion that the returned err wraps storage.ErrNotFound to ensure the classification contract is enforced; locate the test function TestVerifyHeight_HeaderNotFound and the call to verifyHeight and add the ErrorIs check against storage.ErrNotFound (keeping the existing stats equality assertion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/verification/verifier/verifiers_test.go`:
- Around line 55-63: The test case for "Multiple errors, lowest height returned"
is nondeterministic; modify the mock worker used by verifyConcurrently so the
higher-height worker (height 4) is allowed to complete before the lower-height
worker (height 2) by introducing a synchronization gate (e.g., a channel or wait
group) that blocks the lower-height goroutine until the higher-height one
signals completion, then run verifyConcurrently and assert expectedErr ==
errTwo; apply the same gating pattern to the other two similar cases (the blocks
around lines referenced 70-83 and 91-105) so each explicitly makes the
higher-height finish first while still expecting the minimum-height error to be
returned.
---
Nitpick comments:
In `@engine/verification/verifier/verifiers_test.go`:
- Around line 347-363: Update TestVerifyHeight_HeaderNotFound to assert the
specific sentinel error: after calling verifyHeight (and keeping the existing
require.Error check), add an errors.Is or assert.ErrorIs assertion that the
returned err wraps storage.ErrNotFound to ensure the classification contract is
enforced; locate the test function TestVerifyHeight_HeaderNotFound and the call
to verifyHeight and add the ErrorIs check against storage.ErrNotFound (keeping
the existing stats equality assertion).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cceaa5f0-219a-4b16-be52-8ef6b40f645c
📒 Files selected for processing (1)
engine/verification/verifier/verifiers_test.go
Comment on lines
+55
to
+63
| name: "Multiple errors, lowest height returned", | ||
| from: 1, | ||
| to: 5, | ||
| nWorker: 3, | ||
| errors: map[uint64]error{ | ||
| 2: errTwo, | ||
| 4: errFour, | ||
| }, | ||
| expectedErr: errTwo, |
Contributor
There was a problem hiding this comment.
Make the “lowest height returned” case deterministic.
This never forces height 4 to fail before height 2, so it still passes if verifyConcurrently returns the first worker error instead of the minimum-height one. Gate the mock so the higher height completes first, then assert that errTwo still wins.
One way to tighten the case
+ releaseHeightTwo := make(chan struct{})
+
mockVerifyHeight := func(
height uint64,
headers storage.Headers,
chunkDataPacks storage.ChunkDataPacks,
results storage.ExecutionResults,
state protocol.State,
verifier module.ChunkVerifier,
stopOnMismatch bool,
) (BlockVerificationStats, error) {
+ if tt.expectedErr == errTwo {
+ switch height {
+ case 2:
+ <-releaseHeightTwo
+ case 4:
+ close(releaseHeightTwo)
+ }
+ }
+
if err, ok := tt.errors[height]; ok {
return BlockVerificationStats{}, err
}
return BlockVerificationStats{}, nil
}Also applies to: 70-83, 91-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/verification/verifier/verifiers_test.go` around lines 55 - 63, The
test case for "Multiple errors, lowest height returned" is nondeterministic;
modify the mock worker used by verifyConcurrently so the higher-height worker
(height 4) is allowed to complete before the lower-height worker (height 2) by
introducing a synchronization gate (e.g., a channel or wait group) that blocks
the lower-height goroutine until the higher-height one signals completion, then
run verifyConcurrently and assert expectedErr == errTwo; apply the same gating
pattern to the other two similar cases (the blocks around lines referenced 70-83
and 91-105) so each explicitly makes the higher-height finish first while still
expecting the minimum-height error to be returned.
Member
Author
|
@zhangchiqing PTAL 🙏 |
zhangchiqing
approved these changes
Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Log the number of matching/mismatching blocks/transactions when verifying
Summary by CodeRabbit
New Features
Improvements
Tests