Bug: ExecutionResources specs mismatch (v7 and v8) #2497

hudem1 · 2025-02-16T14:27:35Z

No refactor after all but bug solving.

DRAFT: I realized I also need to update simulateTransaction where ExecutionResources is also used.

Created another PR with a different approach as agreed.

Context

// in vm pkg

type TransactionTrace struct {
	Type                  TransactionType     
	ValidateInvocation    *FunctionInvocation 
	ExecuteInvocation     *ExecuteInvocation  
	FeeTransferInvocation *FunctionInvocation 
	ConstructorInvocation *FunctionInvocation 
	FunctionInvocation    *FunctionInvocation 
	StateDiff             *StateDiff          
	ExecutionResources    *ExecutionResources <-- What i call root level ExecutionResources
}

type FunctionInvocation struct {
	ContractAddress    felt.Felt                
	EntryPointSelector *felt.Felt               
	Calldata           []felt.Felt              
	CallerAddress      felt.Felt                
	ClassHash          *felt.Felt               
	EntryPointType     string                   
	CallType           string                   
	Result             []felt.Felt              
	Calls              []FunctionInvocation     
	Events             []OrderedEvent           
	Messages           []OrderedL2toL1Message   
	ExecutionResources *ExecutionResources <-- What i call inner call ExecutionResources
}

Problem

In this PR, instead of refactoring, I actually corrected the returned trace according to the specs, more specifically, the ExecutionResources struct in a trace:

v7:

ExecutionResources at the trace root level should contain: ComputationResources + DataAvailability --> no matter the type of tx, the TRANSACTION_TRACE'sExecutionResources field always lead to ExecutionResources
ExecutionResources in trace's inner calls should contain: ComputationResources

v8:

ExecutionResources at the trace root level should contain: L1Gas, L1DataGas, L2Gas --> no matter the type of tx, the TRANSACTION_TRACE's ExecutionResources field always lead to ExecutionResources
ExecutionResources in trace's inner calls should contain: L1Gas, L2Gas

I think the reason for the different fields in inner and root-level ExecutionResources within a same rpc version, is: for example for v8, it's maybe because L1DataGas is for putting the tx's data on L1. And this applies to the whole tx, and cant be applied to part of it. Whereas, in a tx, some inner calls can have specific L1Gas (L2ToL1Msg) and L2Gas. Same for reasoning for the differences in v7's inner and root-level ExecutionResources.

Solution

To adjust the ExecutionResources field correctly, instead of re-creating an ExecutionResources type for trace root-level as well as for inner calls for v7 and v8 (which would also force me to re-create the whole TransactionTrace type just to refer to the custom v7 and v8 ExecutionResources inner and root-level types) and then marshal for specific pkgs, I kept the original vm.TransactionTrace, transform some fields into pointers + added omitempty on those fields. To make it work, I added some cleaning functions for traces executed and retrieved by the VM to clean the ExecutionResources.

Btw for those cleaning fcts, I used an iterative approach because even though recursion gives the same time & space complexities and looks a bit easier to understand imo, the iterative approach allows to avoid any stack overflow (even though probably the inner calls of traces will not go deep enough as to create a stack overflow, but just in case). That being said, appending to my custom slice stack might add a bit of overhead for memory allocation compared to recursion. A good compromise would have been using tail recursion but it seems like Go's compiler doesn't do tail call opti unfortunately :(

codecov · 2025-02-16T14:35:15Z

Codecov Report

Attention: Patch coverage is 83.75000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 73.53%. Comparing base (0e9e53f) to head (6cb59c1).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
rpc/v8/trace.go	83.60%	8 Missing and 2 partials ⚠️
rpc/v6/simulation.go	0.00%	1 Missing ⚠️
rpc/v6/trace.go	50.00%	1 Missing ⚠️
rpc/v7/simulation.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2497      +/-   ##
==========================================
+ Coverage   73.49%   73.53%   +0.04%     
==========================================
  Files         136      136              
  Lines       16657    16650       -7     
==========================================
+ Hits        12242    12244       +2     
+ Misses       3547     3540       -7     
+ Partials      868      866       -2

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

AnkushinDaniil

We actually have to have

		L1Gas:     l1Gas,
		L1DataGas: l1DataGas,
		L2Gas:     l2Gas,

in v8 and ComputationResources in v7. I described this issue in #2430

rpc/v6/trace.go

…tx in v8

rodrigo-pino · 2025-02-24T13:42:37Z

vm/trace.go


-	ComputationResources
+	*ComputationResources


Why should be this one be a pointer?

Because, in v8, we don't want to have ComputationResources fields inside ExecutionResources. So, i needed to set it to its zero-value so that it doesn't get marshalled after, which means:

1- It creates a whole ComputationResources instance (14x uint64) just to finally get ignored when marshalled. So, I thought it was not the most efficient.

2- ComputationResources contains the field Step that doesn't have omitempty. So, without the pointer to the whole struct, it would still end up getting marshalled. I could've added omitempty on the Step field but as I thought about the efficiency issue that I mentioned in 1-, I thought might be better putting the ComputationResources as a pointer.

rodrigo-pino · 2025-02-24T13:42:58Z

vm/trace.go

+	L1Gas     *uint64 `json:"l1_gas,omitempty"`
+	L1DataGas *uint64 `json:"l1_data_gas,omitempty"`
+	L2Gas     *uint64 `json:"l2_gas,omitempty"`


Are all of this optional?

Mmh the thing is we reuse the TransactionTrace from the VM pkg for all rpc versions. So, we don't want those fields for v6 and v7, but we want them for v8. That's why, i had to put them as optional, and make sure they are set in v8

rodrigo-pino · 2025-02-24T13:46:00Z

rpc/v8/trace.go

+		if fnInvocation.ExecutionResources != nil {
+			// Still return them if they are nil
+			if fnInvocation.ExecutionResources.L1Gas == nil {
+				fnInvocation.ExecutionResources.L1Gas = utils.Ptr(uint64(0))


Use new to create an object and get a reference. Eg:

x := new(uint64)

rodrigo-pino · 2025-02-24T13:53:03Z

rpc/v8/trace.go

+	for len(stack) > 0 {
+		// Pop a FunctionInvocation from the stack
+		n := len(stack) - 1
+		fnInvocation := stack[n]
+		stack = stack[:n]


Re-slicing can be very expensive. I think in this case is better just to move the pointer to the left

yes i thought about that but its slicing to remove the last element on the slice, so it should be O(1)

rodrigo-pino · 2025-02-24T13:57:16Z

rpc/v8/trace.go

+	pushFnInvocationIfNotNil := func(fn *vm.FunctionInvocation) {
+		if fn != nil {
+			stack = append(stack, fn)
+		}
+	}
+
+	pushFnInvocationIfNotNil(trace.FeeTransferInvocation)
+	pushFnInvocationIfNotNil(trace.ValidateInvocation)


I think adding a function to avoid writing an if three times might be a bit of an over stretch. I think it might be a bit more readable if just remove the function definition and write the ifs.

Also, I believe it is not performant friendly and there is little optimizations that compiler can due to the existence of a closure in the definition

Oh okay good to know!
Done!

rodrigo-pino · 2025-02-24T13:58:28Z

rpc/v8/trace.go

+// Clean trace's inner execution resources to return only the expected fields
+func cleanTraceInnerExecutionResources(trace *vm.TransactionTrace) {
+	// Stack to process FunctionInvocations iteratively (dfs)
+	stack := []*vm.FunctionInvocation{}


You know this variable is going to have at maximum size 3. Create the array with default capacity of 3 (using make).

Actually, it might have more than 3 because when doing dfs, i push the inner/nested FunctionInvocation (to clean the nested ExecutionResources)

rodrigo-pino

I was reading the comment on the PR and this PR is affecting the very first thing we wanted to avoid. We are changing structs across diferent packages to match different specs (or am I missing something).

It's ok to have a vX and a vY structs that are 99% equal duplicated. The whole goal of the refactor was to make it simple to support at the cost of duplicated code.

hudem1 · 2025-02-24T15:59:12Z

I was reading the comment on the PR and this PR is affecting the very first thing we wanted to avoid. We are changing structs across diferent packages to match different specs (or am I missing something).

It's ok to have a vX and a vY structs that are 99% equal duplicated. The whole goal of the refactor was to make it simple to support at the cost of duplicated code.

Indeed, the main issue seems to be that the TransactionTrace type from the VM pkg is used across all 3 rpc versions. So, the current code is trying to use the same type while still returning different versions of it (because of the ExecutionResources differences b/w rpc versions).

To solve that bug/issue, we could duplicate the TransactionTrace type for the 3 rpc versions and have them use their own ExecutionResources.

rodrigo-pino · 2025-02-27T10:03:45Z

Superceded by #2563

Refactor: Reuse v7 handler for v8

1a85577

hudem1 mentioned this pull request Feb 16, 2025

Cleanup rpc pkg #2437

Open

23 tasks

AnkushinDaniil approved these changes Feb 16, 2025

View reviewed changes

hudem1 marked this pull request as draft February 16, 2025 16:43

hudem1 marked this pull request as ready for review February 16, 2025 17:06

hudem1 changed the title ~~Refactor: Reuse v7 handler for v8~~ Refactor: Reuse v7 starknet_traceTransaction handler for v8 Feb 16, 2025

hudem1 requested a review from AnkushinDaniil February 16, 2025 17:56

AnkushinDaniil requested changes Feb 16, 2025

View reviewed changes

rodrigo-pino reviewed Feb 17, 2025

View reviewed changes

rpc/v6/trace.go Show resolved Hide resolved

fix: Add missing L1Gas, L1DataGas, L2Gas for the exec resources of a …

ddae411

…tx in v8

hudem1 added RPC JSON RPC API Refactor labels Feb 17, 2025

hudem1 marked this pull request as draft February 17, 2025 15:18

hudem1 added 2 commits February 19, 2025 20:40

fix: Adjust ExecutionResources for v7 and v8 to match specs

16c8cf4

opti: Use iterative version instead of recursion

3920536

hudem1 marked this pull request as ready for review February 19, 2025 21:57

hudem1 requested review from AnkushinDaniil and rodrigo-pino February 19, 2025 21:57

hudem1 added the High priority Needs to be solved this week or the next label Feb 24, 2025

hudem1 changed the title ~~Refactor: Reuse v7 starknet_traceTransaction handler for v8~~ Bug: ExecutionResources specs mismatch (v7 and v8) Feb 24, 2025

hudem1 added Bug Something isn't working or security issue and removed Refactor labels Feb 24, 2025

hudem1 added 2 commits February 24, 2025 13:32

Merge branch 'main' into hudem1/rpc-cleanup-TraceTransaction

4512edf

clean: Remove actually useless inner ExecResources cleaning for v7

e911614

hudem1 marked this pull request as draft February 24, 2025 13:58

rodrigo-pino reviewed Feb 24, 2025

View reviewed changes

PR-reviews: new(uint64) and remove pushFnInvocationIfNotNil

6cb59c1

hudem1 mentioned this pull request Feb 25, 2025

fix: ExecutionResources match specs #2563

Closed

rodrigo-pino closed this Feb 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug: ExecutionResources specs mismatch (v7 and v8) #2497

Bug: ExecutionResources specs mismatch (v7 and v8) #2497

hudem1 commented Feb 16, 2025 •

edited

Loading

codecov bot commented Feb 16, 2025 •

edited

Loading

AnkushinDaniil left a comment

rodrigo-pino Feb 24, 2025

hudem1 Feb 24, 2025 •

edited

Loading

rodrigo-pino Feb 24, 2025

hudem1 Feb 24, 2025

rodrigo-pino Feb 24, 2025

rodrigo-pino Feb 24, 2025

hudem1 Feb 24, 2025 •

edited

Loading

rodrigo-pino Feb 24, 2025

hudem1 Feb 24, 2025

rodrigo-pino Feb 24, 2025

hudem1 Feb 24, 2025

rodrigo-pino left a comment

hudem1 commented Feb 24, 2025 •

edited

Loading

rodrigo-pino commented Feb 27, 2025

Bug: ExecutionResources specs mismatch (v7 and v8) #2497

Bug: ExecutionResources specs mismatch (v7 and v8) #2497

Conversation

hudem1 commented Feb 16, 2025 • edited Loading

Context

Problem

Solution

codecov bot commented Feb 16, 2025 • edited Loading

Codecov Report

AnkushinDaniil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudem1 Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudem1 Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigo-pino left a comment

Choose a reason for hiding this comment

hudem1 commented Feb 24, 2025 • edited Loading

rodrigo-pino commented Feb 27, 2025

hudem1 commented Feb 16, 2025 •

edited

Loading

codecov bot commented Feb 16, 2025 •

edited

Loading

hudem1 Feb 24, 2025 •

edited

Loading

hudem1 Feb 24, 2025 •

edited

Loading

hudem1 commented Feb 24, 2025 •

edited

Loading