[Util] Improve compare-debug-tx#8512
Merged
turbolent merged 2 commits intoApr 8, 2026
masteronflow/flow-go:masterfrom
bastian/improve-compare-debug-txonflow/flow-go:bastian/improve-compare-debug-txCopy head branch name to clipboard
Merged
[Util] Improve compare-debug-tx#8512turbolent merged 2 commits intomasteronflow/flow-go:masterfrom bastian/improve-compare-debug-txonflow/flow-go:bastian/improve-compare-debug-txCopy head branch name to clipboard
turbolent merged 2 commits into
masteronflow/flow-go:masterfrom
bastian/improve-compare-debug-txonflow/flow-go:bastian/improve-compare-debug-txCopy head branch name to clipboard
Conversation
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/util/cmd/compare-debug-tx/cmd.go (1)
82-92:⚠️ Potential issue | 🔴 CriticalDefault
--block-count=1now forces block mode.Line 84 now enters the block-only flow on the default value, so a normal
compare-debug-tx <tx-id...>invocation hits the--block-id/ positional-args fatals. That regresses the existing tx-ID workflow and no longer matches the Line 65 help text.💡 Suggested fix
- if flagBlockCount > 0 { + if flagBlockCount < 1 { + log.Fatal().Msg("--block-count must be >= 1") + } + if flagBlockID != "" || flagBlockCount > 1 { if flagBlockID == "" { log.Fatal().Msg("--block-count requires --block-id to be set") } if len(args) > 0 { log.Fatal().Msg("--block-count cannot be used with positional transaction IDs") } blockIDs = resolveBlockChain(flagBlockID, flagBlockCount) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/util/cmd/compare-debug-tx/cmd.go` around lines 82 - 92, The code treats flagBlockCount > 0 as entering block-only mode but the flag's default is 1, so normal positional tx-ID usage is broken; update the flag definition for flagBlockCount to default to 0 (so block mode only when user explicitly passes --block-count > 0) or, alternatively, change the conditional to only trigger when the flag was explicitly set (e.g., detect and check that flagBlockCount != 1 if you intend 1 to mean "unset"); adjust the flag declaration where flagBlockCount is defined and ensure the conditional around resolveBlockChain(flagBlockID, flagBlockCount) continues to use flagBlockCount for the resolved logic and that error messages referencing flagBlockID/positional args remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/util/cmd/compare-debug-tx/cmd.go`:
- Around line 368-387: The loop currently compares sections1[j] vs sections2[j]
by slice index, which misattributes mismatches when sections are inserted or
removed; instead parse the transaction ID from each section (the "# ID:" line
returned by splitTxSections) and build maps id->section for both branches (use
the same ID parsing logic for both), then iterate the union of IDs to compare
corresponding sections by ID and increment txsWithMismatch and blockTxMismatches
for IDs whose byte content differs; update txCount/totalTxs to be the size of
the union and keep existing variables (sections1, sections2, splitTxSections,
blockTxMismatches, txsWithMismatch, totalTxs) while replacing the index-based
loop with an ID-keyed comparison.
- Around line 134-145: The code only prints diffs when len(blockIDs)==1, which
skips diffs for the positional-TX path where blockIDs is empty and leads to a
meaningless 0/0 stats output; update the logic so diffFiles(result1.Name(),
result2.Name(), flagBranch1, flagBranch2) (and the trace diff guarded by
flagShowTraceDiff using trace1/trace2) are emitted for both the single-block
case and the positional-TX case (i.e., when len(blockIDs)==1 ||
len(blockIDs)==0), and also guard the printMismatchStats(perBlock1, perBlock2)
call so it is only invoked when there are actual per-block entries (e.g.,
len(blockIDs)>0 or non-empty perBlock1/perBlock2) to avoid the 0/0 summary.
---
Outside diff comments:
In `@cmd/util/cmd/compare-debug-tx/cmd.go`:
- Around line 82-92: The code treats flagBlockCount > 0 as entering block-only
mode but the flag's default is 1, so normal positional tx-ID usage is broken;
update the flag definition for flagBlockCount to default to 0 (so block mode
only when user explicitly passes --block-count > 0) or, alternatively, change
the conditional to only trigger when the flag was explicitly set (e.g., detect
and check that flagBlockCount != 1 if you intend 1 to mean "unset"); adjust the
flag declaration where flagBlockCount is defined and ensure the conditional
around resolveBlockChain(flagBlockID, flagBlockCount) continues to use
flagBlockCount for the resolved logic and that error messages referencing
flagBlockID/positional args remain valid.
🪄 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: 3b171796-937e-4e68-bef7-81bf5e4d078b
📒 Files selected for processing (1)
cmd/util/cmd/compare-debug-tx/cmd.go
Comment on lines
+134
to
+145
| if len(blockIDs) == 1 { | ||
| fmt.Printf("=== Result diff (%s vs %s) ===\n", flagBranch1, flagBranch2) | ||
| diffFiles(result1.Name(), result2.Name(), flagBranch1, flagBranch2) | ||
|
|
||
| if flagShowTraceDiff { | ||
| fmt.Printf("=== Trace diff (%s vs %s) ===\n", flagBranch1, flagBranch2) | ||
| diffFiles(trace1.Name(), trace2.Name(), flagBranch1, flagBranch2) | ||
| } | ||
| } | ||
|
|
||
| fmt.Printf("=== Result diff (%s vs %s) ===\n", flagBranch1, flagBranch2) | ||
| diffFiles(result1.Name(), result2.Name(), flagBranch1, flagBranch2) | ||
| printMismatchStats(blockIDs, perBlock1, perBlock2) | ||
| } |
Contributor
There was a problem hiding this comment.
Single-invocation TX comparisons stop showing diffs.
This branch only emits diffs when exactly one block ID was resolved. In the positional-TX path blockIDs is empty, so the normal result diff disappears, --show-trace-diff becomes a no-op, and the unconditional printMismatchStats call falls through to a meaningless 0/0 block summary.
💡 Suggested fix
- if len(blockIDs) == 1 {
+ if len(blockIDs) <= 1 {
fmt.Printf("=== Result diff (%s vs %s) ===\n", flagBranch1, flagBranch2)
diffFiles(result1.Name(), result2.Name(), flagBranch1, flagBranch2)
if flagShowTraceDiff {
fmt.Printf("=== Trace diff (%s vs %s) ===\n", flagBranch1, flagBranch2)
diffFiles(trace1.Name(), trace2.Name(), flagBranch1, flagBranch2)
}
}
- printMismatchStats(blockIDs, perBlock1, perBlock2)
+ if len(blockIDs) > 0 {
+ printMismatchStats(blockIDs, perBlock1, perBlock2)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(blockIDs) == 1 { | |
| fmt.Printf("=== Result diff (%s vs %s) ===\n", flagBranch1, flagBranch2) | |
| diffFiles(result1.Name(), result2.Name(), flagBranch1, flagBranch2) | |
| if flagShowTraceDiff { | |
| fmt.Printf("=== Trace diff (%s vs %s) ===\n", flagBranch1, flagBranch2) | |
| diffFiles(trace1.Name(), trace2.Name(), flagBranch1, flagBranch2) | |
| } | |
| } | |
| fmt.Printf("=== Result diff (%s vs %s) ===\n", flagBranch1, flagBranch2) | |
| diffFiles(result1.Name(), result2.Name(), flagBranch1, flagBranch2) | |
| printMismatchStats(blockIDs, perBlock1, perBlock2) | |
| } | |
| if len(blockIDs) <= 1 { | |
| fmt.Printf("=== Result diff (%s vs %s) ===\n", flagBranch1, flagBranch2) | |
| diffFiles(result1.Name(), result2.Name(), flagBranch1, flagBranch2) | |
| if flagShowTraceDiff { | |
| fmt.Printf("=== Trace diff (%s vs %s) ===\n", flagBranch1, flagBranch2) | |
| diffFiles(trace1.Name(), trace2.Name(), flagBranch1, flagBranch2) | |
| } | |
| } | |
| if len(blockIDs) > 0 { | |
| printMismatchStats(blockIDs, perBlock1, perBlock2) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/util/cmd/compare-debug-tx/cmd.go` around lines 134 - 145, The code only
prints diffs when len(blockIDs)==1, which skips diffs for the positional-TX path
where blockIDs is empty and leads to a meaningless 0/0 stats output; update the
logic so diffFiles(result1.Name(), result2.Name(), flagBranch1, flagBranch2)
(and the trace diff guarded by flagShowTraceDiff using trace1/trace2) are
emitted for both the single-block case and the positional-TX case (i.e., when
len(blockIDs)==1 || len(blockIDs)==0), and also guard the
printMismatchStats(perBlock1, perBlock2) call so it is only invoked when there
are actual per-block entries (e.g., len(blockIDs)>0 or non-empty
perBlock1/perBlock2) to avoid the 0/0 summary.
Comment on lines
+368
to
+387
| sections1 := splitTxSections(data1) | ||
| sections2 := splitTxSections(data2) | ||
|
|
||
| blockTxMismatches := 0 | ||
| txCount := max(len(sections1), len(sections2)) | ||
| totalTxs += txCount | ||
|
|
||
| for j := range txCount { | ||
| var s1, s2 []byte | ||
| if j < len(sections1) { | ||
| s1 = sections1[j] | ||
| } | ||
| if j < len(sections2) { | ||
| s2 = sections2[j] | ||
| } | ||
| if !bytes.Equal(s1, s2) { | ||
| txsWithMismatch++ | ||
| blockTxMismatches++ | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Compare mismatch sections by transaction ID, not slice index.
splitTxSections only gives this loop ordered sections, so it compares sections1[j] with sections2[j]. If one branch drops or inserts a single # ID: section, every following transaction is counted as mismatched even when the later IDs still match. Since the output already carries transaction IDs, key the comparison by ID so the per-TX summary stays accurate.
Also applies to: 411-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/util/cmd/compare-debug-tx/cmd.go` around lines 368 - 387, The loop
currently compares sections1[j] vs sections2[j] by slice index, which
misattributes mismatches when sections are inserted or removed; instead parse
the transaction ID from each section (the "# ID:" line returned by
splitTxSections) and build maps id->section for both branches (use the same ID
parsing logic for both), then iterate the union of IDs to compare corresponding
sections by ID and increment txsWithMismatch and blockTxMismatches for IDs whose
byte content differs; update txCount/totalTxs to be the size of the union and
keep existing variables (sections1, sections2, splitTxSections,
blockTxMismatches, txsWithMismatch, totalTxs) while replacing the index-based
loop with an ID-keyed comparison.
janezpodhostnik
approved these changes
Apr 6, 2026
zhangchiqing
approved these changes
Apr 6, 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.
Show differences per TX
Summary by CodeRabbit
New Features
Changes