Unify & optimise serialization in v0.7 #141
Conversation
(cherry picked from commit 5ee0400)
src/elliptic/curves/secp256_k1.rs
Outdated
|
|
||
| Ok(Secp256k1Point::from_coor(&bx, &by)) | ||
| let bp = BigInt::from_hex(&p).map_err(E::Error::custom)?; | ||
| Ok(Secp256k1Point::from_bytes(&bp.to_bytes()[1..33]).unwrap()) |
There was a problem hiding this comment.
Malformed input that comes from untrusted source may cause this method to panic. Deserialize implementation must validate input and return error if it's invalid. You need to:
- Ensure that
bp.to_bytes() >= 33(otherwise&bp.to_bytes()[1..33]may panic) - Properly handle error returned by
Secp256k1Point::from_bytes
Something like that should work:
| Ok(Secp256k1Point::from_bytes(&bp.to_bytes()[1..33]).unwrap()) | |
| let bp = bp.to_bytes(); | |
| if bp.len() < 33 { | |
| return E::Error::invalid_length(bp.len(), &"33 bytes") | |
| } | |
| Secp256k1Point::from_bytes(&bp[1..33]) | |
| .map_err(|_e| E::Error::custom("invalid point")) |
There was a problem hiding this comment.
Nit (not important): I think BigInt can be removed from this function by using hex::decode - this would save a couple allocations. You can even remove all the allocations in this function by using hex::decode_to_slice. That would optimise deserialization.
src/elliptic/curves/secp256_k1.rs
Outdated
|
|
||
| Ok(Secp256k1Point::from_coor(&bx, &by)) | ||
| let bp = BigInt::from_hex(p).map_err(V::Error::custom)?; | ||
| Ok(Secp256k1Point::from_bytes(&bp.to_bytes()[1..33]).unwrap()) |
There was a problem hiding this comment.
The same as visit_map:
- Ensure that
bp.to_bytes()is large enough - Propagate error returned by
Secp256k1Point::from_bytes - Consider using
hex::decode/hex::decode_sliceoverBigInt::from_hex
src/elliptic/curves/secp256_k1.rs
Outdated
| state.serialize_field("x", &self.x_coor().unwrap().to_hex())?; | ||
| state.serialize_field("y", &self.y_coor().unwrap().to_hex())?; | ||
| let mut state = serializer.serialize_struct("Secp256k1Point", 1)?; | ||
| state.serialize_field("p", &self.bytes_compressed_to_big_int().to_hex())?; |
There was a problem hiding this comment.
Nit:
- Since serialized structure
Secp256k1Pointconsist of only one field, maybe just serialize it as string, not as structure? Ie. simpleserializer.serialize_str(...)can be used here - In curv v0.8 we serialize point as raw bytes, not as hex-encoded bytes. This removes verbosity, but makes it highly efficient for binary data formats like
bincode. Simply saying, it reduces size of serialized point by half. Do you prefer hex format?
There was a problem hiding this comment.
- True, it is shorter and simpler than my idea about newtype struct.
- As a matter of fact, I prefer raw bytes as well. I'll change the code soon.
Indeed, we should freeze serialization format at some point in the future. It's not expected that bumping curv v0.7.1 to v0.7.2 will invalidate all serialized points/bigints. However, we do not follow any policy regarding serialization format, so it may be considered unstable. |
- panic-less implementation with input validation; - slightly less verbose and more efficient serialization.
I added only the basic requested changes yesterday and still want to change from hex to raw bytes, so please don't merge it in yet. Thank you for your patience. 🙏🏼 |
|
Sure, take your time! I won't merge until PR is marked as ready for review. I just noticed that there are no unwraps anymore. Converting to raw bytes is also an important optimisation! |
You are probably wondering how I got hereBackground: earlier today I opened #140, where @survived suggested either to wait a couple of months or open a PR. The former is not a viable option for us, hence here comes the latter.Resolves #138 and #140 in v0.7:
BigInt;Secp256k1Point(compressed form).The main question here is if you consider serialization format public API or not. I am very tempted to say that it is not, and the only publicly guaranteed invariants are Deserialize(Serialize(Point)) = Point and Serialize(Deserialize(String)) = String.
However, my opinion is clearly biased, and there are different approaches to this issue. For example, one may argue that if they got a file on a hard drive produced by the former version of Serialize, then they won't be able to Deserialize it by the current version of Deserialize, which is not ideal, to say the least.
However, #139 appears to be a bug-fix either way, so the situation above can occur in the current setting as well (when using different
BigIntbackends), so I assume it has to be addressed somehow...