perf(rpc): derive tx status subscription updates from notification payloads - #3863
perf(rpc): derive tx status subscription updates from notification payloads#3863Ehsan-saradar wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes SubscribeTransactionStatus in rpc/v9 and rpc/v10 by deriving transaction status updates directly from incoming new-head / pre-confirmed / pre-latest notification payloads, avoiding repeated full TransactionStatus RPC lookups (DB receipt reads and potential feeder fallback) on every notification.
Changes:
- Derive
ACCEPTED_ON_L2,PRE_CONFIRMED, andCANDIDATEsubscription updates from the notification payload contents instead of re-runningTransactionStatus. - Add helper logic to extract execution outcome (success/revert + reason) from index-aligned receipts within the payload.
- Update tests and add a new case validating reverted execution status derived from a new-head payload.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rpc/v9/subscription_status.go | Switch subscription update handlers to derive status directly from payloads; add helper for extracting status from blocks. |
| rpc/v9/subscriptions_test.go | Update expectations to reflect fewer DB/syncer calls; add reverted-execution-from-new-head test. |
| rpc/v10/subscription_status.go | Same optimization as v9 for v10 subscription status handling. |
| rpc/v10/subscriptions_test.go | Same test updates and new reverted-execution-from-new-head coverage for v10. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Finality: finality, | ||
| Execution: TxnSuccess, | ||
| } | ||
| if receipt := block.Receipts[i]; receipt.Reverted { | ||
| status.Execution = TxnFailure |
There was a problem hiding this comment.
Good catch — added a length guard, and execution status is now only set when the receipt is actually present (otherwise it stays omitted).
| status := TransactionStatus{ | ||
| Finality: finality, | ||
| Execution: TxnSuccess, | ||
| } | ||
| if receipt := block.Receipts[i]; receipt.Reverted { |
There was a problem hiding this comment.
Same as the v9 one — guarded now, execution status is only populated when the receipt exists.
cd416fd to
56d1110
Compare
| for i, txn := range block.Transactions { | ||
| if txn.Hash().Equal(s.txHash) { | ||
| status := TransactionStatus{Finality: finality} | ||
| if i < len(block.Receipts) { | ||
| if receipt := block.Receipts[i]; receipt.Reverted { | ||
| status.Execution = TxnFailure | ||
| status.FailureReason = receipt.RevertReason | ||
| } else { | ||
| status.Execution = TxnSuccess | ||
| } | ||
| } |
There was a problem hiding this comment.
Receipts can't actually be nil here — every payload producer builds one per tx, and block verification dereferences Receipts[i] long before this point. The rest of the RPC code indexes them the same way, so I'll skip the extra guard to keep things consistent.
| if status.Finality == s.lastStatus { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This check is pre-existing — it just moved from checkTxStatus into sendStatus. Execution can't change while finality stays the same: after ACCEPTED_ON_L2 the result is final, and during PRE_CONFIRMED the handlers skip further rounds anyway (same as before). The definitive result always lands with the ACCEPTED_ON_L2 update, so nothing gets lost.
| for i, txn := range block.Transactions { | ||
| if txn.Hash().Equal(s.txHash) { | ||
| status := TransactionStatus{Finality: finality} | ||
| if i < len(block.Receipts) { | ||
| if receipt := block.Receipts[i]; receipt.Reverted { | ||
| status.Execution = TxnFailure | ||
| status.FailureReason = receipt.RevertReason | ||
| } else { | ||
| status.Execution = TxnSuccess | ||
| } | ||
| } |
There was a problem hiding this comment.
Same as the v9 thread — nil entries can't happen in these payloads, so skipping the guard here too.
| if status.Finality == s.lastStatus { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Same as the v9 thread — pre-existing behavior, and the final execution result always arrives with the ACCEPTED_ON_L2 update.
| } | ||
| mockSyncer.EXPECT().PreConfirmedChain(). | ||
| Return(mustNewChain(t, preConfirmed), nil).Times(1) | ||
| // PreConfirmed status is derived from the notification payload, no DB lookups. |
There was a problem hiding this comment.
Right that these lines are leftovers from the removed DB-lookup expectations - dropped them. Small correction though: they do not actually break the compile, since the field assignment on rpcTx counts as a use. Dead code either way, so removed.
| } | ||
| mockSyncer.EXPECT().PreConfirmedChain(). | ||
| Return(mustNewChain(t, preConfirmed), nil).Times(1) | ||
| // PreConfirmed status is derived from the notification payload, no DB lookups. |
There was a problem hiding this comment.
Same as the v9 one — removed. (No compile error here either, the field assignment counts as a use, but it was dead code regardless.)
…yloads SubscribeTransactionStatus used to run the full TransactionStatus RPC on every new head, pre-latest, and pre-confirmed notification, re-reading receipts from the DB and potentially falling back to the feeder gateway. The notification payloads already carry everything we need: a transaction included in a new head is ACCEPTED_ON_L2, one in a pre-latest or pre-confirmed block is PRE_CONFIRMED, and a candidate is CANDIDATE. The execution result comes from the index-aligned receipt in the same payload, so these handlers now serve status updates purely from memory. The initial status check on subscribe and the L1 head handler still use the full lookup, since neither the subscribe call nor L1 head events carry the transaction. Closes NethermindEth#3862
e727696 to
07f8ce2
Compare
Closes #3862
SubscribeTransactionStatusused to run the fullTransactionStatusRPC on every new head / pre-confirmed notification, re-reading receipts from the DB and potentially falling back to the feeder gateway. The notification payloads already carry everything we need though:ACCEPTED_ON_L2PRE_CONFIRMEDThe execution result (success / reverted + revert reason) comes from the index-aligned receipt in the same payload, so these handlers are now served purely from memory. Status only moves forward, so re-published pre-confirmed rounds don't emit duplicates or downgrades.
The initial check on subscribe and
onL1Headkeep the full lookup: neither the subscribe call nor an L1 head event carries the transaction, and L1 head events are infrequent so the cost is negligible.One trade-off worth calling out: subscription feeds keep only the last event (
SubscribeKeepLast, buffer of 1), so a slow subscriber can in theory miss the exact head block containing its tx. Previously the next head's full lookup would catch that; now the update is recovered at the next L1 head event instead, which still does a full check and delivers whatever status the tx has by then (usuallyACCEPTED_ON_L2, since L1 lags L2). That felt in line with the issue's "served purely from the notification payloads", withonL1Headacting as the safety net.Applied to both
rpc/v9andrpc/v10, tests adjusted accordingly plus a new case covering a reverted tx derived from the new head payload.