diff --git a/api/src/foreign_rpc.rs b/api/src/foreign_rpc.rs index 028a69e..6b67b6e 100644 --- a/api/src/foreign_rpc.rs +++ b/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" } } } diff --git a/api/src/owner_rpc.rs b/api/src/owner_rpc.rs index 8b8a2bf..6318071 100644 --- a/api/src/owner_rpc.rs +++ b/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" } } } diff --git a/libwallet/src/slate.rs b/libwallet/src/slate.rs index 07d59dd..025e7e2 100644 --- a/libwallet/src/slate.rs +++ b/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, } } } diff --git a/libwallet/src/slate_versions/mod.rs b/libwallet/src/slate_versions/mod.rs index 5249a38..1b94c45 100644 --- a/libwallet/src/slate_versions/mod.rs +++ b/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::(&bytes) + .unwrap() + .into(); + assert_eq!(recovered.version(), version); + } + } + + Ok(()) + } + #[test] fn slatepack_version_v4_v5_bin() -> Result<(), Error> { set_local_chain_type(ChainTypes::Mainnet); diff --git a/libwallet/src/slate_versions/ser.rs b/libwallet/src/slate_versions/ser.rs index 845c351..7a7a431 100644 --- a/libwallet/src/slate_versions/ser.rs +++ b/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) { diff --git a/libwallet/src/slate_versions/v4_bin.rs b/libwallet/src/slate_versions/v4_bin.rs index dfb073c..8a66a31 100644 --- a/libwallet/src/slate_versions/v4_bin.rs +++ b/libwallet/src/slate_versions/v4_bin.rs @@ -463,8 +463,17 @@ impl Writeable for SlateV4Bin { impl Readable for SlateV4Bin { fn read(reader: &mut R) -> Result { + // 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; diff --git a/libwallet/src/slate_versions/v5_bin.rs b/libwallet/src/slate_versions/v5_bin.rs index 43ed3ef..7af92f7 100644 --- a/libwallet/src/slate_versions/v5_bin.rs +++ b/libwallet/src/slate_versions/v5_bin.rs @@ -382,8 +382,17 @@ impl Writeable for SlateV5Bin { impl Readable for SlateV5Bin { fn read(reader: &mut R) -> Result { + // 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; diff --git a/libwallet/src/slatepack/packer.rs b/libwallet/src/slatepack/packer.rs index a19de72..d4324ad 100644 --- a/libwallet/src/slatepack/packer.rs +++ b/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)?;