Integration with zgp whitelist contract #4215

svyatonik · 2017-01-19T06:00:48Z

Currently deployed on testnet: 0x4154A2671b7474883286a4493B97848f116395C6 - to be deployed on mainnet

svyatonik · 2017-01-19T06:06:38Z

ethcore/src/miner/miner.rs

@@ -614,12 +615,19 @@ impl Miner {
 							}
 						}).unwrap_or(default_origin);

+						// try to install zgp-checker before appending transactions


There are 2 concerns about this:

set_zero_gas_price_checker() must be called on Miner() initialization + on new blocks, but when Miner is initialized, Client (which is used by OnChainZeroGasPriceChecker::from_chain) is not yet ready => I placed it here

as I understand, registry can be updated => address can be changed => possibly this condition if !transaction_queue.is_zero_gas_price_checker_set() is unnecessary. But I have checked how updater works:
https://github.com/ethcore/parity/blob/master/updater/src/updater.rs#L242
it works the same way. So maybe I'm wrong && it is ok to have single initialization.

it's highly unlikely that it will ever be changed once set. if it is, then it is not unreasonable to expect miners to restart in order to get the "correct" ZGP addresses. as such i don't see it as a huge problem.

I think it's better to make sure you're using correct one, otherwise you can try to do some attacks on the syncing node (like feeding a chain with different ZGP contract, that will be re-organized later, but the ZGP contract stays the same)

@gavofyork @tomusdrw I personally agree with @tomusdrw , but I think maybe you both could reach a consensus in this question :)

svyatonik · 2017-01-19T06:10:15Z

scripts/contractABI.js

@@ -39,14 +42,15 @@ function convertContract(name, json, prefs) {
 	return `${prefs._pub ? "pub " : ""}struct ${name} {
 	contract: ethabi::Contract,
 	address: util::Address,
-	do_call: Box<Fn(util::Address, Vec<u8>) -> Result<Vec<u8>, String> + Send ${prefs._sync ? "+ Sync " : ""}+ 'static>,
+	${prefs._explicit_do_call ? "" : `do_call: Box<Fn(util::Address, Vec<u8>) -> Result<Vec<u8>, String> + Send${prefs._sync ? " + Sync " : ""}+ 'static>,`}


The idea of _explicit_do_call is to pass do_call to contract' methods, instead of constructor. That's because Miner() don't have Arc, but &Client is passed to its methods => I can construct closure when calling a contract' method, but can't make a persistent closure.

svyatonik · 2017-01-19T06:11:13Z

scripts/contractABI.js

 let jsonabi = [{"constant":true,"inputs":[],"name":"getValidators","outputs":[{"name":"","type":"address[]"}],"payable":false,"type":"function"}];

-let out = makeContractFile("Contract", jsonabi, {"_pub": true, "_": {"_client": {"string": true}, "_platform": {"string": true}}, "_sync": true});
+// parse command line options
+for (let i = 1; i < process.argv.length; ++i) {


Just added some command line arguments parsing, as it is seems more convenient to me.

svyatonik · 2017-01-19T06:12:33Z

sync/src/chain.rs

+	if !client_id.starts_with("Parity/v") {
+		return false;
+	}
+	let ver: Vec<u32> = client_id[8..].split('.')


Don't know if checking Parity' version is necessary - seems like current Parity version doesn't drop connection, event when zgp-transaction is received => maybe this is an optional check

svyatonik · 2017-01-19T06:15:16Z

ethcore/src/miner/transaction_queue.rs

-				tx.gas_price,
-				self.minimal_gas_price
-			);
+		if origin != TransactionOrigin::Local && tx.gas_price < self.minimal_gas_price {


As I understand from the code, local transactions were always accepted, even if gas_price was too low => added zgp-checks for non-local transactions only.

keorn · 2017-01-19T08:47:22Z

So right now the only way to disable it is to change/remove the "registrar" from the Ethereum spec?

gavofyork · 2017-01-19T08:53:46Z

ethcore/src/miner/zero_gas_price_checker.rs

+
+impl OnChainZeroGasPriceChecker {
+	/// Try to create instance, reading contract address from given chain.
+	pub fn from_chain(chain: &MiningBlockChainClient) -> Option<Self> {


i would rename chain to client since it is, in fact, a Client, not a Blockchain.

gavofyork · 2017-01-19T08:54:35Z

ethcore/src/miner/zero_gas_price_checker.rs

+
+impl ZeroGasPriceChecker for OnChainZeroGasPriceChecker {
+	/// Check if we can append zgp-transaction to the transaction queue
+	fn check(&self, chain: Option<&MiningBlockChainClient>, tx: &SignedTransaction) -> Result<bool, String> {


chain -> maybe_client?

gavofyork · 2017-01-19T08:54:47Z

ethcore/src/miner/zero_gas_price_checker.rs

+
+		chain.ok_or("Client not set".to_owned())
+			.and_then(|chain| {
+				let do_call = |a, d| chain.call_contract(a, d);


chain -> client

svyatonik · 2017-01-19T09:04:29Z

@keorn Will update this PR with cli-option

tomusdrw

Looks good, apart from grumble regarding MiningBlockChainClient

tomusdrw · 2017-01-19T08:46:22Z

ethcore/src/miner/zero_gas_price_checker.rs

+}
+
+mod provider {
+	// Autogenerated from JSON contract definition using Rust contract convertor.


IMHO we should turn the generator into macro sometime soon, it's producing A LOT of difficult to maintain code.

yes, I have no idea why we're checking in autogenerated code. we should check in the contract ABIs and generate rust bindings at build time.

That would be ideal, as a compromise (short-term solution) a macro like this:

contract_client! { impl ContactName { abi = include!("./contract_name.json"); fn set_owner=setOwner(new: &Address) as -> Result<(), String>; fn address=getAddress(who: &Address, field: &str) -> Result<Address, String>; fn revoke(who: &Address) -> Result<(), String> } }

should be fairly easy to implement.

tomusdrw · 2017-01-19T08:52:58Z

sync/src/chain.rs

+	if !client_id.starts_with("Parity/v") {
+		return false;
+	}
+	let ver: Vec<u32> = client_id[8..].split('.')


I think this would be cleaner:

let prefix = "Parity/v" if !client_id.starts_with(prefix) { return false; } let ver: Vec<u32> = client_id[prefix.len()..].split('.)

tomusdrw · 2017-01-19T09:03:42Z

ethcore/src/miner/banning_queue.rs

@@ -78,6 +79,7 @@ impl BanningTransactionQueue {
 	/// May reject transaction because of the banlist.
 	pub fn add_with_banlist<F, G>(
 		&mut self,
+		chain: Option<&MiningBlockChainClient>,


IMHO it would be better to avoid passing whole Client to transaction queue, I was trying to abstract out parts that are needed. Seems that with ZeroGasPriceChecker it could also play nicely (you get get rid of couple of Option and Box)

pub fn add_with_banlist<F, G, H>( &mut self, transaction: SignedTransaction, time: QueingInstant, account_details: &F, gas_estimator: &G, zgp_checker: &H, ) -> ... where F: Fn(&Address) -> AccountDetails, G: Fn(&SignedTransaction) -> U256, H: Fn(&Address) -> bool/Result<(), String> { }

In such case you don't need neither Option<&MiningBlockChainClient> nor Option<Box<GasPriceChecker>>

Since we already have 3 generic functions, probably it could be further refactored to a single trait returning all those details.

done, but maybe you have a better name for this trait (currently it is TransactionDetailsProvider) :)

tomusdrw · 2017-01-19T14:32:09Z

ethcore/src/miner/service_transaction_checker.rs

+impl ServiceTransactionChecker {
+	/// Try to create instance, reading contract address from given chain client.
+	pub fn update_from_chain_client(&self, client: &MiningBlockChainClient) {
+		let mut contract = self.contract.lock();


Wouldn't it be easier to construct provider::Contract on each call without memoizing it?
You could probably just memoize the parsed ABI and then construct provider::Contract - in such case explicit_do_call would be unecessary.

yes, or perhaps a lazy_static! contract would suffice as well.

Yes - I can construct provider::Contract on each call, but I need to store contract_address here (or in Miner, but I want to avoid it) anyway => so the code will stay almost the same, except that I'll break provider::Contract initialization to 2 steps (parse ABI once, then pass result to the Contract::new). Not sure if that is better than it is now :) But feel free to insist, if you're sure this is required

coveralls · 2017-01-19T20:14:35Z

Changes Unknown when pulling 2b9462e on zgp_checker into ** on master**.

gavofyork · 2017-01-20T12:29:20Z

conflicts

gavofyork · 2017-01-22T15:15:46Z

should be backported to 1.4 & 1.5

* zgp-transactions checker * polishing * rename + refactor * refuse-service-transactions cl option * fixed tests compilation

* v1.5.1 * Disable notifications (#4243) * Fix wrong token handling (#4254) * Fixing wrong token displayed * Linting * Revert filtering out * Revert the revert * Don't panic on uknown git commit hash (#4231) * Additional logs for own transactions (#4278) * Integration with zgp whitelist contract (#4215) * zgp-transactions checker * polishing * rename + refactor * refuse-service-transactions cl option * fixed tests compilation * Renaming signAndSendTransaction to sendTransaction (#4351) * Fixed deadlock in external_url (#4354) * Fixing web3 in console (#4382) * Fixing estimate gas in case histogram is not available (#4387) * Restarting fetch client every now and then (#4399)

svyatonik added 3 commits January 18, 2017 17:10

zgp-transactions checker

7aa9098

polishing

fb47ec1

Merge branch 'master' into zgp_checker

ad1e9b1

svyatonik commented Jan 19, 2017

View reviewed changes

svyatonik added the A0-pleasereview 🤓 Pull request needs code review. label Jan 19, 2017

gavofyork reviewed Jan 19, 2017

View reviewed changes

svyatonik added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 19, 2017

tomusdrw reviewed Jan 19, 2017

View reviewed changes

rphmeier added the M4-core ⛓ Core client code / Rust. label Jan 19, 2017

svyatonik added 2 commits January 19, 2017 15:11

rename + refactor

2557fef

refuse-service-transactions cl option

518e5f0

tomusdrw reviewed Jan 19, 2017

View reviewed changes

fixed tests compilation

2b9462e

svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jan 19, 2017

gavofyork added A8-looksgood 🦄 Pull request is reviewed well. A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. labels Jan 20, 2017

Merge branch 'master' into zgp_checker

e471b63

svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Jan 20, 2017

gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 21, 2017

gavofyork merged commit 092e24b into master Jan 22, 2017

gavofyork deleted the zgp_checker branch January 22, 2017 15:15

gavofyork added the B0-patch label Jan 22, 2017

gavofyork added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Jan 24, 2017

arkpar pushed a commit that referenced this pull request Feb 3, 2017

Integration with zgp whitelist contract (#4215)

e95c86c

* zgp-transactions checker * polishing * rename + refactor * refuse-service-transactions cl option * fixed tests compilation

5chdn mentioned this pull request Jul 5, 2018

Parity Ethereum 2.0.0 #9052

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration with zgp whitelist contract #4215

Integration with zgp whitelist contract #4215

svyatonik commented Jan 19, 2017

svyatonik Jan 19, 2017

gavofyork Jan 19, 2017

tomusdrw Jan 19, 2017

svyatonik Jan 19, 2017

svyatonik Jan 19, 2017

svyatonik Jan 19, 2017

svyatonik Jan 19, 2017

svyatonik Jan 19, 2017

keorn commented Jan 19, 2017 •

edited

gavofyork Jan 19, 2017

svyatonik Jan 19, 2017

gavofyork Jan 19, 2017

svyatonik Jan 19, 2017

gavofyork Jan 19, 2017

svyatonik Jan 19, 2017

svyatonik commented Jan 19, 2017

tomusdrw left a comment

tomusdrw Jan 19, 2017

rphmeier Jan 19, 2017 •

edited

tomusdrw Jan 19, 2017

tomusdrw Jan 19, 2017

svyatonik Jan 19, 2017

tomusdrw Jan 19, 2017

svyatonik Jan 19, 2017

tomusdrw Jan 19, 2017

rphmeier Jan 19, 2017

svyatonik Jan 19, 2017

coveralls commented Jan 19, 2017

gavofyork commented Jan 20, 2017

gavofyork commented Jan 22, 2017

Integration with zgp whitelist contract #4215

Integration with zgp whitelist contract #4215

Conversation

svyatonik commented Jan 19, 2017

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

Choose a reason for hiding this comment

keorn commented Jan 19, 2017 • edited

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

svyatonik commented Jan 19, 2017

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Jan 19, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 19, 2017

gavofyork commented Jan 20, 2017

gavofyork commented Jan 22, 2017

keorn commented Jan 19, 2017 •

edited

rphmeier Jan 19, 2017 •

edited