Introduce CommitOnly variant of Inputs (#3419)

* Introduce CommitOnly variant of Inputs.
Introduce CommitWrapper so we can sort commit only inputs correctly.

* rememebr to resort if converting

* write inputs based on variant and protocol version

* read and write protocol version specific inputs

* store full blocks in local db in v3
convert to v2 when relaying to v2 peers

* add debug version_str for inputs

* no assumptions about spent index sort order

* add additional version debug logs

* fix ser/deser tests for proto v3

* cleanup coinbase maturity

* rework pool to better handle v2 conversion robustly

* cleanup txpool add_to_pool

* fix nrd kernel test

* move init conversion earlier

* cleanup

* cleanup based on PR feedback

core/src/core/block.rs

@@ -272,7 +272,7 @@ impl PMMRable for BlockHeader {
 /// Serialization of a block header
 impl Writeable for BlockHeader {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ser::Error> {
-		if writer.serialization_mode() != ser::SerializationMode::Hash {
+		if !writer.serialization_mode().is_hash_mode() {
 			self.write_pre_pow(writer)?;
 		}
 		self.pow.write(writer)?;
@@ -507,8 +507,7 @@ impl Hashed for Block {
 impl Writeable for Block {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ser::Error> {
 		self.header.write(writer)?;
-
-		if writer.serialization_mode() != ser::SerializationMode::Hash {
+		if !writer.serialization_mode().is_hash_mode() {
 			self.body.write(writer)?;
 		}
 		Ok(())
@@ -609,7 +608,7 @@ impl Block {
 		kernels.extend_from_slice(cb.kern_full());
 
 		// Initialize a tx body and sort everything.
-		let body = TransactionBody::init(inputs, &outputs, &kernels, false)?;
+		let body = TransactionBody::init(inputs.into(), &outputs, &kernels, false)?;
 
 		// Finally return the full block.
 		// Note: we have not actually validated the block here,

core/src/core/transaction.rs

@@ -331,7 +331,7 @@ impl Writeable for KernelFeatures {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ser::Error> {
 		// Care must be exercised when writing for hashing purposes.
 		// All kernels are hashed using original v1 serialization strategy.
-		if writer.serialization_mode() == ser::SerializationMode::Hash {
+		if writer.serialization_mode().is_hash_mode() {
 			return self.write_v1(writer);
 		}
 
@@ -458,6 +458,8 @@ pub struct TxKernel {
 impl DefaultHashable for TxKernel {}
 hashable_ord!(TxKernel);
 
+/// We want to be able to put kernels in a hashset in the pool.
+/// So we need to be able to hash them.
 impl ::std::hash::Hash for TxKernel {
 	fn hash<H: ::std::hash::Hasher>(&self, state: &mut H) {
 		let mut vec = Vec::new();
@@ -691,12 +693,23 @@ impl Readable for TransactionBody {
 			return Err(ser::Error::TooLargeReadErr);
 		}
 
-		let inputs = read_multi(reader, num_inputs)?;
+		// Read protocol version specific inputs.
+		let inputs = match reader.protocol_version().value() {
+			0..=2 => {
+				let inputs: Vec<Input> = read_multi(reader, num_inputs)?;
+				Inputs::from(inputs.as_slice())
+			}
+			3..=ser::ProtocolVersion::MAX => {
+				let inputs: Vec<CommitWrapper> = read_multi(reader, num_inputs)?;
+				Inputs::from(inputs.as_slice())
+			}
+		};
+
 		let outputs = read_multi(reader, num_outputs)?;
 		let kernels = read_multi(reader, num_kernels)?;
 
 		// Initialize tx body and verify everything is sorted.
-		let body = TransactionBody::init(&inputs, &outputs, &kernels, true)
+		let body = TransactionBody::init(inputs, &outputs, &kernels, true)
 			.map_err(|_| ser::Error::CorruptedData)?;
 
 		Ok(body)
@@ -751,13 +764,13 @@ impl TransactionBody {
 	/// the provided inputs, outputs and kernels.
 	/// Guarantees inputs, outputs, kernels are sorted lexicographically.
 	pub fn init(
-		inputs: &[Input],
+		inputs: Inputs,
 		outputs: &[Output],
 		kernels: &[TxKernel],
 		verify_sorted: bool,
 	) -> Result<TransactionBody, Error> {
 		let mut body = TransactionBody {
-			inputs: inputs.into(),
+			inputs,
 			outputs: outputs.to_vec(),
 			kernels: kernels.to_vec(),
 		};
@@ -792,11 +805,19 @@ impl TransactionBody {
 	/// inputs, if any, are kept intact.
 	/// Sort order is maintained.
 	pub fn with_input(mut self, input: Input) -> TransactionBody {
-		let mut inputs: Vec<_> = self.inputs.into();
-		if let Err(e) = inputs.binary_search(&input) {
-			inputs.insert(e, input)
+		match &mut self.inputs {
+			Inputs::CommitOnly(inputs) => {
+				let commit = input.into();
+				if let Err(e) = inputs.binary_search(&commit) {
+					inputs.insert(e, commit)
+				};
+			}
+			Inputs::FeaturesAndCommit(inputs) => {
+				if let Err(e) = inputs.binary_search(&input) {
+					inputs.insert(e, input)
+				};
+			}
 		};
-		self.inputs = inputs.into();
 		self
 	}
 
@@ -1166,14 +1187,15 @@ impl Transaction {
 
 	/// Creates a new transaction initialized with
 	/// the provided inputs, outputs, kernels
-	pub fn new(inputs: &[Input], outputs: &[Output], kernels: &[TxKernel]) -> Transaction {
-		let offset = BlindingFactor::zero();
-
+	pub fn new(inputs: Inputs, outputs: &[Output], kernels: &[TxKernel]) -> Transaction {
 		// Initialize a new tx body and sort everything.
 		let body =
 			TransactionBody::init(inputs, outputs, kernels, false).expect("sorting, not verifying");
 
-		Transaction { offset, body }
+		Transaction {
+			offset: BlindingFactor::zero(),
+			body,
+		}
 	}
 
 	/// Creates a new transaction using this transaction as a template
@@ -1398,7 +1420,7 @@ pub fn aggregate(txs: &[Transaction]) -> Result<Transaction, Error> {
 					kernels + tx.kernels().len(),
 				)
 			});
-	let mut inputs: Vec<Input> = Vec::with_capacity(n_inputs);
+	let mut inputs: Vec<CommitWrapper> = Vec::with_capacity(n_inputs);
 	let mut outputs: Vec<Output> = Vec::with_capacity(n_outputs);
 	let mut kernels: Vec<TxKernel> = Vec::with_capacity(n_kernels);
 
@@ -1427,7 +1449,8 @@ pub fn aggregate(txs: &[Transaction]) -> Result<Transaction, Error> {
 	//   * full set of tx kernels
 	//   * sum of all kernel offsets
 	// Note: We sort input/outputs/kernels when building the transaction body internally.
-	let tx = Transaction::new(inputs, outputs, &kernels).with_offset(total_kernel_offset);
+	let tx =
+		Transaction::new(Inputs::from(inputs), outputs, &kernels).with_offset(total_kernel_offset);
 
 	Ok(tx)
 }
@@ -1435,7 +1458,7 @@ pub fn aggregate(txs: &[Transaction]) -> Result<Transaction, Error> {
 /// Attempt to deaggregate a multi-kernel transaction based on multiple
 /// transactions
 pub fn deaggregate(mk_tx: Transaction, txs: &[Transaction]) -> Result<Transaction, Error> {
-	let mut inputs: Vec<Input> = vec![];
+	let mut inputs: Vec<CommitWrapper> = vec![];
 	let mut outputs: Vec<Output> = vec![];
 	let mut kernels: Vec<TxKernel> = vec![];
 
@@ -1494,7 +1517,10 @@ pub fn deaggregate(mk_tx: Transaction, txs: &[Transaction]) -> Result<Transactio
 	kernels.sort_unstable();
 
 	// Build a new tx from the above data.
-	Ok(Transaction::new(&inputs, &outputs, &kernels).with_offset(total_kernel_offset))
+	Ok(
+		Transaction::new(Inputs::from(inputs.as_slice()), &outputs, &kernels)
+			.with_offset(total_kernel_offset),
+	)
 }
 
 /// A transaction input.
@@ -1531,14 +1557,6 @@ impl From<&OutputIdentifier> for Input {
 	}
 }
 
-impl ::std::hash::Hash for Input {
-	fn hash<H: ::std::hash::Hasher>(&self, state: &mut H) {
-		let mut vec = Vec::new();
-		ser::serialize_default(&mut vec, &self).expect("serialization failed");
-		::std::hash::Hash::hash(&vec, state);
-	}
-}
-
 /// Implementation of Writeable for a transaction Input, defines how to write
 /// an Input as binary.
 impl Writeable for Input {
@@ -1588,30 +1606,102 @@ impl Input {
 		self.features.is_plain()
 	}
 }
+
+/// We need to wrap commitments so they can be sorted with hashable_ord.
+#[derive(Serialize, Deserialize, Debug, Clone, Copy)]
+#[serde(transparent)]
+pub struct CommitWrapper {
+	#[serde(
+		serialize_with = "secp_ser::as_hex",
+		deserialize_with = "secp_ser::commitment_from_hex"
+	)]
+	commit: Commitment,
+}
+
+impl DefaultHashable for CommitWrapper {}
+hashable_ord!(CommitWrapper);
+
+impl From<Commitment> for CommitWrapper {
+	fn from(commit: Commitment) -> Self {
+		CommitWrapper { commit }
+	}
+}
+
+impl From<Input> for CommitWrapper {
+	fn from(input: Input) -> Self {
+		CommitWrapper {
+			commit: input.commitment(),
+		}
+	}
+}
+
+impl From<&Input> for CommitWrapper {
+	fn from(input: &Input) -> Self {
+		CommitWrapper {
+			commit: input.commitment(),
+		}
+	}
+}
+
+impl AsRef<Commitment> for CommitWrapper {
+	fn as_ref(&self) -> &Commitment {
+		&self.commit
+	}
+}
+
+impl Readable for CommitWrapper {
+	fn read<R: Reader>(reader: &mut R) -> Result<CommitWrapper, ser::Error> {
+		let commit = Commitment::read(reader)?;
+		Ok(CommitWrapper { commit })
+	}
+}
+
+impl Writeable for CommitWrapper {
+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ser::Error> {
+		self.commit.write(writer)
+	}
+}
+
+impl From<Inputs> for Vec<CommitWrapper> {
+	fn from(inputs: Inputs) -> Self {
+		match inputs {
+			Inputs::CommitOnly(inputs) => inputs,
+			Inputs::FeaturesAndCommit(inputs) => {
+				let mut commits: Vec<_> = inputs.iter().map(|input| input.into()).collect();
+				commits.sort_unstable();
+				commits
+			}
+		}
+	}
+}
+
+impl From<&Inputs> for Vec<CommitWrapper> {
+	fn from(inputs: &Inputs) -> Self {
+		match inputs {
+			Inputs::CommitOnly(inputs) => inputs.clone(),
+			Inputs::FeaturesAndCommit(inputs) => {
+				let mut commits: Vec<_> = inputs.iter().map(|input| input.into()).collect();
+				commits.sort_unstable();
+				commits
+			}
+		}
+	}
+}
+
+impl CommitWrapper {
+	/// Wrapped commitment.
+	pub fn commitment(&self) -> Commitment {
+		self.commit
+	}
+}
 /// Wrapper around a vec of inputs.
 #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
 #[serde(untagged)]
 pub enum Inputs {
+	/// Vec of commitments.
+	CommitOnly(Vec<CommitWrapper>),
 	/// Vec of inputs.
 	FeaturesAndCommit(Vec<Input>),
-	// Vec of commitments.
-	// CommitOnly(Vec<Commitment>),
-}
-
-impl From<Inputs> for Vec<Input> {
-	fn from(inputs: Inputs) -> Self {
-		match inputs {
-			Inputs::FeaturesAndCommit(inputs) => inputs,
-		}
-	}
-}
-
-impl From<&Inputs> for Vec<Input> {
-	fn from(inputs: &Inputs) -> Self {
-		match inputs {
-			Inputs::FeaturesAndCommit(inputs) => inputs.to_vec(),
-		}
-	}
 }
 
 impl From<&[Input]> for Inputs {
@@ -1620,35 +1710,62 @@ impl From<&[Input]> for Inputs {
 	}
 }
 
-impl From<Vec<Input>> for Inputs {
-	fn from(inputs: Vec<Input>) -> Self {
-		Inputs::FeaturesAndCommit(inputs)
+impl From<&[CommitWrapper]> for Inputs {
+	fn from(commits: &[CommitWrapper]) -> Self {
+		Inputs::CommitOnly(commits.to_vec())
 	}
 }
 
-impl From<Vec<OutputIdentifier>> for Inputs {
-	fn from(outputs: Vec<OutputIdentifier>) -> Self {
-		let inputs = outputs
-			.into_iter()
+/// Used when converting to v2 compatibility.
+/// We want to preserve output features here.
+impl From<&[OutputIdentifier]> for Inputs {
+	fn from(outputs: &[OutputIdentifier]) -> Self {
+		let mut inputs: Vec<_> = outputs
+			.iter()
 			.map(|out| Input {
 				features: out.features,
 				commit: out.commit,
 			})
 			.collect();
+		inputs.sort_unstable();
 		Inputs::FeaturesAndCommit(inputs)
 	}
 }
 
 impl Default for Inputs {
 	fn default() -> Self {
-		Inputs::FeaturesAndCommit(vec![])
+		Inputs::CommitOnly(vec![])
 	}
 }
 
 impl Writeable for Inputs {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ser::Error> {
-		match self {
-			Inputs::FeaturesAndCommit(inputs) => inputs.write(writer)?,
+		// Nothing to write so we are done.
+		if self.is_empty() {
+			return Ok(());
+		}
+
+		// If writing for a hash then simply write all our inputs.
+		if writer.serialization_mode().is_hash_mode() {
+			match self {
+				Inputs::CommitOnly(inputs) => inputs.write(writer)?,
+				Inputs::FeaturesAndCommit(inputs) => inputs.write(writer)?,
+			}
+		} else {
+			// Otherwise we are writing full data and need to consider our inputs variant and protocol version.
+			match self {
+				Inputs::CommitOnly(inputs) => match writer.protocol_version().value() {
+					0..=2 => return Err(ser::Error::UnsupportedProtocolVersion),
+					3..=ProtocolVersion::MAX => inputs.write(writer)?,
+				},
+				Inputs::FeaturesAndCommit(inputs) => match writer.protocol_version().value() {
+					0..=2 => inputs.write(writer)?,
+					3..=ProtocolVersion::MAX => {
+						let inputs: Vec<CommitWrapper> = self.into();
+						inputs.write(writer)?;
+					}
+				},
+			}
 		}
 		Ok(())
 	}
@@ -1658,6 +1775,7 @@ impl Inputs {
 	/// Number of inputs.
 	pub fn len(&self) -> usize {
 		match self {
+			Inputs::CommitOnly(inputs) => inputs.len(),
 			Inputs::FeaturesAndCommit(inputs) => inputs.len(),
 		}
 	}
@@ -1670,15 +1788,26 @@ impl Inputs {
 	/// Verify inputs are sorted and unique.
 	fn verify_sorted_and_unique(&self) -> Result<(), ser::Error> {
 		match self {
+			Inputs::CommitOnly(inputs) => inputs.verify_sorted_and_unique(),
 			Inputs::FeaturesAndCommit(inputs) => inputs.verify_sorted_and_unique(),
 		}
 	}
 
+	/// Sort the inputs.
 	fn sort_unstable(&mut self) {
 		match self {
+			Inputs::CommitOnly(inputs) => inputs.sort_unstable(),
 			Inputs::FeaturesAndCommit(inputs) => inputs.sort_unstable(),
 		}
 	}
+
+	/// For debug purposes only. Do not rely on this for anything.
+	pub fn version_str(&self) -> &str {
+		match self {
+			Inputs::CommitOnly(_) => "v3",
+			Inputs::FeaturesAndCommit(_) => "v2",
+		}
+	}
 }
 
 // Enum of various supported kernel "features".
@@ -1752,14 +1881,6 @@ impl AsRef<Commitment> for Output {
 	}
 }
 
-impl ::std::hash::Hash for Output {
-	fn hash<H: ::std::hash::Hasher>(&self, state: &mut H) {
-		let mut vec = Vec::new();
-		ser::serialize_default(&mut vec, &self).expect("serialization failed");
-		::std::hash::Hash::hash(&vec, state);
-	}
-}
-
 /// Implementation of Writeable for a transaction Output, defines how to write
 /// an Output as binary.
 impl Writeable for Output {

core/src/global.rs

@@ -23,9 +23,12 @@ use crate::consensus::{
 	PROOFSIZE, SECOND_POW_EDGE_BITS, STATE_SYNC_THRESHOLD,
 };
 use crate::core::block::HeaderVersion;
-use crate::pow::{
-	self, new_cuckaroo_ctx, new_cuckarood_ctx, new_cuckaroom_ctx, new_cuckarooz_ctx,
-	new_cuckatoo_ctx, PoWContext,
+use crate::{
+	pow::{
+		self, new_cuckaroo_ctx, new_cuckarood_ctx, new_cuckaroom_ctx, new_cuckarooz_ctx,
+		new_cuckatoo_ctx, PoWContext,
+	},
+	ser::ProtocolVersion,
 };
 use std::cell::Cell;
 use util::OneTime;
@@ -42,7 +45,7 @@ use util::OneTime;
 /// Note: We also use a specific (possible different) protocol version
 /// for both the backend database and MMR data files.
 /// This defines the p2p layer protocol version for this node.
-pub const PROTOCOL_VERSION: u32 = 2;
+pub const PROTOCOL_VERSION: ProtocolVersion = ProtocolVersion(3);
 
 /// Automated testing edge_bits
 pub const AUTOMATED_TESTING_MIN_EDGE_BITS: u8 = 10;

core/src/ser.rs

@@ -69,6 +69,8 @@ pub enum Error {
 	DuplicateError,
 	/// Block header version (hard-fork schedule).
 	InvalidBlockVersion,
+	/// Unsupported protocol version
+	UnsupportedProtocolVersion,
 }
 
 impl From<io::Error> for Error {
@@ -92,6 +94,7 @@ impl fmt::Display for Error {
 			Error::TooLargeReadErr => f.write_str("too large read"),
 			Error::HexError(ref e) => write!(f, "hex error {:?}", e),
 			Error::InvalidBlockVersion => f.write_str("invalid block version"),
+			Error::UnsupportedProtocolVersion => f.write_str("unsupported protocol version"),
 		}
 	}
 }
@@ -115,6 +118,7 @@ impl error::Error for Error {
 			Error::TooLargeReadErr => "too large read",
 			Error::HexError(_) => "hex error",
 			Error::InvalidBlockVersion => "invalid block version",
+			Error::UnsupportedProtocolVersion => "unsupported protocol version",
 		}
 	}
 }
@@ -128,6 +132,16 @@ pub enum SerializationMode {
 	Hash,
 }
 
+impl SerializationMode {
+	/// Hash mode?
+	pub fn is_hash_mode(&self) -> bool {
+		match self {
+			SerializationMode::Hash => true,
+			_ => false,
+		}
+	}
+}
+
 /// Implementations defined how different numbers and binary structures are
 /// written to an underlying stream or container (depending on implementation).
 pub trait Writer {
@@ -319,7 +333,7 @@ impl ProtocolVersion {
 	/// negotiation in the p2p layer. Connected peers will negotiate a suitable
 	/// protocol version for serialization/deserialization of p2p messages.
 	pub fn local() -> ProtocolVersion {
-		ProtocolVersion(PROTOCOL_VERSION)
+		PROTOCOL_VERSION
 	}
 
 	/// We need to specify a protocol version for our local database.

core/tests/block.rs

@@ -19,7 +19,7 @@ use crate::core::core::block::{Block, BlockHeader, Error, HeaderVersion, Untrust
 use crate::core::core::hash::Hashed;
 use crate::core::core::id::ShortIdentifiable;
 use crate::core::core::transaction::{
-	self, KernelFeatures, NRDRelativeHeight, Output, OutputFeatures, Transaction,
+	self, KernelFeatures, NRDRelativeHeight, Output, OutputFeatures, OutputIdentifier, Transaction,
 };
 use crate::core::core::verifier_cache::{LruVerifierCache, VerifierCache};
 use crate::core::core::{Committed, CompactBlock};
@@ -522,9 +522,53 @@ fn block_single_tx_serialized_size() {
 	let prev = BlockHeader::default();
 	let key_id = ExtKeychain::derive_key_id(1, 1, 0, 0, 0);
 	let b = new_block(&[tx1], &keychain, &builder, &prev, &key_id);
+
+	// Default protocol version (3)
 	let mut vec = Vec::new();
 	ser::serialize_default(&mut vec, &b).expect("serialization failed");
+	assert_eq!(vec.len(), 2_669);
+
+	// Protocol version 3
+	let mut vec = Vec::new();
+	ser::serialize(&mut vec, ser::ProtocolVersion(3), &b).expect("serialization failed");
+	assert_eq!(vec.len(), 2_669);
+
+	// Protocol version 2.
+	// Note: block must be in "v2" compatibility with "features and commit" inputs for this.
+	// Normally we would convert the block by looking inputs up in utxo but we fake it here for testing.
+	let inputs: Vec<_> = b.inputs().into();
+	let inputs: Vec<_> = inputs
+		.iter()
+		.map(|input| OutputIdentifier {
+			features: OutputFeatures::Plain,
+			commit: input.commitment(),
+		})
+		.collect();
+	let b = Block {
+		header: b.header,
+		body: b.body.replace_inputs(inputs.as_slice().into()),
+	};
+
+	// Protocol version 2
+	let mut vec = Vec::new();
+	ser::serialize(&mut vec, ser::ProtocolVersion(2), &b).expect("serialization failed");
 	assert_eq!(vec.len(), 2_670);
+
+	// Protocol version 1 (fixed size kernels)
+	let mut vec = Vec::new();
+	ser::serialize(&mut vec, ser::ProtocolVersion(1), &b).expect("serialization failed");
+	assert_eq!(vec.len(), 2_694);
+
+	// Check we can also serialize a v2 compatibility block in v3 protocol version
+	// without needing to explicitly convert the block.
+	let mut vec = Vec::new();
+	ser::serialize(&mut vec, ser::ProtocolVersion(3), &b).expect("serialization failed");
+	assert_eq!(vec.len(), 2_669);
+
+	// Default protocol version (3) for completeness
+	let mut vec = Vec::new();
+	ser::serialize_default(&mut vec, &b).expect("serialization failed");
+	assert_eq!(vec.len(), 2_669);
 }
 
 #[test]
@@ -571,25 +615,10 @@ fn block_10_tx_serialized_size() {
 	let key_id = ExtKeychain::derive_key_id(1, 1, 0, 0, 0);
 	let b = new_block(&txs, &keychain, &builder, &prev, &key_id);
 
-	// Default protocol version.
 	{
 		let mut vec = Vec::new();
 		ser::serialize_default(&mut vec, &b).expect("serialization failed");
-		assert_eq!(vec.len(), 16_836);
-	}
-
-	// Explicit protocol version 1
-	{
-		let mut vec = Vec::new();
-		ser::serialize(&mut vec, ser::ProtocolVersion(1), &b).expect("serialization failed");
-		assert_eq!(vec.len(), 16_932);
-	}
-
-	// Explicit protocol version 2
-	{
-		let mut vec = Vec::new();
-		ser::serialize(&mut vec, ser::ProtocolVersion(2), &b).expect("serialization failed");
-		assert_eq!(vec.len(), 16_836);
+		assert_eq!(vec.len(), 16_826);
 	}
 }
 
@@ -732,14 +761,13 @@ fn same_amount_outputs_copy_range_proof() {
 
 	// now we reconstruct the transaction, swapping the rangeproofs so they
 	// have the wrong privkey
-	let ins: Vec<_> = tx.inputs().into();
 	let mut outs = tx.outputs().to_vec();
 	outs[0].proof = outs[1].proof;
 
 	let key_id = keychain::ExtKeychain::derive_key_id(1, 4, 0, 0, 0);
 	let prev = BlockHeader::default();
 	let b = new_block(
-		&[Transaction::new(&ins, &outs, tx.kernels())],
+		&[Transaction::new(tx.inputs(), &outs, tx.kernels())],
 		&keychain,
 		&builder,
 		&prev,
@@ -784,7 +812,6 @@ fn wrong_amount_range_proof() {
 	.unwrap();
 
 	// we take the range proofs from tx2 into tx1 and rebuild the transaction
-	let ins: Vec<_> = tx1.inputs().into();
 	let mut outs = tx1.outputs().to_vec();
 	outs[0].proof = tx2.outputs()[0].proof;
 	outs[1].proof = tx2.outputs()[1].proof;
@@ -792,7 +819,7 @@ fn wrong_amount_range_proof() {
 	let key_id = keychain::ExtKeychain::derive_key_id(1, 4, 0, 0, 0);
 	let prev = BlockHeader::default();
 	let b = new_block(
-		&[Transaction::new(&ins, &outs, tx1.kernels())],
+		&[Transaction::new(tx1.inputs(), &outs, tx1.kernels())],
 		&keychain,
 		&builder,
 		&prev,

core/tests/common.rs

@@ -15,7 +15,9 @@
 //! Common test functions
 
 use grin_core::core::hash::DefaultHashable;
-use grin_core::core::{Block, BlockHeader, KernelFeatures, Transaction};
+use grin_core::core::{
+	Block, BlockHeader, KernelFeatures, OutputFeatures, OutputIdentifier, Transaction,
+};
 use grin_core::libtx::{
 	build::{self, input, output},
 	proof::{ProofBuild, ProofBuilder},
@@ -64,6 +66,24 @@ pub fn tx1i1o() -> Transaction {
 	tx
 }
 
+#[allow(dead_code)]
+pub fn tx1i10_v2_compatible() -> Transaction {
+	let tx = tx1i1o();
+
+	let inputs: Vec<_> = tx.inputs().into();
+	let inputs: Vec<_> = inputs
+		.iter()
+		.map(|input| OutputIdentifier {
+			features: OutputFeatures::Plain,
+			commit: input.commitment(),
+		})
+		.collect();
+	Transaction {
+		body: tx.body.replace_inputs(inputs.as_slice().into()),
+		..tx
+	}
+}
+
 // utility producing a transaction with a single input
 // and two outputs (one change output)
 // Note: this tx has an "offset" kernel

core/tests/core.rs

@@ -21,7 +21,8 @@ use self::core::core::block::Error::KernelLockHeight;
 use self::core::core::hash::{Hashed, ZERO_HASH};
 use self::core::core::verifier_cache::{LruVerifierCache, VerifierCache};
 use self::core::core::{
-	aggregate, deaggregate, KernelFeatures, Output, Transaction, TxKernel, Weighting,
+	aggregate, deaggregate, KernelFeatures, Output, OutputFeatures, OutputIdentifier, Transaction,
+	TxKernel, Weighting,
 };
 use self::core::libtx::build::{self, initial_tx, input, output, with_excess};
 use self::core::libtx::{aggsig, ProofBuilder};
@@ -42,26 +43,51 @@ fn test_setup() {
 fn simple_tx_ser() {
 	let tx = tx2i1o();
 
-	// Default protocol version.
-	{
-		let mut vec = Vec::new();
-		ser::serialize_default(&mut vec, &tx).expect("serialization failed");
-		assert_eq!(vec.len(), 947);
-	}
+	// Default protocol version (3).
+	let mut vec = Vec::new();
+	ser::serialize_default(&mut vec, &tx).expect("serialization failed");
+	assert_eq!(vec.len(), 945);
+
+	// Explicit protocol version 3.
+	let mut vec = Vec::new();
+	ser::serialize(&mut vec, ser::ProtocolVersion(3), &tx).expect("serialization failed");
+	assert_eq!(vec.len(), 945);
+
+	// We need to convert the tx to v2 compatibility with "features and commitment" inputs
+	// to serialize to any previous protocol version.
+	// Normally we would do this conversion against the utxo and txpool but we fake it here for testing.
+	let inputs: Vec<_> = tx.inputs().into();
+	let inputs: Vec<_> = inputs
+		.iter()
+		.map(|input| OutputIdentifier {
+			features: OutputFeatures::Plain,
+			commit: input.commitment(),
+		})
+		.collect();
+	let tx = Transaction {
+		body: tx.body.replace_inputs(inputs.as_slice().into()),
+		..tx
+	};
 
 	// Explicit protocol version 1.
-	{
-		let mut vec = Vec::new();
-		ser::serialize(&mut vec, ser::ProtocolVersion(1), &tx).expect("serialization failed");
-		assert_eq!(vec.len(), 955);
-	}
+	let mut vec = Vec::new();
+	ser::serialize(&mut vec, ser::ProtocolVersion(1), &tx).expect("serialization failed");
+	assert_eq!(vec.len(), 955);
 
 	// Explicit protocol version 2.
-	{
-		let mut vec = Vec::new();
-		ser::serialize(&mut vec, ser::ProtocolVersion(2), &tx).expect("serialization failed");
-		assert_eq!(vec.len(), 947);
-	}
+	let mut vec = Vec::new();
+	ser::serialize(&mut vec, ser::ProtocolVersion(2), &tx).expect("serialization failed");
+	assert_eq!(vec.len(), 947);
+
+	// Check we can still serialize to protocol version 3 without explicitly converting the tx.
+	let mut vec = Vec::new();
+	ser::serialize(&mut vec, ser::ProtocolVersion(3), &tx).expect("serialization failed");
+	assert_eq!(vec.len(), 945);
+
+	// And default protocol version for completeness.
+	let mut vec = Vec::new();
+	ser::serialize_default(&mut vec, &tx).expect("serialization failed");
+	assert_eq!(vec.len(), 945);
 }
 
 #[test]

core/tests/transaction.rs

@@ -15,7 +15,7 @@
 //! Transaction integration tests
 
 pub mod common;
-use crate::common::tx1i1o;
+use crate::common::tx1i10_v2_compatible;
 use crate::core::core::transaction::{self, Error};
 use crate::core::core::verifier_cache::LruVerifierCache;
 use crate::core::core::{KernelFeatures, Output, OutputFeatures, Transaction, Weighting};
@@ -32,8 +32,10 @@ use util::RwLock;
 // This test ensures we exercise this serialization/deserialization code.
 #[test]
 fn test_transaction_json_ser_deser() {
-	let tx1 = tx1i1o();
+	let tx1 = tx1i10_v2_compatible();
+
 	let value = serde_json::to_value(&tx1).unwrap();
+	println!("{:?}", value);
 
 	assert!(value["offset"].is_string());
 	assert_eq!(value["body"]["inputs"][0]["features"], "Plain");
@@ -50,7 +52,6 @@ fn test_transaction_json_ser_deser() {
 	let tx2: Transaction = serde_json::from_value(value).unwrap();
 	assert_eq!(tx1, tx2);
 
-	let tx1 = tx1i1o();
 	let str = serde_json::to_string(&tx1).unwrap();
 	println!("{}", str);
 	let tx2: Transaction = serde_json::from_str(&str).unwrap();