From d86333af6b51afa006b438de63e7acbd05f05985 Mon Sep 17 00:00:00 2001 From: Anton Gavrilov Date: Fri, 20 Apr 2018 15:45:53 +0200 Subject: [PATCH] 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 | 1 + ethcore/private-tx/src/lib.rs | 11 +- ethcore/sync/Cargo.toml | 1 + ethcore/sync/src/lib.rs | 1 + ethcore/sync/src/res/private_spec.json | 30 +++++ ethcore/sync/src/tests/consensus.rs | 4 +- ethcore/sync/src/tests/helpers.rs | 42 +------ ethcore/sync/src/tests/mod.rs | 1 + ethcore/sync/src/tests/private.rs | 150 +++++++++++++++++++++++++ 9 files changed, 193 insertions(+), 48 deletions(-) create mode 100644 ethcore/sync/src/res/private_spec.json create mode 100644 ethcore/sync/src/tests/private.rs diff --git a/Cargo.lock b/Cargo.lock index 547c3c88b..ae38937ba 100644 --- a/Cargo.lock +++ b/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", diff --git a/ethcore/private-tx/src/lib.rs b/ethcore/private-tx/src/lib.rs index 80510938c..12028ddf1 100644 --- a/ethcore/private-tx/src/lib.rs +++ b/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"); - bail!(ErrorKind::BadTransactonType); + warn!("Incorrect type of action for the transaction"); } }, Err(e) => { - trace!("Cannot retrieve descriptor for transaction with error {:?}", e); - bail!(e); + warn!("Cannot retrieve descriptor for transaction with error {:?}", e); } } verification_queue.remove_private_transaction(&transaction_hash); diff --git a/ethcore/sync/Cargo.toml b/ethcore/sync/Cargo.toml index a09f161e2..6930baa10 100644 --- a/ethcore/sync/Cargo.toml +++ b/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" } diff --git a/ethcore/sync/src/lib.rs b/ethcore/sync/src/lib.rs index bf3b475fc..a3e24bdb8 100644 --- a/ethcore/sync/src/lib.rs +++ b/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; diff --git a/ethcore/sync/src/res/private_spec.json b/ethcore/sync/src/res/private_spec.json new file mode 100644 index 000000000..f93d754a6 --- /dev/null +++ b/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 } } } } + } +} \ No newline at end of file diff --git a/ethcore/sync/src/tests/consensus.rs b/ethcore/sync/src/tests/consensus.rs index 287a61916..8825bad2c 100644 --- a/ethcore/sync/src/tests/consensus.rs +++ b/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> = Arc::new(TestIoHandler::new(net.peer(0).chain.clone())); let io_handler1: Arc> = 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> = Arc::new(TestIoHandler::new(net.peer(0).chain.clone())); let io_handler1: Arc> = Arc::new(TestIoHandler::new(net.peer(1).chain.clone())); // Push transaction to both clients. Only one of them issues a proposal. diff --git a/ethcore/sync/src/tests/helpers.rs b/ethcore/sync/src/tests/helpers.rs index 54b62c79a..086dc503b 100644 --- a/ethcore/sync/src/tests/helpers.rs +++ b/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 where C: FlushingBlockChainClient { pub snapshot_service: Arc, pub sync: RwLock, pub queue: RwLock>, - pub private_tx_handler: Arc, + pub private_tx_handler: Arc, pub io_queue: RwLock>, new_blocks_queue: RwLock>, } @@ -335,7 +335,7 @@ impl TestNet> { 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> { n: usize, config: SyncConfig, spec_factory: F, - accounts: Option>, - private_tx_handler: bool, + accounts: Option> ) -> Self where F: Fn() -> Spec { @@ -373,11 +372,7 @@ impl TestNet> { 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.add_peer_with_private_config(config.clone(), spec_factory(), accounts.clone()); } net } @@ -410,33 +405,6 @@ impl TestNet> { //private_provider.add_notify(peer.clone()); self.peers.push(peer); } - - pub fn add_peer(&mut self, config: SyncConfig, spec: Spec, accounts: Option>) { - 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

TestNet

where P: Peer { diff --git a/ethcore/sync/src/tests/mod.rs b/ethcore/sync/src/tests/mod.rs index 8b9059fc1..eb0110828 100644 --- a/ethcore/sync/src/tests/mod.rs +++ b/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; diff --git a/ethcore/sync/src/tests/private.rs b/ethcore/sync/src/tests/private.rs new file mode 100644 index 000000000..a9e8718e5 --- /dev/null +++ b/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 . + +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> = Arc::new(TestIoHandler::new(net.peer(0).chain.clone())); + let io_handler1: Arc> = 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); +} \ No newline at end of file