Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Commit

Permalink
Allow disabling local-by-default for transactions with new config ent…
Browse files Browse the repository at this point in the history
…ry (#8882)

* 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
  • Loading branch information
XertroV authored and andresilva committed Jun 18, 2018
1 parent e0b7da9 commit 455329e
Show file tree
Hide file tree
Showing 8 changed files with 108 additions and 6 deletions.
80 changes: 78 additions & 2 deletions ethcore/src/miner/miner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,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.
Expand All @@ -149,6 +151,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,
Expand Down Expand Up @@ -768,8 +771,9 @@ impl miner::MinerService for Miner {
fn import_own_transaction<C: miner::BlockChainClient>(
&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);

Expand Down Expand Up @@ -797,6 +801,28 @@ impl miner::MinerService for Miner {
imported
}

fn import_claimed_local_transaction<C: miner::BlockChainClient>(
&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<H256, pool::local_transactions::Status> {
self.transaction_queue.local_transactions()
}
Expand Down Expand Up @@ -1133,6 +1159,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 {
Expand All @@ -1147,8 +1174,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 {
Expand Down Expand Up @@ -1222,6 +1251,53 @@ mod tests {
assert_eq!(miner.ready_transactions(&client).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();
Expand Down
6 changes: 6 additions & 0 deletions ethcore/src/miner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,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<C>(&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)
Expand Down
7 changes: 7 additions & 0 deletions parity/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,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.",
Expand Down Expand Up @@ -1241,6 +1245,7 @@ struct Mining {
tx_queue_strategy: Option<String>,
tx_queue_ban_count: Option<u16>,
tx_queue_ban_time: Option<u16>,
tx_queue_no_unfamiliar_locals: Option<bool>,
remove_solved: Option<bool>,
notify_work: Option<Vec<String>>,
refuse_service_transactions: Option<bool>,
Expand Down Expand Up @@ -1657,6 +1662,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,
Expand Down Expand Up @@ -1922,6 +1928,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,
Expand Down
1 change: 1 addition & 0 deletions parity/cli/tests/config.full.toml
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,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"]
Expand Down
1 change: 1 addition & 0 deletions parity/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +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_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()?,
Expand Down
9 changes: 6 additions & 3 deletions rpc/src/v1/helpers/dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,13 @@ impl<C: miner::BlockChainClient, M: MinerService> FullDispatcher<C, M> {
}

/// Imports transaction to the miner's queue.
pub fn dispatch_transaction(client: &C, miner: &M, signed_transaction: PendingTransaction) -> Result<H256> {
pub fn dispatch_transaction(client: &C, miner: &M, signed_transaction: PendingTransaction, trusted: bool) -> Result<H256> {
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)
}
Expand Down Expand Up @@ -180,7 +183,7 @@ impl<C: miner::BlockChainClient + BlockChainClient, M: MinerService> Dispatcher
}

fn dispatch_transaction(&self, signed_transaction: PendingTransaction) -> Result<H256> {
Self::dispatch_transaction(&*self.client, &*self.miner, signed_transaction)
Self::dispatch_transaction(&*self.client, &*self.miner, signed_transaction, true)
}
}

Expand Down
1 change: 1 addition & 0 deletions rpc/src/v1/impls/eth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
&*self.client,
&*self.miner,
signed_transaction.into(),
false
)
})
.map(Into::into)
Expand Down
9 changes: 8 additions & 1 deletion rpc/src/v1/tests/helpers/miner_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,14 @@ impl MinerService for TestMinerService {
}

/// Imports transactions to transaction queue.
fn import_own_transaction<C: Nonce + Sync>(&self, chain: &C, pending: PendingTransaction)
fn import_own_transaction<C: Nonce + Sync>(&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<C: Nonce + Sync>(&self, chain: &C, pending: PendingTransaction, _trusted: bool)
-> Result<(), transaction::Error> {

// keep the pending nonces up to date
Expand Down

0 comments on commit 455329e

Please sign in to comment.