-
Notifications
You must be signed in to change notification settings - Fork 5
Refactor transaction count update logic in EoaExecutorWorker #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Moved the cached transaction count update logic to ensure it always executes, regardless of the presence of transactions to confirm. - Added error logging for cases where the latest fetched transaction count is lower than the cached count, indicating potential re-orgs or RPC block lag. - This change enhances the robustness of transaction management and improves error handling in the EoaExecutorWorker.
WalkthroughThe confirm worker now updates the cached on-chain transaction count at the start of the confirmation flow, before collecting waiting transactions, and removes the previous post-processing cache update. If the latest count is less than the cached value, it logs an error and still updates the cache. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant W as ConfirmWorker
participant C as Cache
participant R as Chain RPC
participant Q as PendingTxQueue
rect rgba(200,230,255,0.25)
note over W: New flow
W->>R: fetch latest tx count
R-->>W: latestCount
W->>C: read cachedCount
alt latestCount < cachedCount
W->>W: log error (reorg/RPC lag)
end
W->>C: update cachedCount = latestCount
W->>Q: gather waiting txs
W->>R: fetch receipts for waiting txs
R-->>W: receipts
W->>W: build confirmation report
end
sequenceDiagram
autonumber
participant W as ConfirmWorker
participant C as Cache
participant R as Chain RPC
participant Q as PendingTxQueue
rect rgba(255,240,200,0.25)
note over W: Old flow
W->>Q: gather waiting txs
W->>R: fetch receipts for waiting txs
R-->>W: receipts
W->>W: build confirmation report
W->>R: fetch latest tx count
R-->>W: latestCount
W->>C: read cachedCount
alt latestCount < cachedCount
W->>W: log error (reorg/RPC lag)
end
W->>C: update cachedCount = latestCount
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this 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 (2)
executors/src/eoa/worker/confirm.rs (2)
152-159
: Enrich error log context (EOA, chain_id) and tighten wording.Adding identifiers makes incidents actionable; small grammar fixes improve clarity.
- tracing::error!( - current_chain_transaction_count = transaction_counts.latest, - cached_transaction_count = cached_transaction_count, - "Fresh fetched chain transaction count is lower than cached transaction count. \ - This indicates a re-org or RPC block lag. Engine will use the newest fetched transaction count from now (assuming re-org).\ - Transactions already confirmed will not be attempted again, even if their nonce was higher than the new chain transaction count. - In case this is RPC misbehaviour not reflective of actual chain state, Engine's nonce management might be affected." - ); + tracing::error!( + eoa = ?self.eoa, + chain_id = self.chain_id, + current_chain_transaction_count = transaction_counts.latest, + cached_transaction_count = cached_transaction_count, + "Freshly fetched chain transaction count is lower than cached transaction count. \ + This indicates a re-org or RPC block lag. Engine will use the newly fetched transaction count from now (assuming re-org). \ + Transactions already confirmed will not be attempted again, even if their nonce was higher than the new chain transaction count. \ + If this reflects RPC misbehaviour (not actual chain state), Engine's nonce management might be affected." + );
162-165
: Add a post-update debug log for before/after values.Helps correlate incidents without re-reading store state.
self.store .update_cached_transaction_count(transaction_counts.latest) .await?; + tracing::debug!( + cached_transaction_count_before = cached_transaction_count, + cached_transaction_count_after = transaction_counts.latest, + "Updated cached transaction count" + );
📜 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 (1)
executors/src/eoa/worker/confirm.rs
(1 hunks)
🔇 Additional comments (2)
executors/src/eoa/worker/confirm.rs (2)
149-165
: Always-update cached tx count before scanning waiting txs — good change.This fixes the early-return stale-cache path and surfaces nonce regressions.
149-165
: No duplicate cached tx-count updates remain; only the initial sync and the intended update exist.Found calls at executors/src/eoa/worker/confirm.rs:64 (initial "Nonce sync required" branch) and executors/src/eoa/worker/confirm.rs:163 (current always-update block); function defined at executors/src/eoa/store/atomic.rs:410. No other occurrences detected.
Summary by CodeRabbit