-
Notifications
You must be signed in to change notification settings - Fork 144
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
Serialization #176
Comments
@adria0 Here is my idea about your proposal:
I think this
Overall, I think we first need documentation. |
Thanks @adria0 for the in depth description of the issue! I think we should tackle fields and curve serialization separately: For curves:
For fields, let's stick to prime fields only for now ( field extensions can be represented in multiple ways and that is a bigger problem that exceeds the scope of this issue).
I can't see why this kind of access needs to be exposed publicly. Specially when Montgomery/non-Montgomery form introduces
Lastly, I agree 100% with @guorong009 remark about documentation. Whichever design we land on, we should make an effort to have it well documented to prevent more confusion. |
Round two, proposed changes
documentationField Encodings
Curve Encodings
Notes:
|
Related #39 |
I tried to new version of the serialization where |
I think that implementing this helper in halo2curves, just adds more confusion, a library only has to provide one way to do the same thing. I also question, why does this helper exists? It's just a simple wrapper. Exists maybe because halo2curves serialization was not really documented and was a way for implementer to clarify what can be done in halo2curves? I do not see this usefull in Halo2 in terms of cryptographic protocol. |
@adria0 @davidnevadoc I believe we should check why @jonathanpwang added these. |
The reason they were added is that previously the default in So I haven't been up to date on the current situation, so I can't tell if they are needed anymore, but this was the original motivation. Edit: |
quick question: I see discussion of |
Yes, it is. @srinathsetty |
from @davidnevadoc
Related issue: #109
First round, initial idea
Halo2curves serializations comes with two flavours:
Standard serialization
Field.{from,to}_repr
fngroup::GroupEncoding (for compressed)
, andgroup::UncompresseEncoding
(for uncompressed), allowing "unchecked" reads.endian
infield_impl!
macroEndianRepr
for endianess, implCompressed
for flags and to uncompress.Vainilla "raw" serialization
{from, to}_raw_bytes_[unchecked]
{read,write}_raw_[unchecked]
Others
Notes:
EndianRepr
but it seems like some kind of type-hack to allow to knowing the endiannes of CurveAffine::BASE when encoding compressing curves. Since our halo2curves takes traits from pasta, we are not able to modify it.Unserialize performance:
legend:
code: https://gist.github.com/adria0/c440185d765a368aaf21ca5741a63ab7
Initial proposal:
EndianRepr::from_bytes
,EndianRepr::to_bytes
is implemented in Field (that aready have these fields). This is extra confusing, these are just intended to be used internally as helpers. So rename this fields for something like endianrepr_{from,to}_bytesComments: @davidnevadoc @duguorong009 @kilic ?
The text was updated successfully, but these errors were encountered: