Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle base conversion nicer #281

Merged
merged 2 commits into from
Jan 14, 2022
Merged

Handle base conversion nicer #281

merged 2 commits into from
Jan 14, 2022

Conversation

ChihChengLiang
Copy link
Collaborator

@ChihChengLiang ChihChengLiang commented Jan 14, 2022

Problem

The current codebase uses many patterns that are difficult to reason and debug.

For example,

pattern 1

b13_chunks.iter().fold(0, |acc, x| { acc * B9 + convert_b13_coef(*x)})

This requires b13_chunks to be in big endianness, but a reader might not be aware of it.

Solution: We collect them in the same function f_from_radix_be so that we have to reason only once. And from the function name, we know the input must be in big-endian.

pattern 2

We do a lot of manual base conversion like

    for _ in 0..64 {
        let remainder: u64 = mod_u64(&raw, B9);
        acc += coef_transform(remainder) * base.clone();
        raw /= from_base;
        base *= to_base;
    }

Solution: The bigUint has an expressive to_radix_be API to allow us to perform base conversion easily.

@github-actions github-actions bot added the crate-keccak Issues related to the keccak workspace member label Jan 14, 2022
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for this. As we discussed will make us easier to debug the error in #268

@ChihChengLiang ChihChengLiang merged commit 0230b6a into main Jan 14, 2022
@CPerezz CPerezz deleted the handle-lane-nicer branch November 30, 2022 11:58
lispc pushed a commit to zhenfeizhang/zkevm-circuits that referenced this pull request Feb 1, 2023
…explorations#281)

* Add `SECRET_REPO_DEPLOY_KEY` to CI.

* Revert `Cargo.toml`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate-keccak Issues related to the keccak workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants