bring down optimistic nonce in case recycled transactions fail #52
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
WalkthroughExpanded Redis WATCH sets to include recycled nonces and an optimistic transaction count key; added a write to set that count after recycling. Switched a worker timestamp source to EoaExecutorStore::now(). Minor formatting updates. No public API changes except a new key accessor in the store keys. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant W as Worker
participant S as EoaExecutorStore
participant R as Redis
rect rgb(240,248,255)
note over S,R: ProcessBorrowedTransactions - expanded WATCH
W->>S: process_borrowed_transactions()
S->>R: WATCH borrowed_transactions_hashmap, recycled_nonces_zset
alt keys stable
S->>R: MULTI
S->>R: HGETALL borrowed_transactions_hashmap
S->>R: ZRANGE recycled_nonces_zset
R-->>S: results
S->>R: EXEC
R-->>S: OK
S-->>W: processed entries
else conflict
R-->>S: EXEC aborted (watched key changed)
S-->>W: retry/backoff
end
end
sequenceDiagram
autonumber
participant C as Cleaner
participant S as EoaExecutorStore
participant R as Redis
rect rgb(245,255,245)
note over S,R: CleanAndGetRecycledNonces - includes optimistic count update
C->>S: clean_and_get_recycled_nonces()
S->>R: WATCH recycled_nonces_zset, submitted_nonces_..., optimistic_tx_count_key
alt keys stable
S->>R: MULTI
S->>R: ZREMRANGEBYSCORE recycled_nonces_zset (prune)
S->>R: HGET submitted_nonces_... (highest_submitted_nonce)
S->>R: SET optimistic_tx_count_key highest_submitted_nonce + 1
S->>R: EXEC
R-->>S: OK
S-->>C: pruned set, updated count
else conflict
R-->>S: EXEC aborted
S-->>C: retry/backoff
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
joaquim-verges
approved these changes
Sep 23, 2025
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
executors/src/eoa/worker/send.rs (1)
21-27: Prevent underflow in balance staleness checkIf
health.balance_fetched_atis ahead ofnow(clock skew across workers, persisted future timestamp),now - fetched_atunderflows on u64 (debug panic / release wrap), causing pathological refresh behavior. Usesaturating_sub.Apply:
- let now = EoaExecutorStore::now(); + let now = EoaExecutorStore::now(); // Update balance if it's stale // TODO: refactor this, very ugly if health.balance <= health.balance_threshold { - if now - health.balance_fetched_at > HEALTH_CHECK_INTERVAL_MS { + if now.saturating_sub(health.balance_fetched_at) > HEALTH_CHECK_INTERVAL_MS {
🧹 Nitpick comments (5)
executors/src/eoa/worker/send.rs (1)
229-236: Rename typo:clean_prepration_results→clean_preparation_resultsSpelling nit leaks into call sites; fix for clarity/searchability.
Apply:
- async fn clean_prepration_results( + async fn clean_preparation_results(And update callers:
- let cleaned_results = self - .clean_prepration_results(prepared_results_with_pending, false) + let cleaned_results = self + .clean_preparation_results(prepared_results_with_pending, false) .await?;- let cleaned_results = self - .clean_prepration_results(prepared_results_with_pending, true) + let cleaned_results = self + .clean_preparation_results(prepared_results_with_pending, true) .await?;Also applies to: 124-126, 347-349
executors/src/eoa/store/borrowed.rs (2)
106-106: Unify time sourceUse the centralized clock helper for consistency and testability.
Apply:
- let now = chrono::Utc::now().timestamp_millis().max(0) as u64; + let now = EoaExecutorStore::now();
77-87: Avoid O(n·m) membership checks: useHashSet
borrowed_transaction_ids.contains(..)on aVecis linear per lookup. Convert toHashSet<String>to make validation O(n).Apply:
+use std::collections::HashSet; @@ - let borrowed_transaction_ids: Vec<String> = conn + let borrowed_transaction_ids: HashSet<String> = conn .hkeys(self.keys.borrowed_transactions_hashmap_name()) .await?; @@ - if borrowed_transaction_ids.contains(&transaction_id.to_string()) { + if borrowed_transaction_ids.contains(transaction_id) { valid_results.push(result.clone()); } else {executors/src/eoa/store/submitted.rs (2)
622-626: Guard against optimistic count regressionSetting the optimistic count to
highest_submitted_nonce + 1can move it backwards if another path already advanced it. Persist the max of the current value and the computed value.Minimal change using validation to read current value:
-impl SafeRedisTransaction for CleanAndGetRecycledNonces<'_> { - type ValidationData = (u64, Vec<u64>); +impl SafeRedisTransaction for CleanAndGetRecycledNonces<'_> { + type ValidationData = (u64, Vec<u64>, Option<u64>); @@ async fn validation( @@ - // get the highest submitted nonce + // get the highest submitted nonce let highest_submitted: Vec<SubmittedTransactionStringWithNonce> = conn .zrange_withscores(self.keys.submitted_transactions_zset_name(), -1, -1) .await?; @@ - Ok((highest_submitted_nonce, recycled_nonces)) + // read current optimistic count to ensure monotonicity + let current_opt: Option<u64> = conn + .get(self.keys.optimistic_transaction_count_key_name()) + .await?; + Ok((highest_submitted_nonce, recycled_nonces, current_opt)) @@ - fn operation( + fn operation( &self, pipeline: &mut Pipeline, - (highest_submitted_nonce, recycled_nonces): Self::ValidationData, + (highest_submitted_nonce, recycled_nonces, current_opt): Self::ValidationData, ) -> Self::OperationResult { @@ - pipeline.set( - self.keys.optimistic_transaction_count_key_name(), - highest_submitted_nonce + 1, - ); + let next = (highest_submitted_nonce + 1).max(current_opt.unwrap_or(0)); + pipeline.set(self.keys.optimistic_transaction_count_key_name(), next);
308-309: Unify time sourcePrefer the shared helper for timestamps here too.
Apply:
- let now = chrono::Utc::now().timestamp_millis().max(0) as u64; + let now = EoaExecutorStore::now();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
executors/src/eoa/store/borrowed.rs(4 hunks)executors/src/eoa/store/submitted.rs(2 hunks)executors/src/eoa/worker/send.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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-09-20T05:30:35.171Z
Learnt from: joaquim-verges
PR: thirdweb-dev/engine-core#48
File: executors/src/eoa/worker/send.rs:20-21
Timestamp: 2025-09-20T05:30:35.171Z
Learning: In executors/src/eoa/worker/send.rs, there is a critical bug where HEALTH_CHECK_INTERVAL is defined as 300 seconds but compared against millisecond timestamps, causing balance checks to occur every 300ms instead of every 5 minutes (1000x more frequent than intended).
Applied to files:
executors/src/eoa/worker/send.rsexecutors/src/eoa/store/borrowed.rs
📚 Learning: 2025-09-20T06:58:40.230Z
Learnt from: d4mr
PR: thirdweb-dev/engine-core#48
File: executors/src/eoa/store/submitted.rs:229-230
Timestamp: 2025-09-20T06:58:40.230Z
Learning: The diff in executors/src/eoa/store/submitted.rs shows correct brace structure - the first closing brace closes the remove_transaction_from_redis_submitted_zset method and the second closing brace closes the impl CleanSubmittedTransactions block. The change only adds whitespace formatting.
Applied to files:
executors/src/eoa/store/submitted.rsexecutors/src/eoa/store/borrowed.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: 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/submitted.rs
🧬 Code graph analysis (3)
executors/src/eoa/worker/send.rs (2)
executors/src/eoa/worker/error.rs (2)
is_retryable_preparation_error(250-281)should_update_balance_threshold(215-235)executors/src/eoa/store/mod.rs (1)
now(805-807)
executors/src/eoa/store/submitted.rs (1)
executors/src/eoa/store/mod.rs (1)
optimistic_transaction_count_key_name(235-246)
executors/src/eoa/store/borrowed.rs (2)
executors/src/metrics.rs (2)
calculate_duration_seconds(225-227)current_timestamp_ms(236-238)executors/src/eoa/store/mod.rs (2)
borrowed_transactions_hashmap_name(196-204)recycled_nonces_zset_name(213-224)
🔇 Additional comments (5)
executors/src/eoa/worker/send.rs (2)
8-11: Imports reorg +SendContextin scope — OKNo issues spotted with the import reshuffle; symbols remain correct.
317-336: Confirm optimistic nonce is only ever advanced — review findings and action needed
- Verified: Moves from pending→borrowed with incremented nonces are validated against the current optimistic nonce and set optimistic = highest_nonce + 1 (MovePendingToBorrowedWithIncrementedNonces) — monotonic increase guaranteed by validation.
- Verified: update_cached_transaction_count only sets optimistic to current_chain_tx_count when current_chain_tx_count > optimistic or when optimistic is uninitialized — monotonic/non-decreasing.
- Action required: submitted.rs unconditionally sets optimistic = highest_submitted_nonce + 1 in its operation. I couldn't confirm from the current snippet whether highest_submitted_nonce is always >= the current optimistic value; please confirm that highest_submitted_nonce is computed as the max of submitted nonces and the current optimistic (or otherwise ensure the pipeline only sets optimistic if new_value > current). If it can be lower, change to a conditional set (pipeline.set only when highest_submitted_nonce + 1 > current optimistic) or compute new_optimistic = max(current_optimistic, highest_submitted_nonce + 1) before setting.
Location to check: executors/src/eoa/store/submitted.rs — the operation block that calls pipeline.set(self.keys.optimistic_transaction_count_key_name(), highest_submitted_nonce + 1).
executors/src/eoa/store/borrowed.rs (2)
64-69: Watch recycled nonces alongside borrowed — good expansionIncluding
recycled_nonces_zsetinwatch_keys()matches the new recycling semantics.Please confirm the lock key is also WATCHed by the transaction wrapper (per prior design), and that mutating
pending_transactions_zsetin this op doesn’t require adding it towatch_keys()for your invariants.
194-196: Marking nonces for recycle on Nack/Fail — OKCorrectly reintroduces nonces that weren’t consumed.
Also applies to: 229-231
executors/src/eoa/store/submitted.rs (1)
559-566: Watching optimistic count with recycled/submitted — OKThe expanded watch set aligns with the optimistic-nonce maintenance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by CodeRabbit
Bug Fixes
Chores