Private packets verification and queue refactoring #8715

grbIzl · 2018-05-25T14:10:52Z

This PR addresses two major TODOs from private transactions PR

Verification of private transactions packets and prevention of packets resending (closes Add private transactions validation before propagating #8250 )
Refactoring of queue's implementation in order to switch to direct usage of new tx queue

grbIzl · 2018-05-25T14:16:23Z

ethcore/private-tx/src/private_transactions.rs

+#[derive(Debug)]
+pub struct PrivateScorying;
+
+impl txpool::Scoring<VerifiedPrivateTransaction> for PrivateScorying {


@tomusdrw I reworked my queue's implementation in order to use txpool directly for storing transaction in it. The main problem, that I'm facing, is scoring implementation. Actually I'd like to fully re-use miner's scoring, but I don't know, how to do it, because it's bound with miner's VerifiedTransaction trait.

So as result I had to copy-past scoring code from miner.

Let's make the two (Scoring and Ready) generic then.

Tbaut · 2018-05-28T08:45:17Z

@grbIzl we should update the documentation for that one right?

grbIzl · 2018-05-28T23:41:50Z

@Tbaut i don't think so. It's mostly the internal improvements and refactorings

tomusdrw

Couple of grumbles and suggestions.

tomusdrw · 2018-06-04T12:16:21Z

ethcore/private-tx/src/error.rs

@@ -167,6 +168,12 @@ error_chain! {
 			description("General ethcore error."),
 			display("General ethcore error {}", err),
 		}
+
+		#[doc = "Tx pool error."]
+		Txpool(err: TxPoolError) {


Should be just foreign_link (and From implementation wouldn't be needed in such case)

tomusdrw · 2018-06-22T12:09:25Z

ethcore/private-tx/src/lib.rs

 		}
 		Ok(())
 	}

+	/// Add signed private transaction into the store	/// Creates corresponding public transaction if last required signature collected and sends it to the chain


Split into two lines

tomusdrw · 2018-06-22T12:10:10Z

ethcore/private-tx/src/lib.rs

 					}
-					let account = desc.validator_account;
-					if let Action::Call(contract) = transaction.signed().action {
+					let signed_tx = transaction.transaction.clone();


Do we really need to clone the entire transaction? It seems that we only need a reference in :283

tomusdrw · 2018-06-22T12:17:59Z

ethcore/private-tx/src/lib.rs

+				None => {
+					trace!("Propagating transaction further");
+					self.broadcast_private_transaction(private_hash, transaction.private_transaction.rlp_bytes().into_vec());
+					return Ok(())


I suspect that error handling in this block might be incorrect:

We drain all transactions for verification

But we stop at first incorrect transaction

That means that every other transaction will just be ignored. Most likely we should run all the transactions and only return error after that (or even return a Vec<Result<>).

Please review all return, bail! and ? statements in that loop.

tomusdrw · 2018-06-22T12:21:59Z

ethcore/private-tx/src/lib.rs

+		let mut signed_transactions = self.imported_signed_transactions.lock();
+		for tx in signed_transactions.drain(..) {
+			let private_hash = tx.private_transaction_hash();
+			let desc = match self.transactions_for_signing.lock().get(&private_hash) {


We acquire a second lock here (after acquiring imported_signed_transactions).

Couple of issues with this:

Seems that RwLock would do (you sometimes only need read access, not write)

Multiple, dependent locks are always error-prone, is it really necessary to have the two? Can't we put it into separate structs?

Calling private functions self.* after acquiring a lock is always super error-prone, we have to be sure that the lock is not re-acquired in that private function.

Last, but not least: the locking happens in opposite order than the declaration - to avoid deadlocks we should always acquire locks in the same order as the fields are declared in the struct.

tomusdrw · 2018-06-22T13:48:00Z

ethcore/private-tx/src/private_transactions.rs

+		// Use pool's verifying pipeline for original transaction's verification
+		let verifier = pool::verifier::Verifier::new(client, options, Default::default());
+		let _verified_tx = verifier.verify_transaction(pool::verifier::Transaction::Unverified(transaction.clone()))?;
+		let signed_tx = SignedTransaction::new(transaction)?;


Should take SignedTransaction from _verified_tx, otherwise you are recovering the signature twice

tomusdrw · 2018-06-22T13:49:44Z

ethcore/private-tx/src/private_transactions.rs

+	pub fn drain<C: pool::client::NonceClient>(&self, client: C) -> Vec<Arc<VerifiedPrivateTransaction>> {
+		let ready = PrivateReadyState::new(client);
+		let mut hashes: Vec<H256> = Vec::new();
+		let res: Vec<Arc<VerifiedPrivateTransaction>> = self.verification_pool.read().pending(ready).collect();


let transactions: Vec<_> = self.verification_pool.read().pending(ready).collect() let mut pool = self.verification_pool.write(); for tx in &transactions { pool.remove(tx.hash(), true); } transactions

doesn't make sense to allocate the vec twice.

tomusdrw · 2018-06-22T13:56:40Z

ethcore/sync/src/chain/mod.rs

@@ -329,6 +329,10 @@ pub struct PeerInfo {
 	ask_time: Instant,
 	/// Holds a set of transactions recently sent to this peer to avoid spamming.
 	last_sent_transactions: HashSet<H256>,
+	/// Holds a set of private transactions recently sent to this peer to avoid spamming.
+	last_sent_private_transactions: HashSet<H256>,


Why do we need to separate the two? The collision is not possibly anyway, for simplicity let's have a single set.

tomusdrw · 2018-06-22T14:02:26Z

ethcore/sync/src/chain/mod.rs

@@ -1083,6 +1117,9 @@ impl ChainSync {
 				peer_info.last_sent_transactions.clear()
 			);
 		}
+
+		// reset stats for private transaction packets
+		self.clear_private_stats();


So it seems that we always re-broadcast after the the block, right? Why can't we use the same logic as for regular transactions?

Also this statements should be guarded by similar ifs as the above code - we don't need to clear it when we import uncle block or we are syncing.

tomusdrw · 2018-06-22T14:04:17Z

ethcore/sync/src/chain/mod.rs

@@ -1121,13 +1158,13 @@ impl ChainSync {
 	}

 	/// Broadcast private transaction message to peers.
-	pub fn propagate_private_transaction(&mut self, io: &mut SyncIo, packet: Bytes) {
-		SyncPropagator::propagate_private_transaction(self, io, packet);
+	pub fn propagate_private_transaction(&mut self, io: &mut SyncIo, transaction_hash: H256, packet: Bytes) {


Might be worth merging the two methods (and the methods they are calling into one) and just discriminate using enum to avoid duplication.

5chdn · 2018-07-02T12:17:43Z

Marking as stale @grbIzl. 🐥

5chdn · 2018-07-09T06:48:42Z

Will reopen once you continue working on it. 👍

niklasad1 · 2018-08-22T12:59:42Z

ethcore/private-tx/src/lib.rs

 		}
 		Ok(())
 	}

+	/// Add signed private transaction into the store
+	/// Creates corresponding public transaction if last required signature collected and sends it to the chain
+	pub fn process_signature(&self, signed_tx: SignedPrivateTransaction) -> Result<(), Error> {


take signed_tx by reference instead and get rid of a clone(): https://github.com/paritytech/parity-ethereum/blob/2778d4e99bb25c8e2df3f15617e57b16156fbc10/ethcore/private-tx/src/lib.rs#L678

niklasad1 · 2018-08-22T13:01:58Z

ethcore/private-tx/src/lib.rs

 				}
 			}
-			verification_queue.remove_private_transaction(&transaction_hash);
+			return Ok(());


needless return i.e, enough with Ok(())

niklasad1 · 2018-08-22T13:08:26Z

ethcore/private-tx/src/lib.rs

+					warn!("Failed to remove transaction from signing store, error: {:?}", err);
+					bail!(err);
+				}
+			}


Replace single-pattern match with:

if let Err(err) = self.transactions_for_signing.write().remove(&private_hash) { ..... }

niklasad1 · 2018-08-22T13:49:01Z

ethcore/private-tx/src/lib.rs

-	fn broadcast_private_transaction(&self, message: Bytes) {
-		self.notify(|notify| notify.broadcast(ChainMessageType::PrivateTransaction(message.clone())));
+	fn broadcast_private_transaction(&self, transaction_hash: H256, message: Bytes) {
+		self.notify(|notify| notify.broadcast(ChainMessageType::PrivateTransaction(transaction_hash, message.clone())));


It looks suspect to cloning the vector here instead of moving into the closure! Any particular reason why?

It's not possible to move captured outer variable (message)

Just a note: in the future we may consider to change type ChainMessageType to only take reference of [u8]. In that case this should solve this message clone issue.

niklasad1 · 2018-08-22T14:43:31Z

ethcore/private-tx/src/private_transactions.rs

 use error::{Error, ErrorKind};

+type Pool = txpool::Pool<VerifiedPrivateTransaction, pool::scoring::NonceAndGasPrice>;
+
 /// Maximum length for private transactions queues.
 const MAX_QUEUE_LEN: usize = 8312;

 /// Desriptor for private transaction stored in queue for verification


Typo in Descriptor

Rephrase this, something like Private transaction type!

niklasad1 · 2018-08-22T14:44:19Z

ethcore/private-tx/src/private_transactions.rs

 	/// Address that should be used for verification
-	pub validator_account: Address,
+	pub validator_account: Option<Address>,
+	/// Resulted verified


Verified result

niklasad1 · 2018-08-22T14:44:44Z

ethcore/private-tx/src/private_transactions.rs

+	pub validator_account: Option<Address>,
+	/// Resulted verified
+	pub transaction: SignedTransaction,
+	/// Original transaction's hash


Original transaction hash

niklasad1 · 2018-08-22T14:46:29Z

ethcore/private-tx/src/private_transactions.rs

+	pub transaction: SignedTransaction,
+	/// Original transaction's hash
+	pub transaction_hash: H256,
+	/// Original transaction's sender


Orginal transaction sender or Transaction origin address

niklasad1 · 2018-08-22T15:00:39Z

ethcore/private-tx/src/private_transactions.rs

+		let options = self.verification_options.clone();
+		// Use pool's verifying pipeline for original transaction's verification
+		let verifier = pool::verifier::Verifier::new(client, options, Default::default(), None);
+		let verified_tx = verifier.verify_transaction(pool::verifier::Transaction::Unverified(transaction.clone()))?;


Needless clone() call

You can either take the transaction (UnverifiedTransaction) by reference and clone it or take it by value and move it!

niklasad1 · 2018-08-22T15:01:11Z

ethcore/private-tx/src/private_transactions.rs

 		}
 	}
 }

 impl VerificationStore {
 	/// Adds private transaction for verification into the store
 	pub fn add_transaction<C: pool::client::Client>(
-		&mut self,
+		&self,
 		transaction: UnverifiedTransaction,


See comment below!

niklasad1 · 2018-08-22T15:27:13Z

ethcore/sync/src/private_tx.rs


 /// Trait which should be implemented by a private transaction handler.
 pub trait PrivateTxHandler: Send + Sync + 'static {
 	/// Function called on new private transaction received.
-	fn import_private_transaction(&self, rlp: &[u8]) -> Result<(), String>;
+	/// Returns hash of the imported transaction


the hash

niklasad1 · 2018-08-22T15:27:20Z

ethcore/sync/src/private_tx.rs


 	/// Function called on new signed private transaction received.
-	fn import_signed_private_transaction(&self, rlp: &[u8]) -> Result<(), String>;
+	/// Returns hash of the imported transaction


the hash

niklasad1 · 2018-08-22T15:34:59Z

miner/src/pool/scoring.rs

-				insertion_id: i as u64,
-				transaction: Arc::new(verified),
-			}
+		let txs = vec![tx1, tx2, tx3, tx4].into_iter().enumerate().map(|(_, tx)| {


remove enumerate() because it is not used!

niklasad1

Hey,

Looks overall good, I have left a few comments that can improve that code a bit!

I have not commented on the logging but ideally, all these logs should have a target i.e., trace!(target: "foo", "some very important message. Not required for this PR

But, this PR needs to rebased/merged to master before merging (that's why I used the Request changes option!

niklasad1

👍

sorpaas

LGTM!

sorpaas · 2018-08-29T12:19:30Z

ethcore/private-tx/src/lib.rs

-	fn broadcast_private_transaction(&self, message: Bytes) {
-		self.notify(|notify| notify.broadcast(ChainMessageType::PrivateTransaction(message.clone())));
+	fn broadcast_private_transaction(&self, transaction_hash: H256, message: Bytes) {
+		self.notify(|notify| notify.broadcast(ChainMessageType::PrivateTransaction(transaction_hash, message.clone())));


Just a note: in the future we may consider to change type ChainMessageType to only take reference of [u8]. In that case this should solve this message clone issue.

* master: evmbin: Fix gas_used issue in state root mismatch and handle output better (#9418) Update hardcoded sync (#9421) Add block reward contract config to ethash and allow off-chain contracts (#9312) Private packets verification and queue refactoring (#8715) Update tobalaba.json (#9419) docs: add parity ethereum logo to readme (#9415) build: update rocksdb crate (#9414) Updating the CI system (#8765) Better support for eth_getLogs in light mode (#9186) Add update docs script to CI (#9219) `gasleft` extern implemented for WASM runtime (kip-6) (#9357) block view! removal in progress (#9397) Prevent sync restart if import queue full (#9381) nonroot CentOS Docker image (#9280) ethcore: kovan: delay activation of strict score validation (#9406)

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

grbIzl commented May 25, 2018

View reviewed changes

5chdn added this to the 1.12 milestone May 31, 2018

grbIzl added 6 commits June 19, 2018 11:23

Verify private transaction before propagating

843b2ba

Private transactions queue reworked with tx pool queue direct usage

fbdf3de

Styling fixed

a4778e3

Prevent resending private packets to the sender

fc113ba

Process signed private transaction packets via io queue

e5feccd

Test fixed

6436745

grbIzl force-pushed the PrivatePacketsVerification branch from 63f4402 to 6436745 Compare June 19, 2018 10:05

Build and test fixed after merge

800aa19

grbIzl 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 20, 2018

grbIzl requested a review from tomusdrw June 20, 2018 09:49

tomusdrw reviewed Jun 22, 2018

View reviewed changes

5chdn added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 2, 2018

5chdn closed this Jul 9, 2018

grbIzl added 5 commits July 13, 2018 16:15

Comments after review fixed

55eef9e

Signed transaction taken from verified

c8898cd

Merge with head

12873cf

Fix after merge

59ddce1

Pool scoring generalized in order to use externally

7e15ebb

grbIzl reopened this Jul 18, 2018

Forgotten case reworked

2778d4e

5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Aug 20, 2018

niklasad1 reviewed Aug 22, 2018

View reviewed changes

niklasad1 suggested changes Aug 22, 2018

View reviewed changes

5chdn modified the milestones: 2.0 Ethereum, 2.1 Aug 23, 2018

grbIzl added 4 commits August 24, 2018 15:53

Review comments fixed

af28236

Logging reworked, target added

f6338eb

Merge with head

62131e0

Fix after merge

064d814

niklasad1 approved these changes Aug 28, 2018

View reviewed changes

niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 28, 2018

sorpaas approved these changes Aug 29, 2018

View reviewed changes

sorpaas merged commit 1073d56 into master Aug 29, 2018

sorpaas deleted the PrivatePacketsVerification branch August 29, 2018 12:31

Private packets verification and queue refactoring #8715

Private packets verification and queue refactoring #8715

Conversation

grbIzl commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tbaut commented May 28, 2018

grbIzl commented May 28, 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

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

5chdn commented Jul 2, 2018

5chdn commented Jul 9, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

sorpaas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 left a comment •

edited

Loading