Private transactions integration pr #6422

grbIzl · 2017-08-30T15:47:18Z

This pr is used for integrating all the changes in terms of building private transactions system.
We tried to review changes made on every step, but didn't request the merge into the main brunch till the functionality was ready and tested
So it's ready now. The documentation with detailed description of the solution is available on wiki: https://paritytech.github.io/wiki/Private-Transactions.html

rphmeier · 2017-08-30T16:04:22Z

ethcore/src/private_transactions/private_transactions.rs

+	pub fn import(&self, transaction: UnverifiedTransaction, _peer_id: usize) -> Result<(), Error> {
+		{
+			let mut transactions = self.transactions.write();
+			transactions.push(transaction.clone());


why clone? also seems strange to put this in its own block.

rphmeier · 2017-08-30T16:04:53Z

ethcore/src/private_transactions/private_transactions.rs

+
+	pub fn get_list(&self) -> Vec<UnverifiedTransaction> {
+		let transactions = self.transactions.read();
+		transactions.clone()


self.transactions().read().clone()

rphmeier · 2017-08-30T16:05:00Z

ethcore/src/service.rs

-	NewMessage(Bytes)
+	NewConsensusMessage(Bytes),
+	/// New private transaction received
+	NewPrivateTransaction(Bytes, usize)


missing trailing comma. what's the usize param?

rphmeier · 2017-08-30T16:06:08Z

sync/src/chain.rs


-pub const SNAPSHOT_SYNC_PACKET_COUNT: u8 = 0x16;
+pub const SNAPSHOT_SYNC_PACKET_COUNT: u8 = 0x17;


that requires a protocol upgrade, doesn't it? @arkpar

rphmeier · 2017-08-30T16:07:52Z

How are private transactions differentiated from regular transactions if they both serialize to the UnverifiedTransaction type? I don't really see why this requires custom messages. It will also need light client protocol support as well.

rphmeier · 2017-09-18T14:33:31Z

ethcore/src/client/chain_notify.rs

@@ -18,6 +18,12 @@ use ipc::IpcConfig;
 use bigint::hash::H256;
 use bytes::Bytes;

+pub enum ChainMessageType {


missing documentation

rphmeier · 2017-09-18T14:34:11Z

ethcore/src/client/client.rs

@@ -1215,6 +1218,10 @@ impl BlockChainClient for Client {
 		Ok(results)
 	}

+	fn get_private_transactions_provider(&self) -> Arc<PrivateTransactionsProvider> {


style: generally we don't prefix getters with get_

rphmeier · 2017-09-18T14:35:31Z

ethcore/src/private_transactions/private_transactions.rs

+/// Storage for private transactions
+pub struct PrivateTransactions {
+	transactions: RwLock<Vec<UnverifiedTransaction>>,
+	signed_transactions:  RwLock<Vec<UnverifiedTransaction>>,


I'm a little confused why it needs locks here. Isn't it protected by a mutex within the Provider?

rphmeier · 2017-09-18T14:35:39Z

ethcore/src/private_transactions/private_transactions.rs

+}
+
+impl PrivateTransactions {
+	pub fn new() -> Self {


documentation on methods

rphmeier · 2017-09-18T14:38:21Z

sync/src/tests/private_transactions.rs

+use SyncConfig;
+
+#[test]
+pub fn send_private_transaction() {


i don't think test functions need to be public

rphmeier · 2017-09-18T14:41:06Z

ethcore/src/private_transactions/mod.rs

+	/// Add private transaction into the store
+	pub fn import_private_transaction(&self, rlp: &[u8], peer_id: usize) -> Result<(), EthcoreError> {
+		let tx: UnverifiedTransaction = UntrustedRlp::new(rlp).as_val()?;
+		// TODO: notify engines about private transactions


tomusdrw

Some grumbles addressed in #8249

tomusdrw · 2018-03-28T11:06:46Z

ethcore/private-tx/src/lib.rs

+use ethcore::trace::{Tracer, VMTracer};
+use rustc_hex::FromHex;
+
+// Source avaiable at https://github.com/paritytech/contracts/blob/master/contracts/PrivateContract.sol


Outdated, right?

Replaced with the actual link

tomusdrw · 2018-03-28T11:30:50Z

ethcore/private-tx/src/lib.rs

+		match validation_account {
+			None => {
+				// Not for verification, broadcast further to peers
+				self.broadcast_private_transaction(rlp.into());


I don't think it makes much sense. How is it supposed to work if we are blidnly forwarding all transactions?
If you send a bunch of incorrect private transactions they will be forever relayed by peers but never included.

Good point. Some basic validation should be introduced. Is it okay to do that in the separate PR?

Added an issue for that #8250

tomusdrw · 2018-03-28T11:42:05Z

ethcore/private-tx/src/messages.rs

+			private_transaction_hash: private_transaction_hash,
+			r: sig.r().into(),
+			s: sig.s().into(),
+			v: sig.v() as u64 + if let Some(n) = chain_id { 35 + n * 2 } else { 27 },


Seems duplicated with ethcore-transaction? Could be re-used?

Not sure. Only with some refactoring of Transaction struct (like creating of TransactionSignature trait and so on). Didn't want to touch so vital code for that.

I specifically meant the chain-id encoding within v I think the rest is fine.

niklasad1 · 2018-03-29T15:23:12Z

ethcore/transaction/src/transaction.rs

+	/// 0 if `v` would have been 27 under "Electrum" notation, 1 if 28 or 4 if invalid.
+	pub fn check_replay_protection(v: u64) -> u8 {
+		match v {
+			v if v == 27 => 0 as u8,


27 => 0 as u8

niklasad1 · 2018-03-29T15:23:35Z

ethcore/transaction/src/transaction.rs

+	pub fn check_replay_protection(v: u64) -> u8 {
+		match v {
+			v if v == 27 => 0 as u8,
+			v if v == 28 => 1 as u8,


28 => 1 as u8

* Tiny fixes part 1. * A bunch of additional comments and todos. * Fix ethsync tests.

…rity into PrivateTransactionsNetwork

…nsactionsNetwork

* added cli option that enables private transactions * fixed failing test * fixed failing test

debris · 2018-04-09T13:00:36Z

to simplify merge with master, I removed I made ethcore depend on kvdb_rocksb again... cc #8320

grbIzl added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 30, 2017

grbIzl requested a review from arkpar August 30, 2017 15:47

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

rphmeier reviewed Aug 30, 2017

View reviewed changes

grbIzl force-pushed the PrivateTransactionsNetwork branch from b4ed35c to cc7c5c2 Compare September 15, 2017 12:57

rphmeier reviewed Sep 18, 2017

View reviewed changes

grbIzl and others added 8 commits September 25, 2017 15:30

Private transaction message added

b7fa847

Empty line removed

2c6973b

Private transactions logic removed from client into the separate module

e7936cd

Fixed compilation after merge with head

79c22d0

Signed private transaction message added as well

7d33510

Comments after the review fixed

ff19e8e

Private tx execution

1011053

Test update

5c44a93

5chdn added this to the 1.9 milestone Oct 11, 2017

5chdn added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Oct 11, 2017

arkpar added 2 commits October 26, 2017 16:24

Renamed some methods

7301916

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

52a6ee1

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 Mar 28, 2018

tomusdrw reviewed Mar 28, 2018

View reviewed changes

grbIzl added 2 commits March 28, 2018 15:28

Link to the contract changed

6e01991

Transaction signature chain replay protection module created

058a191

niklasad1 reviewed Mar 29, 2018

View reviewed changes

grbIzl added 2 commits March 29, 2018 18:12

Redundant type conversion removed

3487b8f

Contract address returned by private provider

752ac4f

grbIzl mentioned this pull request Apr 3, 2018

Addressing grumbles in PrivateTransactions #8249

Merged

grbIzl and others added 9 commits April 3, 2018 13:51

Test fixed

268688e

Addressing grumbles in PrivateTransactions (#8249)

5b7205a

* Tiny fixes part 1. * A bunch of additional comments and todos. * Fix ethsync tests.

Merge branch 'PrivateTransactionsNetwork' of github.com:paritytech/pa…

c4e5f58

…rity into PrivateTransactionsNetwork

Merge branch 'master' into PrivateTransactionsNetwork

36cf804

Merge branch 'master' into PrivateTransactionsNetwork

fe60ef7

Merge branch 'master' into PrivateTransactionsNetwork

372e451

Merge branch 'master' of github.com:paritytech/parity into PrivateTra…

c0a234a

…nsactionsNetwork

resolved merge conflicts

704ad64

final private tx pr (#8318)

dd36b22

* added cli option that enables private transactions * fixed failing test * fixed failing test

debris approved these changes Apr 9, 2018

View reviewed changes

5chdn added the P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible. label Apr 9, 2018

Merge branch 'master' into PrivateTransactionsNetwork

8c583d5

fixed failing test

1e43be6

debris mentioned this pull request Apr 9, 2018

Use hyper 0.11 in ethcore-miner and improvements in parity-reactor #8335

Merged

fixed failing test

1e29498

debris merged commit e6f75bc into master Apr 9, 2018

debris deleted the PrivateTransactionsNetwork branch April 9, 2018 14:15

5chdn added A9-buythatmanabeer 🍻 Pull request is reviewed well and worth buying the author a beer. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Private transactions integration pr #6422

Private transactions integration pr #6422

grbIzl commented Aug 30, 2017 •

edited

Loading

rphmeier Aug 30, 2017 •

edited

Loading

rphmeier Aug 30, 2017

rphmeier Aug 30, 2017 •

edited

Loading

rphmeier Aug 30, 2017

rphmeier commented Aug 30, 2017 •

edited

Loading

rphmeier Sep 18, 2017

rphmeier Sep 18, 2017

rphmeier Sep 18, 2017

rphmeier Sep 18, 2017

rphmeier Sep 18, 2017

rphmeier Sep 18, 2017

tomusdrw left a comment

tomusdrw Mar 28, 2018

grbIzl Mar 28, 2018

tomusdrw Mar 28, 2018

grbIzl Mar 28, 2018

grbIzl Mar 28, 2018

tomusdrw Mar 28, 2018

grbIzl Mar 28, 2018

tomusdrw Mar 28, 2018

niklasad1 Mar 29, 2018

grbIzl Mar 29, 2018

niklasad1 Mar 29, 2018

grbIzl Mar 29, 2018

debris commented Apr 9, 2018


		pub const SNAPSHOT_SYNC_PACKET_COUNT: u8 = 0x16;
		pub const SNAPSHOT_SYNC_PACKET_COUNT: u8 = 0x17;

Private transactions integration pr #6422

Private transactions integration pr #6422

Conversation

grbIzl commented Aug 30, 2017 • edited Loading

rphmeier Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier commented Aug 30, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debris commented Apr 9, 2018

grbIzl commented Aug 30, 2017 •

edited

Loading

rphmeier Aug 30, 2017 •

edited

Loading

rphmeier Aug 30, 2017 •

edited

Loading

rphmeier commented Aug 30, 2017 •

edited

Loading