Add segment_arena_builtin to the program builtins.

[noaov1]

Use initialize_function_runner_cairo_1 (which uses initialize_all_builtins. It is based on CairoFunctionRunner).

This change is 

[noaov1]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion

---

crates/blockifier/src/execution/contract_class.rs line 246 at r1 (raw file):

            BuiltinName::ec_op,
            BuiltinName::poseidon,
        ];

We initialize the program builtins in initialize_function_runner_cairo_1.

Code quote:

        // Initialize program with all builtins.
        let builtins = vec![
            BuiltinName::output,
            BuiltinName::pedersen,
            BuiltinName::range_check,
            BuiltinName::ecdsa,
            BuiltinName::bitwise,
            BuiltinName::ec_op,
            BuiltinName::poseidon,
        ];

[elintul]

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alonh5 and @noaov1)

---

crates/blockifier/src/execution/cairo1_execution.rs line 123 at r2 (raw file):

    ];

    runner.initialize_function_runner_cairo_1(&mut vm, &program_builtins)?;

Remove blank line above.

Code quote:

runner.initialize_function_runner_cairo_1(&mut vm, &program_builtins)?;

---

crates/blockifier/src/execution/cairo1_execution.rs line 123 at r2 (raw file):

    ];

    runner.initialize_function_runner_cairo_1(&mut vm, &program_builtins)?;

Is it necessary? I see it is done anyway. It's okay to be explicit, though. If you keep it, please alphabetize the builtins.

Code quote:

&program_builtins

---

crates/blockifier/src/execution/contract_class.rs line 244 at r2 (raw file):

        let program = Program::new(
            vec![],

Give this a name, and add an inline comment that the builtins are initialized later.
It's weird there are two decoupled flows to do the same thing...

Code quote:

vec![]

---

crates/blockifier/src/transaction/transaction_utils.rs line 68 at r2 (raw file):

    // "segment_arena" built-in is not a SHARP built-in - i.e., it is not part of any proof layout.
    // Each instance requires approximately 10 steps in the OS.
    // TODO(Noa, 01/07/23): Verify the removal of the segmen_arena builtin.

WDYM?

Code quote:

Verify the removal of the segmen_arena builtin.

[noaov1]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5 and @elintul)

---

crates/blockifier/src/execution/cairo1_execution.rs line 123 at r2 (raw file):

Previously, elintul (Elin) wrote…

Is it necessary? I see it is done anyway. It's okay to be explicit, though. If you keep it, please alphabetize the builtins.

You are right. They are missing the segmen_arena though. I chose to initialize with all the builtins until I figure out the content of starknet_preset_builtins.
Also, I'm not sure whether this field will be used later by LC (currently it is not used). I asked them.
Done.

---

crates/blockifier/src/execution/contract_class.rs line 244 at r2 (raw file):

Previously, elintul (Elin) wrote…

Give this a name, and add an inline comment that the builtins are initialized later.
It's weird there are two decoupled flows to do the same thing...

Done. And I agree.

---

crates/blockifier/src/transaction/transaction_utils.rs line 68 at r2 (raw file):

Previously, elintul (Elin) wrote…

WDYM?

I want to create an invoke transaction that "triggers" the segment arena builtin and to make sure it is not contained in the actual resources.

[elintul]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @alonh5 and @noaov1)

---

crates/blockifier/src/execution/cairo1_execution.rs line 123 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

You are right. They are missing the segmen_arena though. I chose to initialize with all the builtins until I figure out the content of starknet_preset_builtins.
Also, I'm not sure whether this field will be used later by LC (currently it is not used). I asked them.
Done.

Thanks!
Also, let's add keccak there on main; once this (and the merge to main) is merged.

---

crates/blockifier/src/execution/cairo1_execution.rs line 115 at r3 (raw file):

        BuiltinName::bitwise,
        BuiltinName::ecdsa,
        BuiltinName::ec_op,

Suggestion:

        BuiltinName::ec_op,
        BuiltinName::ecdsa,
        

---

crates/blockifier/src/execution/contract_class.rs line 244 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Done. And I agree.

Worth dropping this Q on the channel too.

---

crates/blockifier/src/transaction/transaction_utils.rs line 68 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I want to create an invoke transaction that "triggers" the segment arena builtin and to make sure it is not contained in the actual resources.

Why shouldn't it be?
I'm not familiar with this builtin.

[noaov1]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @elintul)

---

crates/blockifier/src/transaction/transaction_utils.rs line 68 at r2 (raw file):

Previously, elintul (Elin) wrote…

Why shouldn't it be?
I'm not familiar with this builtin.

It is not a part of any proof layout. We added it as a built-in (invoked when creating a new dict) to easily estimate its resources. Therefore we remove it from the actual resources not before we consider its counter in the n_steps field.

[elintul]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @noaov1)

---

crates/blockifier/src/transaction/transaction_utils.rs line 68 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

It is not a part of any proof layout. We added it as a built-in (invoked when creating a new dict) to easily estimate its resources. Therefore we remove it from the actual resources not before we consider its counter in the n_steps field.

Got it, thanks.