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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4fee0e
Add tx_queue_allow_unknown_local config option
XertroV Jun 13, 2018
7a54866
Refactor flag name + don't change import_own_tx behaviour
XertroV Jun 13, 2018
c92845b
Add fn to TestMinerService
XertroV Jun 15, 2018
07125c7
Avoid race condition from trusted sources
XertroV Jun 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
140 changes: 105 additions & 35 deletions ethcore/src/miner/miner.rs
View file
Open in desktop
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 as local if we don't have the sending account?
pub tx_queue_allow_unknown_local: 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_allow_unknown_local: true,
refuse_service_transactions: false,
pool_limits: pool::Options {
max_count: 8_192,
Expand Down Expand Up @@ -799,21 +802,34 @@ impl miner::MinerService for Miner {

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");
// 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
|| self.accounts.as_ref().map(|accts| accts.has_account(sender)).unwrap_or(false);

// --------------------------------------------------------------------------
// | 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);
}
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");

imported
// --------------------------------------------------------------------------
// | 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
} else {
Copy link
Collaborator

@sorpaas sorpaas Jun 13, 2018 •
edited

Choose a reason for hiding this comment

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

I would suggest we:

  • Leave import_own_transaction unchanged.
  • Create another function, probably called import_local_or_external_transaction (or maybe a better name, can't think of right now), which dispatch to import_own_transaction or import_external_transactions.
  • Make eth_submitRawTransaction call that function instead, through Dispatcher.

The issue is that import_own_transaction is used in many other places in ethcore, and this change may break some of the previous assumptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, agreed. Much more elegant . Will get on that tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does import_foreign_transaction sound? Or maybe just import_unknown_local_transaction, which corresponds to the argument name very well.

On the one hand introducing a new term is not good, but coming up with a good name is hard. Maybe normalise_unknown_local? like mixing them in with external instead of treating like known locals. I think the External/Local thing makes sense and should be left how it is (no redefining what a local tx is). (though maybe better names would be Indirect/Direct transactions (either that you get them second-hand or directly.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both sound fine to me. :)

// 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()])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be <whitespace> not <tab> right before vec![.

.pop().expect("one result per tx, as below")
}
}

fn local_transactions(&self) -> BTreeMap<H256, pool::local_transactions::Status> {
Expand Down Expand Up @@ -1146,37 +1162,44 @@ mod tests {
assert!(miner.submit_seal(hash, vec![]).is_ok());
}

fn default_miner_opts() -> MinerOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra whitespace after fn.

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(
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,
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(),
},
},
default_miner_opts(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why having an extra default_miner_opts? I think this may be confusing as MinerOptions already implements Default trait.

Copy link
Contributor Author

@XertroV XertroV Jun 13, 2018 •
edited

Choose a reason for hiding this comment

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

Yup, it is confusing. The config returned is the default used in testing (which is not Default:: default()).

probs better to extract config from the miner produced by miner(). (I need to change just one config item)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative is to rename it to something like std_test_miner_opts

GasPricer::new_fixed(0u64.into()),
&Spec::new_test(),
None, // accounts provider
)
}

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 +1273,53 @@ mod tests {
assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 1);
}

#[test]
fn should_import_own_transaction_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_allow_unknown_local: false,
..default_miner_opts()
},
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_own_transaction(&client, PendingTransaction::new(transaction.clone(), None));

// 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_own_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.
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
7 changes: 7 additions & 0 deletions parity/cli/mod.rs
View file
Open in desktop
Expand Up @@ -712,6 +712,10 @@ 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(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe change this to a flag, as it is a boolean?

Copy link
Contributor Author

@XertroV XertroV Jun 13, 2018 •
edited

Choose a reason for hiding this comment

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

I wanted the default (for the flag/arg) to be true, which I can't do with a flag (seems like default must be false?) Also, unknown_local was the simplest name I could come up with that also explains the concept, and keeps all the flags/config items the same name (not negations). Length was also a cosideration. If there's a better name happy to change it.

(Like: allow_unknown_local is better than disallow_unknown_local (or deny_...), less thinking to do about what it means. and unknown_local is not great to start with, so already takes some thinking. Not super happy with the name, tbh)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have many "negative" flags already in cli, like --no-warp, --no-discovery, etc. And I searched the code, we don't have any boolean arg. So I still think that flag may be better for consistency, like --disallow-unknown-local/--demote-unkown-local-transactions or something like that.

Or maybe like you said, we can set --allow-unknown-local default to false? I can't think of any case where this may break existing usages -- eth_submitRawTransaction specification doesn't require giving special promotion to the submitted transaction.

"--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.",
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_allow_unknown_local: Option<bool>,
remove_solved: Option<bool>,
notify_work: Option<Vec<String>>,
refuse_service_transactions: Option<bool>,
Expand Down Expand Up @@ -1670,6 +1675,7 @@ 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,
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_allow_unknown_local: 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
View file
Open in desktop
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_allow_unknown_local = true
extra_data = "Parity"
remove_solved = false
notify_work = ["http://localhost:3001"]
Expand Down
1 change: 1 addition & 0 deletions parity/configuration.rs
View file
Open in desktop
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_allow_unknown_local: self.args.arg_tx_queue_allow_unknown_local,
refuse_service_transactions: self.args.flag_refuse_service_transactions,

pool_limits: self.pool_limits()?,
Expand Down