[DNM] https://github.com/mimblewimble/grin/issues/3620

Cargo.toml

@@ -13,7 +13,7 @@ edition = "2018"
 
 [workspace]
 members = ["api", "chain", "config", "core", "keychain", "p2p", "servers", "store", "util", "pool"]
-exclude = ["etc/gen_gen"]
+exclude = ["etc/gen_gen", "etc/perf"]
 
 [[bin]]
 name = "grin"

chain/tests/mine_simple_chain.rs

@@ -817,6 +817,111 @@ fn output_header_mappings() {
 	clean_output_dir(".grin_header_for_output");
 }
 
+/// Test the duplicate rangeproof bug
+#[test]
+fn test_overflow_cached_rangeproof() {
+	clean_output_dir(".grin_overflow");
+	global::set_local_chain_type(ChainTypes::AutomatedTesting);
+
+	util::init_test_logger();
+	{
+		let chain = init_chain(".grin_overflow", pow::mine_genesis_block().unwrap());
+		let prev = chain.head_header().unwrap();
+		let kc = ExtKeychain::from_random_seed(false).unwrap();
+		let pb = ProofBuilder::new(&kc);
+
+		let mut head = prev;
+
+		// mine the first block and keep track of the block_hash
+		// so we can spend the coinbase later
+		let b = prepare_block(&kc, &head, &chain, 2);
+
+		assert!(b.outputs()[0].is_coinbase());
+		head = b.header.clone();
+		chain
+			.process_block(b.clone(), chain::Options::SKIP_POW)
+			.unwrap();
+
+		// now mine three further blocks
+		for n in 3..6 {
+			let b = prepare_block(&kc, &head, &chain, n);
+			head = b.header.clone();
+			chain.process_block(b, chain::Options::SKIP_POW).unwrap();
+		}
+
+		// create a few keys for use in txns
+		let key_id2 = ExtKeychainPath::new(1, 2, 0, 0, 0).to_identifier();
+		let key_id30 = ExtKeychainPath::new(1, 30, 0, 0, 0).to_identifier();
+		let key_id31 = ExtKeychainPath::new(1, 31, 0, 0, 0).to_identifier();
+		let key_id32 = ExtKeychainPath::new(1, 32, 0, 0, 0).to_identifier();
+
+		// build a regular transaction so we have a rangeproof to copy
+		let tx1 = build::transaction(
+			KernelFeatures::Plain { fee: 20000.into() },
+			&[
+				build::coinbase_input(consensus::REWARD, key_id2.clone()),
+				build::output(consensus::REWARD - 20000, key_id30.clone()),
+			],
+			&kc,
+			&pb,
+		)
+		.unwrap();
+
+		// mine block with tx1
+		let next = prepare_block_tx(&kc, &head, &chain, 7, &[tx1.clone()]);
+		let prev_main = next.header.clone();
+		chain
+			.process_block(next.clone(), chain::Options::SKIP_POW)
+			.unwrap();
+		chain.validate(false).unwrap();
+
+		// create a second tx that contains a negative output
+		// and a positive output for 1m grin
+		let mut tx2 = build::transaction(
+			KernelFeatures::Plain { fee: 0.into() },
+			&[
+				build::input(consensus::REWARD - 20000, key_id30.clone()),
+				build::output(
+					consensus::REWARD - 20000 + 1_000_000_000_000_000,
+					key_id31.clone(),
+				),
+				build::output_negative(1_000_000_000_000_000, key_id32.clone()),
+			],
+			&kc,
+			&pb,
+		)
+		.unwrap();
+
+		// make sure tx1 only has one output as expected
+		assert_eq!(tx1.body.outputs.len(), 1);
+		let last_rp = tx1.body.outputs[0].proof;
+
+		// overwrite all our rangeproofs with the rangeproof from last block
+		for i in 0..tx2.body.outputs.len() {
+			tx2.body.outputs[i].proof = last_rp;
+		}
+
+		let next = prepare_block_tx(&kc, &prev_main, &chain, 8, &[tx2.clone()]);
+		// process_block fails with verifier_cache disabled or with correct verifier_cache
+		// implementations
+		let res = chain.process_block(next, chain::Options::SKIP_POW);
+
+		match res {
+			Err(e) => {
+				// error should contain "Invalid Block Proof"
+				// TODO: Better way to check error
+				let error_string = format!("{}", e.to_string());
+				assert_eq!(error_string.contains("Invalid Block Proof"), true);
+			}
+			Ok(_) => {
+				// the block was accepted. This is wrong.
+				panic!("Negative output accepted into block!");
+			}
+		}
+	}
+	clean_output_dir(".grin_overflow");
+}
+
 // Use diff as both diff *and* key_idx for convenience (deterministic private key for test blocks)
 fn prepare_block<K>(kc: &K, prev: &BlockHeader, chain: &Chain, diff: u64) -> Block
 where

config/src/config.rs

@@ -180,7 +180,7 @@ impl GlobalConfig {
 					.unwrap()
 					.wallet_listener_url = "http://127.0.0.1:13415".to_owned();
 			}
-			global::ChainTypes::UserTesting => {
+			global::ChainTypes::UserTesting | global::ChainTypes::PerfTesting => {
 				defaults.api_http_addr = "127.0.0.1:23413".to_owned();
 				defaults.p2p_config.port = 23414;
 				defaults.p2p_config.seeding_type = p2p::Seeding::None;

core/src/consensus.rs

@@ -145,7 +145,9 @@ pub fn header_version(height: u64) -> HeaderVersion {
 	let hf_interval = (1 + height / HARD_FORK_INTERVAL) as u16;
 	match global::get_chain_type() {
 		global::ChainTypes::Mainnet => HeaderVersion(min(5, hf_interval)),
-		global::ChainTypes::AutomatedTesting | global::ChainTypes::UserTesting => {
+		global::ChainTypes::AutomatedTesting
+		| global::ChainTypes::UserTesting
+		| global::ChainTypes::PerfTesting => {
 			let testing_hf_interval = (1 + height / TESTING_HARD_FORK_INTERVAL) as u16;
 			HeaderVersion(min(5, testing_hf_interval))
 		}

core/src/core/transaction.rs

@@ -1246,23 +1246,42 @@ impl TransactionBody {
 	pub fn validate(
 		&self,
 		weighting: Weighting,
-		_verifier: Arc<RwLock<dyn VerifierCache>>,
+		verifier: Arc<RwLock<dyn VerifierCache>>,
 	) -> Result<(), Error> {
 		self.validate_read(weighting)?;
 
+		// Find all the outputs that have not had their rangeproofs verified.
+		let outputs = {
+			let mut verifier = verifier.write();
+			verifier.filter_rangeproof_unverified(&self.outputs)
+		};
+
 		// Now batch verify all those unverified rangeproofs
-		if !self.outputs.is_empty() {
+		if !outputs.is_empty() {
 			let mut commits = vec![];
 			let mut proofs = vec![];
-			for x in &self.outputs {
+			for x in &outputs {
 				commits.push(x.commitment());
 				proofs.push(x.proof);
 			}
 			Output::batch_verify_proofs(&commits, &proofs)?;
 		}
 
+		// Find all the kernels that have not yet been verified.
+		let kernels = {
+			let mut verifier = verifier.write();
+			verifier.filter_kernel_sig_unverified(&self.kernels)
+		};
+
 		// Verify the unverified tx kernels.
-		TxKernel::batch_sig_verify(&self.kernels)?;
+		TxKernel::batch_sig_verify(&kernels)?;
+
+		// Cache the successful verification results for the new outputs and kernels.
+		{
+			let mut verifier = verifier.write();
+			verifier.add_rangeproof_verified(outputs);
+			verifier.add_kernel_sig_verified(kernels);
+		}
 		Ok(())
 	}
 }
@@ -2092,6 +2111,8 @@ impl Readable for Output {
 	}
 }
 
+impl DefaultHashable for Output {}
+
 impl OutputFeatures {
 	/// Is this a coinbase output?
 	pub fn is_coinbase(self) -> bool {

core/src/core/verifier_cache.rs

@@ -73,11 +73,7 @@ impl VerifierCache for LruVerifierCache {
 	fn filter_rangeproof_unverified(&mut self, outputs: &[Output]) -> Vec<Output> {
 		let res = outputs
 			.iter()
-			.filter(|x| {
-				!self
-					.rangeproof_verification_cache
-					.contains_key(&x.proof.hash())
-			})
+			.filter(|x| !self.rangeproof_verification_cache.contains_key(&x.hash()))
 			.cloned()
 			.collect::<Vec<_>>();
 		trace!(
@@ -96,8 +92,75 @@ impl VerifierCache for LruVerifierCache {
 
 	fn add_rangeproof_verified(&mut self, outputs: Vec<Output>) {
 		for o in outputs {
-			self.rangeproof_verification_cache
-				.insert(o.proof.hash(), ());
+			self.rangeproof_verification_cache.insert(o.hash(), ());
 		}
 	}
 }
+
+#[cfg(test)]
+mod test {
+	#[test]
+	fn test_verifier_cache() {
+		use crate::core::transaction::KernelFeatures;
+		use crate::core::transaction::Output;
+		use crate::core::verifier_cache::LruVerifierCache;
+		use crate::core::verifier_cache::VerifierCache;
+		use crate::global;
+		use crate::libtx::build;
+		use crate::libtx::build::input;
+		use crate::libtx::build::output;
+		use crate::libtx::ProofBuilder;
+		use keychain::{ExtKeychain, Keychain};
+
+		// build some txns to use with cache.
+		global::set_local_chain_type(global::ChainTypes::AutomatedTesting);
+		let keychain = ExtKeychain::from_random_seed(false).unwrap();
+		let builder = ProofBuilder::new(&keychain);
+		let key_id1 = ExtKeychain::derive_key_id(1, 1, 0, 0, 0);
+		let key_id2 = ExtKeychain::derive_key_id(1, 2, 0, 0, 0);
+		let key_id3 = ExtKeychain::derive_key_id(1, 3, 0, 0, 0);
+		let key_id4 = ExtKeychain::derive_key_id(1, 4, 0, 0, 0);
+
+		let mut verifier_cache = LruVerifierCache::new();
+
+		let tx = build::transaction(
+			KernelFeatures::Plain { fee: 2.into() },
+			&[input(7, key_id1), output(5, key_id2)],
+			&keychain,
+			&builder,
+		)
+		.unwrap();
+
+		let tx2 = build::transaction(
+			KernelFeatures::Plain { fee: 3.into() },
+			&[input(9, key_id3), output(6, key_id4)],
+			&keychain,
+			&builder,
+		)
+		.unwrap();
+
+		let output1 = tx.outputs()[0];
+		let output2 = tx2.outputs()[0];
+		let mixed_output = Output {
+			identifier: output2.identifier,
+			proof: output1.proof,
+		};
+
+		// add first output to verifier cache
+		verifier_cache.add_rangeproof_verified(vec![output1]);
+
+		// filter output2, should not be removed
+		let outputs = verifier_cache.filter_rangeproof_unverified(&vec![output2]);
+		assert_eq!(outputs[0], output2);
+		assert_eq!(outputs.len(), 1);
+
+		// filter mixed_output (would fail before the fix)
+		let outputs = verifier_cache.filter_rangeproof_unverified(&vec![mixed_output]);
+		assert_eq!(outputs[0], mixed_output);
+		assert_eq!(outputs.len(), 1);
+
+		// filter output 1, which has been verified
+		let outputs = verifier_cache.filter_rangeproof_unverified(&vec![output1]);
+		assert_eq!(outputs.len(), 0);
+	}
+}