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

Fix ecrecover input rlc comparison (right-padding zeroes) #585

Merged
merged 5 commits into from
Jul 4, 2023

Conversation

roynalnaruto
Copy link

Description

Input RLC comparison in precompiles ecrecover, ecAdd, ecMul` can fail as inputs are right-padded with zeroes. Since those zeroes were not accounted for in the copy circuit, the RLC value was incorrect.

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

Rationale

We add a lookup table PowOfRand which represents the below table:

exponent power of randomness
0 1
1 r
2 r ^ 2
... ...
126 r ^ 126
127 r ^ 127

where r represents the keccak randomness (which is used as the randomness to calculate RLC of input bytes in the copy circuit).

The precompiles ecrecover, ecAdd and ecMul expect 128, 128 and 96 input bytes respectively, but if the calldata does not have sufficient number of bytes, zero bytes are padded to the right before running the precompile computation.

We use the following approach:

// no. of right padded zeroes is the difference between required input length and calldata length.
let n_padded_zeroes = input_len - calldata_len;
cb.range_lookup(n_padded_zeroes.expr(), 128);

// cell to hold the RLC value with padded zeroes.
let padded_input_rlc = cb.query_cell_phase2();

// get the power of randomness we are interested in.
let power_of_rand = cb.query_cell_phase2();
cb.lookup_pow_of_rand(n_padded_zeroes.expr(), power_of_rand.expr());

// validate value of padded RLC.
cb.require_equal("rlc eq", padded_input_rlc.expr(), input_rlc.expr() * power_of_rand.expr());

@@ -57,22 +91,22 @@ impl<F: Field> PrecompileGadget<F> {
};
cb.require_equal(
"input bytes (RLC) = [msg_hash | sig_v | sig_r | sig_s]",
input_bytes_rlc.expr(),
padding_gadget.padded_rlc(),
Copy link

Choose a reason for hiding this comment

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

a bit concern. when cb.condition(pad_right.expr() is false, i think the constraints inside PaddingGadget is all disabled. so there are no constraints to enforce relationship between padding_gadget.padded_rlc and input_bytes_rlc/cd_length/input_len when pad_right == false.

Copy link
Author

Choose a reason for hiding this comment

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

That's a very good catch. I will handle input_bytes_rlc == padded_rlc if pad_right == false

@lispc
Copy link

lispc commented Jul 3, 2023

i think cb.pow_of_rand_lookup(0, 0) is valid (of course it is not part of the first 128 rows we assigned but following rows, default value there is all 0, so pow_of_rand_lookup(0, 0) is valid).

Will this behavior be ok for soundness? if ok, then we can add some comments above the pow_of_rand_table definition, if not ok, we can add the "fixed" q_enable col.

ref:
privacy-scaling-explorations#866
#471

@roynalnaruto roynalnaruto requested a review from lispc July 3, 2023 14:18
@lispc lispc merged commit 1d909da into feat/ecrecover Jul 4, 2023
@lispc lispc deleted the fix/ecrecover-input-rlc branch July 4, 2023 09:59
roynalnaruto added a commit that referenced this pull request Jul 4, 2023
* feat: preliminary work

* wip

* finish assignment to ecrecover

* fix: input length may be different than call data length (for precompiles)

* fix: input bytes to ecrecover

* minor edits

* Fix ecrecover input rlc comparison (right-padding zeroes) (#585)

* potential approach (pow of rand lookup)

* add lookup for pow of rand

* fix: right pad only if needed

* fix: missing constraint on padded_rlc

* constrain pow of rand table

* Update step.rs

* fix: sig_v sanity check (remove assertions to allow garbage input)

* fix: calldata length == 0 handled

* chore: renaming precompile_* and parallel iter for tests

---------

Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants