Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Traces as of 1.10.9 do not include memory (or storage as of 1.0.5) #24146

Closed
haltman-at opened this issue Dec 22, 2021 · 10 comments
Closed

Traces as of 1.10.9 do not include memory (or storage as of 1.0.5) #24146

haltman-at opened this issue Dec 22, 2021 · 10 comments
Labels

Comments

@haltman-at
Copy link

haltman-at commented Dec 22, 2021

System information

Geth version: 1.10.9 and up for memory; 1.10.5 and up for storage (most recent at time of submission: 1.10.13)
OS & Version: Linux

Expected behaviour

Traces obtained from debug_traceTransaction ought to include memory (and storage).

Actual behaviour

No memory field (nor storage) is present.

Steps to reproduce the behaviour

Do a debug_traceTransaction on Geth 1.10.9 or higher (at least up to 1.10.13, the most recent at time of submission), or on 1.10.5 or higher if you just want to see the problem for storage.

Significance

I'm reporting this as a bug even though it's possible it was a deliberate decision. But, doing this makes Geth traces (as of 1.10.9) unusable with Truffle Debugger and presumably other debuggers. Right now Truffle Debugger will just crash; we can of course fix the crash, but then we're still stuck with no memory! What do we do, just pretend the memory is all zero? We could of course attempt to keep track of memory ourself, and this might not seem unreasonable, as we already do this with storage; but in actual fact I don't think this is quite so feasible as that due to how many things can alter memory, whereas storage can only be interacted with via SSTORE and SLOAD.

(Edit: Actually, I forgot, we do actually still use storage in certain cases.)

By contrast, with memory, in addition to being able to store things to it from the stack, one can also read things into it from other locations, such as code, calldata, or returndata; and old-style CALLs, with a nonempty output region set, will write to it directly. I don't think it's practical for the debugger itself to compute all this. The worst case is that of precompiled contracts, where, in order for us to get the output (whether written directly into memory or copied into memory from returndata), we'd have to actually compute it ourselves, because there are no trace steps to go by. (And some languages, such as Vyper, do in fact use precompile 0x4 to copy memory around...) I don't think it's reasonable for the debugger to compute outputs of precompiles.

So, please restore memory (and storage) to traces! Thank you.

@holiman
Copy link
Contributor

holiman commented Dec 22, 2021 via email

@kasvtv
Copy link

kasvtv commented Dec 22, 2021

Can confirm this as well for Remix. After many hours yesterday, I found that the Remix debugger does not work when connected through the Web3 Provider to a local geth instance ever since version 1.10.9, including the latest 1.10.13.

To any beginner trying to solve the issue, if you have Golang installed, you can rebuild geth 1.10.8 from source using:

go get github.com/ethereum/go-ethereum@v1.10.8
cd $GOPATH/pkg/mod/github.com/ethereum/go-ethereum@v1.10.8/cmd/geth
go build -o $GOPATH/bin .

Replace $GOPATH with %GOPATH% if using Windows command line.

Make sure the old installation of geth is removed. Then verify the version by running geth version.

@haltman-at
Copy link
Author

Hi,Just make sure to provide the option 'disableMemory:false' and you should be fine

I have just tested it and this is false. Passing disableMemory: false had no effect.

Btw, I said that Truffle Debugger doesn't make use of storage; I forgot actually we still do in certain cases. So I should complain that that's missing too, really. I'll add that to the issue.

This is quite a change to drop on us in a supposedly non-breaking release!

@haltman-at haltman-at changed the title Traces as of 1.10.9 do not include memory Traces as of 1.10.9 do not include memory (or storage) Dec 22, 2021
@haltman-at
Copy link
Author

Ah, it looks like this was introduced in #23558 as a deliberate change. That is a breaking change and, IMO, should never have made it into a non-breaking release...

@haltman-at
Copy link
Author

Ah, I see, one can pass enableMemory to enable memory. So that provides something of a way of handling this; I haven't yet figured out how to enable storage as well (since we still use that in certain cases).

...not closing the issue, though, for the reasons stated above.

@haltman-at haltman-at changed the title Traces as of 1.10.9 do not include memory (or storage) Traces as of 1.10.9 do not include memory (or storage as of 1.0.5) Jan 3, 2022
@haltman-at
Copy link
Author

OK, I'm definitely not closing the issue, because while memory can be turned on with enableMemory, there doesn't seem to be anything similar for storage!

@s1na
Copy link
Contributor

s1na commented Jan 6, 2022

We're sorry this change broke your code. It was a deliberate and necessary change, but perhaps it could've been highlighted better in the release notes for example.

Re storage, I just tried it on master and disableStorage: false does work with one caveat. It's only returned for SSTORE and SLOAD objects. Is that enough information for your purpose or would you need a flag to enable storage for every trace step?

@holiman
Copy link
Contributor

holiman commented Jan 6, 2022

Storage was only ever accessible on SSTORE/SLOAD, so if disableStorage:false works, I'm closing this.

@holiman holiman closed this as completed Jan 6, 2022
@haltman-at
Copy link
Author

Storage was only ever accessible on SSTORE/SLOAD

Huh, I don't remember this being the case... the really old versions of our code, that relied heavily on storage, wouldn't have worked with that, and I remember them working fine...

In any case, we've now changed our code to eliminate the reliance on storage, so our code now works fine (we get memory with enableMemory, and storage we simply no longer need).

So, there's no longer an issue for us, and I guess it's fine that this is closed... but, um, yes, I wanted to highlight that just removing fields from the trace like that is pretty breaking and it happening in a supposedly non-breaking release was kind of a shock.

In any case, thanks for the response.

@gnidan
Copy link
Member

gnidan commented Jan 6, 2022

but, um, yes, I wanted to highlight that just removing fields from the trace like that is pretty breaking and it happening in a supposedly non-breaking release was kind of a shock.

I'd like to second this. Please don't introduce breaking changes in non-breaking change releases!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants