Slate: validate the declared version when deserializing

VersionedSlate and VersionedBinSlate are untagged and try V5 first, so
without a version check a V4 slate parses as V5. Only visible without a
payment proof; with one the V4 and V5 proof shapes differ enough for
serde to pick the right variant by chance.

Reject a mismatched version in the V4 and V5 deserializers, for both the
JSON and binary encodings, and set the declared version from the
conversion so it always matches the structure emitted. Add V4/V5 round
trip tests over both encodings, with and without a proof.

The V3 API examples showed V4 slates declaring version 5; they now
declare 4.

api/src/foreign_rpc.rs

@@ -144,7 +144,7 @@ pub trait ForeignRpc {
 					}
 				],
 				"sta": "S1",
-				"ver": "5:2"
+				"ver": "4:2"
 			},
 			null,
 			null
@@ -179,7 +179,7 @@ pub trait ForeignRpc {
 					}
 				],
 				"sta": "S2",
-				"ver": "5:2"
+				"ver": "4:2"
 			}
 		}
 	}

api/src/owner_rpc.rs

@@ -511,7 +511,7 @@ pub trait OwnerRpc {
 						}
 					],
 					"sta": "S1",
-					"ver": "5:2"
+					"ver": "4:2"
 				}
 			}
 		}
@@ -589,7 +589,7 @@ pub trait OwnerRpc {
 						}
 					],
 					"sta": "I1",
-					"ver": "5:2"
+					"ver": "4:2"
 				}
 			}
 		}
@@ -960,7 +960,7 @@ pub trait OwnerRpc {
 				"id": "0436430c-2b02-624c-2032-570501212b00",
 				"sigs": [],
 				"sta": "S3",
-				"ver": "5:3"
+				"ver": "4:3"
 			}
 		}
 	}

libwallet/src/slate.rs

@@ -914,14 +914,15 @@ impl From<&KernelFeaturesArgs> for KernelFeaturesArgsV5 {
 impl From<&VersionCompatInfo> for VersionCompatInfoV5 {
 	fn from(data: &VersionCompatInfo) -> VersionCompatInfoV5 {
 		let VersionCompatInfo {
-			version,
+			version: _,
 			block_header_version,
 		} = data;
-		let version = *version;
-		let block_header_version = *block_header_version;
+		// The declared version describes the structure we are producing, so it is fixed
+		// here rather than carried over from the internal slate. Readers rely on it to
+		// select the matching variant.
 		VersionCompatInfoV5 {
-			version,
-			block_header_version,
+			version: 5,
+			block_header_version: *block_header_version,
 		}
 	}
 }
@@ -1360,14 +1361,13 @@ impl From<&KernelFeaturesArgs> for KernelFeaturesArgsV4 {
 impl From<&VersionCompatInfo> for VersionCompatInfoV4 {
 	fn from(data: &VersionCompatInfo) -> VersionCompatInfoV4 {
 		let VersionCompatInfo {
-			version,
+			version: _,
 			block_header_version,
 		} = data;
-		let version = *version;
-		let block_header_version = *block_header_version;
+		// See VersionCompatInfoV5: the declared version always matches the structure.
 		VersionCompatInfoV4 {
-			version,
-			block_header_version,
+			version: 4,
+			block_header_version: *block_header_version,
 		}
 	}
 }

libwallet/src/slate_versions/mod.rs

@@ -153,13 +153,15 @@ pub mod tests {
 	use crate::slate::{KernelFeaturesArgs, ParticipantData, PaymentInfo, PaymentMemo};
 	use crate::slate_versions::v5::{CommitsV5, SlateV5};
 	use crate::{
-		slate, Error, Slate, Slatepacker, SlatepackerArgs, VersionedBinSlate, VersionedSlate,
+		slate, Error, Slate, SlateVersion, Slatepacker, SlatepackerArgs, VersionedBinSlate,
+		VersionedSlate,
 	};
 	use chrono::{DateTime, Utc};
 	use ed25519_dalek::Signature as DalekSignature;
 	use ed25519_dalek::VerifyingKey as DalekPublicKey;
 	use grin_core::global::{set_local_chain_type, ChainTypes};
 	use grin_keychain::{ExtKeychain, Keychain, SwitchCommitmentType};
+	use grin_wallet_util::byte_ser;
 	use std::convert::TryInto;
 
 	/// Populate a test internal slate with all fields to test conversions
@@ -323,6 +325,64 @@ pub mod tests {
 		Ok(())
 	}
 
+	// A slate must deserialize back into the version it declares. VersionedSlate is
+	// untagged and tries V5 first, so without a version check in each deserializer a
+	// V4 slate is silently parsed (and reported) as V5.
+	#[test]
+	fn versioned_slate_json_round_trip() -> Result<(), Error> {
+		set_local_chain_type(ChainTypes::Mainnet);
+		let slate_internal = populate_test_slate()?;
+
+		// A slate without a payment proof is the case that matters: the V4 and V5 proof
+		// shapes differ, so a proof makes the variants distinguishable by accident.
+		for with_proof in [true, false] {
+			for version in [SlateVersion::V4, SlateVersion::V5] {
+				let mut slate = slate_internal.clone();
+				if !with_proof {
+					slate.payment_proof = None;
+				}
+				slate.version_info.version = match version {
+					SlateVersion::V4 => 4,
+					SlateVersion::V5 => 5,
+				};
+				let versioned = VersionedSlate::into_version(slate, version.clone())?;
+				let json = serde_json::to_string(&versioned).unwrap();
+				let recovered: VersionedSlate = serde_json::from_str(&json).unwrap();
+				assert_eq!(recovered.version(), version, "json round trip: {}", json);
+			}
+		}
+
+		Ok(())
+	}
+
+	#[test]
+	fn versioned_bin_slate_round_trip() -> Result<(), Error> {
+		set_local_chain_type(ChainTypes::Mainnet);
+		let slate_internal = populate_test_slate()?;
+
+		for with_proof in [true, false] {
+			for version in [SlateVersion::V4, SlateVersion::V5] {
+				let mut slate = slate_internal.clone();
+				if !with_proof {
+					slate.payment_proof = None;
+				}
+				slate.version_info.version = match version {
+					SlateVersion::V4 => 4,
+					SlateVersion::V5 => 5,
+				};
+				let versioned = VersionedSlate::into_version(slate, version.clone())?;
+				let bin: VersionedBinSlate = versioned.try_into().unwrap();
+				let bytes = byte_ser::to_bytes(&bin).unwrap();
+				let recovered: VersionedSlate = byte_ser::from_bytes::<VersionedBinSlate>(&bytes)
+					.unwrap()
+					.into();
+				assert_eq!(recovered.version(), version);
+			}
+		}
+
+		Ok(())
+	}
+
 	#[test]
 	fn slatepack_version_v4_v5_bin() -> Result<(), Error> {
 		set_local_chain_type(ChainTypes::Mainnet);

libwallet/src/slate_versions/ser.rs

@@ -526,7 +526,15 @@ pub mod version_info_v4 {
 				return Err(Error::custom("Cannot parse version"));
 			}
 			match u16::from_str_radix(v[0], 10) {
-				Ok(u) => retval.version = u,
+				// VersionedSlate is untagged, so the declared version must be checked here:
+				// otherwise a slate is silently parsed as the wrong variant.
+				Ok(4) => retval.version = 4,
+				Ok(u) => {
+					return Err(Error::custom(format!(
+						"Expected a V4 slate, found version {}",
+						u
+					)))
+				}
 				Err(e) => return Err(Error::custom(format!("Cannot parse version: {}", e))),
 			}
 			match u16::from_str_radix(v[1], 10) {
@@ -568,7 +576,15 @@ pub mod version_info_v5 {
 				return Err(Error::custom("Cannot parse version"));
 			}
 			match u16::from_str_radix(v[0], 10) {
-				Ok(u) => retval.version = u,
+				// See the V4 deserializer: the declared version disambiguates the
+				// untagged VersionedSlate variants.
+				Ok(5) => retval.version = 5,
+				Ok(u) => {
+					return Err(Error::custom(format!(
+						"Expected a V5 slate, found version {}",
+						u
+					)))
+				}
 				Err(e) => return Err(Error::custom(format!("Cannot parse version: {}", e))),
 			}
 			match u16::from_str_radix(v[1], 10) {

libwallet/src/slate_versions/v4_bin.rs

@@ -463,8 +463,17 @@ impl Writeable for SlateV4Bin {
 
 impl Readable for SlateV4Bin {
 	fn read<R: Reader>(reader: &mut R) -> Result<SlateV4Bin, grin_ser::Error> {
+		// VersionedBinSlate is untagged, so reject a slate that does not declare V4 here;
+		// otherwise it can be parsed as the wrong variant.
+		let version = reader.read_u16()?;
+		if version != 4 {
+			return Err(grin_ser::Error::UnexpectedData {
+				expected: 4u16.to_be_bytes().to_vec(),
+				received: version.to_be_bytes().to_vec(),
+			});
+		}
 		let ver = VersionCompatInfoV4 {
-			version: reader.read_u16()?,
+			version,
 			block_header_version: reader.read_u16()?,
 		};
 		let id = UuidWrap::read(reader)?.0;

libwallet/src/slate_versions/v5_bin.rs

@@ -382,8 +382,17 @@ impl Writeable for SlateV5Bin {
 
 impl Readable for SlateV5Bin {
 	fn read<R: Reader>(reader: &mut R) -> Result<SlateV5Bin, grin_ser::Error> {
+		// See SlateV4Bin: the declared version disambiguates the untagged
+		// VersionedBinSlate variants.
+		let version = reader.read_u16()?;
+		if version != 5 {
+			return Err(grin_ser::Error::UnexpectedData {
+				expected: 5u16.to_be_bytes().to_vec(),
+				received: version.to_be_bytes().to_vec(),
+			});
+		}
 		let ver = VersionCompatInfoV5 {
-			version: reader.read_u16()?,
+			version,
 			block_header_version: reader.read_u16()?,
 		};
 		let id = UuidWrap::read(reader)?.0;

libwallet/src/slatepack/packer.rs

@@ -97,16 +97,12 @@ impl<'a> Slatepacker<'a> {
 		// Emit the lowest version that can represent this slate, for maximum
 		// interoperability. Only the (opt-in) payment proof needs V5 fields
 		// (timestamp/memo) that V4 cannot carry; everything else stays V4.
-		let (version, version_num) = if slate.payment_proof.is_some() {
-			(SlateVersion::V5, 5)
+		let version = if slate.payment_proof.is_some() {
+			SlateVersion::V5
 		} else {
-			(SlateVersion::V4, 4)
+			SlateVersion::V4
 		};
-		// Keep the declared version consistent with the emitted structure so readers
-		// select the right variant.
-		let mut slate = slate.clone();
-		slate.version_info.version = version_num;
-		let out_slate = VersionedSlate::into_version(slate, version)?;
+		let out_slate = VersionedSlate::into_version(slate.clone(), version)?;
 		let bin_slate = VersionedBinSlate::try_from(out_slate).map_err(|_| Error::SlatepackSer)?;
 		let mut slatepack = Slatepack::default();
 		slatepack.payload = byte_ser::to_bytes(&bin_slate).map_err(|_| Error::SlatepackSer)?;