fix(datarace): make failedBlocks to sync.Map for concurrent access
#1549
Conversation
WalkthroughThe recent update modifies the Changes
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 (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- mod/execution/pkg/deposit/service.go (3 hunks)
- mod/execution/pkg/deposit/sync.go (3 hunks)
Additional comments not posted (4)
mod/execution/pkg/deposit/sync.go (2)
72-81: The use ofsync.MapindepositCatchupFetcherfunction is correctly implemented for concurrent access.
94-94: Correct implementation ofsync.Mapmethods (StoreandDelete) infetchAndStoreDepositsenhances thread safety.Also applies to: 107-107, 111-111
mod/execution/pkg/deposit/service.go (2)
66-66: The declaration offailedBlocksassync.Mapin theServicestruct is appropriate for ensuring thread safety.
109-109: Proper initialization offailedBlocksassync.Mapin theNewServiceconstructor ensures immediate readiness for thread-safe operations.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- mod/execution/pkg/deposit/sync.go (3 hunks)
Additional comments not posted (1)
mod/execution/pkg/deposit/sync.go (1)
Line range hint
1-111: Well-structured error handling and logging.The file demonstrates consistent and structured error handling and logging, which enhances maintainability and traceability of the deposit handling operations.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1549 +/- ##
===========================================
+ Coverage 52.83% 64.15% +11.32%
===========================================
Files 7 7
Lines 106 106
Branches 21 21
===========================================
+ Hits 56 68 +12
+ Misses 49 38 -11
+ Partials 1 0 -1 |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- mod/execution/pkg/deposit/service.go (3 hunks)
- mod/execution/pkg/deposit/sync.go (3 hunks)
Additional comments not posted (4)
mod/execution/pkg/deposit/sync.go (1)
102-102: Correct usage ofsync.Mapfor deletion.The deletion operation on
sync.Mapis correctly implemented. This ensures that the entries are properly removed from the map after successful handling, contributing to the prevention of memory leaks or stale data accumulation.mod/execution/pkg/deposit/service.go (3)
25-25: Import ofsyncpackage.The import of the
syncpackage is necessary for the use ofsync.Map. This is a straightforward change but essential for the modifications made in this file.
56-56: Declaration offailedBlocksassync.Map.The change from a regular map to
sync.Mapis aligned with the objective of enhancing thread safety. This is a critical update to ensure that theServicestruct can safely handle concurrent operations without leading to race conditions.
93-93: Initialization offailedBlocksinNewService.The initialization of
failedBlocksassync.Map{}in the constructor ensures that the map is ready to be used safely across different goroutines immediately upon the creation of theServiceinstance.
itsdevbear
force-pushed
the
main
branch
4 times, most recently
from
bf610aa
to
3baa52e
Compare
|
@itsdevbear @ocnc please review it! |
itsdevbear
force-pushed
the
main
branch
4 times, most recently
from
7fdec0e
to
bc9f002
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- mod/execution/pkg/deposit/service.go (3 hunks)
- mod/execution/pkg/deposit/sync.go (3 hunks)
Additional comments not posted (5)
mod/execution/pkg/deposit/sync.go (3)
65-74: Refine iteration control infailedBlocks.Rangefor error handling.The iteration over
failedBlockscontinues even after a key fails to parse, which might not be the intended behavior. It's essential to ensure that the system behaves predictably in error scenarios. Consider refining the control logic to either skip the iteration or log and continue based on the severity of the error.- return true + return ok // Continue only if the key was successfully parsed.
85-85: Consistency issue in stored values insync.Map.The
Storemethod is currently used to markfailedBlocks, but the value stored is an empty struct (struct{}{}). This is inconsistent with previous discussions to storetrue. Aligning this throughout the code will ensure consistency and clarity.- s.failedBlocks.Store(blockNum, struct{}{}) + s.failedBlocks.Store(blockNum, true)
98-98: Consistency issue in stored values insync.Map.The
Storemethod is currently used to markfailedBlocks, but the value stored is an empty struct (struct{}{}). This is inconsistent with previous discussions to storetrue. Aligning this throughout the code will ensure consistency and clarity.- s.failedBlocks.Store(blockNum, struct{}{}) + s.failedBlocks.Store(blockNum, true)mod/execution/pkg/deposit/service.go (2)
25-25: Import ofsyncpackage is appropriate.The addition of the
syncpackage import is necessary for usingsync.Mapand is correctly implemented.
53-54: Transition tosync.MapforfailedBlocksis appropriate.The change from a standard map to
sync.Mapensures thread-safe access tofailedBlocks, which is crucial in a concurrent environment. The initialization is correctly updated to reflect this change.Also applies to: 85-85
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- mod/execution/pkg/deposit/service.go (3 hunks)
- mod/execution/pkg/deposit/sync.go (3 hunks)
Additional comments not posted (7)
mod/execution/pkg/deposit/sync.go (4)
56-65: Improved concurrency and error handling indepositCatchupFetcher.The changes in this function address the data race issue mentioned in the PR objectives and improve error handling:
- Using
s.failedBlocks.Range()suggests thatfailedBlocksis now async.Map, which is thread-safe for concurrent access.- The addition of error handling for key type assertion enhances the robustness of the code.
- Improved logging provides better visibility into the retry process.
These changes are approved as they align with the PR objectives and improve the overall quality of the code.
76-76: Approved: Thread-safe storage of failed blocks.The change to use
s.failedBlocks.Store()is consistent with the move tosync.Map, addressing the data race issue.Regarding the use of
struct{}{}as the value:
- This is a common Go idiom for using a map as a set, as mentioned in a previous comment.
- It's memory-efficient as an empty struct occupies zero bytes.
- We're only interested in the keys (block numbers), not the values.
This implementation is correct and follows Go best practices for set-like behavior.
89-89: Approved: Consistent use of thread-safe operations.The changes on lines 89 and 93 are consistent with the use of
sync.Map:
- Line 89: Using
s.failedBlocks.Store()ensures thread-safe addition of failed blocks.- Line 93: Using
s.failedBlocks.Delete()ensures thread-safe removal of successfully processed blocks.These modifications are crucial for maintaining the integrity of the
failedBlocksdata structure in a concurrent environment, directly addressing the data race issue mentioned in the PR objectives.Also applies to: 93-93
56-65: Summary: Effective implementation of thread-safe operations forfailedBlocks.The changes in this file consistently implement the use of
sync.MapforfailedBlocks, addressing the data race issue mentioned in the PR objectives. Key improvements include:
- Thread-safe iteration in
depositCatchupFetcherusingRange().- Consistent use of
Store()andDelete()methods for managing failed blocks.- Improved error handling and logging for better debuggability.
These changes enhance the robustness and concurrency safety of the deposit handling process. The implementation aligns well with Go best practices for concurrent map usage.
Also applies to: 76-76, 89-89, 93-93
mod/execution/pkg/deposit/service.go (3)
25-25: LGTM: Import of "sync" package added.The addition of the "sync" package import is correct and necessary for using
sync.Map.
Line range hint
1-140: Summary: Changes effectively address the data race issue.The modifications in this file successfully implement the use of
sync.MapforfailedBlocks, addressing the data race issue mentioned in the PR objectives. The changes are well-implemented and improve the thread-safety of theServicestruct.A few minor suggestions were made:
- Expanding the comment for
failedBlocksto provide more context on the benefits of usingsync.Map.- Verifying the type consistency between
math.U64andmath.SlotforfailedBlocks.Overall, these changes enhance the robustness of the code for concurrent operations.
87-87: LGTM: Correct initialization ofsync.Map.The change from
make(map[math.Slot]struct{})tosync.Map{}correctly initializes thefailedBlocksfield with an emptysync.Map.I noticed a potential inconsistency in the types used. The struct definition uses
math.U64, but the old initialization usedmath.Slot. Could you verify if this was intentional or if it needs to be addressed? Here's a script to check for any remaining occurrences ofmath.Slotin relation tofailedBlocks:Verification successful
No issues found with the transition to
sync.MapforfailedBlocks.The change from
make(map[math.Slot]struct{})tosync.Map{}forfailedBlocksis correct and does not affect other parts of the codebase that usemath.Slot. The transition appears to be isolated and appropriate.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining occurrences of math.Slot in relation to failedBlocks # Test: Search for math.Slot in relation to failedBlocks rg --type go -A 5 $'failedBlocks.*math\.Slot'Length of output: 46
Script:
#!/bin/bash # Description: Search for any occurrences of math.Slot in the codebase # Test: Search for math.Slot in the entire codebase rg --type go 'math\.Slot'Length of output: 16148
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range comments (4)
contracts/src/eip4788/SSZ.sol (3)
Line range hint
15-19: LGTM! Consider minor formatting adjustment for consistency.The change to move the parameter to the same line as the function name improves code conciseness without affecting functionality.
For consistency with other functions in this file, consider moving the
internalandviewmodifiers to the same line as well:- function validatorPubkeyHashTreeRoot(bytes memory pubkey) - internal - view - returns (bytes32 root) + function validatorPubkeyHashTreeRoot(bytes memory pubkey) internal view returns (bytes32 root)
Line range hint
35-39: LGTM! Consider minor formatting adjustment for consistency.The change to move the parameter to the same line as the function name improves code conciseness without affecting functionality.
For consistency with other functions in this file, consider moving the
internalandpuremodifiers to the same line as well:- function addressHashTreeRoot(address v) - internal - pure - returns (bytes32 root) + function addressHashTreeRoot(address v) internal pure returns (bytes32 root)
Line range hint
1-119: Consider a security audit for this cryptographic library.While the changes in this PR are purely cosmetic and don't affect functionality, this file contains complex cryptographic operations and low-level assembly code. Given its critical nature in handling validator public keys and Merkle proofs, it's advisable to have a thorough security audit performed on this library.
Consider engaging a third-party security firm specializing in smart contract audits to review this SSZ library, focusing on:
- Correctness of the cryptographic implementations
- Potential vulnerabilities in the assembly code
- Gas optimization without compromising security
- Proper error handling and edge case management
contracts/test/BeaconDepositContract.t.sol (1)
Line range hint
1-236: Overall feedback: Good formatting improvements with suggestions for enhancementThe changes made to this file are primarily formatting improvements that enhance readability without altering functionality. These changes are consistent with common Solidity style guidelines and are approved.
While the test suite appears comprehensive, consider the following suggestions to further improve the contract's test coverage and maintainability:
- Add more edge cases: Consider adding tests for extreme values, such as
type(uint256).maxfor amounts.- Test for revert reasons: Ensure all possible revert reasons are tested, including those from the
BeaconDepositContract.- Use constants for repeated values: Consider defining constants for frequently used values like
32 etherto improve maintainability.- Add documentation: Consider adding NatSpec comments to describe the purpose of each test function.
- Gas optimization tests: If relevant, consider adding tests to ensure gas usage remains within expected bounds.
These suggestions aim to enhance the robustness and maintainability of your test suite.
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (5)
- contracts/src/eip4788/BeaconVerifier.sol (2 hunks)
- contracts/src/eip4788/SSZ.sol (2 hunks)
- contracts/src/eip4788/Verifier.sol (1 hunks)
- contracts/src/eip4788/interfaces/IBeaconVerifier.sol (1 hunks)
- contracts/test/BeaconDepositContract.t.sol (2 hunks)
Additional comments not posted (6)
contracts/src/eip4788/BeaconVerifier.sol (4)
52-57: LGTM: Function signature reformatting.The function signature has been reformatted to a single line, improving readability without changing functionality.
Line range hint
60-65: LGTM: Function signature reformatting.The function signature has been reformatted to a single line, improving readability without changing functionality.
Line range hint
83-88: LGTM: Function signature reformatting.The function signature has been reformatted to a single line, improving readability without changing functionality.
Line range hint
52-88: Summary: Improved code readability through function signature reformatting.The changes in this file are purely cosmetic, focusing on reformatting function signatures to single lines. This improves code readability without altering the contract's functionality. The affected functions are:
setZeroValidatorPubkeyGIndexsetExecutionNumberGIndexgetParentBeaconBlockRootAtThese changes are consistent with good coding practices and do not introduce any functional modifications or security risks.
contracts/test/BeaconDepositContract.t.sol (2)
63-65: Approved: Improved function signature formattingThe consolidation of the function signature for
testFuzz_DepositWrongCredentialsinto a single line improves readability and aligns with common Solidity style guidelines.
120-122: Approved: Improved function signature formatting with a minor suggestionThe consolidation of the function signature for
testFuzz_DepositNativeWrongMinAmountinto a single line improves readability and aligns with common Solidity style guidelines.Consider using the
uint256type aliasetherinstead of multiplying by 1 gwei in the function body. This would make the code more idiomatic and potentially easier to read. Here's a suggested change:- uint256 amountInGwei = amountInEther * 1 gwei; + uint256 amountInGwei = amountInEther * 1 ether;This change maintains the same functionality while using a more standard Solidity convention.
Likely invalid or redundant comment.
|
Other changes:
|
|
@itsdevbear Please check again sir! |
|
Subsumed by c640dfd |
I reopened the PR to fix the data race (#1472) with
failedBlocksbut I switched to usingsync.Mapto protect a map.Summary by CodeRabbit
Refactor
Bug Fixes
Style