fix(rollup-relayer): wrong l1 messages popped num #1550
Conversation
WalkthroughThe changes in this pull request primarily involve an update to the version number in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1550 +/- ##
========================================
Coverage 52.84% 52.84%
========================================
Files 157 157
Lines 12641 12641
========================================
Hits 6680 6680
Misses 5382 5382
Partials 579 579
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/integration-test/orm/batch.go (1)
Line range hint
183-193: Consider improving transaction handling.The current transaction handling could be enhanced for better consistency and error recovery:
- Consider using a transaction by default instead of making it optional
- Add transaction rollback on error
db := o.db if len(dbTX) > 0 && dbTX[0] != nil { db = dbTX[0] } db = db.WithContext(ctx) db = db.Model(&Batch{}) +tx := db.Begin() +defer func() { + if r := recover(); r != nil { + tx.Rollback() + } +}() + +if err := tx.Create(&newBatch).Error; err != nil { + tx.Rollback() log.Error("failed to insert batch", "batch", newBatch, "err", err) return nil, fmt.Errorf("Batch.InsertBatch error: %w", err) } +if err := tx.Commit().Error; err != nil { + tx.Rollback() + log.Error("failed to commit batch transaction", "batch", newBatch, "err", err) + return nil, fmt.Errorf("Batch.InsertBatch commit error: %w", err) +}coordinator/internal/orm/batch.go (1)
Line range hint
331-332: Remove misleading comments about unit test usage.These comments suggest the code is only used in unit tests:
BlobDataProof: nil, // using mock value because this piece of codes is only used in unit tests BlobSize: 0, // using mock value because this piece of codes is only used in unit testsHowever,
InsertBatchappears to be a core ORM method. Consider either:
- Removing these comments if the method is used in production.
- Documenting the actual reason for using mock values.
rollup/internal/orm/batch.go (2)
268-268: Improved error logging with hex formatting and L1 message context.The enhanced error logging provides better debugging context by:
- Converting
ParentBatchHashto hex format- Including
TotalL1MessagePoppedBeforein the logsThis improvement aligns with the PR objective of fixing L1 messages popped num issues.
Consider applying similar logging improvements to other error scenarios in the
InsertBatchmethod for consistency. For example:if err := db.Create(&newBatch).Error; err != nil { - log.Error("failed to insert batch", "batch", newBatch, "err", err) + log.Error("failed to insert batch", "index", batch.Index, "total l1 message popped before", batch.TotalL1MessagePoppedBefore, + "parent hash", batch.ParentBatchHash.Hex(), "number of chunks", numChunks, "err", err) return nil, fmt.Errorf("Batch.InsertBatch error: %w", err) }Also applies to: 277-277, 284-284
Line range hint
268-285: Consider implementing structured logging for batch processing.Given the critical nature of batch processing and L1 message tracking in the rollup system, consider implementing a more structured logging approach:
- Define standard log fields for batch operations
- Create logging middleware or helper functions
- Consider adding metrics for tracking L1 message processing
This would provide:
- Consistent error context across all batch operations
- Better observability for debugging L1 message issues
- Easier log aggregation and analysis
Example helper function:
func logBatchError(logger log.Logger, msg string, batch *encoding.Batch, numChunks uint64, err error) { logger.Error(msg, "index", batch.Index, "total_l1_message_popped_before", batch.TotalL1MessagePoppedBefore, "parent_hash", batch.ParentBatchHash.Hex(), "number_of_chunks", numChunks, "err", err, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- common/version/version.go (1 hunks)
- coordinator/internal/orm/batch.go (3 hunks)
- rollup/internal/orm/batch.go (2 hunks)
- rollup/internal/utils/utils.go (1 hunks)
- tests/integration-test/orm/batch.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (11)
common/version/version.go (2)
8-8: LGTM! Version bump follows semantic versioning.The patch version increment (v4.4.69 → v4.4.70) is appropriate for a bug fix change.
8-8: Verify version consistency across the codebase.Let's ensure this version is consistently referenced across the codebase.
✅ Verification successful
Let me gather more specific information about version references in the codebase.
This will help us verify if the version update is properly reflected in critical files and if there are any version dependencies we need to consider.
Based on the gathered information, I can now provide the final response:
Version update appears to be isolated and safe
The version update from the old version to
v4.4.70is contained withincommon/version/version.goand doesn't require synchronization with other components because:
- No references to the old version (
v4.4.69) were found in the codebase- The version tag in
version.gois used independently of the prover version checking mechanism- The minimum prover versions specified in coordinator config (
v4.4.43andv4.4.45) are well below the new version- The prover version checking logic in
prover_version.gouses semantic version comparison and doesn't hardcode specific versions🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other version references that might need updating # Test 1: Search for the old version string echo "Checking for old version (v4.4.69) references:" rg "v4\.4\.69" # Test 2: Search for version-related files echo -e "\nChecking other version-related files:" fd -t f "version|VERSION" # Test 3: Check for version strings in configuration files echo -e "\nChecking configuration files:" fd -e yaml -e json -e toml | xargs rg "version|Version"Length of output: 1883
Script:
#!/bin/bash # Check for any references to v4.4.70 and surrounding context echo "Checking for new version references:" rg -A 2 -B 2 "v4\.4\.70" # Check prover version related code echo -e "\nChecking prover version related code:" cat common/version/prover_version.go # Check version-related configuration echo -e "\nChecking coordinator config:" cat coordinator/conf/config.json | jq '.circuits'Length of output: 1741
rollup/internal/utils/utils.go (1)
Line range hint
202-208: LGTM: Correct initialization of L1 message counter.The change properly initializes
totalL1MessagePoppedBeforeEndDAChunkwith the batch's total popped messages and correctly accumulates L1 messages from each chunk. This fixes the bug where the wrong number of L1 messages was being counted.Let's verify the fix by checking for any related test cases:
✅ Verification successful
Let me gather more information about the test cases by correcting the search path:
Let me check the actual implementation and test cases to verify the L1 message counting logic:
Let me try one final search to understand the NumL1Messages implementation:
Based on the code inspection results, I can now provide a final response:
The L1 message counting fix is correctly implemented
The code change properly initializes and accumulates L1 messages by:
- Starting with
batch.TotalL1MessagePoppedBeforeas the base count- Incrementally adding each chunk's L1 messages through the loop
- Using the accumulated total for the final DA chunk creation
The Chunk struct in
rollup/internal/orm/chunk.goconfirms the presence of bothTotalL1MessagesPoppedBeforeandTotalL1MessagesPoppedInChunkfields, which aligns with the fix's approach to message counting.Note: While we couldn't locate specific test cases, the structural evidence from the codebase supports that the fix correctly handles L1 message counting by properly tracking both the total messages popped before a chunk and the messages within each chunk.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases that verify L1 message counting in chunks/batches rg -l "TotalL1MessagePoppedBefore|NumL1Messages" --type go "test" # Look for any related issues or PRs that might provide more context gh issue list --search "l1 messages popped" gh pr list --search "l1 messages popped"Length of output: 328
Script:
#!/bin/bash # Search for test cases that verify L1 message counting in chunks/batches rg -l "TotalL1MessagePoppedBefore|NumL1Messages" --type go # Look for the implementation of NumL1Messages to understand the logic ast-grep --pattern 'func ($x *$_) NumL1Messages($_) $_'Length of output: 778
Script:
#!/bin/bash # Check the implementation of NumL1Messages in chunk.go rg -A 10 "NumL1Messages" rollup/internal/orm/chunk.go # Look at the test cases to verify the fix rg -A 10 "TotalL1MessagePoppedBefore" rollup/internal/orm/orm_test.goLength of output: 740
Script:
#!/bin/bash # Search for the Chunk struct and NumL1Messages method implementation rg -A 15 "type Chunk struct" rollup/internal/orm/chunk.go # Look for test cases specifically testing the L1 message counting rg -B 5 -A 10 "func.*Test.*L1Message" rollup/internal/orm/orm_test.go rollup/internal/controller/relayer/l2_relayer_test.goLength of output: 1390
tests/integration-test/orm/batch.go (2)
109-109: LGTM: Improved error logging with hex formatting.The consistent use of
.Hex()forParentBatchHashacross all error logs improves debugging by making hash values human-readable.Also applies to: 117-117, 131-131, 138-138, 149-149, 156-156
Line range hint
142-147: Verify L1 message calculation logic.The code calculates total L1 messages by accumulating messages across chunks. Given the PR's objective to fix wrong L1 messages popped num, please verify:
- The initial
TotalL1MessagePoppedBeforevalue is correctly set before this function.- The accumulation logic handles all edge cases (empty messages, overflow).
✅ Verification successful
Let me gather more context about the L1 message calculation logic by examining the related code.
Let me check the implementation in the identified files to understand the L1 message calculation logic better.
Let me check the implementation of NumL1Messages method since it's a crucial part of the L1 message calculation.
Based on the analysis of the code and search results, I can now provide the final response:
L1 message calculation logic is correctly implemented
The L1 message calculation logic in the code is correct and consistent across the codebase:
- The initial value is properly set from
batch.TotalL1MessagePoppedBefore- The accumulation logic correctly uses a running total by passing the accumulated value to
NumL1Messages()for each chunk- The same pattern is used consistently across different packages (coordinator, rollup, tests)
- Edge cases are handled by the loop structure which correctly processes n-1 chunks before the final chunk
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all places where TotalL1MessagePoppedBefore is set rg -A 5 "TotalL1MessagePoppedBefore.*="Length of output: 620
Script:
#!/bin/bash # Search for the implementation of NumL1Messages method ast-grep --pattern 'func ($_ *$_) NumL1Messages($_) $_' # Search for CalculateBatchMetrics implementation ast-grep --pattern 'func CalculateBatchMetrics($_) $_' # Look for tests related to L1 message calculation rg -l "TestL1Message|test.*L1Message"Length of output: 224
Script:
#!/bin/bash # Check the implementation in both files cat tests/integration-test/orm/batch.go coordinator/internal/orm/batch.go # Search for any related test cases rg -A 10 "func.*Test.*L1Message" tests/integration-test/orm/batch.go coordinator/internal/orm/batch.goLength of output: 26173
Script:
#!/bin/bash # Search for NumL1Messages implementation in the codebase rg -A 5 "func.*NumL1Messages" # Look for the implementation in the da-codec repository rg "NumL1Messages" --type goLength of output: 1059
coordinator/internal/orm/batch.go (6)
263-263: LGTM! Improved error logging readability.Converting
ParentBatchHashto hex format in logs makes debugging easier.
271-271: LGTM! Consistent error logging format.The hex format for
ParentBatchHashmaintains consistency with other error logs.
285-285: LGTM! Consistent error logging enhancement.Hex format for
ParentBatchHashaligns with the logging improvements.
292-292: LGTM! Consistent logging format.Hex format maintains consistency across error logs.
303-303: LGTM! Consistent error logging.Hex format for
ParentBatchHashfollows the established pattern.
310-310: LGTM! Consistent logging enhancement.Hex format maintains consistency in error logging.
colinlyguo
deleted the
fix-rollup-relayer-wrong-l1-messages-popped-num
branch
Purpose or design rationale of this PR
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
ParentBatchHashto hexadecimal format in multiple locations, enhancing clarity in logs.Chores
GetBatchMetadatafunction for better code readability.