Skip to content

feat(rpc): add progressive trace range and checkpoint helpers - #4079

Open
danielntmd wants to merge 4 commits into
danielntmd/shared-trace-cache-integrationfrom
danielntmd/progressive-trace-primitives
Open

danielntmd wants to merge 4 commits into
danielntmd/shared-trace-cache-integrationfrom
danielntmd/progressive-trace-primitives

Conversation

@danielntmd

@danielntmd danielntmd commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

User description

User description

User description

This PR adds the shared building blocks for tracing a block incrementally.

Progressive Cache Behavior:

  • Cached transaction traces provide the state checkpoint for executing the remaining transactions, and extensions preserve previously published results.
  • Requests for initial reads replay the full block (retained logic).
  • Continuation checkpoints are rebuilt from cached per-transaction state diffs when needed. This trades recomputation during resumption for lower cache memory usage, avoiding a duplicate cumulative state diff.

Note: State resumption, checkpoint class loading, and execution-error offsets are covered by RPC integration tests in #4080.


PR Type

Enhancement, Tests


Description

  • Introduce progressive trace caching primitives.

    • Add Range for incremental block execution.
    • Implement state checkpointing via ResumeState.
    • Allow merging cached traces via Combine.
  • Add Complete flag to BlockTrace.

  • Introduce TransactionTarget for coverage validation.

  • Mark completed block traces in RPC handlers.


File Walkthrough

Relevant files
Tests
3 files
trace_adaptation_test.go
Mark mock trace records as complete in tests                         
+1/-0     
progressive_test.go
Test state resumption and trace range combinations             
+262/-0 
result_test.go
Update tests for completeness and coverage checks               
+6/-4     
Enhancement
6 files
progressive.go
Introduce primitives for incremental progressive block tracing
+232/-0 
result.go
Mark block traces from feeder as complete                               
+2/-1     
trace.go
Add `Complete` flag and `TransactionTarget` logic               
+31/-0   
trace.go
Mark v10 block traces as complete before publishing           
+1/-0     
trace.go
Mark v8 block traces as complete before publishing             
+1/-0     
trace.go
Mark v9 block traces as complete before publishing             
+3/-0     

@danielntmd
danielntmd added this pull request to stack #4068 September 15, 2026 07:33
@danielntmd
danielntmd marked this pull request as ready for review September 15, 2026 07:36
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Review in progress

  • Gather context (diff, CLAUDE.md, changed files)
  • Understand the request
  • Review rpc/tracecache/progressive.go
  • Review rpc/tracecache/progressive_test.go
  • Review rpc/tracecache/result.go and rpc/tracecache/trace.go
  • Post inline comments (if any)
  • Post final summary

View job run

@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 79.81651% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.45%. Comparing base (81e483f) to head (62964ea).

Files with missing lines Patch % Lines
rpc/tracecache/progressive.go 78.35% 21 Missing ⚠️
rpc/tracecache/trace.go 85.71% 1 Missing ⚠️
Additional details and impacted files
@@                              Coverage Diff                              @@
##           danielntmd/shared-trace-cache-integration    #4079      +/-   ##
=============================================================================
- Coverage                                      79.46%   79.45%   -0.02%     
=============================================================================
  Files                                            469      470       +1     
  Lines                                          36160    36268     +108     
=============================================================================
+ Hits                                           28736    28818      +82     
- Misses                                          7415     7441      +26     
  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.

@danielntmd
danielntmd force-pushed the danielntmd/progressive-trace-primitives branch from 5d92b10 to 5132f3f Compare September 15, 2026 21:47
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Review in progress

  • Gather context (diff, CLAUDE.md, changed files)
  • Understand the request
  • Review rpc/tracecache/progressive.go
  • Review rpc/tracecache/progressive_test.go
  • Review rpc/tracecache/result.go and rpc/tracecache/trace.go
  • Post inline comments (if any)
  • Post final summary

View job run

@danielntmd
danielntmd force-pushed the danielntmd/progressive-trace-primitives branch from 5132f3f to 62964ea Compare September 18, 2026 06:58
@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Review in progress

  • Gather context (diff, CLAUDE.md, changed files)
  • Understand the request
  • Review rpc/tracecache/progressive.go
  • Review rpc/tracecache/progressive_test.go
  • Review rpc/tracecache/result.go and rpc/tracecache/trace.go
  • Post inline comments (if any)
  • Post final summary

View job run

@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
⚠️ Risk level: Medium
📂 Priority files

  • rpc/tracecache/progressive.go
  • rpc/tracecache/trace.go
🏅 Score: 72
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible nil dereference

checkpointFromTraces calls mergeVMStateDiff(&result, traces[index].vmTrace.StateDiff) for every trace in the cached prefix without checking that vmTrace or vmTrace.StateDiff is non-nil. Combine only validates that state diffs are non-nil for the newly executed suffix, not for traces that entered the cache directly via FromVM (e.g. result_test.go's TestVMResult builds a BlockTrace from vm.TransactionTrace{Type: vm.TxnInvoke} with no StateDiff). If such a block trace is later cached and used as the prefix for ResumeState, mergeVMStateDiff will dereference a nil *vm.StateDiff and panic. Whether callers always guarantee a non-nil StateDiff for cached VM traces isn't visible in this diff, so this may only be exploitable if a full-block trace without a StateDiff gets published to the cache.

// checkpointFromTraces rebuilds a core.StateDiff checkpoint from cached
// vm.StateDiff values, avoiding a duplicate cumulative diff in the cache.
func checkpointFromTraces(traces []TransactionTrace) core.StateDiff {
	result := core.EmptyStateDiff()
	for index := range traces {
		mergeVMStateDiff(&result, traces[index].vmTrace.StateDiff)
	}
	return result
}
Prefix invariant not fully validated

PlanRange only checks cached.Source != LocalVM before reusing cached.Traces as the prefix; it does not verify that every trace in the prefix actually has a non-nil VM state diff before it's later consumed by ResumeState/checkpointFromTraces. This means the panic described above is only guarded against by an assumption elsewhere (e.g., that all LocalVM-sourced traces always carry state diffs), which is not enforced in this file.

	if cached != nil && !initialReads {
		if cached.Source != LocalVM {
			return nil, errors.New("cannot extend a feeder trace")
		}
		plan.prefix = cached.Traces
		plan.Start = uint64(len(plan.prefix))
	}
	if plan.Start > plan.End {
		return nil, fmt.Errorf(
			"cached trace prefix [0, %d) exceeds requested range [0, %d)",
			plan.Start,
			plan.End,
		)
	}
	return plan, nil
}

//
// See [Cache] for the mutability contract for published traces.
type Range struct {
Start, End uint64

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.

nit: please one param per line.

Honestly, in this case makes a lot of sense and it is readable so feel free to ignore. It is mostly a nitpick about keeping the "status quo" style

transactions []core.Transaction,
target *TransactionTarget,
initialReads bool,
) (*Range, error) {

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.

Why can Range be returned by value?

Comment on lines +60 to +63
if target != nil && (target.Hash == nil || target.Index >= uint64(len(transactions)) ||
!transactions[target.Index].Hash().Equal(target.Hash)) {
return nil, ErrTargetNotFound
}

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.

another style guide we follow is separating the conditions from the if (or for) statements if they become too big.

Suggestion:

validTarget := <long cond>
if validTarget {
  // ...
}

// prefix, typically the chain's head state.
// - blockNumber: the number of the block being traced.
func (r *Range) ResumeState(
parent, classes core.StateReader,

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.

one parameter per line

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants