Implement caching for service transactions checker by VladLupashevskyi · Pull Request #10088 · openethereum/parity-ethereum

VladLupashevskyi · 2018-12-19T23:30:41Z

In cases when certain address is allowed for executing free transactions it makes sense in my opinion to submit transactions which are automatically submitted by the client (such as validators reporting etc) with the gas price set to zero.

Here I propose my code improvement to achieve this by adding check for free tx certification in transact_contract fn which is called while doing validators reporting and other actions.

Looking forward for the feedback :)

# Conflicts: # ethcore/res/contracts/tx_acl.json # ethcore/src/tx_filter.rs

…ract

parity-cla-bot · 2018-12-19T23:30:43Z

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

Many thanks,

Parity Technologies CLA Bot

tomusdrw

The idea looks reasonable I'm only concerned with circular call to state - we should make sure that all the calls are not vulnerable to deadlocks or some data races now.
(and obv the implementation needs some improvements)

tomusdrw · 2018-12-24T10:06:35Z

 			nonce: self.latest_nonce(&authoring_params.author),
 			action: Action::Call(address),
-			gas: self.importer.miner.sensible_gas_limit(),
+			gas: gas_price,


I think you meant to change gas_price not gas.

tomusdrw · 2018-12-24T10:12:05Z

 	fn transact_contract(&self, address: Address, data: Bytes) -> Result<(), transaction::Error> {
 		let authoring_params = self.importer.miner.authoring_params();
+		let service_transaction_checker = ServiceTransactionChecker::default();
+		let gas_price = match service_transaction_checker.check_address(self, address) {


address is contract address you try to call, not the sender (which should be checked here).

tomusdrw · 2018-12-24T10:12:39Z


 mod miner;
-mod service_transaction_checker;
+pub mod service_transaction_checker;


please group with other pub mod

VladLupashevskyi · 2019-01-09T20:59:21Z

@tomusdrw I pushed the fixes and sorry for these typos.

I am not sure what kind of deadlocks or data races could occur because free tx permission is checked for the latest mined block and not for the pending one. What I can think of is that issues could occur if we would call transact_contract inside of check_address function, but calling state modifying function inside state checking one does not make sense for me.

Maybe I am missing something, so I would be happy to get more feedback and dig into solving it :)

# Conflicts: # ethcore/src/client/client.rs

tomusdrw

lgtm

sorpaas · 2019-01-11T17:16:15Z


+	/// Checks if given address is whitelisted to send service transactions.
+	pub fn check_address<C: CallContract + RegistryInfo>(&self, client: &C, sender: Address) -> Result<bool, String> {
+		let contract_address = client.registry_address(SERVICE_TRANSACTION_CONTRACT_REGISTRY_NAME.to_owned(), BlockId::Latest)


Would the performance be a concern here? registry_address would need to query the state if registrar is set.

I don't think that an issue, some cache would be nice (best would be if previously queried registry entries would be refreshed after a block is imported), but the code is not super hot (only in \.transact which is called by the engine, or by the miner, called only for zero-gas-price transactions attempting to enter the pool).

VladLupashevskyi · 2019-01-21T19:24:59Z

I added a cache for certified addresses and provided logic as @tomusdrw described.

This is first time I'm working with caching so there is definitely something to improve and I'm always happy to get a feedback :)

VladLupashevskyi · 2019-02-05T15:20:18Z

@tomusdrw I think idea with moving ServiceTxChecker to one place sounds reasonable! I would create a public method to get the checker instance from miner, so it's done more generic than just exposing a check method.
So I'm gonna dig into this and will notify about the results!

This reverts commit 4e7371d, reversing changes made to bffd73e.

This reverts commit bffd73e.

… ChainNotify for ServiceTransactionChecker" This reverts commit 6e73d1e.

This reverts commit ec610a3.

This reverts commit 06a4b2d.

This reverts commit 51bbad3.

…t + revert unnecessary changes

VladLupashevskyi · 2019-02-05T19:48:39Z

@tomusdrw I updated the code according to the comments, now it looks much cleaner! I'm thinking it would be nice to introduce some cache limiting, I would propose to use the solution I introduced in another PR for NodeFilter:

https://github.com/paritytech/parity-ethereum/blob/2202646acefc04e4a26c6fed3952728f0c355762/ethcore/node-filter/src/lib.rs#L107

where basically I used Vec in order to store sequence of recent calls to the contract and remove the oldest ones when cache hits limit.

What do you think about it?

5chdn · 2019-02-07T11:40:02Z

Could you revisit this, @tomusdrw

tomusdrw

Looks good, few nitpicks.

tomusdrw · 2019-02-07T13:07:41Z

 /// Service transactions checker.
-#[derive(Default, Clone)]
-pub struct ServiceTransactionChecker;
+#[derive(Clone)]


Do we need Clone?

We need Clone in order to share ServiceTransactionChecker with Client and PoolClient.

tomusdrw · 2019-02-07T13:09:35Z

 	/// Checks if given address is whitelisted to send service transactions.
 	pub fn check_address<C: CallContract + RegistryInfo>(&self, client: &C, sender: Address) -> Result<bool, String> {
+		trace!(target: "txqueue", "Checking service transaction checker contract from {}", sender);
+		if let Some(allowed) = self.certified_addresses_cache.try_read().as_ref().and_then(|c| c.get(&sender)){


.as_ref() seems redundant, also missing space before {

I cannot do this without .as_ref() since the c doesn't live long enough in this case. I tried to do it with .map, but in this case Option<Option<&bool>> is returned, so one more check is needed. Not sure what would be better solution here.

# Conflicts: # Cargo.lock # ethcore/private-tx/src/lib.rs # ethcore/src/miner/miner.rs # ethcore/src/miner/pool_client.rs

tomusdrw

👌

soc1c · 2019-02-22T14:34:11Z

Needs a 2nd review 👍

* master: fix(light cull): poll light cull instead of timer (#10559) Update Issue Template to direct security issue to email (#10562) RPC: Implements eth_subscribe("syncing") (#10311) Explicitly enable or disable Stratum in config file (Issue 9785) (#10521) version: bump master to 2.6 (#10560) tx-pool: check transaction readiness before replacing (#10526) fix(light account response): update `tx_queue` (#10545) Update light client harcoded headers (#10547) fix(light eth_gasPrice): ask network if not in cache (#10535) Implement caching for service transactions checker (#10088) build android with cache, win fixes (#10546) clique: make state backfill time measurement more accurate (#10551) updated lru-cache to 0.1.2 (#10542)

VladLupashevskyi added 7 commits April 13, 2018 16:01

Tx permission contract improvement

234e582

Merge remote-tracking branch 'upstream/master'

da93f5c

Merge branch 'master' of https://github.com/paritytech/parity

fbe439f

Merge remote-tracking branch 'remotes/upstream/master'

7f56abe

# Conflicts: # ethcore/res/contracts/tx_acl.json # ethcore/src/tx_filter.rs

Merge branch 'master' of https://github.com/paritytech/parity

1f9c595

Merge branch 'master' of https://github.com/paritytech/parity

5984a41

Take in account zero gas price certification when doing transact_cont…

16571bc

…ract

tomusdrw suggested changes Dec 24, 2018

View reviewed changes

tomusdrw added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. M4-core ⛓ Core client code / Rust. labels Dec 24, 2018

5chdn added this to the 2.3 milestone Jan 2, 2019

VladLupashevskyi added 2 commits January 9, 2019 22:35

DRY in ServiceTransactionChecker

2bf7312

Fix typos and regroup mod

33c769f

Merge branch 'master' of https://github.com/paritytech/parity

fb74e9d

# Conflicts: # ethcore/src/client/client.rs

5chdn modified the milestones: 2.3, 2.4 Jan 10, 2019

5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Jan 10, 2019

tomusdrw approved these changes Jan 11, 2019

View reviewed changes

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 11, 2019

sorpaas reviewed Jan 11, 2019

View reviewed changes

VladLupashevskyi added 4 commits January 21, 2019 21:18

Introduce CertifiedAddressesCache

b2c127d

Introduce refresh_cache for CertifiedAddressesCache

086cc5b

Add CertifiedAddressesCache read and write on checking

6cab68f

Refresh CertifiedAddressesCache on new imported block

1ccf28c

Merge branch 'master' of https://github.com/paritytech/parity

0f4df80

VladLupashevskyi added 8 commits February 5, 2019 17:31

Revert "Merge branch 'master' into master"

0a97ea2

This reverts commit 4e7371d, reversing changes made to bffd73e.

Revert "Fix watcher test"

aba3273

This reverts commit bffd73e.

Revert "WIP: Separated ChainNotify from ethcore trait and implemented…

f383cec

… ChainNotify for ServiceTransactionChecker" This reverts commit 6e73d1e.

Revert "Fix Client::new in add_peer_with_private_config"

fbcf37b

This reverts commit ec610a3.

Revert "Fix node_filter test"

49cdf39

This reverts commit 06a4b2d.

Revert "Fix Client and Miner in tests"

8c2c152

This reverts commit 51bbad3.

Implement ServiceTransactionChecker in miner and delegate it to clien…

9c41cdc

…t + revert unnecessary changes

Merge master

93955a7

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 Feb 7, 2019

tomusdrw reviewed Feb 7, 2019

View reviewed changes

VladLupashevskyi added 3 commits February 7, 2019 16:44

Code improvements

3415776

Merge branch 'master' of https://github.com/paritytech/parity-ethereum

497b951

# Conflicts: # Cargo.lock # ethcore/private-tx/src/lib.rs # ethcore/src/miner/miner.rs # ethcore/src/miner/pool_client.rs

Merge branch 'master' of https://github.com/paritytech/parity-ethereum

04841c6

# Conflicts: # Cargo.lock # ethcore/private-tx/src/lib.rs # ethcore/src/miner/miner.rs # ethcore/src/miner/pool_client.rs

tomusdrw approved these changes Feb 8, 2019

View reviewed changes

5chdn modified the milestones: 2.4, 2.5, 2.6 Feb 21, 2019

soc1c added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 22, 2019

soc1c merged commit 7b2afdf into openethereum:master Mar 31, 2019

tomusdrw mentioned this pull request Oct 28, 2019

[json-tests] Sort out the logging as tests run #11084

Closed

Conversation

VladLupashevskyi commented Dec 19, 2018

Uh oh!

parity-cla-bot commented Dec 19, 2018

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VladLupashevskyi commented Jan 9, 2019

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VladLupashevskyi commented Jan 21, 2019

Uh oh!

VladLupashevskyi commented Feb 5, 2019

Uh oh!

VladLupashevskyi commented Feb 5, 2019

Uh oh!

5chdn commented Feb 7, 2019

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

soc1c commented Feb 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants