Fix duplicate submitted -> pending in case of gas bumped transactions #53
Conversation
WalkthroughIntroduces per-transaction_id deduplication in the processing loop, moves Redis cleanup into a post-match block with scenario-based rules, updates webhook queuing messages and logic for confirmed/replaced paths, adjusts pending count increments, and refines diagnostics around flashblock propagation. No exported/public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant P as SubmittedProcessor
participant R as Redis (hashes)
participant C as Chain/Flashblock
participant W as WebhookQueue
Note over P: Per-transaction_id dedup (processed_ids)
P->>P: Receive submitted tx event
alt first occurrence
P->>C: Evaluate status (confirmed/replaced/pending)
opt Confirmed
P->>W: Enqueue "confirmed" webhook
rect rgba(200,255,200,0.3)
Note right of P: Post-match cleanup (Scenario 1)
P->>R: Remove all hashes for transaction_id
end
end
opt Replaced
P->>W: Enqueue "replaced" webhook
rect rgba(200,200,255,0.3)
Note right of P: Increment pending counts only when needed
end
rect rgba(240,240,200,0.5)
Note right of P: Post-match cleanup (Scenario 2)
P->>C: Get latest confirmed nonce
alt nonce <= latest confirmed
P->>R: Remove hashes
else
P->>R: Keep submitted hashes
end
end
end
opt NOOP
P->>W: Enqueue NOOP-related message (if any)
rect rgba(255,240,200,0.4)
Note right of P: Always remove hashes for NOOP
P->>R: Remove hashes
end
end
Note over P,C: Diagnostics note for flashblock propagation delays
else duplicate
P-->>P: Skip business logic (already processed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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
🧹 Nitpick comments (5)
executors/src/eoa/store/submitted.rs (5)
342-346: Good per-transaction_id dedup; prefer owning Strings to avoid borrow-coupling.Using HashSet<&str> ties lifetimes to the hydrated slice and can become brittle with future refactors. Owning IDs here is cheap and safer.
Apply:
- let mut processed_ids = HashSet::new(); + let mut processed_ids: HashSet<String> = HashSet::new(); ... - let is_first_occurrence = processed_ids.insert(tx.transaction_id()); + let is_first_occurrence = processed_ids.insert(tx.transaction_id().to_string());
392-392: Add structured fields to the error log.Include transaction_id/chain_id for debuggability; avoid losing context.
- tracing::error!("Failed to queue webhook for confirmed transaction: {}", e); + tracing::error!( + transaction_id = %event.transaction_id, + chain_id = self.keys.chain_id, + error = %e, + "Failed to queue webhook for confirmed transaction" + );
428-428: Add structured context to the replaced-webhook error log.Same rationale as confirmed-path.
- tracing::error!("Failed to queue webhook for replaced transaction: {}", e); + tracing::error!( + transaction_id = %event.transaction_id, + chain_id = self.keys.chain_id, + error = %e, + "Failed to queue webhook for replaced transaction" + );
452-471: Ensure cleanup prevents dual-state presence after moving to pending.Augment the cleanup to always remove submitted hashes for ids moved to pending.
- match (tx.transaction_id(), confirmed_ids.get(tx.transaction_id())) { + match (tx.transaction_id(), confirmed_ids.get(tx.transaction_id())) { // SCENARIO 1: Transaction ID was confirmed - remove ALL hashes (even if beyond latest due to flashblocks) (_, Some(_)) => { self.remove_transaction_from_redis_submitted_zset(pipeline, tx); } // NOOP transactions: always clean up (NO_OP_TRANSACTION_ID, _) => { self.remove_transaction_from_redis_submitted_zset(pipeline, tx); } // SCENARIO 2: Transaction ID was NOT confirmed - only clean up if within latest confirmed nonce range _ => { - if let SubmittedTransactionHydrated::Real(_) = tx { - if tx_nonce <= self.transaction_counts.latest { + if let SubmittedTransactionHydrated::Real(_) = tx { + // If this id was moved to pending, remove all its submitted hashes to avoid dual-state presence. + if moved_to_pending_ids.contains(tx.transaction_id()) { + self.remove_transaction_from_redis_submitted_zset(pipeline, tx); + } else if tx_nonce <= self.transaction_counts.latest { self.remove_transaction_from_redis_submitted_zset(pipeline, tx); } // If beyond latest confirmed nonce, keep in submitted state (don't clean up) } } }Note: ensure
moved_to_pending_idsis declared as suggested in the previous comment.
342-346: Redundant range gate.Because validation already restricts submitted_txs to max(preconfirmed, latest), the
should_processcheck is redundant. Removing it slightly simplifies control flow.Also applies to: 400-447, 452-471
📜 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/store/submitted.rs(4 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
🔇 Additional comments (2)
executors/src/eoa/store/submitted.rs (2)
352-399: Confirmed-path updates and metrics look correct.Status/receipt writes, metrics, and webhook enqueue are coherently scoped to the first occurrence per transaction_id.
400-447: Prevent dual presence of transaction_id in pending + submittedMoving a tx to pending while leaving other submitted hashes for the same transaction_id (nonce > latest) can cause duplicate downstream processing. Track ids moved to pending and remove all their submitted hashes (regardless of nonce).
Add near
replaced_transactions:// declare alongside `replaced_transactions` let mut moved_to_pending_ids: HashSet<String> = HashSet::new();Mark ids when pushing to pending (inside SCENARIO 2):
replaced_transactions .push((tx.queued_at, tx.transaction_id.clone())); + moved_to_pending_ids.insert(tx.transaction_id.clone());Amend cleanup (see next comment) to remove all submitted hashes if id ∈ moved_to_pending_ids.
Confirm whether any consumers assume uniqueness of transaction_id across states; if so, this change is required before merge.
Summary by CodeRabbit
New Features
Bug Fixes