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

Core tracedb functionality. #996

Merged
merged 37 commits into from Apr 30, 2016
Merged

Core tracedb functionality. #996

merged 37 commits into from Apr 30, 2016

Conversation

debris
Copy link
Collaborator

@debris debris commented Apr 24, 2016

separated core tracedb functionality from pr #924.

99% of this pr is a new code. Removed lines are mostly variables/functions renaming. Most of the new code is added to ethcore/trace module. Changes in this pr won't store traces in the database, but they provide everything that is required to do so.

this pr includes:

  • fixed encoding of 0u8 (fixed encoding 0u8 #992 ,merged)
  • import route contains ommited blocks (import route contains ommited blocks #994 ,merged)
  • trace::ExecutiveTracer and trace::NoopTracer moved to separate files
  • traces rlp encoding / decoding
  • traces bloom
  • traces filter
  • tracedb
  • tracedb config
  • removed have_tracing from blockchain
  • blockchain bloom filtering is done using external crate bloomchain
  • turned on tracing of transactions to builtins. It simplifies querying traces, cause number of traces is always equal to number of transactions in given block.

this pr does not include:

  • client api for tracing
  • actual storing traces in database
  • tracing cli
  • traces rpc

@debris debris added the A0-pleasereview 🤓 Pull request needs code review. label Apr 24, 2016
@debris debris mentioned this pull request Apr 24, 2016
pub fn new(mut config: Config, path: &Path, extras: Arc<T>) -> Self {
let mut tracedb_path = path.to_path_buf();
tracedb_path.push("tracedb");
let tracesdb = Database::open_default(tracedb_path.to_str().unwrap()).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will path be automatically created? (path and tracedb_path)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

Copy link
Collaborator

Choose a reason for hiding this comment

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

what about versioning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the idea behind having a separate DB? are we going to start having loads of different DBs now? VM tracing, and fat db (i.e. hash preimages) are both independent additions. is there a storage/memory/compute/file-handle cost to loads of open databases?

@tomusdrw
Copy link
Collaborator

Had a quick look, but I need more time to dig into the logic. Would be cool for some more experienced guys to look into general design / logic.

@@ -245,30 +246,51 @@ impl<'a> Executive<'a> {
}
trace!("Executive::call(params={:?}) self.env_info={:?}", params, self.info);

let delegate_call = params.code_address != params.address;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this correct? What if contract A calls B which calls back A?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh... you are right. But this edge case is not trivial to fix. With our current code design Executive does not know if A callcode B which callcode A, because it's irrelevant to A state (the result of this operation is the same as A calling itself).

I suggest leaving this as it is and fixing later, cause it may require additional changes in evm (exposing extra data).

Copy link
Contributor

Choose a reason for hiding this comment

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

delegate calls are not traced further - if A delegatecalls B delegatecalls A, then, correctly, nothing will be traced.

@debris debris 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 Apr 29, 2016
@debris
Copy link
Collaborator Author

debris commented Apr 29, 2016

changes in last few commits

  • fixed typos, misspellings
  • trace calls to builtins only when depth == 0
  • Renamed Tracedb to TraceDB, TracedbIndex to TraceDBIndex.
  • Proves in tracers assertions.
  • fixed switch turn_to logic.
  • vector-addressing scheme for Traces

@@ -271,6 +268,8 @@ mod tests {
input: vec![0x5],
}),
result: Res::FailedCall,
trace_address: vec![0],
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct? or should it be vec![]?

@gavofyork
Copy link
Contributor

first 0 of trace address looks redundant.

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 29, 2016
@debris
Copy link
Collaborator Author

debris commented Apr 30, 2016

you are right! 👍

@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 30, 2016
@gavofyork gavofyork merged commit 66477a9 into master Apr 30, 2016
@gavofyork gavofyork deleted the tracedb branch April 30, 2016 15:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants