Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

geth-utils: remove 10mb trace limit#817

Merged
pinkiebell merged 2 commits into
privacy-ethereum:mainfrom
pinkiebell:geth-utils
Oct 4, 2022
Merged

geth-utils: remove 10mb trace limit#817
pinkiebell merged 2 commits into
privacy-ethereum:mainfrom
pinkiebell:geth-utils

Conversation

@pinkiebell
Copy link
Copy Markdown
Contributor

I hit this one while testing the circuits

@pinkiebell pinkiebell requested review from a team and ed255 and removed request for a team October 3, 2022 12:11
@github-actions github-actions Bot added the crate-geth-utils Issues related to the geth-utils workspace member label Oct 3, 2022
@han0110
Copy link
Copy Markdown
Contributor

han0110 commented Oct 3, 2022

@adria0 I saw this limitation is added in #748, is there anything broken when the trace is too large? Otherwise I think it should be fine to process large trace.

@adria0
Copy link
Copy Markdown
Contributor

adria0 commented Oct 3, 2022

@adria0 I saw this limitation is added in #748, is there anything broken when the trace is too large? Otherwise I think it should be fine to process large trace.

Yes, @han0110 happened that there was a crash when rust allocated a ton of memory to deserialize the geth full trace.
Anyway @pinkiebell we can remove this at this moment because now there's a configuration file in the testool that allows to ignore specific tests.

@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Oct 3, 2022

I think that after this was merged some months ago #614 we shouldn't encounter massive memory usage in the traces (as I believe this was coming from having a memory snapshot at each step of the trace). We just need to make sure that we're always querying the trace with EnableMemory: false. I think this may not be the case when using the internal tracer. See: https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/2235254416aefd1bc9b5592e4e06cd58f8516e2a/external-tracer/src/lib.rs#L48

@pinkiebell could you try setting the default enable_memory to false and see if all unit tests still pass?

Copy link
Copy Markdown
Contributor

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM! I think that without memory snapshots this check is unnecessary. (We may still need to update some places to avoid requesting the memory snapshots from the trace)

@github-actions github-actions Bot added the crate-external-tracer Issues related to the external-tracer workspace member label Oct 3, 2022
@pinkiebell
Copy link
Copy Markdown
Contributor Author

I think that after this was merged some months ago #614 we shouldn't encounter massive memory usage in the traces (as I believe this was coming from having a memory snapshot at each step of the trace). We just need to make sure that we're always querying the trace with EnableMemory: false. I think this may not be the case when using the internal tracer. See:

https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/2235254416aefd1bc9b5592e4e06cd58f8516e2a/external-tracer/src/lib.rs#L48

@pinkiebell could you try setting the default enable_memory to false and see if all unit tests still pass?

Looks good 👌

Copy link
Copy Markdown
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM!

@pinkiebell pinkiebell merged commit 746cc24 into privacy-ethereum:main Oct 4, 2022
@pinkiebell pinkiebell deleted the geth-utils branch October 4, 2022 09:35
StefanosChaliasos referenced this pull request in Veridise/zkevm-circuits Oct 7, 2022
* geth-utils: remove 10mb trace limit

* test disable memory
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

crate-external-tracer Issues related to the external-tracer workspace member crate-geth-utils Issues related to the geth-utils workspace member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants