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

Ensure that we always call trace_call for builtins #9236

Closed
wants to merge 1 commit into from

Conversation

udoprog
Copy link
Contributor

@udoprog udoprog commented Jul 27, 2018

I'm trying to implement a tracer for a simple debugger based on parity, but noticed that a contract where I'm calling ecrecover there is no trace_call to determine when a call is done which I need to maintain a call stack.

@parity-cla-bot
Copy link

It looks like @udoprog signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@udoprog
Copy link
Contributor Author

udoprog commented Jul 27, 2018

Issue with GitLab:

________Running Parity Full Test Suite________
 Downloading threadpool v1.7.1
error: unable to get packages from source

Caused by:
  No space left on device (os error 28)
ERROR: Job failed: exit code 1

@sorpaas
Copy link
Collaborator

sorpaas commented Jul 27, 2018

@udoprog I don't know whether we would want this. The issue is that some precompiled contracts are used quite often (like IDENTITY, which is used for memcpy). If we trace all of them, it creates really heavy workload for the client (even with the input/output field stripped), with really small gas costs. So the previous solution was to only trace two kinds of precompiled calls:

  • Trace top-level calls.
  • Trace noticeable sub-level calls (i.e. any sub-level calls with non-zero value transfers).

You can find more on this at #8486.

@sorpaas
Copy link
Collaborator

sorpaas commented Jul 27, 2018

@udoprog Would there be any better ways to solve your use cases? You mentioned that you need to maintain a call stack. Can you explain more how/why you need that?

@5chdn 5chdn added this to the 2.1 milestone Jul 27, 2018
@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 27, 2018
@udoprog
Copy link
Contributor Author

udoprog commented Jul 27, 2018

@sorpaas Sure. Below is a more detailed explanation of what I do.

As I enter a call, I lookup 1) the source map, and 2) the AST for the destination of that call and push it on a stack.
As instructions are processed I use the source map to locate AST constructs which matches in order to decode the value of expressions for debugging.

If the call finishes, I pop the stack so that the correct stack element is maintained on top.
If the call fails, I pop this stack to be able to pinpoint where the failure happened.

I rely on prepare_trace_call for pushing to the stack, since that is called before any instructions in the destination are executed, and trace_call/trace_failed_call for popping the stack. The imbalance caused by certain builtins throws this off.

In order to do the instruction processing, this call stack is shared (through a Mutex) with a VMTracer implementation. That is why it's important that it is maintained at exactly these points, since the top of the stack needs to be set correctly as instructions are processed.

I've tried using subtracer to push, and drain to pop. But prepare_trace_call is called before these, so the interaction becomes messy. If trace_call was called on the subtracer instead it would work.

@udoprog
Copy link
Contributor Author

udoprog commented Jul 27, 2018

I pushed the implementation so you can take a look or make suggestions:
https://github.com/PrimaBlock/parables/blob/debugging/testing/trace.rs#L401

I realized I deal with this right now by checking params.code_address == Address::from(0x1). If there was a more explicit way to test it, that would work as well:
https://github.com/PrimaBlock/parables/blob/debugging/testing/trace.rs#L407

Ultimately a more foolproof API would be nice though.

andresilva pushed a commit that referenced this pull request Aug 14, 2018
…e*call are balanced (#9353)

This refactors `prepare_trace_output` to instead directly take the reference of return values, so that it's simpler and we save a stack item. This should also fixes [the issue](#9236 (comment)) @udoprog is facing. Replaces #9236
This pull request was closed.
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. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants