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

bug: CI test with invalid memory reference #1287

Closed
DreamWuGit opened this issue Mar 7, 2023 · 6 comments
Closed

bug: CI test with invalid memory reference #1287

DreamWuGit opened this issue Mar 7, 2023 · 6 comments
Labels
T-bug Type: bug

Comments

@DreamWuGit
Copy link
Collaborator

DreamWuGit commented Mar 7, 2023

What command(s) is the bug in?

No response

Describe the bug

found this issue in PR #1265 's CI https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/4349281702/jobs/7598772747

Caused by:
  process didn't exit successfully: `/github/runner/_work/zkevm-circuits/zkevm-circuits/target/release/deps/testool-9af89d4775d0c804` (signal: 11, SIGSEGV: invalid memory reference)
Error: The process '/home/ubuntu/.cargo/bin/cargo' failed with exit code 101

Managed to resolve it in edc24ab
by change EvmCircuitCached to EvmCircuit

Concrete steps to reproduce the bug

No response

@DreamWuGit DreamWuGit added the T-bug Type: bug label Mar 7, 2023
@DreamWuGit DreamWuGit changed the title CI invalid memory reference bug: CI test with invalid memory reference Mar 7, 2023
@lispc
Copy link
Collaborator

lispc commented Mar 7, 2023

@ed255 the Cached EvmCircuit is a bit strange..

@lispc
Copy link
Collaborator

lispc commented Mar 8, 2023

oh it seems related to RUST_MIN_STACK: 16777216..

@ChihChengLiang
Copy link
Collaborator

RUST_MIN_STACK: 16777216 is introduced in #1286. cc @ed255

@ed255
Copy link
Member

ed255 commented Mar 8, 2023

Ah yes, I've observed this previously. Whenever this happens signal: 11, SIGSEGV: invalid memory reference running our code, it's most likely a stack overflow. After reading some Rust documentation I found that Rust threads start with 2 MB of stack by default, and this is sometimes not enough for us. Sometimes I've encountered this issue when running the tests in the test profile instead of release profile. Recently I encountered it in release profile while developing the PR that CC linked #1286 so I added an env var to increase the stack size in github actions.
I didn't find a better way to set the default stack size other than using the env var.

My guess is that the big stack requirements comes from having the ExecutionConfig from the EVM Circuit type so big. This type is held under a Box in the EvmCircuitConfig to try to mitigate this problem. But maybe it's when this value is created that it's held in the stack and can cause the overflow.
Notice that the size of ExecutionConfig keeps growing with every execution state that we add, and with every new gadget that we add to an existing execution state.

@ed255
Copy link
Member

ed255 commented Mar 8, 2023

I wish we could find a way to resolve the massive stack usage, so that we don't have to depend on setting a bigger stack via 16777216!

@lispc
Copy link
Collaborator

lispc commented Mar 8, 2023

OK since we know the reason now and it can be resolved i think it is ok to close the issue

@lispc lispc closed this as completed Mar 8, 2023
ed255 added a commit that referenced this issue Mar 8, 2023
### Description

In EVM Circuit's ExecutionConfig, store the gadget for every execution
state in a Box to significantly reduce the size of ExecutionConfig which
inevitably needs to live in the stack at some points. This PR removes
the extended stack setting `RUST_MIN_STACK: 16777216` introduced in
#1286

Let's see if this works!

### Issue Link

Related to
#1287

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

### Rationale

Even though setting the env var `RUST_MIN_STACK=16777216` fixes the
stack overflow we encountered before, I think it's better to resolve
this by avoiding consuming so much stack that it overflows on its
default size. This is because setting this env var can be annoying and
it's easy to forget it in many scenarios.
cameron1024 pushed a commit to cameron1024/zkevm-circuits that referenced this issue Jun 30, 2024
* allow up to 5x compression ratio

* chore: export init_zstd_encoder

* fmt
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-bug Type: bug
Projects
None yet
Development

No branches or pull requests

4 participants