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

add basic serde support for Scalar, G1, G2 with human readable encoding #125

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

Conversation

joschisan
Copy link

@joschisan joschisan commented Mar 10, 2024

We use this library in https://github.com/fedimint/fedimint quite extensively to implement our own threshold cryptography schemes. So far we have manually implemented the serde serialisation for struct that contain bls12_381 scalars or points since we can not derive them. However, we find it increasingly cumbersome and error prone to do so as the implemented schemes get more complex, like amount blinded chaumian ecash or distributed key generation, for example. Therefore we would now like to upstream this improved version of the serialisation we currently use, as we want to built on bls12_381 long term. Please let us know what would be necessary to get this merged as we want to avoid to have to fork.

We serialise using the serdect crate for best efforts constant time serialisation. In the human readable case the types are serialised as hex.

Copy link

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

Since binary encoding is implemented it should also be tested. You could either use bincode (hacky but should work) or use serde_test. By using the latter the dev dependency on serde_json could also be avoided.

.idea/.gitignore Outdated
@@ -0,0 +1,8 @@
# Default ignored files

Choose a reason for hiding this comment

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

IDE-specific files should not be checked in.

@joschisan joschisan force-pushed the serde branch 2 times, most recently from 4b8ce2a to bdb9368 Compare March 29, 2024 06:15
@elsirion
Copy link

@tarcieri left a great pointer to serdect, which seems very useful, on a previous commit version: c2e0175#r140296582

@joschisan joschisan force-pushed the serde branch 3 times, most recently from e465652 to 62b8bc1 Compare March 30, 2024 08:57
@elsirion
Copy link

elsirion commented Apr 1, 2024

@str4d what can we do to progress this PR? I see that #61 was stalled, supposedly on human readable encoding, which is explicitly part of this one.

The use of serdect also makes this implementation much more concise. While it is an additional dependency, it has few transitive dependencies (all serde or crypto related) and is maintained by @tarcieri, who is well-known.

Comment on lines +72 to +73
[dependencies.serdect]
version = "0.2.0"
Copy link

Choose a reason for hiding this comment

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

One important caveat on serdect is we revised the format in the v0.3.0-pre.0 prereleases, using serialize_bytes/deserialize_bytes which unfortunately adds a length header in the binary encodings even when using fixed-sized arrays but provides more compact serialization on formats like MessagePack which are more likely to be constant-time-ish, which is an unfortunate tradeoff where we opted for broader constant time assuredness over a minimally compact encoding.

Barring any complaints about the length header, we just need to fix a bug/regression on v0.3.0 in the slice encoder and we can ship a final release. I would probably suggest using that (or convincing us to roll back) to avoid future breakages in the encoding format.

Copy link
Author

Choose a reason for hiding this comment

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

When would you expect to ship v0.3.0?

Choose a reason for hiding this comment

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

This regression needs to be fixed first: RustCrypto/formats#1322

Choose a reason for hiding this comment

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

Given this regression has already been fixed (and relevant serdect release published), is there a chance to get this PR moving again?

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

Successfully merging this pull request may close these issues.

4 participants