Refactor bytecode circuit

[leolara]

Closes #151

[andyguzmaneth]

@Brechtpd can you assign a reviewer to this task?

[Brechtpd]

This doesn't seem to check that length and hash are set to the correct empty values (the to_header checks only check the current row values, and these checks are disabled on this row), so maybe only rows on not(q_last) be considered valid for lookup?

[leolara]

@Brechtpd

So perhaps we could have at the beginning:

if cur.q_first == 1 || cur.q_last== 1:
        assert cur.tag == BytecodeFieldTag.Header

And then apply the main rules to every row including last? That way when it loops from last to first it will check that last is empty?

Is this the right approach in your oponion?

Or how do we express that q_last is not valid for lookup?

[Brechtpd]

I think the approach you propose here would work perfectly if not for unusable rows. But because we still have unusable rows (because zk is enabled) this wrap around cannot be exploited easily (there are rows with random data in between the first and last rows). I think just having a q_enabled column in the circuit that is 1 on all rows with valid data (which with the current code would exclude the q_last row) would be the standard way to do it. So I would say the current code works, but just have to be careful to disable lookups on the q_last row by disabling q_enabled there, so just adding a warning in the spec I think is okay.

[han0110]

I think it'd be easier to to just make sure the length = 0 and hash = EMPTY_HASH when q_last, otherwise it seems we need to add extra computation to the table.

[leolara]

@han0110 @Brechtpd as the last thing that Han said is exactly the header to header check, I did that the last row does that check, as that check does not use the next row it should be ok.

[Brechtpd]

I think it'd be easier to to just make sure the length = 0 and hash = EMPTY_HASH when q_last, otherwise it seems we need to add extra computation to the table.

Yeah I guess the lookup table is safe to use without any additional selector as long as we ensure q_last is set to 1 at the very last usable row.

[Brechtpd]

Should it? I thought this was currently allowed but I could be wrong.

[leolara]

Is it allowed? Is this because we should allow invalid bytecode to run in the EVM as it would? The EVM would rise an exception and revert the transaction?

In that case perhaps we need some changes to the spec.

[Brechtpd]

As far as I know this currently is allowed, future EIPs like EIP-3670 may introduce these kinds of checks but I don't think any checks (except on total bytecode size) are currently being done.

Executing this kind of code will result in a revert (which is totally valid to do), but the bytecode of a smart contract may also just just contain sections that are never used by transactions but still needs to be loaded in.

[han0110]

I think push without data should be allowed and it's will be padded with n zero when EVM really executes this PUSHn.

The reason this case fails here is because there will be next_push_data_left unassigned in the padding rows, we might need to assign it for the first padding row to pass the assert next.push_data_left == cur.push_data_size

[leolara]

@han0110 @Brechtpd what I have done (that it seemed more semantic to me but perhaps there is something wrong) is to only assert assert next.push_data_left == when we are in a Byte -> Byte transition, please let me know if there is something wrong with this approach.

[han0110]

This approach also looks much better to me!

[andyguzmaneth]

Thanks for the review! FYI @leolara

[han0110]

The refactor looks really great and codes look well organized! Nice work! Only left 2 comments that might need to be addressed.

[han0110]

I think it'd be easier to to just make sure the length = 0 and hash = EMPTY_HASH when q_last, otherwise it seems we need to add extra computation to the table.

[han0110]

I think push without data should be allowed and it's will be padded with n zero when EVM really executes this PUSHn.

The reason this case fails here is because there will be next_push_data_left unassigned in the padding rows, we might need to assign it for the first padding row to pass the assert next.push_data_left == cur.push_data_size

[leolara]

@han0110 @Brechtpd please check again :-)

[han0110]

All LGTM now!

[Brechtpd]

LGTM, great work!

[leolara]

Thanks @han0110 @Brechtpd @ed255