feat(rfq-relayer): add mutex on committable balance consumption #2994
Conversation
WalkthroughThe changes enhance concurrency control in the relayer service's balance management. A new mutex locking mechanism has been introduced in 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
PR Summary
The PR introduces a mutex lock on committable balance consumption to prevent race conditions in the RFQ relayer service.
- Added
balanceMtxtoRelayerstruct inservices/rfq/relayer/service/relayer.gofor synchronizing balance requests. - Integrated
balanceMtxintoQuoteRequestHandlerinservices/rfq/relayer/service/statushandler.goto ensure thread-safe balance operations. - Implemented mutex lock in
handleSeenmethod ofQuoteRequestHandlerinservices/rfq/relayer/service/handlers.goto prevent concurrent balance consumption. - Introduced
getBalanceMtxKeyhelper function inservices/rfq/relayer/service/statushandler.gofor generating mutex keys based on chainID and token address.
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Deploying sanguine-fe with
|
| Latest commit: |
2d98795
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7647ae11.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-chain-mux.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- services/rfq/relayer/service/handlers.go (1 hunks)
- services/rfq/relayer/service/relayer.go (2 hunks)
- services/rfq/relayer/service/statushandler.go (2 hunks)
Additional comments not posted (6)
services/rfq/relayer/service/statushandler.go (3)
55-56: Concurrency control enhancement withbalanceMtx.The addition of
balanceMtxto theQuoteRequestHandlerstruct enhances concurrency control for balance management.
90-90: Initialization ofbalanceMtxinrequestToHandler.The initialization of
balanceMtxinQuoteRequestHandlerensures proper concurrency control setup.Verify consistency of
balanceMtxinitialization.Verification successful
Initialization of
balanceMtxis consistent.The
balanceMtxis initialized consistently usingmapmutex.NewStringMapMutex(), ensuring proper concurrency control across the codebase.
- Location:
services/rfq/relayer/service/relayer.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent initialization of `balanceMtx` in `QuoteRequestHandler`. # Test: Search for `balanceMtx` initialization. Expect: Consistent initialization across the codebase. rg --type go 'balanceMtx\s*:\s*mapmutex.NewStringMapMutex'Length of output: 148
59-60: Correct implementation ofgetBalanceMtxKey.The function correctly generates a unique key for balance mutexes using
chainIDandtoken.Ensure consistent usage of this function across the codebase.
Verification successful
Limited usage of
getBalanceMtxKeyfunction.The
getBalanceMtxKeyfunction is used inservices/rfq/relayer/service/handlers.go. Ensure that this usage aligns with its intended purpose of generating unique keys for balance mutexes. If this is the only intended use, then the implementation is consistent. If more widespread usage is expected, consider reviewing other parts of the codebase.
- Location of usage:
services/rfq/relayer/service/handlers.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of `getBalanceMtxKey` function across the codebase. # Test: Search for the function usage. Expect: Consistent usage across the codebase. rg --type go 'getBalanceMtxKey'Length of output: 259
services/rfq/relayer/service/relayer.go (2)
66-67: Concurrency control enhancement withbalanceMtx.The addition of
balanceMtxto theRelayerstruct enhances concurrency control for balance management.
170-170: Initialization ofbalanceMtxinNewRelayer.The initialization of
balanceMtxinRelayerensures proper concurrency control setup.Verify consistency of
balanceMtxinitialization.Verification successful
Initialization of
balanceMtxis consistent.The
balanceMtxis consistently initialized usingmapmutex.NewStringMapMutex()in therelayer.gofile, ensuring proper concurrency control setup. No conflicting initializations were found elsewhere in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent initialization of `balanceMtx` in `Relayer`. # Test: Search for `balanceMtx` initialization. Expect: Consistent initialization across the codebase. rg --type go 'balanceMtx\s*:\s*mapmutex.NewStringMapMutex'Length of output: 148
services/rfq/relayer/service/handlers.go (1)
165-175: Mutex lock implementation inhandleSeen.The mutex lock implementation effectively manages concurrent access to balance data, preventing race conditions.
Ensure proper usage and handling of the lock across the codebase.
Verification successful
Let's search for the definition of the
handleSeenfunction and its implementation to verify the usage of mutex locks.
Let's search for all function definitions in the
services/rfq/relayer/service/handlers.gofile to identify any potential candidates related to the mutex lock implementation.
Let's examine the
handleSeenfunction for the usage ofTryLockandUnlockto verify proper handling of mutex locks.
Mutex lock implementation in
handleSeenis correct.The
handleSeenfunction inservices/rfq/relayer/service/handlers.gouses a mutex lock to manage concurrent access to balance data effectively. TheTryLockmethod is used to attempt acquiring the lock, andUnlockis deferred to ensure the lock is released properly. This implementation prevents race conditions during concurrent requests.
- File:
services/rfq/relayer/service/handlers.go- Function:
handleSeen(starting at line 138)The implementation appears to be correct and handles concurrent access appropriately. No further issues were found regarding mutex lock usage in this function.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper usage and handling of mutex locks in `handleSeen`. # Test: Search for mutex lock usage in `handleSeen`. Expect: Proper usage and handling across the codebase. rg --type go 'handleSeen' -A 10 | rg 'TryLock|Unlock'Length of output: 53
Script:
#!/bin/bash # Search for the definition and implementation of the `handleSeen` function in Go files. ast-grep --lang go --pattern 'func handleSeen($$$) $$$'Length of output: 57
Script:
#!/bin/bash # List all function definitions in the `services/rfq/relayer/service/handlers.go` file. fd 'handlers.go' --exec gawk '/^func / {print FILENAME ":" FNR ": " $0}' {}Length of output: 2699
Script:
#!/bin/bash # Extract the implementation of the `handleSeen` function to verify mutex lock usage. ast-grep --lang go --pattern 'func (q *QuoteRequestHandler) handleSeen($_, $_, $_) ($_) { $$$ }'Length of output: 7915
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2994 +/- ##
===================================================
- Coverage 25.30840% 25.29907% -0.00933%
===================================================
Files 780 780
Lines 56906 56927 +21
Branches 82 82
===================================================
Hits 14402 14402
- Misses 41022 41043 +21
Partials 1482 1482
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
PR Summary
(updates since last review)
No major changes found since the last review.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/service/handlers.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/service/handlers.go
| span.SetAttributes(attribute.Bool("locked", true)) | ||
| return nil | ||
| } | ||
| defer unlocker.Unlock() |
There was a problem hiding this comment.
In the case of forwarding, won't the lock be released only after the full cycle goes through? We could safely unlock once we update status to CommittedPending (aka spend the destination commitable balance)
There was a problem hiding this comment.
@dwasse by "full cycle" I mean the full status path Seen -> CommitPending -> CommittedConfirmed. I realised that it's not actually full, but still if that's the case we're keeping the lock longer than we should've
There was a problem hiding this comment.
Yeah that makes sense, would still like to use defer to make this error resistant but can just add a helper function here
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR introduces a new locking mechanism for balance management to enhance concurrency control in the relayer service.
- Added
commitPendingBalancefunction inservices/rfq/relayer/service/handlers.gofor thread-safe balance operations. - Modified
handleSeenfunction inservices/rfq/relayer/service/handlers.goto incorporate the new mutex. - Enhanced documentation for mutex usage in the relayer struct for clarity.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/service/handlers.go (3 hunks)
Additional comments not posted (3)
services/rfq/relayer/service/handlers.go (3)
165-179: Verify concurrency control and forwarding logic.The
handleSeenfunction now includes a call tocommitPendingBalance. Ensure that the concurrency control and forwarding logic tohandleCommitPendingis robust and does not introduce race conditions.
Line range hint
182-213:
Verify error handling and synchronization with relayers.The
commitPendingBalancefunction introduces mutex locking and status updates. Ensure that error handling for unsupported chains and inventory checks is robust. Also, verify synchronization with other relayers to avoid reverts.Verification successful
Error Handling and Synchronization Verified
The
commitPendingBalancefunction effectively manages error handling and synchronization. It updates request statuses appropriately for unsupported chains and insufficient inventory. Synchronization with other relayers is achieved through an API acknowledgment mechanism, ensuring transactions are processed correctly. No issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling and synchronization in `commitPendingBalance`. # Test: Search for error handling and synchronization logic. rg --type go -A 10 'func (q \*QuoteRequestHandler) commitPendingBalance'Length of output: 74
Script:
#!/bin/bash # Using ast-grep to locate the `commitPendingBalance` function definition and its context. ast-grep --lang go --pattern 'func (q *QuoteRequestHandler) commitPendingBalance($_, $_, $_) $_'Length of output: 5287
Line range hint
512-533:
Verify locking mechanism and status update logic.The
handleNotEnoughInventoryfunction now acquires a balance lock. Ensure the locking mechanism is correctly implemented and that the logic for updating the request status is accurate.Verification successful
Locking Mechanism and Status Update Logic Verified
The
handleNotEnoughInventoryfunction correctly implements the locking mechanism usingbalanceMtx.TryLockand updates the request status withUpdateQuoteRequestStatus. The relevant functions and methods are defined and used as expected in the codebase.
getBalanceMtxKeyis defined inservices/rfq/relayer/service/statushandler.go.TryLockandGetCommittableBalanceare used inservices/rfq/relayer/service/handlers.go.UpdateQuoteRequestStatusis defined inservices/rfq/relayer/reldb/base/quote.goand used inhandlers.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify locking mechanism and status update logic in `handleNotEnoughInventory`. # Test: Search for the function usage and ensure proper locking and status update. rg --type go -A 10 'func (q \*QuoteRequestHandler) handleNotEnoughInventory'Length of output: 78
Script:
#!/bin/bash # Search for the definition and usage of getBalanceMtxKey rg --type go 'func getBalanceMtxKey' # Search for the definition and usage of q.balanceMtx.TryLock rg --type go 'TryLock' | grep 'balanceMtx' # Search for the definition and usage of q.Inventory.GetCommittableBalance rg --type go 'GetCommittableBalance' | grep 'Inventory' # Search for the definition and usage of q.db.UpdateQuoteRequestStatus rg --type go 'UpdateQuoteRequestStatus' | grep 'db'Length of output: 3597
* Revert "revert(relayer): potential deadlock (#3036)" to reinstate #2994 Co-authored-by: Daniel Wasserman <[email protected]>
Summary by CodeRabbit
New Features
Improvements
QuoteRequestHandlerwith a mutex for thread-safe balance operations.