Repository navigation
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The implementation consistently reuses canonical decoding and covers all affected point fields with focused tests.
0 open findings
What changed in this PR
Aligns Serde point decoding with canonical dusk-bytes subgroup validation for Note and Sender.
Changes:
- Adds subgroup-validating point deserializers.
- Applies them to value commitments and sender encryptions.
- Adds rejection and compatibility tests plus a changelog entry.
| File | Description |
|---|---|
core/src/serde_support.rs |
Adds canonical point deserialization helpers. |
core/src/note.rs |
Applies validation to relevant point fields. |
core/tests/serde.rs |
Tests invalid subgroup rejection and valid decoding. |
core/CHANGELOG.md |
Documents the Serde validation fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d9c5f3d to
4c2d3ab
Compare
|
Force-pushed: rebased on the amended stack. No changes. |
4c2d3ab to
433b322
Compare
|
Force-pushed: rebased on the amended stack. No changes. |
moCello
left a comment
moCello
left a comment
There was a problem hiding this comment.
COMMENT — Every point of a note and a sender encryption now goes through the subgroup check, and the new test fails on the old decoder. Approval waits on one fix: the point decoder must reject a point encoding that is longer than 32 bytes.
| pub(crate) fn deserialize_point<'de, D: Deserializer<'de>>( | ||
| deserializer: D, | ||
| ) -> Result<JubJubAffine, D::Error> { | ||
| let bytes: Vec<u8> = As::<Hex>::deserialize(deserializer)?; | ||
| JubJubAffine::from_slice(&bytes) | ||
| .map_err(|err| SerdeError::custom(format!("{err:?}"))) | ||
| } |
There was a problem hiding this comment.
CAUTION: The new point decoder accepts a hex string longer than 32 bytes and ignores the extra bytes, which the replaced Deserialize of JubJubAffine rejected.
Context: DeserializableSlice::from_slice fails only when the input is shorter than N. It copies the first N bytes and drops the rest. The Deserialize of AffinePoint in dusk-jubjub decodes the hex into [u8; 32] and rejects any other length. The key and stealth-address decoders in this file do the same with try_into, and serde_too_long_encoded asserts that rule for them.
Impact: A value_commitment or sender point written as the 64 honest hex digits plus any whole number of extra hex-encoded bytes now decodes to the same Note. serde_rejects_points_outside_the_subgroup does not test the length.
Fix: Decode into a fixed array, as value_enc does: let bytes: [u8; JubJubAffine::SIZE] = As::<Hex>::deserialize(deserializer)?;. Then call the trait method by its full path: <JubJubAffine as Serializable<32>>::from_bytes(&bytes). Do not write JubJubAffine::from_bytes, because that path resolves to the inherent method, which skips the subgroup check. Add a test that appends one byte to an honest point and expects an error.
There was a problem hiding this comment.
Done as you describe. The point decodes into [u8; JubJubAffine::SIZE], then goes through <JubJubAffine as Serializable<32>>::from_bytes, with a comment that JubJubAffine::from_bytes would resolve to the inherent decoder and skip the subgroup check. A new test adds a byte to each point, then removes one, and checks that neither the note nor the sender decodes. On the previous head, all five points decoded with the extra byte dropped. The other decoders in the file already require exact sizes.
The value commitment of a `Note` and the points of a `Sender` encryption derive their Serde implementation from dusk-jubjub, whose `Deserialize` checks that a point is on the curve, but not, as `Serializable::from_bytes` does, that it lies in the prime-order subgroup (dusk-network/jubjub#192). A note or sender decoded from JSON could carry a point its dusk-bytes decoders reject. Decode those points through `Serializable::from_bytes`, as the Serde implementations of the keys and the stealth address already do through their dusk-bytes decoders. A point must still be exactly 32 bytes. The encoding is unchanged, and the existing JSON vectors still decode. The test replaces the value commitment and each sender point of an honest note with the order-two point, then with the generator plus it, and checks that neither the note nor the sender decodes, while the honest note, and the empty note, whose points are the identity, round-trip. Another test adds a byte to each point, then removes one, and checks that neither the note nor the sender decodes.
433b322 to
e059e38
Compare
Stacked on #325.
When a
NoteorSenderis deserialized with Serde, its value commitment and sender-encryption points now go throughSerializable::from_bytes, so they get the same subgroup check as the dusk-bytes decoders. Each point must still be exactly 32 bytes. The JSON encoding is unchanged, and the existing JSON vectors still decode. dusk-network/jubjub#192 tracks the same alignment in dusk-jubjub's ownDeserialize.Resolves #318