Pool tx weight verification (#2466)

core/src/core/block.rs

@@ -27,7 +27,9 @@ use crate::core::committed::{self, Committed};
 use crate::core::compact_block::{CompactBlock, CompactBlockBody};
 use crate::core::hash::{Hash, Hashed, ZERO_HASH};
 use crate::core::verifier_cache::VerifierCache;
-use crate::core::{transaction, Commitment, Input, Output, Transaction, TransactionBody, TxKernel};
+use crate::core::{
+	transaction, Commitment, Input, Output, Transaction, TransactionBody, TxKernel, Weighting,
+};
 use crate::global;
 use crate::keychain::{self, BlindingFactor};
 use crate::pow::{Difficulty, Proof, ProofOfWork};
@@ -377,7 +379,7 @@ impl Readable for Block {
 		// Treat any validation issues as data corruption.
 		// An example of this would be reading a block
 		// that exceeded the allowed number of inputs.
-		body.validate_read(true)
+		body.validate_read(Weighting::AsBlock)
 			.map_err(|_| ser::Error::CorruptedData)?;
 
 		Ok(Block { header, body })
@@ -608,7 +610,7 @@ impl Block {
 	/// * coinbase sum verification
 	/// * kernel sum verification
 	pub fn validate_read(&self) -> Result<(), Error> {
-		self.body.validate_read(true)?;
+		self.body.validate_read(Weighting::AsBlock)?;
 		self.verify_kernel_lock_heights()?;
 		Ok(())
 	}
@@ -638,7 +640,7 @@ impl Block {
 		prev_kernel_offset: &BlindingFactor,
 		verifier: Arc<RwLock<dyn VerifierCache>>,
 	) -> Result<Commitment, Error> {
-		self.body.validate(true, verifier)?;
+		self.body.validate(Weighting::AsBlock, verifier)?;
 
 		self.verify_kernel_lock_heights()?;
 		self.verify_coinbase()?;

core/src/core/transaction.rs

@@ -365,6 +365,22 @@ impl FixedLength for TxKernelEntry {
 		+ secp::constants::AGG_SIGNATURE_SIZE;
 }
 
+/// Enum of possible tx weight verification options -
+///
+/// * As "transaction" checks tx (as block) weight does not exceed max_block_weight.
+/// * As "block" same as above but allow for additional coinbase reward (1 output, 1 kernel).
+/// * With "no limit" to skip the weight check.
+///
+#[derive(Clone, Copy)]
+pub enum Weighting {
+	/// Tx represents a tx (max block weight, accounting for additional coinbase reward).
+	AsTransaction,
+	/// Tx represents a block (max block weight).
+	AsBlock,
+	/// No max weight limit (skip the weight check).
+	NoLimit,
+}
+
 /// TransactionBody is a common abstraction for transaction and block
 #[derive(Serialize, Deserialize, Debug, Clone)]
 pub struct TransactionBody {
@@ -587,11 +603,36 @@ impl TransactionBody {
 			.unwrap_or(0)
 	}
 
-	// Verify the body is not too big in terms of number of inputs|outputs|kernels.
-	fn verify_weight(&self, with_reward: bool) -> Result<(), Error> {
-		// if as_block check the body as if it was a block, with an additional output and
-		// kernel for reward
-		let reserve = if with_reward { 0 } else { 1 };
+	/// Verify the body is not too big in terms of number of inputs|outputs|kernels.
+	/// Weight rules vary depending on the "weight type" (block or tx or pool).
+	fn verify_weight(&self, weighting: Weighting) -> Result<(), Error> {
+		// If "tx" body then remember to reduce the max_block_weight for block requirements.
+		// If "block" body then verify weight based on full set of inputs|outputs|kernels.
+		// If "pool" body then skip weight verification (pool can be larger than single block).
+		//
+		// Note: Taking a max tx and building a block from it we need to allow room
+		// for the additional coinbase reward (output + kernel).
+		//
+		let reserve = match weighting {
+			Weighting::AsTransaction => 1,
+			Weighting::AsBlock => 0,
+			Weighting::NoLimit => {
+				// We do not verify "tx as pool" weight so we are done here.
+				return Ok(());
+			}
+		};
+
+		// Note: If we add a reserve then we are reducing the number of outputs and kernels
+		// allowed in the body itself.
+		// A block is allowed to be slightly weightier than a tx to account for
+		// the additional coinbase reward output and kernel.
+		// i.e. We need to reserve space for coinbase reward when verifying tx weight.
+		//
+		// In effect we are verifying the tx _can_ be included in a block without exceeding
+		// MAX_BLOCK_WEIGHT.
+		//
+		// max_block = max_tx + coinbase
+		//
 		let tx_block_weight = TransactionBody::weight_as_block(
 			self.inputs.len(),
 			self.outputs.len() + reserve,
@@ -656,8 +697,8 @@ impl TransactionBody {
 	/// Subset of full validation that skips expensive verification steps, specifically -
 	/// * rangeproof verification
 	/// * kernel signature verification
-	pub fn validate_read(&self, with_reward: bool) -> Result<(), Error> {
-		self.verify_weight(with_reward)?;
+	pub fn validate_read(&self, weighting: Weighting) -> Result<(), Error> {
+		self.verify_weight(weighting)?;
 		self.verify_sorted()?;
 		self.verify_cut_through()?;
 		Ok(())
@@ -668,10 +709,10 @@ impl TransactionBody {
 	/// output.
 	pub fn validate(
 		&self,
-		with_reward: bool,
+		weighting: Weighting,
 		verifier: Arc<RwLock<dyn VerifierCache>>,
 	) -> Result<(), Error> {
-		self.validate_read(with_reward)?;
+		self.validate_read(weighting)?;
 
 		// Find all the outputs that have not had their rangeproofs verified.
 		let outputs = {
@@ -892,7 +933,7 @@ impl Transaction {
 	/// * kernel signature verification (on the body)
 	/// * kernel sum verification
 	pub fn validate_read(&self) -> Result<(), Error> {
-		self.body.validate_read(false)?;
+		self.body.validate_read(Weighting::AsTransaction)?;
 		self.body.verify_features()?;
 		Ok(())
 	}
@@ -900,8 +941,12 @@ impl Transaction {
 	/// Validates all relevant parts of a fully built transaction. Checks the
 	/// excess value against the signature as well as range proofs for each
 	/// output.
-	pub fn validate(&self, verifier: Arc<RwLock<dyn VerifierCache>>) -> Result<(), Error> {
-		self.body.validate(false, verifier)?;
+	pub fn validate(
+		&self,
+		weighting: Weighting,
+		verifier: Arc<RwLock<dyn VerifierCache>>,
+	) -> Result<(), Error> {
+		self.body.validate(weighting, verifier)?;
 		self.body.verify_features()?;
 		self.verify_kernel_sums(self.overage(), self.offset)?;
 		Ok(())

core/src/libtx/build.rs

@@ -256,6 +256,7 @@ mod test {
 	use std::sync::Arc;
 
 	use super::*;
+	use crate::core::transaction::Weighting;
 	use crate::core::verifier_cache::{LruVerifierCache, VerifierCache};
 	use crate::keychain::{ExtKeychain, ExtKeychainPath};
 
@@ -283,7 +284,7 @@ mod test {
 		)
 		.unwrap();
 
-		tx.validate(vc.clone()).unwrap();
+		tx.validate(Weighting::AsTransaction, vc.clone()).unwrap();
 	}
 
 	#[test]
@@ -306,7 +307,7 @@ mod test {
 		)
 		.unwrap();
 
-		tx.validate(vc.clone()).unwrap();
+		tx.validate(Weighting::AsTransaction, vc.clone()).unwrap();
 	}
 
 	#[test]
@@ -323,6 +324,6 @@ mod test {
 		)
 		.unwrap();
 
-		tx.validate(vc.clone()).unwrap();
+		tx.validate(Weighting::AsTransaction, vc.clone()).unwrap();
 	}
 }

core/src/libtx/slate.rs

@@ -18,7 +18,7 @@
 use crate::blake2::blake2b::blake2b;
 use crate::core::amount_to_hr_string;
 use crate::core::committed::Committed;
-use crate::core::transaction::{kernel_features, kernel_sig_msg, Transaction};
+use crate::core::transaction::{kernel_features, kernel_sig_msg, Transaction, Weighting};
 use crate::core::verifier_cache::LruVerifierCache;
 use crate::keychain::{BlindSum, BlindingFactor, Keychain};
 use crate::libtx::error::{Error, ErrorKind};
@@ -472,8 +472,9 @@ impl Slate {
 		final_tx.kernels()[0].verify()?;
 
 		// confirm the overall transaction is valid (including the updated kernel)
+		// accounting for tx weight limits
 		let verifier_cache = Arc::new(RwLock::new(LruVerifierCache::new()));
-		let _ = final_tx.validate(verifier_cache)?;
+		let _ = final_tx.validate(Weighting::AsTransaction, verifier_cache)?;
 
 		self.tx = final_tx;
 		Ok(())

core/tests/core.rs

@@ -20,7 +20,7 @@ use self::core::core::block::BlockHeader;
 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};
+use self::core::core::{aggregate, deaggregate, KernelFeatures, Output, Transaction, Weighting};
 use self::core::libtx::build::{
 	self, initial_tx, input, output, with_excess, with_fee, with_lock_height,
 };
@@ -115,7 +115,8 @@ fn build_tx_kernel() {
 	.unwrap();
 
 	// check the tx is valid
-	tx.validate(verifier_cache()).unwrap();
+	tx.validate(Weighting::AsTransaction, verifier_cache())
+		.unwrap();
 
 	// check the kernel is also itself valid
 	assert_eq!(tx.kernels().len(), 1);
@@ -133,15 +134,19 @@ fn transaction_cut_through() {
 	let tx1 = tx1i2o();
 	let tx2 = tx2i1o();
 
-	assert!(tx1.validate(verifier_cache()).is_ok());
-	assert!(tx2.validate(verifier_cache()).is_ok());
+	assert!(tx1
+		.validate(Weighting::AsTransaction, verifier_cache())
+		.is_ok());
+	assert!(tx2
+		.validate(Weighting::AsTransaction, verifier_cache())
+		.is_ok());
 
 	let vc = verifier_cache();
 
 	// now build a "cut_through" tx from tx1 and tx2
 	let tx3 = aggregate(vec![tx1, tx2]).unwrap();
 
-	assert!(tx3.validate(vc.clone()).is_ok());
+	assert!(tx3.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 }
 
 // Attempt to deaggregate a multi-kernel transaction in a different way
@@ -154,26 +159,32 @@ fn multi_kernel_transaction_deaggregation() {
 
 	let vc = verifier_cache();
 
-	assert!(tx1.validate(vc.clone()).is_ok());
-	assert!(tx2.validate(vc.clone()).is_ok());
-	assert!(tx3.validate(vc.clone()).is_ok());
-	assert!(tx4.validate(vc.clone()).is_ok());
+	assert!(tx1.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx2.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx3.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx4.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 
 	let tx1234 = aggregate(vec![tx1.clone(), tx2.clone(), tx3.clone(), tx4.clone()]).unwrap();
 	let tx12 = aggregate(vec![tx1.clone(), tx2.clone()]).unwrap();
 	let tx34 = aggregate(vec![tx3.clone(), tx4.clone()]).unwrap();
 
-	assert!(tx1234.validate(vc.clone()).is_ok());
-	assert!(tx12.validate(vc.clone()).is_ok());
-	assert!(tx34.validate(vc.clone()).is_ok());
+	assert!(tx1234
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
+	assert!(tx12.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx34.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 
 	let deaggregated_tx34 = deaggregate(tx1234.clone(), vec![tx12.clone()]).unwrap();
-	assert!(deaggregated_tx34.validate(vc.clone()).is_ok());
+	assert!(deaggregated_tx34
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
 	assert_eq!(tx34, deaggregated_tx34);
 
 	let deaggregated_tx12 = deaggregate(tx1234.clone(), vec![tx34.clone()]).unwrap();
 
-	assert!(deaggregated_tx12.validate(vc.clone()).is_ok());
+	assert!(deaggregated_tx12
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
 	assert_eq!(tx12, deaggregated_tx12);
 }
 
@@ -185,18 +196,20 @@ fn multi_kernel_transaction_deaggregation_2() {
 
 	let vc = verifier_cache();
 
-	assert!(tx1.validate(vc.clone()).is_ok());
-	assert!(tx2.validate(vc.clone()).is_ok());
-	assert!(tx3.validate(vc.clone()).is_ok());
+	assert!(tx1.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx2.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx3.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 
 	let tx123 = aggregate(vec![tx1.clone(), tx2.clone(), tx3.clone()]).unwrap();
 	let tx12 = aggregate(vec![tx1.clone(), tx2.clone()]).unwrap();
 
-	assert!(tx123.validate(vc.clone()).is_ok());
-	assert!(tx12.validate(vc.clone()).is_ok());
+	assert!(tx123.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx12.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 
 	let deaggregated_tx3 = deaggregate(tx123.clone(), vec![tx12.clone()]).unwrap();
-	assert!(deaggregated_tx3.validate(vc.clone()).is_ok());
+	assert!(deaggregated_tx3
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
 	assert_eq!(tx3, deaggregated_tx3);
 }
 
@@ -208,19 +221,21 @@ fn multi_kernel_transaction_deaggregation_3() {
 
 	let vc = verifier_cache();
 
-	assert!(tx1.validate(vc.clone()).is_ok());
-	assert!(tx2.validate(vc.clone()).is_ok());
-	assert!(tx3.validate(vc.clone()).is_ok());
+	assert!(tx1.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx2.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx3.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 
 	let tx123 = aggregate(vec![tx1.clone(), tx2.clone(), tx3.clone()]).unwrap();
 	let tx13 = aggregate(vec![tx1.clone(), tx3.clone()]).unwrap();
 	let tx2 = aggregate(vec![tx2.clone()]).unwrap();
 
-	assert!(tx123.validate(vc.clone()).is_ok());
-	assert!(tx2.validate(vc.clone()).is_ok());
+	assert!(tx123.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx2.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 
 	let deaggregated_tx13 = deaggregate(tx123.clone(), vec![tx2.clone()]).unwrap();
-	assert!(deaggregated_tx13.validate(vc.clone()).is_ok());
+	assert!(deaggregated_tx13
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
 	assert_eq!(tx13, deaggregated_tx13);
 }
 
@@ -234,11 +249,11 @@ fn multi_kernel_transaction_deaggregation_4() {
 
 	let vc = verifier_cache();
 
-	assert!(tx1.validate(vc.clone()).is_ok());
-	assert!(tx2.validate(vc.clone()).is_ok());
-	assert!(tx3.validate(vc.clone()).is_ok());
-	assert!(tx4.validate(vc.clone()).is_ok());
-	assert!(tx5.validate(vc.clone()).is_ok());
+	assert!(tx1.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx2.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx3.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx4.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx5.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 
 	let tx12345 = aggregate(vec![
 		tx1.clone(),
@@ -248,14 +263,18 @@ fn multi_kernel_transaction_deaggregation_4() {
 		tx5.clone(),
 	])
 	.unwrap();
-	assert!(tx12345.validate(vc.clone()).is_ok());
+	assert!(tx12345
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
 
 	let deaggregated_tx5 = deaggregate(
 		tx12345.clone(),
 		vec![tx1.clone(), tx2.clone(), tx3.clone(), tx4.clone()],
 	)
 	.unwrap();
-	assert!(deaggregated_tx5.validate(vc.clone()).is_ok());
+	assert!(deaggregated_tx5
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
 	assert_eq!(tx5, deaggregated_tx5);
 }
 
@@ -269,11 +288,11 @@ fn multi_kernel_transaction_deaggregation_5() {
 
 	let vc = verifier_cache();
 
-	assert!(tx1.validate(vc.clone()).is_ok());
-	assert!(tx2.validate(vc.clone()).is_ok());
-	assert!(tx3.validate(vc.clone()).is_ok());
-	assert!(tx4.validate(vc.clone()).is_ok());
-	assert!(tx5.validate(vc.clone()).is_ok());
+	assert!(tx1.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx2.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx3.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx4.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx5.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 
 	let tx12345 = aggregate(vec![
 		tx1.clone(),
@@ -286,10 +305,14 @@ fn multi_kernel_transaction_deaggregation_5() {
 	let tx12 = aggregate(vec![tx1.clone(), tx2.clone()]).unwrap();
 	let tx34 = aggregate(vec![tx3.clone(), tx4.clone()]).unwrap();
 
-	assert!(tx12345.validate(vc.clone()).is_ok());
+	assert!(tx12345
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
 
 	let deaggregated_tx5 = deaggregate(tx12345.clone(), vec![tx12.clone(), tx34.clone()]).unwrap();
-	assert!(deaggregated_tx5.validate(vc.clone()).is_ok());
+	assert!(deaggregated_tx5
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
 	assert_eq!(tx5, deaggregated_tx5);
 }
 
@@ -301,22 +324,26 @@ fn basic_transaction_deaggregation() {
 
 	let vc = verifier_cache();
 
-	assert!(tx1.validate(vc.clone()).is_ok());
-	assert!(tx2.validate(vc.clone()).is_ok());
+	assert!(tx1.validate(Weighting::AsTransaction, vc.clone()).is_ok());
+	assert!(tx2.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 
 	// now build a "cut_through" tx from tx1 and tx2
 	let tx3 = aggregate(vec![tx1.clone(), tx2.clone()]).unwrap();
 
-	assert!(tx3.validate(vc.clone()).is_ok());
+	assert!(tx3.validate(Weighting::AsTransaction, vc.clone()).is_ok());
 
 	let deaggregated_tx1 = deaggregate(tx3.clone(), vec![tx2.clone()]).unwrap();
 
-	assert!(deaggregated_tx1.validate(vc.clone()).is_ok());
+	assert!(deaggregated_tx1
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
 	assert_eq!(tx1, deaggregated_tx1);
 
 	let deaggregated_tx2 = deaggregate(tx3.clone(), vec![tx1.clone()]).unwrap();
 
-	assert!(deaggregated_tx2.validate(vc.clone()).is_ok());
+	assert!(deaggregated_tx2
+		.validate(Weighting::AsTransaction, vc.clone())
+		.is_ok());
 	assert_eq!(tx2, deaggregated_tx2);
 }
 
@@ -347,7 +374,9 @@ fn hash_output() {
 #[test]
 fn blind_tx() {
 	let btx = tx2i1o();
-	assert!(btx.validate(verifier_cache()).is_ok());
+	assert!(btx
+		.validate(Weighting::AsTransaction, verifier_cache())
+		.is_ok());
 
 	// Ignored for bullet proofs, because calling range_proof_info
 	// with a bullet proof causes painful errors
@@ -410,7 +439,9 @@ fn tx_build_exchange() {
 	)
 	.unwrap();
 
-	tx_final.validate(verifier_cache()).unwrap();
+	tx_final
+		.validate(Weighting::AsTransaction, verifier_cache())
+		.unwrap();
 }
 
 #[test]
@@ -436,7 +467,7 @@ fn reward_with_tx_block() {
 	let vc = verifier_cache();
 
 	let mut tx1 = tx2i1o();
-	tx1.validate(vc.clone()).unwrap();
+	tx1.validate(Weighting::AsTransaction, vc.clone()).unwrap();
 
 	let previous_header = BlockHeader::default();
 
@@ -524,11 +555,13 @@ fn test_block_with_timelocked_tx() {
 #[test]
 pub fn test_verify_1i1o_sig() {
 	let tx = tx1i1o();
-	tx.validate(verifier_cache()).unwrap();
+	tx.validate(Weighting::AsTransaction, verifier_cache())
+		.unwrap();
 }
 
 #[test]
 pub fn test_verify_2i1o_sig() {
 	let tx = tx2i1o();
-	tx.validate(verifier_cache()).unwrap();
+	tx.validate(Weighting::AsTransaction, verifier_cache())
+		.unwrap();
 }