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

Limit input for _to_biguint56_ in _primefield_ #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

skaunov
Copy link
Contributor

@skaunov skaunov commented Nov 12, 2023

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

@shuklaayush
Copy link
Owner

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
Copy link
Contributor Author

skaunov commented Dec 2, 2023

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
Copy link
Contributor Author

skaunov commented Dec 2, 2023

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
Copy link
Contributor Author

skaunov commented Dec 2, 2023

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)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants