feat: A0-A4 - LOG0-4 Opcode

[Quentash]

Implementation of the LOG0, LOG1, LOG2, LOG3, LOG4 opcode.

Pull Request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build-related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Resolves: #296 #297 #298 #299 #300

What is the new behavior?

Does this introduce a breaking change?

• Yes
• No

[Quentash]

I made an early push to talk about a gas issue I have been facing. TL;DR : load_n() use a lot of gaz even for small byte size. I have confronted 2 solutions against my tests, here is the one using load_n() to retrieve the datas:

        let mut datas: Array<felt252> = ArrayTrait::new();

        let mut loaded: Array<u8> = ArrayTrait::new();
        self.memory.load_n(size, ref loaded, offset);
        let mut element: felt252 = 0;
        let mut i = 0;
        loop {
            if i == size {
                break;
            };
            element *= 256;
            element += (*loaded[i]).into();
            
            i+=1;
            if i%31 == 0 {
                datas.append(element);
                element = 0;
            };
        };
        if size%31 != 0 {
            datas.append(element);
        }

And here is the test output :

test_exec_log0 ... ok (gas usage est.: 2252000)
test_exec_log1 ... ok (gas usage est.: 2336270)
test_exec_log2 ... fail (gas usage est.: 1999965750) ( augmented the test limit but still OOG )
test_exec_log3_and_log4 ... fail (gas usage est.: 1999929590) ( augmented the test limit but still OOG )

The other solution is the one in the pushed file.
It gives this output :

test_exec_log0 ... ok (gas usage est.: 887710)
test_exec_log1 ... ok (gas usage est.: 1988326)
test_exec_log2 ... ok (gas usage est.: 1850076)
test_exec_log3_and_log4 ... ok (gas usage est.: 4058812)

It seems that loading the memory by 32 bytes chunk multiple times is more efficient than load N bytes in once.

EDIT : I just noticed log2 fails for what appear to be an infinite loop although it only needs to read 5 bytes.

[enitrat]

Ok my bad I didn't see the comment you added on the VSCode UI 😛

If load_n uses more gas that doing load and then splitting bytes, then we should refactor load_n.

[enitrat]

I made an early push to talk about a gas issue I have been facing. TL;DR : load_n() use a lot of gaz even for small byte size. I have confronted 2 solutions against my tests, here is the one using load_n() to retrieve the data:

What you're trying to do is to load felts from memory, right?

[Quentash]

I made an early push to talk about a gas issue I have been facing. TL;DR : load_n() use a lot of gaz even for small byte size. I have confronted 2 solutions against my tests, here is the one using load_n() to retrieve the data:

What you're trying to do is to load felts from memory, right?

Yes, I need to fill a data of felt252.
I will push the 2 solutions I have ( load and loadn ) so you can see if I'm doing something wrong that would alterate the gas cost, but for my usage, loadn() seems to be havier than load().

[Quentash]

The last commit ( when writing this ) contains 2 functions in logging_operations.cairo.
One using an algorithm that will load chunks of 32 bytes, format it to keep the 31 required bytes in a felt252 that is then appended to the data array.

One using an algorithm that will load N Bytes and then concat them into a felt that will be appended in the data array.

Something I don't understand tho, is why the "read_only" test fails with 200k more gas although the revert is at the beginning of the function. ( And therefore shouldn't call any of the 2 functions )

I'm well aware my code still contains stuff to optimize (those 2 functions for example) and I would like to add test for the internal function we will keep and for the set_event one added in context.
My internet just comes and goes so I felt like pushing this now so you may look at it too.

[enitrat]

tbh I think rebase is better than merge when bringing remote changes into a PR

[Eikix]

tbh I think rebase is better than merge when bringing remote changes into a PR

some merge are merge.rebase afaik, idk how gh handles those

[enitrat]

Hey @Quentash sorry for the big rebase, we brought a lot of changes to the internal structure. When you have time you can rebase and then we'll merge your PR 💪

[Quentash]

Updated the code to fit the new rebase.
I'm just not very convinced by my "set_read_only" so if you'd mind double-checking it. Am I doing what I am supposed to do or am I overkilling something ?

[Quentash]

• Removed the set_read_only() function and added a setup_machine_with_read_only(). Which actually kind of duplicate a lot of code, I didn't find a way to give functions parameters a default value, is it possible in Cairo ? If yes, maybe refactoring the setup_machine() with a read_only paramater set to false by default would be a cleaner solution ?

• Duplicated the append_event() function in Machine from the ExecutionContext and made it use the execution context one.

• Left the error as StateModificationError as it was used in Kakarot0 as I find it pretty explicit. Tell me if you want me to change it to WriteInStaticContext for the sake of consistency with EELS.

[enitrat]

I didn't find a way to give functions parameters a default value, is it possible in Cairo ?

I don't think so. but what you can do is something like

fn add(a: Option<i32>, b: Option<i32>) -> i32 {
    a.unwrap_or(1) + b.unwrap_or(2)
}

which is equivalent to

fn add(a: int = 1, b: int = 2) { a + b }

on a side note unwrap_or is not implemented in OptionTrait. so you would need to write your own custom impl of it before it's merged in Cairo

[Eikix]

I didn't find a way to give functions parameters a default value, is it possible in Cairo ?

I don't think so. but what you can do is something like

fn add(a: Option<i32>, b: Option<i32>) -> i32 {
    a.unwrap_or(1) + b.unwrap_or(2)
}

which is equivalent to

fn add(a: int = 1, b: int = 2) { a + b }

on a side note unwrap_or is not implemented in OptionTrait. so you would need to write your own custom impl of it before it's merged in Cairo

Opened a feature request for Cairo corelib

starkware-libs/cairo#4196

[Quentash]

• Modified the error message to be consistent with EELS one.
• Updated the setup_machine_with_read_only() so that it changes the read_only variable after setting up a normal machine instead of duplicating a lot of code just to create a call_context with read_only set to true.

[Eikix]

lgtm 🔥

[Eikix]

lgtm