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

Added public key compression specs #137

Merged
merged 17 commits into from
Jun 24, 2020
Merged

Added public key compression specs #137

merged 17 commits into from
Jun 24, 2020

Conversation

Samyoul
Copy link
Member

@Samyoul Samyoul commented Jun 17, 2020

What has changed?

I've added detailed specs for the implementation of public key compression and decompression. The specifications detail the use of the following multiformat features:

  • multibase
  • multicodec
  • unsigned-varint

multiformat is used to ensure that the implementation has as much flexibility and robustness as feasible.

Why make the change?

The usage of key de/compression is outside the typical usage of public keys and requires a degree of background knowledge to correctly implement. The purpose of this specification change is to provide this needed background knowledge.

Please also see

docs/stable/2-account.md Outdated Show resolved Hide resolved
@oskarth
Copy link
Contributor

oskarth commented Jun 18, 2020

Thanks for the spec PR! Is there any chance we could people from Core (or more broadly, Status client implementers like desktop) more involved in these spec reviews as well? Currently I only see @cammellos as part of Core on reviews, where the rest are part of Nimbus/Vac.

cc @andremedeiros @hesterbruikman @iurimatias

@arnetheduck
Copy link
Member

There's a few things I don't understand here:

  • where do these keys expected to appear? ie are they a pure UX feature or do the keys appear on the wire?
  • what are the BLS keys used for
  • why both G1/G2 field variants?

@arnetheduck
Copy link
Member

also, where did the BLS serializations come from? the process of standardizing them has not settled yet cc @mratsim

Copy link
Contributor

@oskarth oskarth left a comment

Choose a reason for hiding this comment

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

Some questions

docs/stable/2-account.md Outdated Show resolved Hide resolved
docs/stable/2-account.md Outdated Show resolved Hide resolved
docs/stable/2-account.md Outdated Show resolved Hide resolved

Released // TODO

- Added details of public key compression and decompression
Copy link
Contributor

Choose a reason for hiding this comment

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

The way the spec is currently writing this is currently a breaking change

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going rewrite the language to make public key de/compression implementation a SHOULD and make all MUSTs conditional on if de/compression functionality is being implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@oskarth oskarth requested a review from gravityblast June 18, 2020 10:12
@Samyoul
Copy link
Member Author

Samyoul commented Jun 18, 2020

@arnetheduck

  • where do these keys expected to appear? ie are they a pure UX feature or do the keys appear on the wire?

Yes, this functionality is (currently) purely for UX purposes, see my comment here status-im/status-go#1937 (comment) and the issue on status-im/status-mobile#10325

  • what are the BLS keys used for
  • why both G1/G2 field variants?

In my implementation I added BLS key handling for the sake of providing functionality that was simple to implement and potentially useful in the future. But on reflection I can't justify the mandatory use of BLS keys in the spec. I'll make them a RECOMMENDATION in the case that there are UX reasons for using BLS keys.

also, where did the BLS serializations come from? the process of standardizing them has not settled yet

The BLS serialisation came from the zkcrypto spec https://github.com/zkcrypto/pairing/tree/master/src/bls12_381#serialization

@arnetheduck
Copy link
Member

In my implementation I added BLS key handling for the sake of providing functionality that was simple to implement and potentially useful in the future

YAGNI / it's going to be obsolete by the time of that future

@@ -213,12 +220,193 @@ All messages sent are encrypted with the public key of the destination and signe

-->

## Public Key Compression
Copy link
Member

@arnetheduck arnetheduck Jun 18, 2020

Choose a reason for hiding this comment

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

this is serialization - it's not compression, as in zip files. when serializing, you have the option to do so in a compressed format which replaces one of the two coordinates with a flag - you don't decompress it either, you deserialize or parse it

Copy link
Member Author

Choose a reason for hiding this comment

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

Most libraries call this functionality "compress" and "decompress", or "from_compressed", "to_compressed", "from_decompressed" and "to_decompressed". The common parlance for this functionality is compress and decompress.

To be technically accurate I can add to the Terms Glossary that the terms "public key compression" and "public key decompression" are colloquial terms for the technical terms "public key serialisation" and "public key deserialisation".

The target reader and beneficiary of this document is much more likely to search for and understand the terms compress and decompress, so my opinion is that we keep the idiomatic phraseology in the main body of the document for this reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

On reflection I've decided to implement this differently, I've taken your suggestion on board and now use the "serialize" terminology, with only a minor sentence detailing the idiomatic version of the technically correct phrases.

Below is a representation of an uncompressed secp256k1 public key.

```go
[]byte{
Copy link
Member

Choose a reason for hiding this comment

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

hm, are we now using golang as lingua franca in specs? the extra []byte{ here, do we define what it is somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've adjusted this. Should we presume the reader understands that the bytes are encoded in hex and are ordered left to right, top to bottom?

docs/stable/2-account.md Outdated Show resolved Hide resolved
Co-authored-by: Dean Eigenmann <7621705+decanus@users.noreply.github.com>
@Samyoul Samyoul requested review from decanus and oskarth June 19, 2020 10:49
Copy link
Contributor

@oskarth oskarth left a comment

Choose a reason for hiding this comment

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

generally lgtm but some more clear rationale / where it is used would be useful

@@ -39,6 +39,13 @@ This specification explains what Status account is, and how a node establishes t
- [Identicon](#identicon)
- [3 word pseudonym / Whisper/Waku key fingerprint](#3-word-pseudonym--whisperwaku-key-fingerprint)
- [ENS name](#ens-name)
- [Public Key Compression](#public-key-compression)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing some rationale for this feature. I know the gist of it but it'd be good to have it documented. Where exactly is it used etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a new subsection detailing where exactly this functionality would be used and why this functionality would be used. d4cc96c

@Samyoul Samyoul requested a review from arnetheduck June 22, 2020 22:12
@Samyoul
Copy link
Member Author

Samyoul commented Jun 22, 2020

@cammellos and @gravityblast would you mind giving me your thoughts on this spec submission?

@decanus would you mind re-reviewing the changes made to this submission?

@oskarth do you feel that the rationale given is sufficient and gives sufficiently specific information about the places that would use key "compression"?

Thank you

@oskarth
Copy link
Contributor

oskarth commented Jun 23, 2020

@Samyoul seems good to me, thanks!

Copy link
Contributor

@cammellos cammellos left a comment

Choose a reason for hiding this comment

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

I am still not completely sold on the why, but I won't rehash the conversation :)
But otherwise looks good to me

@Samyoul Samyoul self-assigned this Jun 23, 2020
@Samyoul
Copy link
Member Author

Samyoul commented Jun 24, 2020

@decanus would you mind unblocking the merge of this PR? Thank you. 🙂

@Samyoul Samyoul merged commit e98a9b7 into master Jun 24, 2020
@Samyoul Samyoul deleted the update/key-compression branch June 24, 2020 10:16
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.

5 participants