cmd, core, eth/tracers: support fancier js tracing#15516
Conversation
There was a problem hiding this comment.
Could we have block number aswell? Relevant since blocknumber determines the 'ruleset' with regards to forks
There was a problem hiding this comment.
It's in there in a crude way https://github.com/karalabe/go-ethereum/blob/d2540d978e9e93b926d6b2183c5e1dac5a62f1ba/eth/tracers/tracer.go#L517.
We can rework it in a followup PR that pulls in all the execution context (parent block, current block, chain config).
There was a problem hiding this comment.
This is maybe correct, using the same check for evm.depth . I think it's a bit unintuitive, though -- would it be possible to put a defer:ed call in the clause above where you call CaptureStart?
There was a problem hiding this comment.
you forgot STATICCALL .. ?
There was a problem hiding this comment.
It might even be possible to just check the first nybble for the 'call' class of opcodes.
There was a problem hiding this comment.
You can't assume all traces will be on byzantium!
There was a problem hiding this comment.
But the precompiles that don't exist won't be called on non-byzantium chains, and if they are, won't do anything interesting.
There was a problem hiding this comment.
Maybe you should call the type calltype/createtype or something, instead of CALL/CREATE, since the opcodes CALL/CREATE is not really involved.
There was a problem hiding this comment.
So there are two cases here:
log.depth == : call failed or there was no code there. Check last stack item to find out which case it was.
log.depth < : probably a value-call within static context, which dropped us back a level. Need to close two calls contexts.
There was a problem hiding this comment.
Might still fail, though.. ? Reverting with too large memory, for example, will result in a common throw
There was a problem hiding this comment.
Wait, you're not checking the last stack item if the call was successfull or not? What if it threw?
|
I think there's something wrong with the nesting. Testing with this one: Etherscan: https://etherscan.io/tx/0x6bcf8c5abc7e530abaca039fdd7e8d9c7620bcb57b2c0ad89b91f56d4ca928e9 It looks like the last one (to EDIT: I've looked more closely, I think it's actually correct, and etherscan shows it erroneously. |
|
A couple of comments...
|
|
Also, I'd love to have some more data in there:
And have access to the database in the end, at #git diff internal/ethapi/tracer.go
diff --git a/internal/ethapi/tracer.go b/internal/ethapi/tracer.go
index b7fef2c..a56cfc9 100644
--- a/internal/ethapi/tracer.go
+++ b/internal/ethapi/tracer.go
@@ -354,6 +354,10 @@ func (jst *JavascriptTracer) CaptureState(env *vm.EVM, pc uint64, op vm.OpCode,
jst.log["depth"] = depth
jst.log["account"] = contract.Address()
+ //A bit of a hack - should be placed in CaptureStart/CaptureEnd, but there's no evm in those methods
+ jst.ctx["blocknumber"] = env.BlockNumber.Uint64()
+ jst.ctx["gasLimit"] = env.GasLimit
+
delete(jst.log, "error")
if err != nil {
jst.log["error"] = err
@@ -375,7 +379,7 @@ func (jst *JavascriptTracer) CaptureEnd(output []byte, gasUsed uint64, t time.Du
jst.ctx["error"] = err.Error()
}
ctxvalue, _ := jst.vm.ToValue(jst.ctx)
- jst.result, jst.err = jst.callSafely("result", ctxvalue)
+ jst.result, jst.err = jst.callSafely("result", ctxvalue, jst.dbvalue)
if jst.err != nil {
jst.err = wrapError("result", jst.err)
} |
There was a problem hiding this comment.
Couldn't you return an error here?
There was a problem hiding this comment.
Well yes, but why bother if it's truly not implemented?
There was a problem hiding this comment.
It's just that panicing when you could return an error seems like a generally bad idea.
There was a problem hiding this comment.
Fair enough, given that it's live code (even if tracing), it's better not to panic. I'll fix.
There was a problem hiding this comment.
Maybe add something like "this allows us to discard entries that are no longer referenced from the current state"
There was a problem hiding this comment.
Isn't this a fast-forward, not a rewind?
There was a problem hiding this comment.
I've renamed it to config.Reexec. It's arguable a bit better. I'm not really fond of fast forward, as it doesn't really convey that it's used to regenerate missing state.
There was a problem hiding this comment.
This seems like a near duplicate of the chain tracing goroutine. Could these be extracted into their own function?
There was a problem hiding this comment.
I don't think the duplication here is that much and I'd rather keep the code easier to read in exchange of a bit of copy-paste.
There was a problem hiding this comment.
Ok, I've deduplicated some minor code by using computeStateDB in traceChain too, but in the end I've reverted it because currently I can interrupt historical chain generation if I close the connection, but this requires returning a subscription to the client before actually doing any data processing. That being said, I can still tell the subscribe fast if historical state is missing even with fast forwarding.
This means that I would need to split computeStateDB into two separate methods, one that looks up if we have a block available, and one that regenerates the state. Furthermore it would also require reworking the chain tracing so that the second phase still happens concurrently on a live subscription. Even if I were to do that however, the chain tracing still needs to process blocks sequentially, so it still needs all that duplicated functionality within itself anyway.
It may be doable the other way around, by using traceChain instead of computeStateDB in the other methods, but that seems a bit messy to spin up all the bells and whistles for a limited subset of the functionality.
If you have some specific ideas where you think this might be simplified I'm all ears, but I don't see any too low hanging fruits where we can dedup without making the code convoluted.
There was a problem hiding this comment.
This also looks like it duplicates functionality in the chain tracer.
There was a problem hiding this comment.
Yes, this is duplicated a bit. It's not trivial cleaning it up due to minor differences internally, but I'll try to make an attempt. It would make messy code appear only once.
There was a problem hiding this comment.
Won't making the size part of the returned key make it hard to match against signatures with variable length inputs?
There was a problem hiding this comment.
Removing the size is trivial during postprocessing, if you want that. The reverse problem is that if you don't know the selector for 0xdeadfeed, and want to brute-force it based on a word-list of methodnames, it definitely helps to know that the input is e.g. 32 bytes, so you can ignore some common args like (uint,uint).
There was a problem hiding this comment.
So iirc, this one spits out ["0xdeadfeed-32", ...]. Makes it also possible to distinguish between actual call data and just 'data', which does not have to align to 32-byte boundaries.
There was a problem hiding this comment.
Since 'op' will be undefined if it's not an 0xf* opcode, why not put the entire following section in an if, or break out early?
There was a problem hiding this comment.
op is accessed various places and there are code segments in between that do not depend on op and still need to run even if op is undefined.
There was a problem hiding this comment.
op will be undefined if syscall is false, so you could at least simplify the following checks to not check both.
There was a problem hiding this comment.
My hunch was that evaluating whether syscall is false or not is a boolean operation and should be faster than mucking around with op. Running both codes for the start of Ropsten, there's indeed a slight performance increase for the current code versus the one that doesn't check for syscall:
Check both:
start=0 end=36863 current=22126 transactions=5027 elapsed=1m21.076843668s
vs. check only op:
start=0 end=36863 current=21778 transactions=4844 elapsed=1m20.62278927s
There was a problem hiding this comment.
Not much, about 4% difference.
|
@Arachnid I think I've fixed or replied to all your comments, PTAL. |
|
@cdetrio Please check whether this breaks any fuzzers or other tools you have for cross client comparisons. If yes, please provide a way to repro. |
holiman
left a comment
There was a problem hiding this comment.
I think this looks good enough to merge: most of it is new functionality, and we can work out kinks in the implementation after merge.
From what I can tell, this should not break any core functionality. It may very well change the way evm --json works, but I think we can handle that.
There are a few outstanding comments, though, e.g. my request for block number in the CaptureStart. It's not critical to get that in, though, but if you have any particular reason not to add it, please answer the comment so I know (otherwise I may add it myself in a later PR).
I'm ok with merging this so it doesn't bitrot over christmas.
|
I think the block number is already part of "ctx", it was you who added it in the first run of "step". That being said, I was also pondering about pushing in the entire parent block, current block, chain config as a contexts to the tracers to allow assembling more complete "prestate" traces. Since these would require a bit of meshing and would also cover the block number, I'd just postpone "properly" doing it in a followup PR. I would also like to support running multiple tracers at once somehow, so if we end up with 2-3-4 really good tracers eventually, we could get them all listen on etherscan without them having to trace the entire chain 4 times over. |
|
Trace comparisons with evmlab are all passing, so |
|
@cdetrio Sweet, thanks for the confirm! |
IIRC, that was a bit of a hack: the step should contain step-specific information. At that time, there was no start-of-execution entrypoint, so I put it there. Now that we have a "start-of-execution", we should put context information which is not per-step in there instead. But as I said, we can fix it later if you prefer. |
|
I'd vote for fixing later when we add the other contextual infos too. |
|
I have currently bookmarked this PR. Would love to know if this description is available in any wiki section. |
* cmd, core, eth/tracers: support fancier js tracing * eth, internal/web3ext: rework trace API, concurrency, chain tracing * eth/tracers: add three more JavaScript tracers * eth/tracers, vendor: swap ottovm to duktape for tracing * core, eth, internal: finalize call tracer and needed extras * eth, tests: prestate tracer, call test suite, rewinding * vendor: fix windows builds for tracer js engine * vendor: temporary duktape fix * eth/tracers: fix up 4byte and evmdis tracer * vendor: pull in latest duktape with my upstream fixes * eth: fix some review comments * eth: rename rewind to reexec to make it more obvious * core/vm: terminate tracing using defers
The first feature of this PR expands our JavaScript tracing capabilities with built in tracers. Tracers supported out of the box:
To add new tracers, create a file called
my_awesome_tracer.jsineth/tracers/intrenal/tracersbased on the above example tracers, rungo generate ./eth/tracers/..., build Geth and from the console calldebug.traceTransaction(txHash, {tracer: "myAwesomeTracer"}).The second feature-set reworks the tracing API endpoints.
It simplifies the return value of a block trace so its's not a
{validated, structlogs, error}, rather simplytracer, error. I.e. if the block fails validation, why not return it in the error? Also, the return type needed to change because with JavaScript tracers, we can have not onlyStructLogsas return types. The new format is consistent withtraceTransaction, just instead of a single result, returns an array.The commit also introduces a
traceChainendpoint, which can trace transactions from multiple blocks. Since that's potentially a very long running operation (hours) and can also result in huge amounts of data, this endpoint is not a plain API call, rather a subscription:The API will stream back one RPC notification per non-empty block. An exception is the very last block, which will be reported even if empty so the user knows the stream is done.
Furthermore, the commit makes individual block tracing concurrent in the transactions (limited to num cores) and also makes chain tracing concurrent in the blocks (limited to num cores).
A further important functionality is transitioning from
ottovmtoduktapeas our JavaScript engine for the tracers. This nets us a 5x performance increase in exchange of a much looser integration between Go <-> JavaScript types. This is not something we want to do for the console, but it's something completely acceptable for the tracer where we don't want to cross over generic types anyway.