-
Notifications
You must be signed in to change notification settings - Fork 163
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
Optimise field arithmetic in BaseUltraVerifier.sol #2636
Comments
Hey I would like to help this issue if it is still available. @zac-williamson |
I am currently dealing with this issue. But I have a question regarding to
or
in that case they exceed bn254 field. public inputs are bytes32 so they can be up to 2^256 and proof is bytes so it can be also up to 2^256. Am i missing something. Thanks. |
Hey! Really appreciate you picking up this issue! I'll have to double check on the edge case front, ill follow up if there are any to be aware of, however i can answet the second part; public inputs although bytes32, are checked to be less than p (254 bits) on line 586
each element within the proof is forced to be mod q ( for elliptic curve points ) see an example here:
or p ( for points in the scalar field ) see here:
for the non public inputs the rationale being that if they are greater than the modulus, the rest of the verifier will fail ( so we just force them to be in range). |
Hey thanks for your response. It makes sense. |
Hey I made some changes about
As you can see Recursive test is giving 2 errors before running correctly. It also same without any changes. Is it normal and is there anything i can do about this problem? |
ah very nice work! These are just debug outputs to std err, and are nothing to worry about! |
I opened a PR with necessary changes. After removing duplicate |
I was running tests to make sure everything is working correctly. But i received
How can i reproduce the error with same inputs. I tried to hard code in contract. But it didn't work.
|
The failure depends on both the inputs and the proof, so if you are rerunning it, it could simply be the proof that differs from the old one making it pass the case it hit earlier. |
How can i also print Proof on error? It hard to encounter with this error on random testing. I should reproduce it. |
I fixed the problem and updated the PR. |
Much of the verification field arithmetic in BaseUltraVerifier.sol can be optimised.
There is currently a lot of duplication of MLOAD operations, we did this so that the code is straightforward to read and reason about, but it is more expensive.
We also have redundant uses of
addmod
. The BN254 field size is <2^254, meaning that we can calladd
up to four times before we need to reduce modulop
. This could save some more gas.Overall gas savings would be very small, ~2,000 tops. Not worth prioritising but could be good for a newcomer who knows Yul and wants to make a contribution to the codebase.
The text was updated successfully, but these errors were encountered: