fix(deposit): synchronized failedBlocks between multiple goroutines #1472
Conversation
WalkthroughThe update transitions the Changes
Sequence Diagram(s)Old Flow with TickersequenceDiagram
participant Ticker
participant Service
participant Fetcher
Ticker-->>Service: Trigger retry every interval
Service->>Fetcher: Process failed blocks
Fetcher-->>Service: Failed block processing results
New Flow with ChannelsequenceDiagram
participant Service
participant Channel as FailedBlocks(Channel)
participant Fetcher as DepositFetcher
Service-)Channel: Push failed block
Channel->>Fetcher: Retrieve failed block
Fetcher-->>Service: Process and retry failed block
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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 (2 hunks)
- mod/execution/pkg/deposit/sync.go (4 hunks)
Additional comments not posted (3)
mod/execution/pkg/deposit/sync.go (2)
64-70: The changes to use a channel (failedBlocks) for managing retries of failed blocks look well-integrated. Ensure that the channel is always properly drained and closed to avoid goroutine leaks.
83-83: The retry logic by re-enqueuing the block number into thefailedBlockschannel upon failure is clear and straightforward. However, ensure there are safeguards against infinite retry loops which could occur if the same block consistently fails.Consider implementing a maximum retry limit or a back-off mechanism to handle persistent failures gracefully.
Also applies to: 96-96
mod/execution/pkg/deposit/service.go (1)
63-65: The initialization offailedBlocksas a channel with a buffer size of 100 in theServicestruct andNewServicefunction is appropriate for handling retry operations asynchronously. Ensure that the buffer size aligns with the expected volume of failed blocks to prevent blocking on channel operations.Also applies to: 108-108
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- mod/execution/pkg/deposit/service.go (3 hunks)
Additional comments not posted (2)
mod/execution/pkg/deposit/service.go (2)
66-68: SwitchingfailedBlocksfrom a map to a channel is a significant change to ensure thread-safe operations across multiple goroutines. This change should help in managing failed blocks more effectively by leveraging Go's built-in concurrency features like channels.
111-111: InitializingfailedBlockswith a buffer size ofdefaultChannelCap(100) is a good practice as it allows the channel to hold multiple items before blocking, facilitating smoother asynchronous processing.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- mod/execution/pkg/deposit/service.go (3 hunks)
Additional comments not posted (2)
mod/execution/pkg/deposit/service.go (2)
Line range hint
123-125: Starting of goroutines inStartmethod looks good.Ensure that the
failedBlockschannel is properly integrated and utilized within the goroutines to handle retries effectively.Verification successful
The initial attempt to verify the integration of the
failedBlockschannel within thedepositFetcheranddepositCatchupFetchergoroutines failed due to a directory not found error. This indicates that either the directory path was incorrect or the files are located elsewhere.To proceed, we need to refine our search to locate the correct directory and files where these goroutines are implemented. Let's use
fdto find the relevant files and then search within them for the usage offailedBlocks.
The recent script execution revealed that the
failedBlockschannel is indeed used within the codebase, specifically inmod/execution/pkg/deposit/sync.goandmod/execution/pkg/deposit/service.go.Here are the relevant lines:
mod/execution/pkg/deposit/sync.go
case blockNum := <-s.failedBlocks:s.failedBlocks <- blockNums.failedBlocks <- blockNummod/execution/pkg/deposit/service.go
// failedBlocks is a channel of blocks that failed to be processed to befailedBlocks chan math.U64failedBlocks: make(chan math.U64, defaultChannelCap)This confirms that the
failedBlockschannel is properly integrated and utilized within the relevant goroutines for handling retries.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper integration of `failedBlocks` channel in goroutines. # Expectation: `failedBlocks` channel should be actively used without errors. # Test: Review the implementation of `depositFetcher` and `depositCatchupFetcher` for channel usage. rg --type go "failedBlocks" src/mod/execution/pkg/deposit/Length of output: 129
Script:
#!/bin/bash # Description: Locate files related to depositFetcher and depositCatchupFetcher implementations and verify the usage of failedBlocks channel. # Step 1: Locate files potentially containing depositFetcher and depositCatchupFetcher implementations. fd -e go "deposit" src/ # Step 2: Search for the usage of failedBlocks within the located files. fd -e go "deposit" src/ | xargs rg "failedBlocks"Length of output: 727
68-68: Change from map to channel forfailedBlocksis approved.Please verify if the buffer size of 100 is adequate based on expected traffic and retry rates.
| @@ -105,7 +108,7 @@ func NewService[ | |||
| metrics: newMetrics(telemetrySink), | |||
| dc: dc, | |||
| ds: ds, | |||
| failedBlocks: make(map[math.Slot]struct{}), | |||
| failedBlocks: make(chan math.U64, defaultChannelCap), | |||
There was a problem hiding this comment.
Initialization of failedBlocks with defaultChannelCap is well-implemented.
Consider adding documentation on how the buffer size was determined to be 100, to aid future maintainability.
|
hi @htiennv I agree with this change, but this piece of the code base needs a broader re-architecture overall your logic here (much like our not so great existing solution) will result in infinite number of go-routines being spun up |
itsdevbear
force-pushed
the
main
branch
3 times, most recently
from
99689e6
to
d77cef4
Compare
thanks for your response, i'll close this PR |
Summary by CodeRabbit