Implement `proof_mode` functionalities #682

MaksymMalicki · 2025-01-17T11:40:26Z

No description provided.

…arguments and initial gas to the memory

* Add parsing logic for input user args * Add flags for available gas, input user args, writing args to memory * Fix unit tests for user arguments parsing * Lint the PR * Add user args to hint context * Refactor the code * Fix unconditional append of ExternalWriteArgsToMemory, bug fixes in integration tests * Add fixes of the call size calculation and include ExternalWriteArgsToMemory hint when gas present * Add layouts for integration tests * Add error handling * Fixes in entry code generation * Address changes mentioned in a discussion * Add comment regarding writing to memory in a hint for the future reference in the integration tests with args * Changes in calculations of the initial PC offset, CALL opcode offset incremented by mainFuncOffset, writing user args to the AP in the hint * Turn back VM config to private field * Add error handling on assign of `userArgs` to the initial scope * Lint project * Bump go version from 1.20 -> 1.21 (#678) * Bump go version from 1.20 -> 1.21 * Update golangci-lint * Simplify the Makefile * Correction in the makefile

…, code cleanups

…ration tests pipeline

cicr99 · 2025-02-11T19:06:36Z

integration_tests/cairo1_test.go

+		"./cairo_1_programs/",
+		"./cairo_1_programs/dict_non_squashed",
+		"./cairo_1_programs/with_input",
+		"./cairo_1_programs/serialized_output",
+		"./cairo_1_programs/serialized_output/with_input",


As discussed in the daily today, it would be nice to refactor the code below to run the tests recursively in the same folder, instead of having to pass all the subdirectories as roots as well. This could be solved in a separate PR, as this already has a lot of things. Let's just link the issue to this comment

Also, I cannot comment on lines that haven't been changed, but looking into the refactor I noticed in line 195 (before this changes) the last argument should be false, as discussed today as well

cicr99 · 2025-02-11T19:25:05Z

pkg/vm/memory/memory.go

@@ -191,6 +191,8 @@ func (segment *Segment) String() string {
 // Represents the whole VM memory divided into segments
 type Memory struct {
 	Segments []*Segment
+	// TemporarySegments is a map of temporary segments, key is the segment index, value is the segment
+	TemporarySegments map[uint64]*Segment


Not sure why we need to use a map here. As far as I see, the only place where this is updated is in AllocateEmptyTemporarySegment function in line:

memory.TemporarySegments[uint64(len(memory.TemporarySegments))] = EmptySegment()

This looks like you could just use an array and append an EmptySegment instead.
Am I missing something?

cicr99 · 2025-02-11T19:29:58Z

pkg/vm/memory/memory.go

@@ -227,6 +229,15 @@ func (memory *Memory) AllocateEmptySegment() MemoryAddress {
 	}
 }

+// Allocates an empty temporary segment and returns its index
+func (memory *Memory) AllocateEmptyTemporarySegment() MemoryAddress {
+	memory.TemporarySegments[uint64(len(memory.TemporarySegments))] = EmptySegment()


This is what I commented above regarding TemporarySegments structure

cicr99 · 2025-02-11T19:35:05Z

pkg/vm/memory/memory.go

+func (memory *Memory) AllocateEmptyTemporarySegment() MemoryAddress {
+	memory.TemporarySegments[uint64(len(memory.TemporarySegments))] = EmptySegment()
+	return MemoryAddress{
+		SegmentIndex: uint64(len(memory.TemporarySegments)),


Is this the correct SegmentIndex? After the previous line the length of the map increased by 1, so the index you are returning here is different from the one you used as key

cicr99 · 2025-02-11T19:37:20Z

pkg/hintrunner/hinter/dict.go

@@ -38,19 +38,27 @@ func (d *Dictionary) InitNumber() uint64 {
 type DictionaryManager struct {
 	// a map that links a segment index to a dictionary
 	dictionaries map[uint64]Dictionary
+	// useTemporarySegments is a flag that indicates if the dictionaries should located in temporary segments, and later relocated to memory segments


Missing word in comment, it should be: "the dictionaries should be located in temporary segments"

cicr99 · 2025-02-11T20:15:26Z

pkg/runner/runner.go

+	case ExecutionModeZero, ExecutionModeCairo:
 		returnFp := memory.AllocateEmptySegment()
 		mvReturnFp := mem.MemoryValueFromMemoryAddress(&returnFp)
-		if runner.runnerMode == ProofModeCairo {
-			// In Cairo mainPCOffset is equal to the offset of program segment base
-			return runner.initializeEntrypoint(uint64(0), nil, &mvReturnFp, memory, stack, 2)
-		} else if runner.runnerMode == ExecutionModeCairo {
-			return runner.initializeEntrypoint(uint64(0), nil, &mvReturnFp, memory, stack, 0)
+		if runner.runnerMode == ExecutionModeCairo {
+			return runner.initializeEntrypoint(uint64(0), nil, &mvReturnFp, memory, stack)
 		} else {
 			mainPCOffset, ok := runner.program.Entrypoints["main"]
 			if !ok {
 				return mem.UnknownAddress, errors.New("can't find an entrypoint for main")
 			}
-			return runner.initializeEntrypoint(mainPCOffset, nil, &mvReturnFp, memory, stack, 0)
+			return runner.initializeEntrypoint(mainPCOffset, nil, &mvReturnFp, memory, stack)
+		}


Perhaps it's better to separate these two, at the end you have the logic in different branches already. I think it will be more readable this way. Should we re-add the comment regarding mainPCOffset in Cairo?

cicr99 · 2025-02-11T20:33:09Z

pkg/runner/runner.go

+	for _, builtin := range function.Builtins {
+		adjustedRetOffset += 1
+		_, ok := builtinsOffsetsMap[builtin]
+		if ok {
+			builtinsOffsetsMap[builtin] = adjustedRetOffset
+		} else {
+			continue
+		}
+	}


Suggested change

for _, builtin := range function.Builtins {

adjustedRetOffset += 1

_, ok := builtinsOffsetsMap[builtin]

if ok {

builtinsOffsetsMap[builtin] = adjustedRetOffset

} else {

continue

}

}

for _, builtin := range function.Builtins {

adjustedRetOffset += 1

if _, ok := builtinsOffsetsMap[builtin]; ok {

builtinsOffsetsMap[builtin] = adjustedRetOffset

}

}

cicr99 · 2025-02-11T20:37:38Z

pkg/runner/runner.go

-	ctx.AddInlineCASM(fmt.Sprintf("call rel %d; ret;", int(totalSize)))
-	return ctx.instructions, hints, nil
+	ctx.instructions[callRelArgLocation] = new(fp.Element).SetUint64(uint64(ctx.currentCodeOffset) + codeOffsetBeforeCallRel)
+	return ctx.instructions, hints, ctx.currentCodeOffset, programBuiltins, gotGasBuiltin, gotSegmentArena, nil


Is this function returning an error ever?

cicr99 · 2025-02-11T20:45:03Z

pkg/runner/runner.go

+	// Check if return type is either:
+	// 1. PanicResult with inner type of Array<felt252> with size 2
+	// 2. Array<felt252> with size 2
+	isPanicResultFeltArray := false
+	if strings.Contains(mainFunc.ReturnArgs[0].DebugName, "core::panics::PanicResult::") {
+		isPanicResultFeltArray = strings.Contains(mainFunc.ReturnArgs[0].PanicInnerType.DebugName, "Array<felt252>")
+	}
+	isFeltArray := mainFunc.ReturnArgs[0].DebugName == "Array<felt252>" &&
+		mainFunc.ReturnArgs[0].Size == 2
+
+	if !isPanicResultFeltArray && !isFeltArray {
+		return fmt.Errorf("main function return argument should be either PanicResult of size 3 or Felt Array of size 2")
+	}


I think there is a mismatch between the comment point 1 for PanicResult and the error at the end regarding the size: in the first case it says 2 but in the error it says 3, what's the correct one?

cicr99 · 2025-02-11T20:45:21Z

pkg/runner/runner.go

+	// 2. Array<felt252> with size 2
+	isPanicResultFeltArray := false
+	if strings.Contains(mainFunc.ReturnArgs[0].DebugName, "core::panics::PanicResult::") {
+		isPanicResultFeltArray = strings.Contains(mainFunc.ReturnArgs[0].PanicInnerType.DebugName, "Array<felt252>")


Don't you have to check for the size here?

MaksymMalicki and others added 25 commits November 15, 2024 16:16

Fixes for the generation of entry code, fixes of hints parsing

d7b74e6

Add modifications to the runner

dfd9ec8

Merge branch 'main' into add_code_entry

a7814a1

Add fixes for the entrycode generation

83872d8

Refactor main CLI, offset the hints indexes by entry code size, load …

cd04099

…arguments and initial gas to the memory

Fix the integration tests

8812042

Fixes in the runner

78cf788

Fixes in the runner

af47a3d

Fix the unit tests, uncomment pythonVm execution in integration tests…

73f6dd5

…, code cleanups

Add writing tokens gas cost to memory

17b17df

Proper builtins initialization for cairo mode

688b3d2

Address comments in the PR

f105506

Fix bugs regarding dicts

cc82938

Remove prints

a32add7

Merge branch 'main' into missing_dict_functionalities

07f4016

Fixes of the last tests for the dicts

53d4190

Add dict_non_squashed dir to the integration tests

31aeeed

Add checks for the matching args size, rename files, modify the integ…

086011a

…ration tests pipeline

Almost all pass

fc09b66

Fix lint and unit tests

72b3c14

Fix loading gas to the memory as an argument

ce93d4f

Fix run for tensor__small.cairo

734d1e7

Merge branch 'main' into inputs

b54e9bb

check for input and output types for proofmode

c511329

MaksymMalicki marked this pull request as draft January 17, 2025 11:43

Base automatically changed from inputs to main January 22, 2025 16:04

MaksymMalicki and others added 3 commits January 23, 2025 15:21

Implementation for entry code

74c2110

Implement inline casm for segment arena

6883060

Merge branch 'main' into proof_mode

b757a2e

MaksymMalicki added 7 commits January 23, 2025 19:51

Lint the project

8d50465

remove system type

78d3bbc

Modifications for the runner

eeef9a2

Add relocate all dictionaries hint

b1a2881

allocate empty segment

4fc752b

Implement dict relocation

6a71d5d

Lint project, add hint to the entry code

71d2c33

MaksymMalicki marked this pull request as ready for review January 29, 2025 18:32

MaksymMalicki and others added 5 commits January 31, 2025 12:30

Merge branch 'main' into proof_mode

6504516

Fixes in the runner for the proofmode

bd040d8

enable tests

8793737

adjust the builtins offsets by ret types

b5eff91

Merge branch 'main' into proof_mode

bcc6003

cicr99 reviewed Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `proof_mode` functionalities #682

Implement `proof_mode` functionalities #682

MaksymMalicki commented Jan 17, 2025

cicr99 Feb 11, 2025

cicr99 Feb 11, 2025

cicr99 Feb 11, 2025

cicr99 Feb 11, 2025

cicr99 Feb 11, 2025

cicr99 Feb 11, 2025

cicr99 Feb 11, 2025

cicr99 Feb 11, 2025

cicr99 Feb 11, 2025

cicr99 Feb 11, 2025

cicr99 Feb 11, 2025

Implement proof_mode functionalities #682

Are you sure you want to change the base?

Implement proof_mode functionalities #682

Conversation

MaksymMalicki commented Jan 17, 2025

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

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

Choose a reason for hiding this comment

Implement `proof_mode` functionalities #682

Implement `proof_mode` functionalities #682