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

XertroV · 2018-06-13T03:02:05Z

Transactions submitted through import_own_transaction are checked to see if we have the account (provided the tx_queue_allow_unknown_local flag is false).

The exact logic is:

let treat_as_local = self.options.tx_queue_allow_unknown_local 
		|| self.accounts.and_then(|accts| accts.has_account(sender).ok()).unwrap_or(false);

BTW - I've never written Rust before, so please check this twice to make sure it is any good.

I'm also not sure if I should add any tests and/or where.

parity-cla-bot · 2018-06-13T03:02:07Z

It looks like @XertroV signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

XertroV · 2018-06-13T07:31:34Z

@5chdn - Do you guys want this to be squashed into one commit? I know some communities like that.

(Note: asking you because you just added the tag)

5chdn · 2018-06-13T07:36:26Z

Yes, we squash-merge :)

XertroV · 2018-06-13T08:27:06Z

@5chdn - if you don't mind my bothering you, is there any way to quickly run a test? I haven't actually gotten to the point of running my test until https://gitlab.parity.io/parity/parity/-/jobs/89906 showed me the failure.

have tried cargo test -j 8 miner::miner::tests::should_import_own_transaction_selectively with and without --lib, without any test-name-argument (which doesn't seem to run any miner tests..), with partial names; the ./test.sh script doesn't seem to run it, and so on.

I'm not sure if I need to cargo clear or anything, but it's been a very long cycle time so far.

Edit: just realised that ethcore is it's own package/cargo-thing, so trying to run tests from within that directory instead of root. anyway feels like I'm making progress finally.

Edit2: YES! Great success - cargo test -j 8 should_import_own_transaction_selectively from ./ethcore/ worked! (I mean the test failed, but I got it to run! And it was only like 2 min to compile)

XertroV · 2018-06-13T09:05:36Z

- 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

XertroV · 2018-06-13T09:55:39Z

Sweet - I think we're good to go. Thanks for being super on top of labels and such @5chdn. Parity really is one of the most functional and responsive github orgs I've ever come across. Massive props 👏.

Notes:

The only part I'm not happy with is some of the repeated / copypasted code between the tests should_import_external_transaction and the new should_import_own_transaction_selectively. That could be refactored to be identical, which (if we want to replicate external tx behaviour) seems like a good idea. I'm willing to do that if it's desired.
At one stage in dev I explicitly returned from import_own_transactions but refactored to a bigger if statement so there are no explicit returns. Not sure what y'all preferences are in this regard, but this is cleaner to me (coming from purescript/haskell)
This feels cleaner than the two options in Option to not mark transactions as local when received over RPC #8820 and the behaviour is clearer / more predictable than either of those options.
I chose to act exactly like import_external_transactions in the case we don't treat it as local to avoid any new or undefined behaviour.
I think the issue arose because many moons ago the assumption was that transactions either came from the network or a trusted source (e.g. eth_sendRawTransaction).
I'd like to see the default on tx_queue_allow_unknown_local be changed to false in an upcoming release. It's safer as the default and isn't likely to affect many people b/c it checks for local accounts. The edge case is people who submit low-fee transactions (which they want the node to keep) from an external source. I'm not sure if a watch-only wallet would solve that (though it looks like AccountProvider doesn't support that atm).
It'd be nice to have some docs that could prevent new contributors going through the growing pains I did above.
I was pleasantly surprised by Rust, having never used it before. Despite some unintuitive things it feels like a very productive and capable language (and ❤️ for anything monadic, like Option and Result). do syntax would be amazing if it doesn't exist yet.

Bonus fun fact: this is the first work I've done relating to anything core to Ethereum since February 2014.

5chdn · 2018-06-13T09:57:24Z

Parity really is one of the most functional and responsive github orgs I've ever come across. Massive props clap.

Haha, wait till you (eventually) get a review. 🤣

XertroV · 2018-06-13T10:00:12Z

Haha, wait till you (eventually) get a review. 🤣

Heh, I guess I haven't had the full experience yet 😛, but I also get that merging code fast-and-loose into something like parity is probably not very wise. Totally happy if that part takes a bit. On that note, also happy to rework things to bring it up to Parity's standards.

sorpaas

I think we should wrap import_own_transaction rather than modifying it, as it may break some previous assumptions for this function used elsewhere. Other comments are just minor grumbles.

sorpaas · 2018-06-13T11:42:29Z

ethcore/src/miner/miner.rs

+
+
+			imported
+		} else {


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.

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

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.)

Both sound fine to me. :)

sorpaas · 2018-06-13T11:43:00Z

ethcore/src/miner/miner.rs

+		} 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()])


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

sorpaas · 2018-06-13T11:43:15Z

ethcore/src/miner/miner.rs

@@ -1146,37 +1162,44 @@ mod tests {
 		assert!(miner.submit_seal(hash, vec![]).is_ok());
 	}

+	fn  default_miner_opts() -> MinerOptions {


Extra whitespace after fn.

sorpaas · 2018-06-13T11:44:59Z

ethcore/src/miner/miner.rs

-					tx_gas_limit: U256::max_value(),
-				},
-			},
+			default_miner_opts(),


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

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)

Alternative is to rename it to something like std_test_miner_opts

sorpaas · 2018-06-13T11:45:54Z

parity/cli/mod.rs

@@ -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(),


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

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)

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.

XertroV · 2018-06-13T14:05:51Z

@sorpaas - The best name I think I've come up with is something like demote_unfamiliar_locals for the flag name, and import_local_transaction for the import function. By the definition of local the txs we get over eth_sendRawTransaction are local, we just don't want to prioritise the ones that don't come from us. These names still feel clear, and import_own_transaction actually does mean our own transaction now.

sorpaas · 2018-06-13T14:15:14Z

In our current transaction pool implementation (in miner/src/pool/verifier.rs) we call transactions handled by import_own_transaction to be Transaction::Local(...), and import_external_transaction to be Transaction::Unverified(...). So I guess we may not want to overload the term "local" too much in our code.

But I think that's just a minor naming issue and both sound fine to me. :)

XertroV · 2018-06-13T23:44:46Z

@sorpaas - Fixed up as per above. Note I went with tx_queue_no_unfamiliar_locals as the flag/config name, and import_claimed_local_transaction as the function name. These feel quite solid as names to me.

XertroV · 2018-06-14T09:54:07Z

So I seem to be getting this error the last two gitlab-build-tests that have run:

fatal: cannot create directory at 'BlockchainTests/GeneralStateTests/stQuadraticComplexityTest': No space left on device
Cloning into 'ethcore/res/wasm-tests'...
fatal: could not write to /builds/parity/parity/ethcore/res/wasm-tests/.git: No space left on device
fatal: clone of 'https://github.com/paritytech/wasm-tests' into submodule path 'ethcore/res/wasm-tests' failed

Ideas?

fix arg name Note: force commit to try and get gitlab tests working again 😠

sorpaas · 2018-06-15T10:12:22Z

rpc/src/v1/helpers/dispatch.rs

+		// 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)


@XertroV Can we only use import_claimed_local_transaction for eth_sendRawTransaction? I'm worried about a race condition where:

A new transaction, from a vault account, is sent through eth_sendTransaction.

Before the dispatcher gets a chance, the vault is closed, and thus the account is removed from address list.

The dispatcher gives the transaction to miner. Miner doesn't find it in the account list, so it's imported as external transaction -- clearly this is not what we want.

A simple way may be just to add a boolean param in dispatch_transaction, and only set that to true for eth_sendRawTransaction.

Yeah okay. I'm not too familiar with how all of this connects together, FYI.
What I'm doing atm is adding a trusted: bool param to dispatch_transaction that gets called with false from eth_sendRawTx and true from the other uses of dispatch_transaction (which seems to be called from personal.rs or signer.rs. That should mean all RPC methods that involve signing get a free pass regardless of config options.

Also going to add this flag to import_claimed_local_transaction.

sorpaas · 2018-06-15T10:14:26Z

rpc/src/v1/tests/helpers/miner_service.rs

@@ -169,6 +169,21 @@ impl MinerService for TestMinerService {
 		Ok(())
 	}

+	/// Imports transactions to queue - treats as local based on config and tx source
+	fn import_claimed_local_transaction<C: Nonce + Sync>(&self, chain: &C, pending: PendingTransaction)


Maybe just call import_own_transaction below. The function body looks duplicate.

I tried that today and couldn't figure out how to do it (calling the instance of import_own_transaction above it).
The compiler told me to add BlockChainClient to C because self.import_own_transaction is called on MinerService not TestMinerService, but that didn't feel right.

Eventually I realised that TestMinerService::import_own_transaction isn't actually called anymore, so have left the body in my function and added unimplemented!() to import_own_transaction.

XertroV · 2018-06-16T08:18:31Z

ethcore/src/miner/miner.rs

@@ -1191,6 +1220,34 @@ mod tests {
 		}.sign(keypair.secret(), Some(chain_id))
 	}

+	fn post_tx_assertions(


This test refactor isn't correct.

- 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

sorpaas

Looks good to me!

5chdn · 2018-06-18T09:57:58Z

Needs a 2nd review.

tomusdrw

lgtm

tomusdrw · 2018-06-18T13:31:13Z

ethcore/src/miner/miner.rs

@@ -794,8 +797,9 @@ impl miner::MinerService for Miner {
 	fn import_own_transaction<C: miner::BlockChainClient>(
 		&self,
 		chain: &C,
-		pending: PendingTransaction,
+		pending: PendingTransaction


We prefer to keep trailing commas in general.

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

…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

* `duration_ns: u64 -> duration: Duration` (#8457) * duration_ns: u64 -> duration: Duration * format on millis {:.2} -> {} * Keep all enacted blocks notify in order (#8524) * Keep all enacted blocks notify in order * Collect is unnecessary * Update ChainNotify to use ChainRouteType * Fix all ethcore fn defs * Wrap the type within ChainRoute * Fix private-tx and sync api * Fix secret_store API * Fix updater API * Fix rpc api * Fix informant api * Eagerly cache enacted/retracted and remove contain_enacted/retracted * Fix indent * tests: should use full expr form for struct constructor * Use into_enacted_retracted to further avoid copy * typo: not a function * rpc/tests: ChainRoute -> ChainRoute::new * Handle removed logs in filter changes and add geth compatibility field (#8796) * Add removed geth compatibility field in log * Fix mocked tests * Add field block hash in PollFilter * Store last block hash info for log filters * Implement canon route * Use canon logs for fetching reorg logs Light client removed logs fetching is disabled. It looks expensive. * Make sure removed flag is set * Address grumbles * Fixed AuthorityRound deadlock on shutdown, closes #8088 (#8803) * CI: Fix docker tags (#8822) * scripts: enable docker builds for beta and stable * scripts: docker latest should be beta not master * scripts: docker latest is master * ethcore: fix ancient block error msg handling (#8832) * Disable parallel verification and skip verifiying already imported txs. (#8834) * Reject transactions that are already in pool without verifying them. * Avoid verifying already imported transactions. * Fix concurrent access to signer queue (#8854) * Fix concurrent access to signer queue * Put request back to the queue if confirmation failed * typo: fix docs and rename functions to be more specific `request_notify` does not need to be public, and it's renamed to `notify_result`. `notify` is renamed to `notify_message`. * Change trace info "Transaction" -> "Request" * Don't allocate in expect_valid_rlp unless necessary (#8867) * don't allocate via format! in case there's no error * fix test? * fixed ipc leak, closes #8774 (#8876) * Add new ovh bootnodes and fix port for foundation bootnode 3.2 (#8886) * Add new ovh bootnodes and fix port for foundation bootnode 3.2 * Remove old bootnodes. * Remove duplicate 1118980bf48b0a3640bdba04e0fe78b1add18e1cd99bf22d53daac1fd9972ad650df52176e7c7d89d1114cfef2bc23a2959aa54998a46afcf7d91809f0855082 * Block 0 is valid in queries (#8891) Early exit for block nr 0 leads to spurious error about pruning: `…your node is running with state pruning…`. Fixes #7547, #8762 * Add ETC Cooperative-run load balanced parity node (#8892) * Minor fix in chain supplier and light provider (#8906) * fix chain supplier increment * fix light provider block_headers * Check whether we need resealing in miner and unwrap has_account in account_provider (#8853) * Remove unused Result wrap in has_account * Check whether we need to reseal for external transactions * Fix reference to has_account interface * typo: missing ) * Refactor duplicates to prepare_and_update_sealing * Fix build * Allow disabling local-by-default for transactions with new config entry (#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

* master: ethstore: retry deduplication of wallet file names until success (#8910) Update ropsten.json (#8926) Include node identity in the P2P advertised client version. (#8830) Allow disabling local-by-default for transactions with new config entry (#8882) Allow Poll Lifetime to be configured via CLI (#8885)

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Include node identity in the P2P advertised client version. (openethereum#8830) Allow disabling local-by-default for transactions with new config entry (openethereum#8882) Allow Poll Lifetime to be configured via CLI (openethereum#8885) cleanup nibbleslice (openethereum#8915) Hardware-wallets `Clean up things I missed in the latest PR` (openethereum#8890) Remove debian/.deb and centos/.rpm packaging scripts (openethereum#8887) Remove a weird emoji in new_social docs (openethereum#8913) Minor fix in chain supplier and light provider (openethereum#8906)

5chdn added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Jun 13, 2018

5chdn added this to the 1.12 milestone Jun 13, 2018

5chdn added the B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. label Jun 13, 2018

XertroV changed the title ~~[WIP] Allow disabling local-by-default for transactions submitted through eth_sendRawTransaction and related methods~~ Allow disabling local-by-default for transactions submitted through eth_sendRawTransaction and related methods with config entry Jun 13, 2018

5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jun 13, 2018

XertroV changed the title ~~Allow disabling local-by-default for transactions submitted through eth_sendRawTransaction and related methods with config entry~~ Allow disabling local-by-default for transactions with new config entry Jun 13, 2018

XertroV force-pushed the patch-1 branch from 54d8c95 to a4fee0e Compare June 13, 2018 09:44

sorpaas reviewed Jun 13, 2018

View reviewed changes

XertroV closed this Jun 13, 2018

XertroV reopened this Jun 13, 2018

XertroV force-pushed the patch-1 branch 2 times, most recently from c69582c to 2cc5e5e Compare June 14, 2018 09:13

Refactor flag name + don't change import_own_tx behaviour

7a54866

fix arg name Note: force commit to try and get gitlab tests working again 😠

XertroV force-pushed the patch-1 branch from 2cc5e5e to 7a54866 Compare June 15, 2018 00:43

Add fn to TestMinerService

c92845b

sorpaas reviewed Jun 15, 2018

View reviewed changes

XertroV commented Jun 16, 2018

View reviewed changes

Avoid race condition from trusted sources

07125c7

- 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

XertroV force-pushed the patch-1 branch from 68f921b to 07125c7 Compare June 16, 2018 08:26

sorpaas approved these changes Jun 16, 2018

View reviewed changes

5chdn requested a review from tomusdrw June 18, 2018 11:59

5chdn mentioned this pull request Jun 18, 2018

Release next-beta 1.11 #8190

Closed

57 tasks

tomusdrw approved these changes Jun 18, 2018

View reviewed changes

tomusdrw merged commit 6004c39 into openethereum:master Jun 18, 2018

5chdn added the B0-patchthis label Jun 18, 2018

andresilva mentioned this pull request Jun 18, 2018

[beta] Backports #8916

Merged

18 tasks

XertroV deleted the patch-1 branch June 24, 2018 04:18

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

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

Conversation

XertroV commented Jun 13, 2018 • edited Loading

parity-cla-bot commented Jun 13, 2018

XertroV commented Jun 13, 2018

5chdn commented Jun 13, 2018

XertroV commented Jun 13, 2018 • edited Loading

XertroV commented Jun 13, 2018

XertroV commented Jun 13, 2018 • edited Loading

5chdn commented Jun 13, 2018

XertroV commented Jun 13, 2018

sorpaas left a comment

Choose a reason for hiding this comment

sorpaas Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XertroV Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XertroV Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XertroV commented Jun 13, 2018

sorpaas commented Jun 13, 2018

XertroV commented Jun 13, 2018

XertroV commented Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorpaas left a comment

Choose a reason for hiding this comment

5chdn commented Jun 18, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XertroV commented Jun 13, 2018 •

edited

Loading

XertroV commented Jun 13, 2018 •

edited

Loading

XertroV commented Jun 13, 2018 •

edited

Loading

sorpaas Jun 13, 2018 •

edited

Loading

XertroV Jun 13, 2018 •

edited

Loading

XertroV Jun 13, 2018 •

edited

Loading

XertroV commented Jun 14, 2018 •

edited

Loading