Feat/#1474 rw lookup index error

[KimiWu123]

Description

address #1474 and also found #1483 when investigating this issue.

Issue Link

#1474

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Contents

• The root cause is that there are memory operations in block.get_rws() but there is only single copy lookup expression while copy table lookup happens in opcode execution. Which makes the indexes of assigned_rw_values and step.bus_mapping_instance are inconsistent.
• Add a new function get_rws_for_check_rw_lookup specific for check_rw_lookup. This function could bypass memory operations if any copy table lookup happens, and it makes the indexes of assigned_rw_values and step.bus_mapping_instance consistent.

Rationale

• Add a new function instead of modifying get_rws() is because trying to minimize the impact to our codebase since this is a check function.

[hero78119]

Just notice it's in draft, but already review few so the comments is published first, will have another round once it's ready

[hero78119]

Hope I understand this function correctly 😃 (it will be nice to document it more) I think another option is having get_next_non_memoryrw_operef and make skip_memory_rw boolean logic check outside, which I think somehow make this util function dedicate to its functionality (and easier to read).

However, a bit worry about this stateful design. If there are other normal memory rw operation sequence consecutively happen after the memory rw trigger by copy constraints, will this function break and skip them?

[KimiWu123]

I think another option is having get_next_non_memoryrw_operef and make skip_memory_rw boolean logic check outside,

it's a good idea!

However, a bit worry about this stateful design. If there are other normal memory rw operation sequence consecutively happen after the memory rw trigger by copy constraints, will this function break and skip them?

yes, I had the same concern either but after checking our code, only calldataload and memory have memory rw operations and no copy lookup inside. So, it should be fine now. But I'll figuire out other possiblilities.

[ChihChengLiang]

LGTM. I can confirm that the command in #1474 no longer logs errors.
Great work fixing the logs. It must take you a lot of effort to track this bug.

We should do a systematic refactor for check_rw_lookup to make the logging code easier to maintain.

[hero78119]

Last time discussed with @KimiWu123 . Right now this change only able to handle pretty special case: only 1 copy event. I think better way to fix is able to get real rw counter assigned value in check phase. To do that, it need some interface change, where the scope will be larger. Here I have some preliminary trying, 770f6cf Just record here so later we can FYI

Its ok to keep this pr to only fix single copy event issue. Just we need to have proper TODO in comment, and warn that once there are > 1 copy events happened then this check is skip

[KimiWu123]

Refactor for check_rw_lookup created, #1489, cc @ChihChengLiang and @hero78119

[hero78119]

LGTM!