Scheduled Transactions indexing #95

tim-barry · 2025-11-14T00:28:16Z

Enable processing of transactions and events in the system collection.

Summary by CodeRabbit

Bug Fixes
- Always fetches and applies the initial system-collection transaction so scheduled transactions are captured.
- Detects pending scheduled executions, counts them, and fetches those transactions sequentially with retry/backoff to avoid partial captures.
- Adds context-aware cancellation and NotFound handling to stop promptly when items are missing.
- Includes system-collection events in the normal transaction flow so indexing and ordering remain correct.
Improvements
- Reduced initial delays and added per-fetch backoff/cancellation checks for more responsive, reliable processing.

coderabbitai · 2025-11-14T00:28:22Z

Walkthrough

Always fetch the first system-collection transaction; for spork.Version >= 8, inspect its events for PendingExecution and iteratively fetch scheduled and final system transactions with per-transaction retries/backoff and context-cancellation checks, processing system-collection events in the normal flow without a special-case skip.

Changes

Cohort / File(s)	Summary
System transaction retrieval and processing `state/process.go`	Add deferred span.End(); remove per-collection/transaction idle-wait scaffolding; return promptly on context/cancellation errors. Always fetch first system-collection transaction; for `spork.Version >= 8` inspect its Events for `PendingExecution`, iteratively fetch scheduled and final system transactions with per-fetch retries/backoff and `NotFound` handling, add cancellation checks after each fetch, fetch transaction results and then full transaction infos, and remove special-case skip so system-collection events enter the normal processing/indexing flow.

Sequence Diagram(s)

sequenceDiagram
    participant Flow as Process Flow
    participant Fetcher as Transaction Fetcher
    participant Inspector as Event Inspector
    participant Collector as Data Collector

    rect rgb(240,248,255)
    Flow->>Fetcher: Fetch first system-collection transaction
    Fetcher-->>Flow: Return first system transaction
    Flow->>Inspector: Inspect events for PendingExecution
    end

    alt spork.Version >= 8 AND PendingExecution found
        rect rgb(245,255,240)
        loop while PendingExecution present
            Flow->>Fetcher: Fetch next scheduled/final system transaction (retries/backoff, context-aware)
            Fetcher-->>Flow: Return transaction or NotFound
            alt NotFound
                Flow-->>Flow: Break fetch loop
            else Returned
                Flow->>Inspector: Inspect events for more PendingExecution
            end
        end
        end
    end

    rect rgb(255,250,240)
    Flow->>Collector: Fetch transaction results and full infos (with per-fetch backoff and cancellation checks)
    Collector-->>Flow: Complete
    Flow->>Collector: Process/index system-collection events (normal flow)
    Collector-->>Flow: Done
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Extra attention:
- Loop fetching scheduled/final system transactions (retry/backoff, NotFound handling).
- Correct spork.Version gating and PendingExecution detection.
- Context-cancellation checks after collection/transaction/result fetches.
- Deferred span.End() placement and tracing correctness.
- Effects of removing the system-collection skip on indexing order and event handling.

Poem

🐰 I hopped to the first, then sniffed for a clue,

Found PendingExecution — so I fetched a few,
Retries and pauses kept each fetch polite,
No special skips now — they join the main flight,
A rabbit’s small cheer for ledger steps made right.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Scheduled Transactions indexing' directly aligns with the primary objective to enable processing of transactions and events in the system collection, and accurately reflects the main changes in state/process.go.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch enable-scheduled-transactions

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

UlyanaAndrukhiv

Looks good to me!

state/process.go

jordanschalm · 2025-11-14T18:21:58Z

state/process.go

+							select {
+							case <-ctx.Done():
+								return
+							default:
+							}
+							client := spork.AccessNodes.Client()
+							txnResult, err := client.TransactionResult(ctx, hash, uint32(txnIndex))


Suggested change

select {

case <-ctx.Done():

return

default:

}

client := spork.AccessNodes.Client()

txnResult, err := client.TransactionResult(ctx, hash, uint32(txnIndex))

client := spork.AccessNodes.Client()

txnResult, err := client.TransactionResult(ctx, hash, uint32(txnIndex))

Do we need the explicit context check if we are passing it to a blocking network call right away? My expectation would be that client.TransactionResult will exit if the context is canceled.

My expectation would be that client.TransactionResult will exit if the context is canceled.

it will

I was just copying code here; it does seem client.TransactionResult will return with the context's status if the context is canceled. So it would be something like

Suggested change

select {

case <-ctx.Done():

return

default:

}

client := spork.AccessNodes.Client()

txnResult, err := client.TransactionResult(ctx, hash, uint32(txnIndex))

client := spork.AccessNodes.Client()

txnResult, err := client.TransactionResult(ctx, hash, uint32(txnIndex))

if errors.Is(err, context.Canceled) {

return

}

which IMO is not really much better.

to handle the case precisely, you would need this anyway since it's possible the context was canceled during the request.

Updated these instances to check errors for cancellation in 3e83242

state/process.go

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

state/process.go (1)

405-413: Critical: System collection transactions will cause panic here.

This code accesses col.txns[idx].Payer for all collections, including system collections. In the slow-path processing (lines 188-253), system collections have col.txnResults populated but col.txns remains empty. When this loop processes system collection transactions, accessing col.txns[idx] will panic with an index out of range error.

This issue stems from the incomplete system transaction fetching in lines 216-252 (see my previous comment). The fix requires ensuring full transaction objects are fetched for system transactions, not just transaction results.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e492f0 and 3e83242.

📒 Files selected for processing (1)

state/process.go (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

state/process.go (1)

log/log.go (1)

Errorf (72-74)

🔇 Additional comments (5)

state/process.go (5)

7-7: LGTM!

The errors import is necessary for the new errors.Is(err, context.Canceled) checks added throughout the slow-path processing logic.

139-177: LGTM - Context cancellation handling is correct.

The explicit context cancellation checks after fetch operations properly end the span and return gracefully. While the client calls will return with context status, these checks ensure cleanup happens immediately after detecting cancellation.

206-209: LGTM - Consistent context cancellation handling.

237-240: LGTM - Context cancellation check is correct.

196-215: LGTM - First system transaction fetch is correct.

The logic correctly retrieves the first transaction of the system collection with appropriate retry and cancellation handling.

state/process.go

stop entirely when system collection is shorter than the expected length

peterargue · 2025-11-14T23:05:40Z

state/process.go

-							if initialTxn {
-								initialTxn = false
-							} else {
-								time.Sleep(time.Second)


this is different. we're no longer sleeping for each request, just failing requests. I think that's ok since we will receive an error if the request is rate limited, which will cause it to sleep.

In each of these cases there's only one request, and the sleep was only occurring on retries - notice that initialTxn := true was run for every transactionID (so every unique request that is made). The updated code should have the same behaviour as far as I can tell - no sleep on the first attempt, and sleep before retrying.

peterargue · 2025-11-15T00:00:08Z

state/process.go

 							txnIndex, hash, height, err,
 						)
+						if errors.Is(err, context.Canceled) {
+							span.End()


handling a canceled context previously didn't call end. is it OK to add it here?

In case of a canceled context, we will be returning anyways and abandoning the processing of the block (this was already the case). I think the previous code was incorrect in not calling end or deferring a call to it, as the span will be going out of scope.

peterargue · 2025-11-15T00:01:06Z

state/process.go

 							col.CollectionId, hash, height, err,
 						)
+						if errors.Is(err, context.Canceled) {
+							span.End()


is this something that could be called in a defer, or are there cases where we return without calling it?

I avoided using a defer because of the trace.EndSpanOk() being called when the function is successful, but it seems that span.End should be idempotent (so ok to be defer). I don't know if there is a case where we would want to return without calling it, and I assumed not.

state/process.go

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

state/process.go (1)
44-75: Defer on span.End() should defensively handle zero-value span and double-end

Deferring span.End() before any call to trace.NewSpan assumes the zero value of trace.Span is safe to call End() on. If trace.Span is an interface whose zero value is nil, this will panic on early returns (for example if rctx is already canceled before the first span is created). In addition, you now both call trace.EndSpanOk(span) and span.End() on the same span, which may be redundant if EndSpanOk also ends the span.

To make this robust regardless of the concrete trace.Span type, consider guarding the deferred call and relying on it as the single place that ends the span:
var span trace.Span
defer func() {
	if span != nil {
		span.End()
	}
}()
and, if EndSpanOk internally calls End, consider adjusting it (or dropping the explicit span.End() here) so the span is only ended in one path.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ee0f16 and 730a34e.

📒 Files selected for processing (1)

state/process.go (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

state/process.go (2)

access/access.go (1)

Client (66-69)

log/log.go (1)

Errorf (72-74)

🔇 Additional comments (2)

state/process.go (2)

139-143: Cancellation-aware retries in slow-path fetch loops look good

The new codes.Canceled checks in the slow-path collection/transaction/transaction-result loops correctly bail out on cancellation and only sleep/retry on other errors. This prevents spinning when the context is canceled while preserving the previous retry-with-backoff behavior for transient failures.

Also applies to: 157-161, 174-178

196-271: Syntax for range scheduledTxs + 1 is valid in Go 1.22+ and does not require changes

Go 1.22 introduced support for ranging over integers with the syntax for range N. The repository uses Go 1.25.0 (per go.mod), which fully supports this feature. The code at line 227 in state/process.go—for range scheduledTxs + 1 {—is valid Go syntax and will compile without error. When ranging over an integer, the loop iterates the specified number of times (0 through N-1), so this loop correctly executes scheduledTxs + 1 iterations.

The suggested refactor to replace it with a traditional 3-clause for i := 0; i < scheduledTxs+1; i++ loop is unnecessary.

Likely an incorrect or invalid review comment.

enable reading events from system collection

64a5b88

tim-barry requested review from jordanschalm and peterargue November 14, 2025 00:29

add to slow path (when GetTransactionResultsByBlockID has failed)

4218c2c

tim-barry marked this pull request as ready for review November 14, 2025 17:48

tim-barry requested a review from UlyanaAndrukhiv November 14, 2025 17:55

UlyanaAndrukhiv approved these changes Nov 14, 2025

View reviewed changes

jordanschalm reviewed Nov 14, 2025

View reviewed changes

peterargue reviewed Nov 14, 2025

View reviewed changes

state/process.go Show resolved Hide resolved

tim-barry and others added 3 commits November 14, 2025 11:04

fix for final system transaction

24b67c4

Update state/process.go

4e492f0

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

update context cancellation handling on slow path

3e83242

tim-barry requested review from jordanschalm and peterargue November 14, 2025 20:03

coderabbitai bot reviewed Nov 14, 2025

View reviewed changes

state/process.go Show resolved Hide resolved

tim-barry added 2 commits November 14, 2025 12:26

minor fix

2890176

stop entirely when system collection is shorter than the expected length

retrieve transaction infos for system collection on slow path

5ee0f16

jordanschalm approved these changes Nov 15, 2025

View reviewed changes

peterargue reviewed Nov 15, 2025

View reviewed changes

tim-barry added 2 commits November 14, 2025 16:53

use defer to end the tracing span

d0701d0

check grpc status code for cancellation

730a34e

coderabbitai bot reviewed Nov 15, 2025

View reviewed changes

peterargue approved these changes Nov 15, 2025

View reviewed changes

tim-barry merged commit 3818570 into main Nov 15, 2025
1 check passed

tim-barry deleted the enable-scheduled-transactions branch November 15, 2025 19:03

Scheduled Transactions indexing #95

Scheduled Transactions indexing #95

Uh oh!

Conversation

tim-barry commented Nov 14, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

UlyanaAndrukhiv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterargue Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tim-barry commented Nov 14, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 14, 2025 •

edited

Loading

peterargue Nov 14, 2025 •

edited

Loading