Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

fix rws padding logic and error handling #1515

Merged
merged 2 commits into from
Jul 6, 2023
Merged

Conversation

hero78119
Copy link
Member

@hero78119 hero78119 commented Jul 5, 2023

Description

Fixed rws padding logic and padding error handling.

Issue Link

#1507

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Contents

  • more documentation on padding logic
  • fix max_rws calculation
  • de-duplicated startop in end_block.tx by skip second one.
  • panic instead of confused error log in padding calculation function.

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jul 5, 2023
Copy link
Contributor

@davidnevadoc davidnevadoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this! The comments and new variables make the calculations around the number of rows much easier to follow :)
LGTM 👍

let padding_length = RwMap::padding_len(self.rows.len(), self.n_rows);
let padding_length = if self.rows.len() < self.n_rows {
RwMap::padding_len(self.rows.len(), self.n_rows)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a small question here:
Is there a valid scenario where we enter this branch? In other words, is it ok to have self.rows.len() == self.n_rows in some case?
I believe this is the situation where the log error was being triggered but the circuits were still correct.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y here is exactly the case self.rows.len() == self.n_rows Because state_circuit receive RwMap which is padding ready, also we set self.n_rows == FixedCParams.max_rws that's why here is strictly equal.

Besides, the result of padding_length is not used for padding here. It's for override, because override idx is counting after padding rows.

I will rename padding_length to first_non_padding_index instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to have better naming in commit 6d5fced

@hero78119 hero78119 requested a review from ed255 July 5, 2023 13:29
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The comments in the code are very useful to explain how the extra rows are calculated :)

@hero78119 hero78119 added this pull request to the merge queue Jul 6, 2023
Merged via the queue into main with commit 515a2bc Jul 6, 2023
@hero78119 hero78119 deleted the feat/fix_rws_padding branch July 6, 2023 01:24
@ChihChengLiang ChihChengLiang linked an issue Jul 7, 2023 that may be closed by this pull request
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate error log on ci light-test padding_len overflow
3 participants