Skip to content

core/vm, cmd/evm: standard vm traces#15035

Merged
fjl merged 1 commit into
ethereum:masterfrom
cdetrio:standard-trace
Sep 22, 2017
Merged

core/vm, cmd/evm: standard vm traces#15035
fjl merged 1 commit into
ethereum:masterfrom
cdetrio:standard-trace

Conversation

@cdetrio
Copy link
Copy Markdown
Member

@cdetrio cdetrio commented Aug 24, 2017

Some standardization (ethereum/tests#249) fixes for EVM traces.

Copy link
Copy Markdown
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

I'm for these changes. They introduce one more if-debug block in the main interpreter loop, but I don't see quite how to get around that. If we want to optimize the evm, there are several other things we can do to shave off some time.

The main crux that this PR handles is that when the CALL gas is shaved down to 63/64 of the total gas, that operation is performed during the operation.gasCost(in.gasTable, in.evm, contract, stack, mem, memorySize) call. When this happens, the actual gas value on the stack is replaced . This PR thus makes a (shallow) copy of the stack (if debug is true), once every loop.

@fjl suggested that a better fix is to remove the swap-gas-stackitem-for-63/64 hack and instead use some callGas in the interpreter object, or somewhere.

That is probably a good idea, but I feel it's a bit too invasive at this point (with metro-changes to the evm already in progress) , and I don't feel comfortable doing that right now.

So I'm all for this patch. The standard vm traces are important for testing metro, and we've already found a post-metro consensus issue using fuzz testing.

Copy link
Copy Markdown
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

I’m fine with this PR, but I doubt that the temporary hack will ever be removed.

Comment thread core/vm/interpreter.go Outdated
Copy link
Copy Markdown
Contributor

@fjl fjl Sep 7, 2017

Choose a reason for hiding this comment

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

Please use

 		// copies used by tracer
         stackCopy = newstack() // needed for Tracer since stack is mutated by 63/64 gas rule 
 		pcCopy    uint64       // needed for the deferred Tracer
 		gasCopy   uint64       // for Tracer to log gas remaining before execution
 		logged    bool         // deferred Tracer should ignore already logged steps

@cdetrio
Copy link
Copy Markdown
Member Author

cdetrio commented Sep 20, 2017

@fjl I made the requested changes.

@fjl fjl merged commit 673007d into ethereum:master Sep 22, 2017
vincentserpoul pushed a commit to vincentserpoul/go-ethereum that referenced this pull request Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants