Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Private packets verification and queue refactoring #8715

Merged
merged 22 commits into from
Aug 29, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions ethcore/private-tx/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ ethjson = { path = "../../json" }
ethkey = { path = "../../ethkey" }
fetch = { path = "../../util/fetch" }
futures = "0.1"
heapsize = "0.4"
keccak-hash = { git = "https://github.com/paritytech/parity-common" }
log = "0.4"
parking_lot = "0.6"
Expand All @@ -35,6 +36,7 @@ serde = "1.0"
serde_derive = "1.0"
serde_json = "1.0"
tiny-keccak = "1.4"
transaction-pool = { path = "../../transaction-pool" }
url = "1"

[dev-dependencies]
Expand Down
2 changes: 1 addition & 1 deletion ethcore/private-tx/src/encryptor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ impl Encryptor for SecretStoreEncryptor {
let key = match self.retrieve_key("", false, contract_address, &*accounts) {
Ok(key) => Ok(key),
Err(Error(ErrorKind::EncryptionKeyNotFound(_), _)) => {
trace!("Key for account wasnt found in sstore. Creating. Address: {:?}", contract_address);
trace!(target: "privatetx", "Key for account wasnt found in sstore. Creating. Address: {:?}", contract_address);
self.retrieve_key(&format!("/{}", self.config.threshold), true, contract_address, &*accounts)
}
Err(err) => Err(err),
Expand Down
2 changes: 2 additions & 0 deletions ethcore/private-tx/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@ use ethcore::account_provider::SignError;
use ethcore::error::{Error as EthcoreError, ExecutionError};
use transaction::Error as TransactionError;
use ethkey::Error as KeyError;
use txpool::Error as TxPoolError;

error_chain! {
foreign_links {
Io(::std::io::Error) #[doc = "Error concerning the Rust standard library's IO subsystem."];
Decoder(DecoderError) #[doc = "RLP decoding error."];
Trie(TrieError) #[doc = "Error concerning TrieDBs."];
Txpool(TxPoolError) #[doc = "Tx pool error."];
}

errors {
Expand Down
338 changes: 173 additions & 165 deletions ethcore/private-tx/src/lib.rs

Large diffs are not rendered by default.

49 changes: 44 additions & 5 deletions ethcore/private-tx/src/messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,41 @@ use transaction::signature::{add_chain_replay_protection, check_replay_protectio
#[derive(Default, Debug, Clone, PartialEq, RlpEncodable, RlpDecodable, Eq)]
pub struct PrivateTransaction {
/// Encrypted data
pub encrypted: Bytes,
encrypted: Bytes,
/// Address of the contract
pub contract: Address,
contract: Address,
/// Hash
hash: H256,
}

impl PrivateTransaction {
/// Compute hash on private transaction
/// Constructor
pub fn new(encrypted: Bytes, contract: Address) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not really a good choice for an API. Both encrypted and contract are pub so you can easily change parameters while the hash will stay cached and will be just pure wrong. It's going to be really error prone.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually like the idea that hash is cached in this way. We just need to make sure that this structure is never mut. Unfortunately rust does not allow us to enforce immutability of struct instances.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @tomusdrw Hashing was incorrectly added into API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked

PrivateTransaction {
encrypted,
contract,
hash: 0.into(),
}.compute_hash()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please get rid of the compute_hash function and just compute hash inline

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO such optimization in order to calculate hash only once is justified. I've reworked it in order to provide access to the fields via getters

}

fn compute_hash(mut self) -> PrivateTransaction {
self.hash = keccak(&*self.rlp_bytes());
self
}

/// Hash of the private transaction
pub fn hash(&self) -> H256 {
keccak(&*self.rlp_bytes())
self.hash
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just pub hash instead of the function?

}

/// Address of the contract
pub fn contract(&self) -> Address {
self.contract
}

/// Encrypted data
pub fn encrypted(&self) -> Bytes {
self.encrypted.clone()
}
}

Expand All @@ -49,6 +75,8 @@ pub struct SignedPrivateTransaction {
r: U256,
/// The S field of the signature
s: U256,
/// Hash
hash: H256,
}

impl SignedPrivateTransaction {
Expand All @@ -59,7 +87,13 @@ impl SignedPrivateTransaction {
r: sig.r().into(),
s: sig.s().into(),
v: add_chain_replay_protection(sig.v() as u64, chain_id),
}
hash: 0.into(),
}.compute_hash()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please get rid of the compute_hash function and just compute hash inline

}

fn compute_hash(mut self) -> SignedPrivateTransaction {
self.hash = keccak(&*self.rlp_bytes());
self
}

pub fn standard_v(&self) -> u8 { check_replay_protection(self.v) }
Expand All @@ -73,4 +107,9 @@ impl SignedPrivateTransaction {
pub fn private_transaction_hash(&self) -> H256 {
self.private_transaction_hash
}

/// Own hash
pub fn hash(&self) -> H256 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe return &H256 and ditch transaction_hash from VerifiedPrivateTransaction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand this comment :-(

self.hash
}
}
Loading