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

Added --tx-queue-no-early-reject flag to disable early tx queue rejects #9143

Merged
merged 3 commits into from
Jul 24, 2018

Conversation

peterbitfly
Copy link

This PR resolves #9087 by adding a CLI flag to selectively disable a transaction queue optimization that early rejects transactions below the minimal effective gas price, without comparing the sender to the list of local accounts of a node first.

This allows local transactions to always enter the pool, despite it being full, but requires an additional ecrecover operation on every transaction.

@parity-cla-bot
Copy link

It looks like @ppratscher hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@peterbitfly
Copy link
Author

[clabot:check]

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Jul 17, 2018
@5chdn 5chdn added this to the 2.1 milestone Jul 17, 2018
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks good, minor grumble regarding comments.

Would actually be good to add a simple test for it as well, should be just a copy of min_effective_gas_price test but with local transaction actaully getting into the pool if that setting is on.

@@ -43,6 +43,8 @@ pub struct Options {
pub block_gas_limit: U256,
/// Maximal gas limit for a single transaction.
pub tx_gas_limit: U256,
/// Skip check checks
pub tx_queue_no_early_reject: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be named just no_early_reject, we are in the context of tx_queue already.

Also the comment could be a bit more clear :) Maybe something like:

Skip checks for early rejection, to make sure that local transactions are always imported.

Copy link
Author

@peterbitfly peterbitfly Jul 17, 2018

Choose a reason for hiding this comment

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

Sorry, that comment was a leftover typo. Will fix it & also fix the failing tests.

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 17, 2018
@peterbitfly
Copy link
Author

peterbitfly commented Jul 17, 2018

Sorry but I am unable to find a test named min_effective_gas_price. Which test are you referring to?

@tomusdrw
Copy link
Collaborator

tomusdrw commented Jul 17, 2018

@ppratscher was thinking about this test: https://github.com/paritytech/parity/blob/21e0cd7781f55f0007d5c7d87929f49513367e27/miner/src/pool/tests/mod.rs#L987-L1023

Should look somewhat like this:

#[test]
fn should_not_reject_early_in_case_of_local_transaction_and_early_rejections_disabled() {
	// given
	let txq = TransactionQueue::new(
		txpool::Options {
			max_count: 1,
			max_per_sender: 2,
			max_mem_usage: 50
		},
		verifier::Options {
			minimal_gas_price: 1.into(),
			block_gas_limit: 1_000_000.into(),
			tx_gas_limit: 1_000_000.into(),
		},
		PrioritizationStrategy::GasPriceOnly,
	);
	let client = TestClient::new().with_balance(1_000_000_000);
	let tx1 = Tx::gas_price(2).signed().local();

	let res = txq.import(client.clone(), vec![tx1]);
	assert_eq!(res, vec![Ok(())]);
	assert_eq!(txq.status().status.transaction_count, 1);
	assert!(client.was_verification_triggered());

	// when
	let client = TestClient::new();
	let tx1 = Tx::default().signed().local();
	let res = txq.import(client.clone(), vec![tx1]);

	// then
	assert_eq!(res, vec![Ok(())]);
	assert_eq!(txq.status().status.transaction_count, 2);
	assert!(client.was_verification_triggered());
}

@5chdn 5chdn modified the milestones: 2.1, 2.2 Jul 17, 2018
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 23, 2018
@ordian ordian merged commit 1b1941a into openethereum:master Jul 24, 2018
ordian added a commit to ordian/parity that referenced this pull request Jul 31, 2018
* 'master' of https://github.com/paritytech/parity:
  removed client error (openethereum#9253)
  Implement EIP-1052 (EXTCODEHASH) and fix several issues in state account cache (openethereum#9234)
  Improve Tracer documentation (openethereum#9237)
  Update Dockerfile (openethereum#9242)
  block cleanup (openethereum#9117)
  Increase the number of sessions. (openethereum#9203)
  add changelog for 1.11.8 stable and 2.0.1 beta (openethereum#9230)
  fix typo (openethereum#9232)
  Fix potential as_usize overflow when casting from U256 in miner (openethereum#9221)
  Allow old blocks from peers with lower difficulty (openethereum#9226)
  Removes duplicate libudev-dev from Dockerfile (openethereum#9220)
  snap: remove ssl dependencies from snapcraft definition (openethereum#9222)
  remove ssl from dockerfiles, closes openethereum#8880 (openethereum#9195)
  Insert PROOF messages for some cases in blockchain (openethereum#9141)
  [Chain] Add more bootnodes (openethereum#9174)
  ethcore: update bn version (openethereum#9217)
  deserialize block only once during verification (openethereum#9161)
  Simple build instruction fix (openethereum#9215)
  Added --tx-queue-no-early-reject flag to disable early tx queue rejects (openethereum#9143)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parity 1.11.6 is rejecting transactions from local accounts
6 participants