From 6004c394d612f7cd68d5e9fe4bf237f92b382b49 Mon Sep 17 00:00:00 2001 From: Max Kaye Date: Mon, 18 Jun 2018 23:32:18 +1000 Subject: [PATCH] Allow disabling local-by-default for transactions with new config entry (#8882) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add tx_queue_allow_unknown_local config option - Previous commit messages: dispatcher checks if we have the sender account Add `tx_queue_allow_unknown_local` to MinerOptions Add `tx_queue_allow_unknown_local` to config fix order in MinerOptions to match Configuration add cli flag for tx_queue_allow_unknown_local Update refs to `tx_queue_allow_unknown_local` Add tx_queue_allow_unknown_local to config test revert changes to dispatcher Move tx_queue_allow_unknown_local to `import_own_transaction` Fix var name if statement should return the values derp de derp derp derp semicolons Reset dispatch file to how it was before fix compile issues + change from FLAG to ARG add test and use `into` import MinerOptions, clone the secret Fix tests? Compiler/linter issues fixed Fix linter msg - case of constants IT LIVES refactor to omit yucky explict return update comments Fix based on diff AccountProvider.has_account method * Refactor flag name + don't change import_own_tx behaviour fix arg name Note: force commit to try and get gitlab tests working again 😠 * Add fn to TestMinerService * Avoid race condition from trusted sources - refactor the miner tests a bit to cut down on code reuse - add `trusted` param to dispatch_transaction and import_claimed_local_transaction Add param to `import_claimed_local_transaction` Fix fn sig in tests --- ethcore/src/miner/miner.rs | 80 ++++++++++++++++++++++- ethcore/src/miner/mod.rs | 6 ++ parity/cli/mod.rs | 7 ++ parity/cli/tests/config.full.toml | 1 + parity/configuration.rs | 1 + rpc/src/v1/helpers/dispatch.rs | 9 ++- rpc/src/v1/impls/eth.rs | 1 + rpc/src/v1/tests/helpers/miner_service.rs | 9 ++- 8 files changed, 108 insertions(+), 6 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 8348fedba57..af03b2d1e0a 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -125,6 +125,8 @@ pub struct MinerOptions { pub tx_queue_strategy: PrioritizationStrategy, /// Simple senders penalization. pub tx_queue_penalization: Penalization, + /// Do we want to mark transactions recieved locally (e.g. RPC) as local if we don't have the sending account? + pub tx_queue_no_unfamiliar_locals: bool, /// Do we refuse to accept service transactions even if sender is certified. pub refuse_service_transactions: bool, /// Transaction pool limits. @@ -148,6 +150,7 @@ impl Default for MinerOptions { infinite_pending_block: false, tx_queue_strategy: PrioritizationStrategy::GasPriceOnly, tx_queue_penalization: Penalization::Disabled, + tx_queue_no_unfamiliar_locals: false, refuse_service_transactions: false, pool_limits: pool::Options { max_count: 8_192, @@ -794,8 +797,9 @@ impl miner::MinerService for Miner { fn import_own_transaction( &self, chain: &C, - pending: PendingTransaction, + pending: PendingTransaction ) -> Result<(), transaction::Error> { + // note: you may want to use `import_claimed_local_transaction` instead of this one. trace!(target: "own_tx", "Importing transaction: {:?}", pending); @@ -816,6 +820,28 @@ impl miner::MinerService for Miner { imported } + fn import_claimed_local_transaction( + &self, + chain: &C, + pending: PendingTransaction, + trusted: bool + ) -> Result<(), transaction::Error> { + // treat the tx as local if the option is enabled, or if we have the account + let sender = pending.sender(); + let treat_as_local = trusted + || !self.options.tx_queue_no_unfamiliar_locals + || self.accounts.as_ref().map(|accts| accts.has_account(sender)).unwrap_or(false); + + if treat_as_local { + self.import_own_transaction(chain, pending) + } else { + // We want to replicate behaviour for external transactions if we're not going to treat + // this as local. This is important with regards to sealing blocks + self.import_external_transactions(chain, vec![pending.transaction.into()]) + .pop().expect("one result per tx, as in `import_own_transaction`") + } + } + fn local_transactions(&self) -> BTreeMap { self.transaction_queue.local_transactions() } @@ -1161,6 +1187,7 @@ mod tests { infinite_pending_block: false, tx_queue_penalization: Penalization::Disabled, tx_queue_strategy: PrioritizationStrategy::GasPriceOnly, + tx_queue_no_unfamiliar_locals: false, refuse_service_transactions: false, pool_limits: Default::default(), pool_verification_options: pool::verifier::Options { @@ -1175,8 +1202,10 @@ mod tests { ) } + const TEST_CHAIN_ID: u64 = 2; + fn transaction() -> SignedTransaction { - transaction_with_chain_id(2) + transaction_with_chain_id(TEST_CHAIN_ID) } fn transaction_with_chain_id(chain_id: u64) -> SignedTransaction { @@ -1250,6 +1279,53 @@ mod tests { assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 1); } + #[test] + fn should_treat_unfamiliar_locals_selectively() { + // given + let keypair = Random.generate().unwrap(); + let client = TestBlockChainClient::default(); + let account_provider = AccountProvider::transient_provider(); + account_provider.insert_account(keypair.secret().clone(), "").expect("can add accounts to the provider we just created"); + + let miner = Miner::new( + MinerOptions { + tx_queue_no_unfamiliar_locals: true, + ..miner().options + }, + GasPricer::new_fixed(0u64.into()), + &Spec::new_test(), + Some(Arc::new(account_provider)), + ); + let transaction = transaction(); + let best_block = 0; + // when + // This transaction should not be marked as local because our account_provider doesn't have the sender + let res = miner.import_claimed_local_transaction(&client, PendingTransaction::new(transaction.clone(), None), false); + + // then + // Check the same conditions as `should_import_external_transaction` first. Behaviour should be identical. + // That is: it's treated as though we added it through `import_external_transactions` + assert_eq!(res.unwrap(), ()); + assert_eq!(miner.pending_transactions(best_block), None); + assert_eq!(miner.pending_receipts(best_block), None); + assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 0); + assert!(miner.prepare_pending_block(&client)); + assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 1); + + // when - 2nd part: create a local transaction from account_provider. + // Borrow the transaction used before & sign with our generated keypair. + let local_transaction = transaction.deconstruct().0.as_unsigned().clone().sign(keypair.secret(), Some(TEST_CHAIN_ID)); + let res2 = miner.import_claimed_local_transaction(&client, PendingTransaction::new(local_transaction, None), false); + + // then - 2nd part: we add on the results from the last pending block. + // This is borrowed from `should_make_pending_block_when_importing_own_transaction` and slightly modified. + assert_eq!(res2.unwrap(), ()); + assert_eq!(miner.pending_transactions(best_block).unwrap().len(), 2); + assert_eq!(miner.pending_receipts(best_block).unwrap().len(), 2); + assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 2); + assert!(!miner.prepare_pending_block(&client)); + } + #[test] fn should_not_seal_unless_enabled() { let miner = miner(); diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs index 631941de611..1d7b9613bbd 100644 --- a/ethcore/src/miner/mod.rs +++ b/ethcore/src/miner/mod.rs @@ -138,6 +138,12 @@ pub trait MinerService : Send + Sync { -> Result<(), transaction::Error> where C: BlockChainClient; + /// Imports transactions from potentially external sources, with behaviour determined + /// by the config flag `tx_queue_allow_unfamiliar_locals` + fn import_claimed_local_transaction(&self, chain: &C, transaction: PendingTransaction, trusted: bool) + -> Result<(), transaction::Error> + where C: BlockChainClient; + /// Removes transaction from the pool. /// /// Attempts to "cancel" a transaction. If it was not propagated yet (or not accepted by other peers) diff --git a/parity/cli/mod.rs b/parity/cli/mod.rs index 4faf12946df..676f591f428 100644 --- a/parity/cli/mod.rs +++ b/parity/cli/mod.rs @@ -640,6 +640,10 @@ usage! { "--remove-solved", "Move solved blocks from the work package queue instead of cloning them. This gives a slightly faster import speed, but means that extra solutions submitted for the same work package will go unused.", + FLAG flag_tx_queue_no_unfamiliar_locals: (bool) = false, or |c: &Config| c.mining.as_ref()?.tx_queue_no_unfamiliar_locals.clone(), + "--tx-queue-no-unfamiliar-locals", + "Transactions recieved via local means (RPC, WS, etc) will be treated as external if the sending account is unknown.", + FLAG flag_refuse_service_transactions: (bool) = false, or |c: &Config| c.mining.as_ref()?.refuse_service_transactions.clone(), "--refuse-service-transactions", "Always refuse service transactions.", @@ -1259,6 +1263,7 @@ struct Mining { tx_queue_strategy: Option, tx_queue_ban_count: Option, tx_queue_ban_time: Option, + tx_queue_no_unfamiliar_locals: Option, remove_solved: Option, notify_work: Option>, refuse_service_transactions: Option, @@ -1675,6 +1680,7 @@ mod tests { arg_gas_floor_target: "4700000".into(), arg_gas_cap: "6283184".into(), arg_extra_data: Some("Parity".into()), + flag_tx_queue_no_unfamiliar_locals: false, arg_tx_queue_size: 8192usize, arg_tx_queue_per_sender: None, arg_tx_queue_mem_limit: 4u32, @@ -1943,6 +1949,7 @@ mod tests { tx_queue_strategy: None, tx_queue_ban_count: None, tx_queue_ban_time: None, + tx_queue_no_unfamiliar_locals: None, tx_gas_limit: None, tx_time_limit: None, extra_data: None, diff --git a/parity/cli/tests/config.full.toml b/parity/cli/tests/config.full.toml index 2bc8778d096..6a081027f2e 100644 --- a/parity/cli/tests/config.full.toml +++ b/parity/cli/tests/config.full.toml @@ -134,6 +134,7 @@ tx_queue_ban_count = 1 tx_queue_ban_time = 180 #s tx_gas_limit = "6283184" tx_time_limit = 100 #ms +tx_queue_no_unfamiliar_locals = false extra_data = "Parity" remove_solved = false notify_work = ["http://localhost:3001"] diff --git a/parity/configuration.rs b/parity/configuration.rs index 8019be516f7..bd85b56daf5 100644 --- a/parity/configuration.rs +++ b/parity/configuration.rs @@ -550,6 +550,7 @@ impl Configuration { tx_queue_penalization: to_queue_penalization(self.args.arg_tx_time_limit)?, tx_queue_strategy: to_queue_strategy(&self.args.arg_tx_queue_strategy)?, + tx_queue_no_unfamiliar_locals: self.args.flag_tx_queue_no_unfamiliar_locals, refuse_service_transactions: self.args.flag_refuse_service_transactions, pool_limits: self.pool_limits()?, diff --git a/rpc/src/v1/helpers/dispatch.rs b/rpc/src/v1/helpers/dispatch.rs index 81fc182e4bc..0e6b9d443d2 100644 --- a/rpc/src/v1/helpers/dispatch.rs +++ b/rpc/src/v1/helpers/dispatch.rs @@ -123,10 +123,13 @@ impl FullDispatcher { } /// Imports transaction to the miner's queue. - pub fn dispatch_transaction(client: &C, miner: &M, signed_transaction: PendingTransaction) -> Result { + pub fn dispatch_transaction(client: &C, miner: &M, signed_transaction: PendingTransaction, trusted: bool) -> Result { let hash = signed_transaction.transaction.hash(); - miner.import_own_transaction(client, signed_transaction) + // use `import_claimed_local_transaction` so we can decide (based on config flags) if we want to treat + // it as local or not. Nodes with public RPC interfaces will want these transactions to be treated like + // external transactions. + miner.import_claimed_local_transaction(client, signed_transaction, trusted) .map_err(errors::transaction) .map(|_| hash) } @@ -180,7 +183,7 @@ impl Dispatcher } fn dispatch_transaction(&self, signed_transaction: PendingTransaction) -> Result { - Self::dispatch_transaction(&*self.client, &*self.miner, signed_transaction) + Self::dispatch_transaction(&*self.client, &*self.miner, signed_transaction, true) } } diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 8cd0afc5bd5..d33d5e860a4 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -828,6 +828,7 @@ impl Eth for EthClient< &*self.client, &*self.miner, signed_transaction.into(), + false ) }) .map(Into::into) diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index 8d0ec23ae1c..63c28fb6cc4 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -155,7 +155,14 @@ impl MinerService for TestMinerService { } /// Imports transactions to transaction queue. - fn import_own_transaction(&self, chain: &C, pending: PendingTransaction) + fn import_own_transaction(&self, _chain: &C, _pending: PendingTransaction) + -> Result<(), transaction::Error> { + // this function is no longer called directly from RPC + unimplemented!(); + } + + /// Imports transactions to queue - treats as local based on trusted flag, config, and tx source + fn import_claimed_local_transaction(&self, chain: &C, pending: PendingTransaction, _trusted: bool) -> Result<(), transaction::Error> { // keep the pending nonces up to date