shuklaayush / noir-bigint

BigInt library for Aztec's noir language
MIT License
30 stars 11 forks source link

Limit input for _to_biguint56_ in _primefield_ #16

Open skaunov opened 10 months ago

skaunov commented 10 months ago

Could you review this one, pls. Am I correct that starting from the modulus val this function starts to produce nonsense?

shuklaayush commented 10 months ago

Not sure why we need this check since we're assuming that self.val will always be less than the modulus i.e. the Fp elements will always be in canonical form

skaunov commented 10 months ago

I will try to remember how did I hit this one, but I actually did.

By best bet is that I was trying to use it "internally", i.e. modifying the behavior of the lib functions and using this one during computations.

Yeah, it's kind of unclear how could it be useful now, but there's a lot of notes about trait and when they're introduced more people can be bumped into this with aforementioned scenario. Also currently it's impossible to protect from instantiating the thing directly with any val via struct notation; and that assuming is implicit (at least I after spending sometime with the lib can't recall it's stated somewhere). So I guess it's very small expenditure of constraints which can prevent some mistakes and makes that assume quite explicit.

skaunov commented 10 months ago

Hm... After some remembering it seems to me that personally I wanted to get the modulus value this way. Can't recollect yet exact scenario, but I needed the modulus value, and my first try was using this one (not sure I will remember what I had on my hands as the argument to the function). While debugging it became clear that I need to get the modulus another way, but this assert would indicate the problem much faster. X)

skaunov commented 10 months ago

Sorry for a lot of comments... Just still thinking about your point/question. I guess it even can be seen as under-constraining since it actually allows that path when the function is fed with a manually constructed PrimeField and returns quite unexpected result. I'm absolutely agree that developer is expected not to use it that way, but if someone do, then it could end as a cute exercise in a future "capture the Aztec" course. X)