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

Various improvements to tracing & diagnostics. #1707

Merged
merged 11 commits into from
Jul 26, 2016
4 changes: 2 additions & 2 deletions ethcore/src/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ impl Account {
nonce: pod.nonce,
storage_root: SHA3_NULL_RLP,
storage_overlay: RefCell::new(pod.storage.into_iter().map(|(k, v)| (k, (Filth::Dirty, v))).collect()),
code_hash: Some(pod.code.sha3()),
code_cache: pod.code,
code_hash: Some(pod.code.as_ref().map_or_else(|| { warn!("POD account with unknown code is being created! Assuming no code."); SHA3_EMPTY.clone() }, |c| c.sha3())),
code_cache: pod.code.as_ref().map_or_else(|| { warn!("POD account with unknown code is being created! Assuming no code."); vec![] }, |c| c.clone()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It will always display both warnings, maybe worth extracting the warnings somewhere above?

filth: Filth::Dirty,
}
}
Expand Down
44 changes: 24 additions & 20 deletions ethcore/src/pod_account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ pub struct PodAccount {
pub balance: U256,
/// The nonce of the account.
pub nonce: U256,
/// The code of the account.
pub code: Bytes,
/// The code of the account or `None` in the special case that it is unknown.
pub code: Option<Bytes>,
/// The storage of the account.
pub storage: BTreeMap<H256, H256>,
}
Expand All @@ -38,7 +38,7 @@ impl PodAccount {
/// Construct new object.
#[cfg(test)]
pub fn new(balance: U256, nonce: U256, code: Bytes, storage: BTreeMap<H256, H256>) -> PodAccount {
PodAccount { balance: balance, nonce: nonce, code: code, storage: storage }
PodAccount { balance: balance, nonce: nonce, code: Some(code), storage: storage }
}

/// Convert Account to a PodAccount.
Expand All @@ -48,7 +48,7 @@ impl PodAccount {
balance: *acc.balance(),
nonce: *acc.nonce(),
storage: acc.storage_overlay().iter().fold(BTreeMap::new(), |mut m, (k, &(_, ref v))| {m.insert(k.clone(), v.clone()); m}),
code: acc.code().unwrap().to_vec(),
code: acc.code().map(|x| x.to_vec()),
}
}

Expand All @@ -58,14 +58,15 @@ impl PodAccount {
stream.append(&self.nonce);
stream.append(&self.balance);
stream.append(&sec_trie_root(self.storage.iter().map(|(k, v)| (k.to_vec(), encode(&U256::from(v.as_slice())).to_vec())).collect()));
stream.append(&self.code.sha3());
stream.append(&self.code.as_ref().unwrap_or(&vec![]).sha3());
Copy link
Contributor

@rphmeier rphmeier Jul 26, 2016

Choose a reason for hiding this comment

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

&[] also works

edit: or should, but the Encodable interface is a little off in what types it accepts...

stream.out()
}

/// Place additional data into given hash DB.
pub fn insert_additional(&self, db: &mut AccountDBMut) {
if !self.code.is_empty() {
db.insert(&self.code);
match &self.code {
&Some(ref c) if !c.is_empty() => { db.insert(c); }
Copy link
Collaborator

@tomusdrw tomusdrw Jul 25, 2016

Choose a reason for hiding this comment

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

match self.code { 
   Some(ref c) ...
}

should work.

_ => {}
}
let mut r = H256::new();
let mut t = SecTrieDBMut::new(db, &mut r);
Expand All @@ -80,7 +81,7 @@ impl From<ethjson::blockchain::Account> for PodAccount {
PodAccount {
balance: a.balance.into(),
nonce: a.nonce.into(),
code: a.code.into(),
code: Some(a.code.into()),
storage: a.storage.into_iter().map(|(key, value)| {
let key: U256 = key.into();
let value: U256 = value.into();
Expand All @@ -95,15 +96,15 @@ impl From<ethjson::spec::Account> for PodAccount {
PodAccount {
balance: a.balance.map_or_else(U256::zero, Into::into),
nonce: a.nonce.map_or_else(U256::zero, Into::into),
code: vec![],
code: Some(vec![]),
storage: BTreeMap::new()
}
}
}

impl fmt::Display for PodAccount {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "(bal={}; nonce={}; code={} bytes, #{}; storage={} items)", self.balance, self.nonce, self.code.len(), self.code.sha3(), self.storage.len())
write!(f, "(bal={}; nonce={}; code={} bytes, #{}; storage={} items)", self.balance, self.nonce, self.code.as_ref().map_or(0, |c| c.len()), self.code.as_ref().map_or(H256::new(), |c| c.sha3()), self.storage.len())
Copy link
Collaborator

@tomusdrw tomusdrw Jul 25, 2016

Choose a reason for hiding this comment

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

map_or_else(H256::new, ..

}
}

Expand All @@ -114,13 +115,13 @@ pub fn diff_pod(pre: Option<&PodAccount>, post: Option<&PodAccount>) -> Option<A
(None, Some(x)) => Some(AccountDiff {
balance: Diff::Born(x.balance),
nonce: Diff::Born(x.nonce),
code: Diff::Born(x.code.clone()),
code: Diff::Born(x.code.as_ref().expect("account is newly created; newly created accounts must be given code; all caches should remain in place; qed").clone()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

x.code.cloned().expec(...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Option doesn't have cloned

storage: x.storage.iter().map(|(k, v)| (k.clone(), Diff::Born(v.clone()))).collect(),
}),
(Some(x), None) => Some(AccountDiff {
balance: Diff::Died(x.balance),
nonce: Diff::Died(x.nonce),
code: Diff::Died(x.code.clone()),
code: Diff::Died(x.code.as_ref().expect("account is deleted; only way to delete account is running SUICIDE; account must have had own code cached to make operation; all caches should remain in place; qed").clone()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

x.code.cloned()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Option doesn't have cloned

storage: x.storage.iter().map(|(k, v)| (k.clone(), Diff::Died(v.clone()))).collect(),
}),
(Some(pre), Some(post)) => {
Expand All @@ -130,7 +131,10 @@ pub fn diff_pod(pre: Option<&PodAccount>, post: Option<&PodAccount>) -> Option<A
let r = AccountDiff {
balance: Diff::new(pre.balance, post.balance),
nonce: Diff::new(pre.nonce, post.nonce),
code: Diff::new(pre.code.clone(), post.code.clone()),
code: match (pre.code.clone(), post.code.clone()) {
(Some(pre_code), Some(post_code)) => Diff::new(pre_code, post_code),
Copy link
Collaborator

@tomusdrw tomusdrw Jul 25, 2016

Choose a reason for hiding this comment

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

match (pre.code, post.code) {
  (Some(ref pre_code), Some(ref post_code)) => Diff::new(pre_code.clone(), post_code.clone()),
  _ => Diff::Same,
}

should work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope - pre is a reference, that match attempts to move code out of it.

_ => Diff::Same,
},
storage: storage.into_iter().map(|k|
(k.clone(), Diff::new(
pre.storage.get(&k).cloned().unwrap_or_else(H256::new),
Expand All @@ -156,7 +160,7 @@ mod test {

#[test]
fn existence() {
let a = PodAccount{balance: 69.into(), nonce: 0.into(), code: vec![], storage: map![]};
let a = PodAccount{balance: 69.into(), nonce: 0.into(), code: Some(vec![]), storage: map![]};
assert_eq!(diff_pod(Some(&a), Some(&a)), None);
assert_eq!(diff_pod(None, Some(&a)), Some(AccountDiff{
balance: Diff::Born(69.into()),
Expand All @@ -168,8 +172,8 @@ mod test {

#[test]
fn basic() {
let a = PodAccount{balance: 69.into(), nonce: 0.into(), code: vec![], storage: map![]};
let b = PodAccount{balance: 42.into(), nonce: 1.into(), code: vec![], storage: map![]};
let a = PodAccount{balance: 69.into(), nonce: 0.into(), code: Some(vec![]), storage: map![]};
let b = PodAccount{balance: 42.into(), nonce: 1.into(), code: Some(vec![]), storage: map![]};
assert_eq!(diff_pod(Some(&a), Some(&b)), Some(AccountDiff {
balance: Diff::Changed(69.into(), 42.into()),
nonce: Diff::Changed(0.into(), 1.into()),
Expand All @@ -180,8 +184,8 @@ mod test {

#[test]
fn code() {
let a = PodAccount{balance: 0.into(), nonce: 0.into(), code: vec![], storage: map![]};
let b = PodAccount{balance: 0.into(), nonce: 1.into(), code: vec![0], storage: map![]};
let a = PodAccount{balance: 0.into(), nonce: 0.into(), code: Some(vec![]), storage: map![]};
let b = PodAccount{balance: 0.into(), nonce: 1.into(), code: Some(vec![0]), storage: map![]};
assert_eq!(diff_pod(Some(&a), Some(&b)), Some(AccountDiff {
balance: Diff::Same,
nonce: Diff::Changed(0.into(), 1.into()),
Expand All @@ -195,13 +199,13 @@ mod test {
let a = PodAccount {
balance: 0.into(),
nonce: 0.into(),
code: vec![],
code: Some(vec![]),
storage: map_into![1 => 1, 2 => 2, 3 => 3, 4 => 4, 5 => 0, 6 => 0, 7 => 0]
};
let b = PodAccount {
balance: 0.into(),
nonce: 0.into(),
code: vec![],
code: Some(vec![]),
storage: map_into![1 => 1, 2 => 3, 3 => 0, 5 => 0, 7 => 7, 8 => 0, 9 => 9]
};
assert_eq!(diff_pod(Some(&a), Some(&b)), Some(AccountDiff {
Expand Down
37 changes: 37 additions & 0 deletions rpc/src/v1/impls/traces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use std::sync::{Weak, Arc};
use jsonrpc_core::*;
use std::collections::BTreeMap;
//use util::H256;
use util::rlp::{UntrustedRlp, View};
use ethcore::client::{BlockChainClient, CallAnalytics, TransactionID, TraceId};
use ethcore::miner::MinerService;
use ethcore::transaction::{Transaction as EthTransaction, SignedTransaction, Action};
Expand Down Expand Up @@ -144,4 +145,40 @@ impl<C, M> Traces for TracesClient<C, M> where C: BlockChainClient + 'static, M:
Ok(Value::Null)
})
}

fn send_raw_transaction(&self, params: Params) -> Result<Value, Error> {
try!(self.active());
trace!(target: "jsonrpc", "call: {:?}", params);
from_params::<(Bytes, _)>(params)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Putting Vec<String> here should work too, no?

.and_then(|(raw_transaction, flags)| {
let raw_transaction = raw_transaction.to_vec();
let flags: Vec<String> = flags;
let analytics = CallAnalytics {
transaction_tracing: flags.contains(&("trace".to_owned())),
vm_tracing: flags.contains(&("vmTrace".to_owned())),
state_diffing: flags.contains(&("stateDiff".to_owned())),
};
match UntrustedRlp::new(&raw_transaction).as_val() {
Ok(signed) => {
let r = take_weak!(self.client).call(&signed, analytics);
if let Ok(executed) = r {
// TODO maybe add other stuff to this?
let mut ret = map!["output".to_owned() => to_value(&Bytes(executed.output)).unwrap()];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a serializable struct instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if this code were to be refactored, it should be done in concert with prior trace function, and therefore is outside of this PR's scope.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Tomek here. It should be a serializable struct. Imo it's a scope of this PR.

if let Some(trace) = executed.trace {
ret.insert("trace".to_owned(), to_value(&Trace::from(trace)).unwrap());
}
if let Some(vm_trace) = executed.vm_trace {
ret.insert("vmTrace".to_owned(), to_value(&VMTrace::from(vm_trace)).unwrap());
}
if let Some(state_diff) = executed.state_diff {
ret.insert("stateDiff".to_owned(), to_value(&StateDiff::from(state_diff)).unwrap());
}
return Ok(Value::Object(ret))
}
Ok(Value::Null)
}
Err(_) => Err(Error::invalid_params()),
}
})
}
}
4 changes: 4 additions & 0 deletions rpc/src/v1/traits/traces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ pub trait Traces: Sized + Send + Sync + 'static {
/// Executes the given call and returns a number of possible traces for it.
fn call(&self, _: Params) -> Result<Value, Error>;

/// Executes the given raw transaction and returns a number of possible traces for it.
fn send_raw_transaction(&self, _: Params) -> Result<Value, Error>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It does not actually send the transaction, does it? Perhaps trace_raw_transaction would be a better name


/// Should be used to convert object to io delegate.
fn to_delegate(self) -> IoDelegate<Self> {
let mut delegate = IoDelegate::new(Arc::new(self));
Expand All @@ -43,6 +46,7 @@ pub trait Traces: Sized + Send + Sync + 'static {
delegate.add_method("trace_transaction", Traces::transaction_traces);
delegate.add_method("trace_block", Traces::block_traces);
delegate.add_method("trace_call", Traces::call);
delegate.add_method("trace_sendRawTransaction", Traces::send_raw_transaction);
Copy link
Collaborator

@arkpar arkpar Jul 26, 2016

Choose a reason for hiding this comment

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

name it trace_callRaw to be consistent with trace_call ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a transaction != a call.

the naming follows conventions of the equivalent eth_ methods; i.e. eth_call -> trace_call; eth_sendRawTransaction -> trace_sendRawTransaction.

that said, i'm quite ambivalent; trace_rawTransaction might well be sensible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trace_transaction and trace_rawTransaction would have been nice but trace_transaction is already used (and irritatingly, not even for a function of the corresponding name: Traces::transaction_traces)


delegate
}
Expand Down
5 changes: 5 additions & 0 deletions rpc/src/v1/types/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

use util::rlp::encode;
use ethcore::contract_address;
use ethcore::transaction::{LocalizedTransaction, Action, SignedTransaction};
use v1::types::{Bytes, H160, H256, U256};
Expand Down Expand Up @@ -49,6 +50,8 @@ pub struct Transaction {
pub input: Bytes,
/// Creates contract
pub creates: Option<H160>,
/// Raw transaction data
pub raw: Bytes,
}

impl From<LocalizedTransaction> for Transaction {
Expand All @@ -72,6 +75,7 @@ impl From<LocalizedTransaction> for Transaction {
Action::Create => Some(contract_address(&t.sender().unwrap(), &t.nonce).into()),
Action::Call(_) => None,
},
raw: encode(&t.signed).to_vec().into(),
}
}
}
Expand All @@ -97,6 +101,7 @@ impl From<SignedTransaction> for Transaction {
Action::Create => Some(contract_address(&t.sender().unwrap(), &t.nonce).into()),
Action::Call(_) => None,
},
raw: encode(&t).to_vec().into(),
}
}
}
Expand Down