Compressed #56
Compressed #56
Conversation
|
|
||
| def decompress_public_key_bytes(self, | ||
| compressed_public_key_bytes: bytes) -> bytes: | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
Note to self that we should convert these to be ABC,abstractmethod based sometime.
Gets rid of duplication between PublicKey and KeyAPI class.
|
Added the suggested hypothesis test, improved the validation error message, and updated the readme. Also, when looking at the readme I realized that the key compression API is duplicated: Once in PublicKey (where one would naturally look for it) and once in |
| prefix = b"\x02" | ||
| else: | ||
| prefix = b"\x03" | ||
| return prefix + pad32(int_to_big_endian(x)) |
There was a problem hiding this comment.
lol, padding the compressed key is a strange spec. I was wondering why the compressed key is always the same length...
|
Anything else to add today @pipermerriam ? If not, I'll merge by EOD. |
There was a problem hiding this comment.
looks good.
The hypothesis tests are probably only running a small number of runs IIRC so you might do a couple of local runs or repeat the hypothesis test runs on a loop for a while to stress test them a bit and see if you find any edge cases that demonstrate the two implementations diverging before you click merge.
| uncompressed_public_key_bytes: bytes) -> bytes: | ||
| point = ( | ||
| big_endian_to_int(uncompressed_public_key_bytes[:32]), | ||
| big_endian_to_int(uncompressed_public_key_bytes[32:]), |
There was a problem hiding this comment.
This allows for an arbitrarily long second value were the provided uncompressed_public_key_bytes to be longer than 64 bytes. Am I missing something? Seems like this would cause weird/hard-to-debug downstream errors in some cases.
There was a problem hiding this comment.
This is not part of the public API, all inputs should pass through datatypes.PublicKey. I still added validation now, just to be safe.
| if prefix not in (2, 3): | ||
| raise ValueError("Invalid compressed public key") | ||
|
|
||
| x = big_endian_to_int(compressed_public_key_bytes[1:]) |
There was a problem hiding this comment.
I've been on a big memoryview kick recently so feel free to ignore me. This would be slightly faster using a memoryview to prevent the copy. This is only safe if the thing you're passing it to doesn't try to mutate it which in this case I believe that condition is satisfied.
There was a problem hiding this comment.
I did a quick benchmark and didn't see any improvement. I think the reason is that it would save one copy when slicing, but it adds one when creating the memoryview. But I will keep it in mind in the future (could be very useful for pyrlp/ssz).
|
I don't have write access here, can someone merge please? |
|
Would you like a release after #58 is merged? |
|
Thanks!
Yep, that would be nice. |
What was wrong?
Missing support for compressed public keys (#55).
How was it fixed?
to/from_compressed_bytesmethods toPublicKeyclass.I'm not an expert in cryptography, so please review carefully, especially the native backend code.
Cute Animal Picture