Skip to content

Conversation

@d4mr
Copy link
Contributor

@d4mr d4mr commented Sep 27, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Prevents over-inflated optimistic transaction counts by capping them to the next expected value, improving accuracy and stability when sending transactions.
  • Chores

    • Adds a warning log when no inflight budget is available, improving operational visibility.
    • Minor log formatting improvements for better readability.

@coderabbitai
Copy link

coderabbitai bot commented Sep 27, 2025

Walkthrough

Extends validation data for CleanAndGetRecycledNonces to include an optional current_optimistic_tx_count from Redis, updates call sites to destructure the new tuple, and conditionally adjusts the optimistic transaction counter during apply. Adds a warning log when inflight budget is unavailable in send_flow. Minor logging-format changes.

Changes

Cohort / File(s) Change Summary
EOA submitted store: validation tuple and optimistic count handling
executors/src/eoa/store/submitted.rs
ValidationData expanded from (u64, Vec) to (u64, Vec, Option<u64)); validation now fetches and returns current_optimistic_tx_count; apply path conditionally reduces optimistic count to highest_submitted_nonce + 1 when applicable; updated destructuring; minor multiline logging formatting.
EOA worker send: inflight budget logging
executors/src/eoa/worker/send.rs
Adds warning log when no inflight budget is available in send_flow; no other behavior changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant SubmittedStore
  participant Redis

  rect rgba(230,240,255,0.5)
  note over Caller,SubmittedStore: Validation phase (CleanAndGetRecycledNonces)
  Caller->>SubmittedStore: validate()
  SubmittedStore->>Redis: GET highest_submitted_nonce, recycled_nonces, current_optimistic_tx_count
  Redis-->>SubmittedStore: (u64, Vec<u64>, Option<u64>)
  SubmittedStore-->>Caller: ValidationData(h, recycled, opt_count)
  end

  rect rgba(235,255,235,0.5)
  note over Caller,SubmittedStore: Apply phase (update optimistic counter if needed)
  Caller->>SubmittedStore: apply(h, recycled, opt_count)
  alt opt_count is Some and h+1 < opt_count
    SubmittedStore->>Redis: SET optimistic_tx_count = h + 1
  else No update needed
    SubmittedStore-->>Caller: proceed without changing counter
  end
  SubmittedStore-->>Caller: done
  end
Loading
sequenceDiagram
  autonumber
  participant Worker
  participant Budget

  Worker->>Budget: check_inflight_budget()
  alt budget available
    Worker-->>Worker: proceed to send
  else no budget
    Worker-->>Worker: log warn "No inflight budget, not sending new transactions"
    Worker-->>Worker: skip send
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly references the primary change of enhancing nonce-lowering logic by improving how optimistic transaction counts are handled and guarded, accurately summarizing the main update in the pull request.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch pb/nonce-lowering-fix

Comment @coderabbitai help to get the list of available commands and usage tips.

@d4mr d4mr merged commit a23171c into main Sep 27, 2025
2 of 3 checks passed
@d4mr d4mr deleted the pb/nonce-lowering-fix branch September 27, 2025 04:20
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
executors/src/eoa/store/submitted.rs (1)

649-656: Harden the optimistic count lowering against overflow

highest_submitted_nonce + 1 will panic in debug builds if we ever reach u64::MAX. It’s astronomically unlikely on Ethereum today, but the saturating form is a zero-cost guard and avoids the panic should we ever roll the counter that far.

-        if let Some(current_optimistic_tx_count) = current_optimistic_tx_count {
-            // if the current optimistic tx count is floating too high, we need to bring it down
-            if highest_submitted_nonce + 1 < current_optimistic_tx_count {
-                pipeline.set(
-                    self.keys.optimistic_transaction_count_key_name(),
-                    highest_submitted_nonce + 1,
-                );
+        if let Some(current_optimistic_tx_count) = current_optimistic_tx_count {
+            // if the current optimistic tx count is floating too high, we need to bring it down
+            let next_nonce = highest_submitted_nonce.saturating_add(1);
+            if next_nonce < current_optimistic_tx_count {
+                pipeline.set(
+                    self.keys.optimistic_transaction_count_key_name(),
+                    next_nonce,
+                );
             }
         }
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 6a41959 and 6744794.

📒 Files selected for processing (2)
  • executors/src/eoa/store/submitted.rs (5 hunks)
  • executors/src/eoa/worker/send.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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.rs
🧬 Code graph analysis (1)
executors/src/eoa/store/submitted.rs (3)
executors/src/eoa/store/borrowed.rs (1)
  • operation (99-262)
executors/src/eoa/store/atomic.rs (4)
  • operation (39-43)
  • operation (708-735)
  • pipeline (244-245)
  • pipeline (362-363)
executors/src/eoa/store/pending.rs (2)
  • operation (121-149)
  • operation (239-262)
🔇 Additional comments (1)
executors/src/eoa/worker/send.rs (1)

69-73: Appreciate the explicit no-budget warning

The added warn! gives much-needed visibility into why the worker skips the new-tx path, which will help during incident triage. Good call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants