Refactor optimistic nonce handling in AtomicEoaExecutorStore #35
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Updated the logic for retrieving the optimistic transaction count to handle cases where synchronization is required, returning `None` when necessary. - Enhanced logging to differentiate between updating the optimistic nonce and initializing it for new EOAs, improving clarity in transaction processing. These changes improve the robustness of the optimistic nonce management and provide better insights during execution.
WalkthroughThis change refines the logic for retrieving and updating the optimistic nonce in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (1)
executors/src/eoa/store/atomic.rs (1)
438-446: Enrich log context for easier tracingThe new “Initializing optimistic nonce for new EOA” message is helpful but lacks the EOA address and chain-id that appear in other logs in this file.
Including them keeps log lines greppable and consistent:tracing::info!( eoa = ?self.eoa(), chain_id = self.chain_id(), current_chain_tx_count, "Initializing optimistic nonce for new EOA" );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
executors/src/eoa/store/atomic.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: The EOA executor store uses comprehensive WATCH-based coordination where every Redis state mutation watches the lock key and validates ownership before proceeding. If the lock is lost during any operation, the transaction fails with LockLost error. This makes aggressive lock acquisition safe because only the actual lock holder can successfully perform state mutations, regardless of who claims the lock.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
📚 Learning: 2025-07-06T15:44:13.701Z
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: The EOA executor store uses comprehensive WATCH-based coordination where every Redis state mutation watches the lock key and validates ownership before proceeding. If the lock is lost during any operation, the transaction fails with LockLost error. This makes aggressive lock acquisition safe because only the actual lock holder can successfully perform state mutations, regardless of who claims the lock.
Applied to files:
executors/src/eoa/store/atomic.rs
📚 Learning: 2025-07-06T15:44:13.701Z
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Applied to files:
executors/src/eoa/store/atomic.rs
🔇 Additional comments (1)
executors/src/eoa/store/atomic.rs (1)
404-408: TreatingNonceSyncRequiredas “un-initialised” may mask real desyncsMapping
NonceSyncRequired→Nonelets the store silently re-initialise the optimistic nonce.
If that error is also emitted for genuine out-of-sync situations (e.g. chain re-org or lost writes), the corrective signal is now lost and the bug is hidden instead of surfaced.Please double-check that
NonceSyncRequiredis only produced for the “no value yet” case.
If it is re-used elsewhere, a more explicit enum variant (e.g.OptimisticNonceMissing) or a boolean flag would avoid ambiguity.
Comment on lines
+425
to
+436
| if let Some(optimistic_nonce) = optimistic_nonce { | ||
| if current_chain_tx_count > optimistic_nonce { | ||
| tracing::warn!( | ||
| current_chain_tx_count = current_chain_tx_count, | ||
| optimistic_nonce = optimistic_nonce, | ||
| "Optimistic nonce was behind fresh chain transaction count, updating to match" | ||
| ); | ||
| pipeline.set( | ||
| self.optimistic_transaction_count_key_name(), | ||
| current_chain_tx_count, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing branch when chain nonce < optimistic nonce
You update the optimistic nonce when current_chain_tx_count > optimistic_nonce but leave the store unchanged if the chain nonce is behind.
A chain that has been reset or re-orged would leave the optimistic nonce ahead, breaking future submissions.
Consider adding a safeguard:
- if current_chain_tx_count > optimistic_nonce {
+ if current_chain_tx_count > optimistic_nonce {
/* … */
+ } else if current_chain_tx_count < optimistic_nonce {
+ tracing::warn!(
+ %current_chain_tx_count,
+ %optimistic_nonce,
+ "Chain nonce is behind optimistic nonce – forcing downward sync"
+ );
+ pipeline.set(self.optimistic_transaction_count_key_name(), current_chain_tx_count);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Some(optimistic_nonce) = optimistic_nonce { | |
| if current_chain_tx_count > optimistic_nonce { | |
| tracing::warn!( | |
| current_chain_tx_count = current_chain_tx_count, | |
| optimistic_nonce = optimistic_nonce, | |
| "Optimistic nonce was behind fresh chain transaction count, updating to match" | |
| ); | |
| pipeline.set( | |
| self.optimistic_transaction_count_key_name(), | |
| current_chain_tx_count, | |
| ); | |
| } | |
| if let Some(optimistic_nonce) = optimistic_nonce { | |
| if current_chain_tx_count > optimistic_nonce { | |
| tracing::warn!( | |
| current_chain_tx_count = current_chain_tx_count, | |
| optimistic_nonce = optimistic_nonce, | |
| "Optimistic nonce was behind fresh chain transaction count, updating to match" | |
| ); | |
| pipeline.set( | |
| self.optimistic_transaction_count_key_name(), | |
| current_chain_tx_count, | |
| ); | |
| } else if current_chain_tx_count < optimistic_nonce { | |
| tracing::warn!( | |
| %current_chain_tx_count, | |
| %optimistic_nonce, | |
| "Chain nonce is behind optimistic nonce – forcing downward sync" | |
| ); | |
| pipeline.set( | |
| self.optimistic_transaction_count_key_name(), | |
| current_chain_tx_count, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In executors/src/eoa/store/atomic.rs around lines 425 to 436, the code updates
the optimistic nonce only when the current chain transaction count is greater
than the optimistic nonce, but does not handle the case when the chain nonce is
less than the optimistic nonce. To fix this, add a branch to detect when the
chain nonce is behind the optimistic nonce and reset or update the optimistic
nonce accordingly to prevent future submission errors after chain resets or
re-orgs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Nonewhen necessary.These changes improve the robustness of the optimistic nonce management and provide better insights during execution.
Summary by CodeRabbit