Skip to content

perf(rpc): dedupe preconfirmed subscriptions before adaptation - #4036

Open
danielntmd wants to merge 1 commit into
mainfrom
danielntmd/refactor-dedup-ordering
Open

perf(rpc): dedupe preconfirmed subscriptions before adaptation#4036
danielntmd wants to merge 1 commit into
mainfrom
danielntmd/refactor-dedup-ordering

Conversation

@danielntmd

@danielntmd danielntmd commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

User description

Performs preconfirmed subscription deduplication before RPC response adaptation, avoiding unnecessary transaction and receipt object creation for entries already emitted in the same preconfirmed round.


PR Type

Enhancement


Description

  • Dedupe preconfirmed entries before adaptation

  • Avoid unnecessary transaction object creation

  • Avoid unnecessary receipt object creation

  • Apply pattern across RPC v9 and v10


File Walkthrough

Relevant files
Enhancement
subscription_receipts.go
Dedupe preconfirmed receipts before adaptation                     

rpc/v10/subscription_receipts.go

  • Adds shouldSend callback to receiptsOf
  • Marks receipts as sent before adaptation
  • Skips already-sent preconfirmed receipts
  • Passes nil callback for canonical blocks
+12/-7   
subscription_transactions.go
Dedupe preconfirmed transactions before adaptation             

rpc/v10/subscription_transactions.go

  • Adds shouldSend callback to transactionsOf
  • Marks transactions as sent before adaptation
  • Uses txn.Hash() for deduplication
  • Passes nil callback for canonical blocks
+11/-7   
subscription_receipts.go
Dedupe preconfirmed receipts before adaptation                     

rpc/v9/subscription_receipts.go

  • Adds shouldSend callback to receiptsOf
  • Marks receipts as sent before adaptation
  • Skips already-sent preconfirmed receipts
  • Uses status variable for canonical blocks
+12/-7   
subscription_transactions.go
Dedupe preconfirmed transactions before adaptation             

rpc/v9/subscription_transactions.go

  • Adds shouldSend callback to transactionsOf
  • Marks transactions as sent before adaptation
  • Uses txn.Hash() for deduplication
  • Passes nil callback for canonical blocks
+11/-7   

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @danielntmd's task in 3m 33s —— View job


Review: perf(rpc): dedupe preconfirmed subscriptions before adaptation

Verdict: clean. The refactor moves the deduper.MarkSent check ahead of AdaptReceiptWithBlockInfo/AdaptTransaction by threading a shouldSend func(core.Transaction) bool predicate into receiptsOf/transactionsOf, so already-emitted preconfirmed entries are skipped before the (relatively expensive) adaptation work rather than after. This is applied consistently across rpc/v9 and rpc/v10 for both receipts and transactions.

Checked:

  • Hash equivalence: switching from the adapted response's Hash field to core.Transaction.Hash() (called on the raw block transaction before adaptation) yields the same *felt.Felt value, so dedup keys are unchanged.
  • Filter ordering: sender-address filtering still happens before the shouldSend check inside the iterator, matching the original semantics — no change in which transactions get marked/consumed by the deduper.
  • onNewHead call sites: correctly pass nil for shouldSend, preserving the existing comment that canonical blocks bypass the deduper entirely.
  • Deduper itself (rpc/rpccore/preconfirmed_deduper.go) is untouched and remains single-goroutine-only, consistent with receiptsSubscriberState/transactionsSubscriberState usage.
  • No test files call receiptsOf/transactionsOf directly, so the new parameter doesn't break existing coverage; go build and go vet pass on rpc/v9 and rpc/v10.

No inline comments — didn't find correctness, concurrency, or resource issues to flag.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 00fc30a)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🏅 Score: 90
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected

@danielntmd
danielntmd force-pushed the danielntmd/refactor-dedup-ordering branch from b5a2334 to 00fc30a Compare September 4, 2026 20:54
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @danielntmd's task in 3m 44s —— View job


Review: perf(rpc): dedupe preconfirmed subscriptions before adaptation

Verdict: clean. No blocker/important/nit findings — no inline comments posted.

Summary of what changed: receiptsOf/transactionsOf (v9 + v10) now take an optional shouldSend func(core.Transaction) bool and run it before AdaptReceiptWithBlockInfo/AdaptTransaction, so already-emitted preconfirmed entries are skipped before the adaptation work instead of after. onNewHead passes nil (canonical blocks still bypass the deduper); onPreConfirmed passes a closure wrapping deduper.MarkSent.

Checked and confirmed correct:

  • Dedup key equivalence: the closure now keys on core.Transaction.Hash() (the raw transaction, pre-adaptation) instead of the previously-adapted response's Hash field. Every concrete core.Transaction implementation (InvokeTransaction, DeclareTransaction, DeployTransaction, DeployAccountTransaction, L1HandlerTransaction) exposes Hash() *felt.Felt, and this is the same hash the adapters derive their Hash field from — no change in dedup semantics.
  • Ordering preserved: filterTxBySender still runs before shouldSend, and shouldSend still runs before adaptation, in both receiptsOf and transactionsOf, for both v9 and v10 — consistent across all 4 files.
  • onNewHead call sites correctly pass nil, preserving the "canonical blocks bypass the deduper" comment/behavior.
  • Deduper (rpc/rpccore/preconfirmed_deduper.go) is untouched; receiptsSubscriberState/transactionsSubscriberState remain single-goroutine-owned per their existing doc comments, so no new concurrency concerns from calling MarkSent earlier in the pipeline.
  • go build ./rpc/... and go vet ./rpc/v9/... ./rpc/v10/... both pass. Was not able to execute go test in this sandbox (command required approval I don't have in this context); no test files call receiptsOf/transactionsOf directly, so existing coverage is exercised only through the subscriber handlers, which are unchanged in interface.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 00fc30a

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.19%. Comparing base (121146c) to head (00fc30a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4036      +/-   ##
==========================================
- Coverage   79.31%   79.19%   -0.12%     
==========================================
  Files         464      464              
  Lines       35793    35793              
==========================================
- Hits        28389    28348      -41     
- Misses       7395     7436      +41     
  Partials        9        9              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@brbrr brbrr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants