Private transactions processing error handling (#8431)

* Integration test for private transaction returned

* Do not interrupt verification in case of errors

* Helpers use specified

* Review comments fixed

Cargo.lock

@@ -858,6 +858,7 @@ dependencies = [
  "ethcore-light 1.11.0",
  "ethcore-network 1.11.0",
  "ethcore-network-devp2p 1.11.0",
+ "ethcore-private-tx 1.0.0",
  "ethcore-transaction 0.1.0",
  "ethereum-types 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "ethkey 0.3.0",

ethcore/private-tx/src/lib.rs

@@ -306,11 +306,6 @@ impl Provider where {
 	/// Retrieve and verify the first available private transaction for every sender
 	///
 	/// TODO [ToDr] It seems that:
-	/// 1. This method will fail on any error without removing invalid transaction.
-	/// 2. It means that the transaction will be stuck there forever and we will never be able to make any progress.
-	///
-	/// It might be more sensible to `drain()` transactions from the queue instead and process all of them,
-	/// possibly printing some errors in case of failures.
 	/// The 3 methods `ready_transaction,get_descriptor,remove` are always used in conjuction so most likely
 	/// can be replaced with a single `drain()` method instead.
 	/// Thanks to this we also don't really need to lock the entire verification for the time of execution.
@@ -339,13 +334,11 @@ impl Provider where {
 						trace!("Sending signature for private transaction: {:?}", signed_private_transaction);
 						self.broadcast_signed_private_transaction(signed_private_transaction.rlp_bytes().into_vec());
 					} else {
-						trace!("Incorrect type of action for the transaction");
+						warn!("Incorrect type of action for the transaction");
-						bail!(ErrorKind::BadTransactonType);
 					}
 				},
 				Err(e) => {
-					trace!("Cannot retrieve descriptor for transaction with error {:?}", e);
+					warn!("Cannot retrieve descriptor for transaction with error {:?}", e);
-					bail!(e);
 				}
 			}
 			verification_queue.remove_private_transaction(&transaction_hash);

ethcore/sync/Cargo.toml

@@ -35,3 +35,4 @@ ipnetwork = "0.12.6"
 [dev-dependencies]
 ethkey = { path = "../../ethkey" }
 kvdb-memorydb = { path = "../../util/kvdb-memorydb" }
+ethcore-private-tx = { path = "../private-tx" }

ethcore/sync/src/lib.rs

@@ -46,6 +46,7 @@ extern crate ethcore_light as light;
 #[cfg(test)] extern crate ethkey;
 #[cfg(test)] extern crate kvdb_memorydb;
 #[cfg(test)] extern crate rustc_hex;
+#[cfg(test)] extern crate ethcore_private_tx;
 
 #[macro_use]
 extern crate macros;

ethcore/sync/src/res/private_spec.json

@@ -0,0 +1,30 @@
+{
+	"name": "PrivateTransactions",
+	"engine": {
+		"instantSeal": null
+	},
+	"params": {
+		"gasLimitBoundDivisor": "0x0400",
+		"accountStartNonce": "0x0",
+		"maximumExtraDataSize": "0x20",
+		"minGasLimit": "0x1388",
+		"networkID" : "0x11"
+	},
+	"genesis": {
+		"seal": {
+			"generic": "0x0"
+		},
+		"difficulty": "0x20000",
+		"author": "0x0000000000000000000000000000000000000000",
+		"timestamp": "0x00",
+		"parentHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
+		"extraData": "0x",
+		"gasLimit": "0x989680"
+	},
+	"accounts": {
+		"0000000000000000000000000000000000000001": { "balance": "1", "builtin": { "name": "ecrecover", "pricing": { "linear": { "base": 3000, "word": 0 } } } },
+		"0000000000000000000000000000000000000002": { "balance": "1", "builtin": { "name": "sha256", "pricing": { "linear": { "base": 60, "word": 12 } } } },
+		"0000000000000000000000000000000000000003": { "balance": "1", "builtin": { "name": "ripemd160", "pricing": { "linear": { "base": 600, "word": 120 } } } },
+		"0000000000000000000000000000000000000004": { "balance": "1", "builtin": { "name": "identity", "pricing": { "linear": { "base": 15, "word": 3 } } } }
+	}
+}

ethcore/sync/src/tests/consensus.rs

@@ -48,7 +48,7 @@ fn authority_round() {
 	ap.insert_account(s1.secret().clone(), "").unwrap();
 
 	let chain_id = Spec::new_test_round().chain_id();
-	let mut net = TestNet::with_spec_and_accounts(2, SyncConfig::default(), Spec::new_test_round, Some(ap), false);
+	let mut net = TestNet::with_spec_and_accounts(2, SyncConfig::default(), Spec::new_test_round, Some(ap));
 	let io_handler0: Arc<IoHandler<ClientIoMessage>> = Arc::new(TestIoHandler::new(net.peer(0).chain.clone()));
 	let io_handler1: Arc<IoHandler<ClientIoMessage>> = Arc::new(TestIoHandler::new(net.peer(1).chain.clone()));
 	// Push transaction to both clients. Only one of them gets lucky to produce a block.
@@ -135,7 +135,7 @@ fn tendermint() {
 	ap.insert_account(s1.secret().clone(), "").unwrap();
 
 	let chain_id = Spec::new_test_tendermint().chain_id();
-	let mut net = TestNet::with_spec_and_accounts(2, SyncConfig::default(), Spec::new_test_tendermint, Some(ap), false);
+	let mut net = TestNet::with_spec_and_accounts(2, SyncConfig::default(), Spec::new_test_tendermint, Some(ap));
 	let io_handler0: Arc<IoHandler<ClientIoMessage>> = Arc::new(TestIoHandler::new(net.peer(0).chain.clone()));
 	let io_handler1: Arc<IoHandler<ClientIoMessage>> = Arc::new(TestIoHandler::new(net.peer(1).chain.clone()));
 	// Push transaction to both clients. Only one of them issues a proposal.

ethcore/sync/src/tests/helpers.rs

@@ -33,7 +33,7 @@ use io::{IoChannel, IoContext, IoHandler};
 use api::WARP_SYNC_PROTOCOL_ID;
 use chain::{ChainSync, ETH_PROTOCOL_VERSION_63, PAR_PROTOCOL_VERSION_3};
 use SyncConfig;
-use private_tx::{NoopPrivateTxHandler, PrivateTxHandler, SimplePrivateTxHandler};
+use private_tx::SimplePrivateTxHandler;
 
 pub trait FlushingBlockChainClient: BlockChainClient {
 	fn flush(&self) {}
@@ -210,7 +210,7 @@ pub struct EthPeer<C> where C: FlushingBlockChainClient {
 	pub snapshot_service: Arc<TestSnapshotService>,
 	pub sync: RwLock<ChainSync>,
 	pub queue: RwLock<VecDeque<TestPacket>>,
-	pub private_tx_handler: Arc<PrivateTxHandler>,
+	pub private_tx_handler: Arc<SimplePrivateTxHandler>,
 	pub io_queue: RwLock<VecDeque<ChainMessageType>>,
 	new_blocks_queue: RwLock<VecDeque<NewBlockMessage>>,
 }
@@ -335,7 +335,7 @@ impl TestNet<EthPeer<TestBlockChainClient>> {
 		for _ in 0..n {
 			let chain = TestBlockChainClient::new();
 			let ss = Arc::new(TestSnapshotService::new());
-			let private_tx_handler = Arc::new(NoopPrivateTxHandler);
+			let private_tx_handler = Arc::new(SimplePrivateTxHandler::default());
 			let sync = ChainSync::new(config.clone(), &chain, private_tx_handler.clone());
 			net.peers.push(Arc::new(EthPeer {
 				sync: RwLock::new(sync),
@@ -362,8 +362,7 @@ impl TestNet<EthPeer<EthcoreClient>> {
 		n: usize,
 		config: SyncConfig,
 		spec_factory: F,
-		accounts: Option<Arc<AccountProvider>>,
+		accounts: Option<Arc<AccountProvider>>
-		private_tx_handler: bool,
 	) -> Self
 		where F: Fn() -> Spec
 	{
@@ -373,11 +372,7 @@ impl TestNet<EthPeer<EthcoreClient>> {
 			disconnect_events: Vec::new(),
 		};
 		for _ in 0..n {
-			if private_tx_handler {
 			net.add_peer_with_private_config(config.clone(), spec_factory(), accounts.clone());
-			} else {
-				net.add_peer(config.clone(), spec_factory(), accounts.clone());
-			}
 		}
 		net
 	}
@@ -410,33 +405,6 @@ impl TestNet<EthPeer<EthcoreClient>> {
 		//private_provider.add_notify(peer.clone());
 		self.peers.push(peer);
 	}
-
-	pub fn add_peer(&mut self, config: SyncConfig, spec: Spec, accounts: Option<Arc<AccountProvider>>) {
-		let miner = Arc::new(Miner::new_for_tests(&spec, accounts));
-		let client = EthcoreClient::new(
-			ClientConfig::default(),
-			&spec,
-			Arc::new(::kvdb_memorydb::create(::ethcore::db::NUM_COLUMNS.unwrap_or(0))),
-			miner.clone(),
-			IoChannel::disconnected(),
-		).unwrap();
-
-		let ss = Arc::new(TestSnapshotService::new());
-		let private_tx_handler = Arc::new(NoopPrivateTxHandler);
-		let sync = ChainSync::new(config, &*client, private_tx_handler.clone());
-		let peer = Arc::new(EthPeer {
-			sync: RwLock::new(sync),
-			snapshot_service: ss,
-			queue: RwLock::new(VecDeque::new()),
-			chain: client,
-			miner,
-			private_tx_handler,
-			io_queue: RwLock::new(VecDeque::new()),
-			new_blocks_queue: RwLock::new(VecDeque::new()),
-		});
-		peer.chain.add_notify(peer.clone());
-		self.peers.push(peer);
-	}
 }
 
 impl<P> TestNet<P> where P: Peer {

ethcore/sync/src/tests/mod.rs

@@ -18,6 +18,7 @@ pub mod helpers;
 pub mod snapshot;
 mod chain;
 mod consensus;
+mod private;
 
 #[cfg(feature = "ipc")]
 mod rpc;

ethcore/sync/src/tests/private.rs

@@ -0,0 +1,150 @@
+// Copyright 2015-2017 Parity Technologies (UK) Ltd.
+// This file is part of Parity.
+
+// Parity is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Parity is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Parity.  If not, see <http://www.gnu.org/licenses/>.
+
+use std::sync::Arc;
+use hash::keccak;
+use io::{IoHandler, IoChannel};
+use ethcore::client::{BlockChainClient, BlockId, ClientIoMessage};
+use ethcore::spec::Spec;
+use ethcore::miner::MinerService;
+use ethcore::CreateContractAddress;
+use transaction::{Transaction, Action};
+use ethcore::executive::{contract_address};
+use ethcore::test_helpers::{push_block_with_transactions};
+use ethcore_private_tx::{Provider, ProviderConfig, NoopEncryptor};
+use ethcore::account_provider::AccountProvider;
+use ethkey::{KeyPair};
+use tests::helpers::{TestNet, TestIoHandler};
+use rustc_hex::FromHex;
+use SyncConfig;
+
+fn seal_spec() -> Spec {
+	let spec_data = include_str!("../res/private_spec.json");
+	Spec::load(&::std::env::temp_dir(), spec_data.as_bytes()).unwrap()
+}
+
+#[test]
+fn send_private_transaction() {
+	// Setup two clients
+	let s0 = KeyPair::from_secret_slice(&keccak("1")).unwrap();
+	let s1 = KeyPair::from_secret_slice(&keccak("0")).unwrap();
+	let ap = Arc::new(AccountProvider::transient_provider());
+	ap.insert_account(s0.secret().clone(), "").unwrap();
+	ap.insert_account(s1.secret().clone(), "").unwrap();
+
+	let mut net = TestNet::with_spec_and_accounts(2, SyncConfig::default(), seal_spec, Some(ap.clone()));
+	let client0 = net.peer(0).chain.clone();
+	let client1 = net.peer(1).chain.clone();
+	let io_handler0: Arc<IoHandler<ClientIoMessage>> = Arc::new(TestIoHandler::new(net.peer(0).chain.clone()));
+	let io_handler1: Arc<IoHandler<ClientIoMessage>> = Arc::new(TestIoHandler::new(net.peer(1).chain.clone()));
+
+	net.peer(0).miner.set_author(s0.address(), Some("".into())).unwrap();
+	net.peer(1).miner.set_author(s1.address(), Some("".to_owned())).unwrap();
+	net.peer(0).chain.engine().register_client(Arc::downgrade(&net.peer(0).chain) as _);
+	net.peer(1).chain.engine().register_client(Arc::downgrade(&net.peer(1).chain) as _);
+	net.peer(0).chain.set_io_channel(IoChannel::to_handler(Arc::downgrade(&io_handler0)));
+	net.peer(1).chain.set_io_channel(IoChannel::to_handler(Arc::downgrade(&io_handler1)));
+
+	let (address, _) = contract_address(CreateContractAddress::FromSenderAndNonce, &s0.address(), &0.into(), &[]);
+	let chain_id = client0.signing_chain_id();
+
+	// Exhange statuses
+	net.sync();
+
+	// Setup private providers
+	let validator_config = ProviderConfig{
+		validator_accounts: vec![s1.address()],
+		signer_account: None,
+		passwords: vec!["".into()],
+	};
+
+	let signer_config = ProviderConfig{
+		validator_accounts: Vec::new(),
+		signer_account: Some(s0.address()),
+		passwords: vec!["".into()],
+	};
+
+	let pm0 = Arc::new(Provider::new(
+			client0.clone(),
+			net.peer(0).miner.clone(),
+			ap.clone(),
+			Box::new(NoopEncryptor::default()),
+			signer_config,
+			IoChannel::to_handler(Arc::downgrade(&io_handler0)),
+	).unwrap());
+	pm0.add_notify(net.peers[0].clone());
+
+	let pm1 = Arc::new(Provider::new(
+			client1.clone(),
+			net.peer(1).miner.clone(),
+			ap.clone(),
+			Box::new(NoopEncryptor::default()),
+			validator_config,
+			IoChannel::to_handler(Arc::downgrade(&io_handler1)),
+	).unwrap());
+	pm1.add_notify(net.peers[1].clone());
+
+	// Create and deploy contract
+	let private_contract_test = "6060604052341561000f57600080fd5b60d88061001d6000396000f30060606040526000357c0100000000000000000000000000000000000000000000000000000000900463ffffffff1680630c55699c146046578063bc64b76d14607457600080fd5b3415605057600080fd5b60566098565b60405180826000191660001916815260200191505060405180910390f35b3415607e57600080fd5b6096600480803560001916906020019091905050609e565b005b60005481565b8060008160001916905550505600a165627a7a723058206acbdf4b15ca4c2d43e1b1879b830451a34f1e9d02ff1f2f394d8d857e79d2080029".from_hex().unwrap();
+	let mut private_create_tx = Transaction::default();
+	private_create_tx.action = Action::Create;
+	private_create_tx.data = private_contract_test;
+	private_create_tx.gas = 200000.into();
+	let private_create_tx_signed = private_create_tx.sign(&s0.secret(), None);
+	let validators = vec![s1.address()];
+	let (public_tx, _) = pm0.public_creation_transaction(BlockId::Latest, &private_create_tx_signed, &validators, 0.into()).unwrap();
+	let public_tx = public_tx.sign(&s0.secret(), chain_id);
+
+	let public_tx_copy = public_tx.clone();
+	push_block_with_transactions(&client0, &[public_tx]);
+	push_block_with_transactions(&client1, &[public_tx_copy]);
+
+	net.sync();
+
+	//Create private transaction for modifying state
+	let mut private_tx = Transaction::default();
+	private_tx.action = Action::Call(address.clone());
+	private_tx.data = "bc64b76d2a00000000000000000000000000000000000000000000000000000000000000".from_hex().unwrap(); //setX(42)
+	private_tx.gas = 120000.into();
+	private_tx.nonce = 1.into();
+	let private_tx = private_tx.sign(&s0.secret(), None);
+	assert!(pm0.create_private_transaction(private_tx).is_ok());
+
+	//send private transaction message to validator
+	net.sync();
+
+	let validator_handler = net.peer(1).private_tx_handler.clone();
+	let received_private_transactions = validator_handler.txs.lock().clone();
+	assert_eq!(received_private_transactions.len(), 1);
+
+	//process received private transaction message
+	let private_transaction = received_private_transactions[0].clone();
+	assert!(pm1.import_private_transaction(&private_transaction).is_ok());
+	assert!(pm1.on_private_transaction_queued().is_ok());
+
+	//send signed response
+	net.sync();
+
+	let sender_handler = net.peer(0).private_tx_handler.clone();
+	let received_signed_private_transactions = sender_handler.signed_txs.lock().clone();
+	assert_eq!(received_signed_private_transactions.len(), 1);
+
+	//process signed response
+	let signed_private_transaction = received_signed_private_transactions[0].clone();
+	assert!(pm0.import_signed_private_transaction(&signed_private_transaction).is_ok());
+	let local_transactions = net.peer(0).miner.local_transactions();
+	assert_eq!(local_transactions.len(), 1);
+}