From c69582c31599df7e204f6a4079d665d99ec64ed2 Mon Sep 17 00:00:00 2001 From: Max Kaye Date: Thu, 14 Jun 2018 09:41:58 +1000 Subject: [PATCH] Refactor flag name + don't change import_own_tx behaviour fix arg name --- ethcore/src/miner/miner.rs | 110 ++++++++++++++++-------------- ethcore/src/miner/mod.rs | 6 ++ parity/cli/mod.rs | 14 ++-- parity/cli/tests/config.full.toml | 2 +- parity/configuration.rs | 2 +- rpc/src/v1/helpers/dispatch.rs | 5 +- 6 files changed, 76 insertions(+), 63 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index f5398440f3d..2b06a785b21 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -125,8 +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 as local if we don't have the sending account? - pub tx_queue_allow_unknown_local: bool, + /// 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. @@ -150,7 +150,7 @@ impl Default for MinerOptions { infinite_pending_block: false, tx_queue_strategy: PrioritizationStrategy::GasPriceOnly, tx_queue_penalization: Penalization::Disabled, - tx_queue_allow_unknown_local: true, + tx_queue_no_unfamiliar_locals: false, refuse_service_transactions: false, pool_limits: pool::Options { max_count: 8_192, @@ -799,36 +799,44 @@ impl miner::MinerService for Miner { chain: &C, 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); + let client = self.pool_client(chain); + let imported = self.transaction_queue.import( + client, + vec![pool::verifier::Transaction::Local(pending)] + ).pop().expect("one result returned per added transaction; one added => one result; qed"); + + // -------------------------------------------------------------------------- + // | NOTE Code below requires sealing locks. | + // | Make sure to release the locks before calling that method. | + // -------------------------------------------------------------------------- + if imported.is_ok() && self.options.reseal_on_own_tx && self.sealing.lock().reseal_allowed() { + self.prepare_and_update_sealing(chain); + } + + imported + } + + fn import_claimed_local_transaction( + &self, + chain: &C, + pending: PendingTransaction, + ) -> 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 = self.options.tx_queue_allow_unknown_local + let treat_as_local = !self.options.tx_queue_no_unfamiliar_locals || self.accounts.as_ref().map(|accts| accts.has_account(sender)).unwrap_or(false); if treat_as_local { - let client = self.pool_client(chain); - let imported = self.transaction_queue.import( - client, - vec![pool::verifier::Transaction::Local(pending)] - ).pop().expect("one result returned per added transaction; one added => one result; qed"); - - // -------------------------------------------------------------------------- - // | NOTE Code below requires sealing locks. | - // | Make sure to release the locks before calling that method. | - // -------------------------------------------------------------------------- - if imported.is_ok() && self.options.reseal_on_own_tx && self.sealing.lock().reseal_allowed() { - self.prepare_and_update_sealing(chain); - } - - - imported + 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 below") + self.import_external_transactions(chain, vec![pending.transaction.into()]) + .pop().expect("one result per tx, as in `import_own_transaction`") } } @@ -1162,34 +1170,30 @@ mod tests { assert!(miner.submit_seal(hash, vec![]).is_ok()); } - fn default_miner_opts() -> MinerOptions { - MinerOptions { - force_sealing: false, - reseal_on_external_tx: false, - reseal_on_own_tx: true, - reseal_on_uncle: false, - reseal_min_period: Duration::from_secs(5), - reseal_max_period: Duration::from_secs(120), - pending_set: PendingSet::AlwaysSealing, - work_queue_size: 5, - enable_resubmission: true, - infinite_pending_block: false, - tx_queue_penalization: Penalization::Disabled, - tx_queue_strategy: PrioritizationStrategy::GasPriceOnly, - tx_queue_allow_unknown_local: true, - refuse_service_transactions: false, - pool_limits: Default::default(), - pool_verification_options: pool::verifier::Options { - minimal_gas_price: 0.into(), - block_gas_limit: U256::max_value(), - tx_gas_limit: U256::max_value(), - }, - } - } - fn miner() -> Miner { Miner::new( - default_miner_opts(), + MinerOptions { + force_sealing: false, + reseal_on_external_tx: false, + reseal_on_own_tx: true, + reseal_on_uncle: false, + reseal_min_period: Duration::from_secs(5), + reseal_max_period: Duration::from_secs(120), + pending_set: PendingSet::AlwaysSealing, + work_queue_size: 5, + enable_resubmission: true, + 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 { + minimal_gas_price: 0.into(), + block_gas_limit: U256::max_value(), + tx_gas_limit: U256::max_value(), + }, + }, GasPricer::new_fixed(0u64.into()), &Spec::new_test(), None, // accounts provider @@ -1274,7 +1278,7 @@ mod tests { } #[test] - fn should_import_own_transaction_selectively() { + fn should_treat_unfamiliar_locals_selectively() { // given let keypair = Random.generate().unwrap(); let client = TestBlockChainClient::default(); @@ -1283,8 +1287,8 @@ mod tests { let miner = Miner::new( MinerOptions { - tx_queue_allow_unknown_local: false, - ..default_miner_opts() + tx_queue_no_unfamiliar_locals: true, + ..miner().options }, GasPricer::new_fixed(0u64.into()), &Spec::new_test(), @@ -1294,7 +1298,7 @@ mod tests { 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_own_transaction(&client, PendingTransaction::new(transaction.clone(), None)); + let res = miner.import_claimed_local_transaction(&client, PendingTransaction::new(transaction.clone(), None)); // then // Check the same conditions as `should_import_external_transaction` first. Behaviour should be identical. @@ -1309,7 +1313,7 @@ mod tests { // 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_own_transaction(&client, PendingTransaction::new(local_transaction, None)); + let res2 = miner.import_claimed_local_transaction(&client, PendingTransaction::new(local_transaction, None)); // 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. diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs index 631941de611..00d73de519f 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) + -> 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 8d93e49436d..6f8169d19fe 100644 --- a/parity/cli/mod.rs +++ b/parity/cli/mod.rs @@ -636,6 +636,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.", @@ -712,10 +716,6 @@ usage! { "--tx-queue-strategy=[S]", "Prioritization strategy used to order transactions in the queue. S may be: gas_price - Prioritize txs with high gas price", - ARG arg_tx_queue_allow_unknown_local: (bool) = true, or |c: &Config| c.mining.as_ref()?.tx_queue_allow_unknown_local.clone(), - "--tx-queue-allow-unknown-local=[ALLOW]", - "Transactions recieved from outside the network will be treated as local even if the sending account is not local.", - ARG arg_stratum_interface: (String) = "local", or |c: &Config| c.stratum.as_ref()?.interface.clone(), "--stratum-interface=[IP]", "Interface address for Stratum server.", @@ -1253,7 +1253,7 @@ struct Mining { tx_queue_strategy: Option, tx_queue_ban_count: Option, tx_queue_ban_time: Option, - tx_queue_allow_unknown_local: Option, + tx_queue_no_unfamiliar_locals: Option, remove_solved: Option, notify_work: Option>, refuse_service_transactions: Option, @@ -1668,6 +1668,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, @@ -1675,7 +1676,6 @@ mod tests { arg_tx_queue_strategy: "gas_factor".into(), arg_tx_queue_ban_count: 1u16, arg_tx_queue_ban_time: 180u16, - arg_tx_queue_allow_unknown_local: true, flag_remove_solved: false, arg_notify_work: Some("http://localhost:3001".into()), flag_refuse_service_transactions: false, @@ -1935,7 +1935,7 @@ mod tests { tx_queue_strategy: None, tx_queue_ban_count: None, tx_queue_ban_time: None, - tx_queue_allow_unknown_local: 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 9e1bafdd5ef..dc762905cf0 100644 --- a/parity/cli/tests/config.full.toml +++ b/parity/cli/tests/config.full.toml @@ -133,7 +133,7 @@ tx_queue_ban_count = 1 tx_queue_ban_time = 180 #s tx_gas_limit = "6283184" tx_time_limit = 100 #ms -tx_queue_allow_unknown_local = true +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 d78b974a5c4..5c5608468cd 100644 --- a/parity/configuration.rs +++ b/parity/configuration.rs @@ -549,7 +549,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_allow_unknown_local: self.args.arg_tx_queue_allow_unknown_local, + 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..1ae11381733 100644 --- a/rpc/src/v1/helpers/dispatch.rs +++ b/rpc/src/v1/helpers/dispatch.rs @@ -126,7 +126,10 @@ impl FullDispatcher { pub fn dispatch_transaction(client: &C, miner: &M, signed_transaction: PendingTransaction) -> 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) .map_err(errors::transaction) .map(|_| hash) }