Transaction permissioning #6441

arkpar · 2017-09-02T08:45:22Z

This adds a smart contract that governs transaction permissions. The contract accepts sender address and returns a mask of allowed transaction types for this address.

NikVolf · 2017-09-02T10:50:16Z

How about short description of the feature? :)

NikVolf · 2017-09-02T14:57:30Z

ethcore/src/tx_filter.rs

+
+	pub fn register_client(&self, client: Weak<BlockChainClient>) {
+		*self.client.write() = Some(client);
+


redundant empty line

NikVolf · 2017-09-02T14:57:46Z

ethcore/src/tx_filter.rs

+		self.permission_cache.lock().clear();
+	}
+
+	pub fn register_client(&self, client: Weak<BlockChainClient>) {


Undocumented public method

NikVolf · 2017-09-02T14:58:05Z

ethcore/src/tx_filter.rs

+
+	}
+
+	pub fn transaction_allowed(&self, parent_hash: &H256, transaction: &SignedTransaction) -> bool {


Undocumented public method

NikVolf · 2017-09-02T15:21:08Z

ethcore/src/tx_filter.rs

+
+mod tx_permissions {
+    pub const _ALL: u32 = 0xffffffff;
+    pub const NONE: u32 = 0x0;


spaces instead of tabs

NikVolf · 2017-09-02T15:21:43Z

ethcore/src/tx_filter.rs

+const MAX_CACHE_SIZE: usize = 4096;
+
+mod tx_permissions {
+    pub const _ALL: u32 = 0xffffffff;


why _ALL and not ALL ?

could use the bitflags crate for this

I did not want to introduce another dependency just for this. This type is used internally so a simple u32 is enough.

NikVolf · 2017-09-02T15:22:06Z

ethcore/src/tx_filter.rs

+	pub const BASIC: u32 = 0b00000001;
+	pub const CALL: u32 = 0b00000010;
+	pub const CREATE: u32 = 0b00000100;
+	pub const _PRIVATE: u32 = 0b00001000;


why _PRIVATE and not PRIVATE ?

To suppress compiler warning. This is currently unused

rphmeier · 2017-09-04T13:01:09Z

ethcore/src/tx_filter.rs

+}
+
+impl ChainNotify for TransactionFilter {
+	fn new_blocks(&self, imported: Vec<H256>, _invalid: Vec<H256>, _enacted: Vec<H256>, _retracted: Vec<H256>, _sealed: Vec<H256>, _proposed: Vec<Bytes>, _duration: u64) {


now that we're adding so many new chain-notify implementations, would be nice to not be cloning each of these vectors each time, especially since most of them only read.

Agreed, although most of the time they only contain 0 or 1 elements.

rphmeier · 2017-09-05T14:56:05Z

ethcore/src/ethereum/ethash.rs

+
+	fn register_client(&self, client: Weak<Client>) {
+		if let Some(ref filter) = self.tx_filter {
+			filter.register_client(client as Weak<BlockChainClient>);


engines are used in light clients too. requiring a full node to run consensus logic should have been a blocker for this PR.

furthermore, I don't see why TX filtering would be coupled with the Ethash memory-hard PoW consensus system in the first place. it makes more sense in a permissioned environment anyway (and the way to make that happen is not by copy-pasting this feature to other engines whenever we want to use them together...)

This reverts commit eed0e8b.

This reverts commit 8f96415.

* Update token updates * Update token info fetching * Update logger * Minor fixes to updates and notifications for balances * Use Pubsub * Fix timeout. * Use pubsub for status. * Fix signer subscription. * Process tokens in chunks. * Fix tokens loaded by chunks * Linting * Dispatch tokens asap * Fix chunks processing. * Better filter options * Parallel log fetching. * Fix signer polling. * Fix initial block query. * Token balances updates : the right(er) way * Better tokens info fetching * Fixes in token data fetching * Only fetch what's needed (tokens) * Fix linting issues * Revert "Transaction permissioning (#6441)" This reverts commit eed0e8b. * Revert "Revert "Transaction permissioning (#6441)"" This reverts commit 8f96415. * Update wasm-tests. * Fixing balances fetching * Fix requests tracking in UI * Fix request watching * Update the Logger * PR Grumbles Fixes * PR Grumbles fixes * Linting...

* Fix slow balances (#6471) * Update token updates * Update token info fetching * Update logger * Minor fixes to updates and notifications for balances * Use Pubsub * Fix timeout. * Use pubsub for status. * Fix signer subscription. * Process tokens in chunks. * Fix tokens loaded by chunks * Linting * Dispatch tokens asap * Fix chunks processing. * Better filter options * Parallel log fetching. * Fix signer polling. * Fix initial block query. * Token balances updates : the right(er) way * Better tokens info fetching * Fixes in token data fetching * Only fetch what's needed (tokens) * Fix linting issues * Revert "Transaction permissioning (#6441)" This reverts commit eed0e8b. * Revert "Revert "Transaction permissioning (#6441)"" This reverts commit 8f96415. * Update wasm-tests. * Fixing balances fetching * Fix requests tracking in UI * Fix request watching * Update the Logger * PR Grumbles Fixes * PR Grumbles fixes * Linting... * eth_call returns output of contract creations (#6420) * eth_call returns output of contract creations * Fix parameters order. * Save outputs for light client as well. * Don't accept transactions above block gas limit. * Expose health status over RPC (#6274) * Node-health to a separate crate. * Initialize node_health outside of dapps. * Expose health over RPC. * Bring back 412 and fix JS. * Add health to workspace and tests. * Fix compilation without default features. * Fix borked merge. * Revert to generics to avoid virtual calls. * Fix node-health tests. * Add missing trailing comma. * Fixing/removing failing JS tests. * do not activate genesis epoch in immediate transition validator contract (#6349) * Fix memory tracing. * Add test to cover that. * ensure balances of constructor accounts are kept * test balance of spec-constructed account is kept

arkpar added A0-pleasereview 🤓 Pull request needs code review. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M4-core ⛓ Core client code / Rust. labels Sep 2, 2017

NikVolf added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 2, 2017

NikVolf reviewed Sep 2, 2017

View reviewed changes

Transaction permissioning

494adf0

arkpar force-pushed the tx-acl branch from 67eabae to 494adf0 Compare September 4, 2017 09:58

arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 4, 2017

NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 4, 2017

rphmeier reviewed Sep 4, 2017

View reviewed changes

gavofyork merged commit eed0e8b into master Sep 5, 2017

gavofyork deleted the tx-acl branch September 5, 2017 09:39

rphmeier reviewed Sep 5, 2017

View reviewed changes

rphmeier mentioned this pull request Sep 5, 2017

Reopen Light-client sync for contract-based PoA #6466

Closed

grbIzl mentioned this pull request Sep 5, 2017

Transaction permissions added into authority round engine as well #6469

Closed

tomusdrw added a commit that referenced this pull request Sep 5, 2017

Revert "Transaction permissioning (#6441)"

8f96415

This reverts commit eed0e8b.

tomusdrw added a commit that referenced this pull request Sep 6, 2017

Revert "Revert "Transaction permissioning (#6441)""

abdb8c7

This reverts commit 8f96415.

arkpar added this to the 1.9 milestone Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction permissioning #6441

Transaction permissioning #6441

arkpar commented Sep 2, 2017 •

edited

Loading

NikVolf commented Sep 2, 2017

NikVolf Sep 2, 2017

NikVolf Sep 2, 2017

NikVolf Sep 2, 2017

NikVolf Sep 2, 2017

NikVolf Sep 2, 2017

rphmeier Sep 3, 2017

arkpar Sep 4, 2017

NikVolf Sep 2, 2017

arkpar Sep 4, 2017

rphmeier Sep 4, 2017

arkpar Sep 4, 2017

rphmeier Sep 5, 2017

rphmeier Sep 5, 2017 •

edited

Loading


		pub fn register_client(&self, client: Weak<BlockChainClient>) {
		*self.client.write() = Some(client);


		}

		pub fn transaction_allowed(&self, parent_hash: &H256, transaction: &SignedTransaction) -> bool {

Transaction permissioning #6441

Transaction permissioning #6441

Conversation

arkpar commented Sep 2, 2017 • edited Loading

NikVolf commented Sep 2, 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

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

rphmeier Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

arkpar commented Sep 2, 2017 •

edited

Loading

rphmeier Sep 5, 2017 •

edited

Loading