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

Allow disabling local-by-default for transactions with new config entry #8882

Merged
merged 4 commits into from
Jun 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 78 additions & 2 deletions ethcore/src/miner/miner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
Expand Down Expand Up @@ -794,8 +797,9 @@ impl miner::MinerService for Miner {
fn import_own_transaction<C: miner::BlockChainClient>(
&self,
chain: &C,
pending: PendingTransaction,
pending: PendingTransaction
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer to keep trailing commas in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh cool, I removed it because I presumed I added it while copy-pasting. My bad.

) -> 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 All @@ -816,6 +820,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 @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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();
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 @@ -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<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 @@ -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.",
Expand Down Expand Up @@ -1249,6 +1253,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 @@ -1663,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,
Expand Down Expand Up @@ -1929,6 +1935,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 @@ -825,6 +825,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