More nonce improvements #625
Conversation
WalkthroughThe pull request introduces a new error handling case in the blockchain publisher to manage "nonce too high" errors with a backoff mechanism using a random sleep. It also implements an Changes
Sequence Diagram(s)sequenceDiagram
participant WithNonce as withNonce
participant Logger
participant Sleeper as RandomSleep
WithNonce->>Logger: Log "nonce too high, backing off"
WithNonce->>Sleeper: Call RandomSleep(500ms)
Note right of WithNonce: Delay execution before retrying
sequenceDiagram
participant Client
participant SQLManager as SQLBackedNonceManager
participant Limiter as OpenConnectionsLimiter
Client->>SQLManager: Request nonce (GetNonce)
SQLManager->>Limiter: Acquire semaphore & add to wait group
Note right of Limiter: Concurrency is limited as per configuration
Client->>SQLManager: Complete operation (Cancel/Consume)
Limiter-->>SQLManager: Release semaphore & mark wait group done
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (5)
pkg/blockchain/blockchainPublisher.go (3)
10-10: Consider random seeding.By default, the global random generator always produces the same sequence of pseudo-random values unless seeded. Confirm whether this is desired or if
rand.Seed(time.Now().UnixNano())should be called at an appropriate entry point.
231-237: Ensure cancellation handling.
randomSleepblocks until the sleep duration completes, which may conflict with context cancellations. Consider passing in the context to exit early if the operation should be canceled:-func randomSleep(maxDuration time.Duration) { +func randomSleep(ctx context.Context, maxDuration time.Duration) { ... + select { + case <-time.After(randDuration): + case <-ctx.Done(): + return + } }
278-292: Add metrics or logs for repeated backoffs.When “nonce too high” appears frequently, it can indicate a backlog or node-side issue. Logging or metering the frequency of this path can help detect systemic errors.
pkg/blockchain/nonceManager.go (2)
23-28: Clear concurrency manager approach.Adding optional metrics for the number of in-flight requests, average wait time, and maximum concurrency usage can provide helpful visibility into how the manager is performing.
91-92: Avoid dual decrement of wait group.Both
CancelandConsumecallwg.Done(). Consider a single finalizer approach if there’s any scenario where both might be invoked, to avoid doublewg.Done().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/blockchain/blockchainPublisher.go(3 hunks)pkg/blockchain/nonceManager.go(4 hunks)pkg/db/sequences_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (9)
pkg/db/sequences_test.go (2)
386-419: Solid concurrency validation.Concurrent deletions are well-handled using a wait group and atomic integer. The final assertion correctly confirms the total deletions.
421-465: Good test coverage with uncommitted transactions.Providing an open transaction while concurrently deleting ensures the system behaves correctly under partial locks. The final count of 199 demonstrates that exactly one nonce was withheld by the open transaction.
pkg/blockchain/nonceManager.go (7)
9-9: No issues with the import.
30-33: Meaningful concurrency limits.Defining these constants in a single location simplifies adjusting concurrency bounds and clarifies intent.
46-48: Initialization logic is consistent.The
SQLBackedNonceManageralways obtains aRequestManager, maintaining concurrency limits consistently.
106-106: Unambiguous function signature.Eliminating named returns enhances clarity and reduces confusion about return paths.
114-114: Consistent simplified signature.Matches the updated style in
fillNoncesfor more uniform code.
34-43:Details
❓ Verification inconclusive
Edge case check for zero or negative concurrency.
While the code caps values above
MaxConcurrentRequests, it doesn’t handle zero or negative concurrency. Verify if such input can happen:
🏁 Script executed:
#!/bin/bash rg "NewRequestManager("Length of output: 112
Action Required: Verify Zero/Negative Concurrency Usage in NewRequestManager
The current implementation only limits values above
MaxConcurrentRequestsand does not explicitly handle cases wheremaxConcurrentis zero or negative. This could result in unintended behavior (e.g., creating an unbuffered channel). Please verify whether any callers might pass zero or negative values.
- Verification Step:
Run the following command (with the escaped parenthesis) to locate all invocations ofNewRequestManagerin the codebase:#!/bin/bash rg "NewRequestManager\(" -n .- Based on the search results, confirm if any call sites could supply zero or negative values. If they can, consider adding explicit validation (e.g., returning an error or defaulting to a minimum valid value).
86-87:Details
❓ Verification inconclusive
Double-check symmetry in semaphore usage.
Confirm that each semaphore acquisition has exactly one corresponding release to avoid potential channel underflow or panic:
🏁 Script executed:
#!/bin/bash rg "semaphore <- struct{}"Length of output: 152
Below is an updated review comment. Since the original literal regex search produced an error, please run the following command manually to verify that every semaphore “send” is matched with a “receive” (ensuring symmetrical usage):
rg -F "semaphore <- struct{}" .Double-check symmetry in semaphore usage.
Please confirm that each semaphore acquisition (i.e. every place where an empty struct is sent into the semaphore channel) is paired with a corresponding release (i.e. reading from the channel such as on lines 86–87 in pkg/blockchain/nonceManager.go) to avoid potential channel underflow or panics.
When hammering our alchemy node in testnet, I noticed a bunch of issues that weren't seen in the local test development.
Otherwise HA payer works perfectly.
Closes #423
Summary by CodeRabbit
Summary by CodeRabbit