-
Notifications
You must be signed in to change notification settings - Fork 130
[ZkTracer] feat: zkasm OOB #1946
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
base: main
Are you sure you want to change the base?
Conversation
...va/net/consensys/linea/zktracer/module/hub/fragment/imc/oob/opcodes/CallDataLoadOobCall.java
Show resolved
Hide resolved
...ain/java/net/consensys/linea/zktracer/module/hub/fragment/imc/oob/opcodes/SstoreOobCall.java
Show resolved
Hide resolved
...ain/java/net/consensys/linea/zktracer/module/hub/fragment/imc/oob/opcodes/CreateOobCall.java
Outdated
Show resolved
Hide resolved
...ea/zktracer/module/hub/fragment/imc/oob/precompiles/common/postCancun/msm/BlsMsmOobCall.java
Show resolved
Hide resolved
...nsys/linea/zktracer/module/hub/fragment/imc/oob/precompiles/modexp/ModexpPricingOobCall.java
Show resolved
Hide resolved
...tracer/module/hub/fragment/imc/oob/precompiles/common/postCancun/BlsPairingCheckOobCall.java
Show resolved
Hide resolved
...thmetization/src/main/java/net/consensys/linea/zktracer/module/hub/fragment/imc/MxpCall.java
Fixed
Show fixed
Hide fixed
...va/net/consensys/linea/zktracer/module/hub/fragment/imc/oob/opcodes/CallDataLoadOobCall.java
Show resolved
Hide resolved
...ea/zktracer/module/hub/fragment/imc/oob/precompiles/common/postCancun/msm/BlsMsmOobCall.java
Show resolved
Hide resolved
.../main/java/net/consensys/linea/zktracer/module/hub/fragment/imc/oob/opcodes/CallOobCall.java
Outdated
Show resolved
Hide resolved
...r/module/hub/fragment/imc/oob/precompiles/common/ecAddMulRecover/EcRecEcAddEcMulOobCall.java
Outdated
Show resolved
Hide resolved
...r/module/hub/fragment/imc/oob/precompiles/common/ecAddMulRecover/EcRecEcAddEcMulOobCall.java
Outdated
Show resolved
Hide resolved
...nsys/linea/zktracer/module/hub/fragment/imc/oob/precompiles/modexp/ModexpExtractOobCall.java
Outdated
Show resolved
Hide resolved
...nsys/linea/zktracer/module/hub/fragment/imc/oob/precompiles/modexp/ModexpPricingOobCall.java
Show resolved
Hide resolved
...java/net/consensys/linea/zktracer/module/hub/fragment/imc/oob/opcodes/DeploymentOobCall.java
Outdated
Show resolved
Hide resolved
...nsys/linea/zktracer/module/hub/fragment/imc/oob/precompiles/modexp/ModexpPricingOobCall.java
Show resolved
Hide resolved
...r/module/hub/fragment/imc/oob/precompiles/common/ecAddMulRecover/EcRecEcAddEcMulOobCall.java
Outdated
Show resolved
Hide resolved
...nsys/linea/zktracer/module/hub/fragment/imc/oob/precompiles/modexp/ModexpPricingOobCall.java
Show resolved
Hide resolved
...nsys/linea/zktracer/module/hub/fragment/imc/oob/precompiles/modexp/ModexpPricingOobCall.java
Show resolved
Hide resolved
.../main/java/net/consensys/linea/zktracer/module/hub/fragment/imc/oob/opcodes/CallOobCall.java
Outdated
Show resolved
Hide resolved
...nsys/linea/zktracer/module/hub/fragment/imc/oob/precompiles/modexp/ModexpPricingOobCall.java
Outdated
Show resolved
Hide resolved
...ain/java/net/consensys/linea/zktracer/module/hub/fragment/imc/oob/opcodes/CreateOobCall.java
Outdated
Show resolved
Hide resolved
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
This puts through some minor tweaks for running the tracer tests.
The calcuation for determining when a JUMP is out-of-bounds was comparing an EWord (pcNew) egainst a Bytes instance (codeSize) which was failing (presumably comparing these two types does something else). Therefore, converting codeSize to be stored in an EWord instance resolves the issue.
These values were not being capped at 0x20 due to the use of Bytes.compareTo instead of e.g. EWord.compareTo.
Again, use of `Bytes.compareTo()` was causing incorrect success flag settings.
818e67e to
a1b3c11
Compare
...nsys/linea/zktracer/module/hub/fragment/imc/oob/precompiles/modexp/ModexpPricingOobCall.java
Outdated
Show resolved
Hide resolved
...nsys/linea/zktracer/module/hub/fragment/imc/oob/precompiles/modexp/ModexpPricingOobCall.java
Outdated
Show resolved
Hide resolved
.../main/java/net/consensys/linea/zktracer/module/hub/fragment/imc/oob/opcodes/CallOobCall.java
Show resolved
Hide resolved
...ain/java/net/consensys/linea/zktracer/module/hub/fragment/imc/oob/opcodes/CreateOobCall.java
Outdated
Show resolved
Hide resolved
This changes a number of fields from having type Bytes into type EWord. Whilst in some cases this may seem odd, since the given value may be consider of "arbitrary size" we know in practice it should not overflow.
This fixes an issue identified by cursor where the remainder was being computed instead of the division.
| public void setInputs(Hub hub, MessageFrame frame) { | ||
| setOffset(EWord.of(frame.getStackItem(1))); | ||
| setSize(EWord.of(frame.getStackItem(2))); | ||
| setRds(Bytes.ofUnsignedLong(frame.getReturnData().size())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing overflow check in RETURNDATACOPY bounds validation
The refactored setOutputs() method in ReturnDataCopyOobCall computes sum = offset.add(size) using 256-bit modular arithmetic and checks sum.compareTo(rds) > 0. The old code first checked if offset.hi() or size.hi() were non-zero (indicating values >= 2^128) and would immediately fail the bounds check. With the new logic, if both offset and size have large values causing their sum to overflow and wrap around to a small value, the comparison with rds could incorrectly return false. For example, if offset = 2^255 and size = 2^255, their sum wraps to 0, making rdcx incorrectly false when it should be true.
| // row i + 3 | ||
| final OobExoCall creatorNonceAbortCall = callToLT(wcp, creatorNonce, EIP2681_MAX_NONCE_BYTES); | ||
| exoCalls.add(creatorNonceAbortCall); | ||
| final boolean creatorNonceAbort = !bytesToBoolean(creatorNonceAbortCall.result()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Inconsistent trace method call for CreateOobCall
The traceOob method in CreateOobCall uses .validateRow() while all other OobCall implementations use .fillAndValidateRow(). This inconsistency suggests a potential oversight during refactoring. If fillAndValidateRow() is responsible for setting default values on unfilled trace columns, using only validateRow() could result in missing data in the trace output.
This adds provisional support for the p256verify precompile contract to the zkasm oob implementation.
| public void setInputs(Hub hub, MessageFrame frame) { | ||
| setOffset(EWord.of(frame.getStackItem(1))); | ||
| setSize(EWord.of(frame.getStackItem(2))); | ||
| setRds(Bytes.ofUnsignedLong(frame.getReturnData().size())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing overflow check in RETURNDATACOPY bounds validation
The refactored setOutputs() method no longer checks for range-out-of-bounds (rdcRoob) when offset.hi() or size.hi() is non-zero. The original implementation explicitly detected this case because: (1) values with non-zero high parts are >= 2^128, which guarantees an out-of-bounds condition for any realistic return data size, and (2) the 256-bit addition of such large values can overflow and wrap around to a small value. The new code offset.add(size).compareTo(rds) > 0 fails when the sum overflows to a value smaller than rds, incorrectly setting rdcx = false when it should be true. This could cause RETURNDATACOPY operations to incorrectly pass bounds checks with specially crafted offset/size values.
| .data9(creatorNonce); | ||
| .data9(creatorNonce) | ||
| .data10(Bytes.ofUnsignedLong(codeSize)) | ||
| .validateRow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Inconsistent trace method used in CreateOobCall
The traceOob() method calls .validateRow() while all other OobCall implementations consistently use .fillAndValidateRow(). This inconsistency could cause issues if validateRow() doesn't fill in default values for unset columns while fillAndValidateRow() does. All other trace methods in the OOB module uniformly use fillAndValidateRow(), suggesting this is likely an oversight during the refactoring.
Another comparison of EWord against Bytes.
| return CT_MAX_RDC; | ||
| public void setOutputs() { | ||
| final EWord sum = offset.add(size); | ||
| setRdcx(sum.compareTo(rds) > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing overflow check in RETURNDATACOPY OOB validation
The refactored setOutputs() method computes offset.add(size) and compares the result to rds, but it's missing the original overflow check. The previous implementation first checked if offset.hi() or size.hi() were non-zero (indicating values >= 2^128), which would immediately set rdcx = true. Since EWord extends BaseUInt256Value and uses modular 256-bit arithmetic, offset.add(size) wraps on overflow. If both values are very large and their sum wraps to a small value, the comparison could incorrectly return false when it should return true. For example, if offset = 2^256 - 1 and size = 2, the sum wraps to 1, which would compare as less than a typical rds value, incorrectly allowing an out-of-bounds operation.
| return CT_MAX_RDC; | ||
| public void setOutputs() { | ||
| final EWord sum = offset.add(size); | ||
| setRdcx(sum.compareTo(rds) > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Addition overflow in RETURNDATACOPY bounds check
The setOutputs() method computes offset.add(size) without first checking if either value's high 128 bits are non-zero. Since EWord extends BaseUInt256Value with modular 256-bit arithmetic, if both offset and size are large (e.g., both >= 2^255), their sum can overflow and wrap to a small value. For example, 2^255 + 2^255 = 0 mod 2^256. This would cause sum.compareTo(rds) > 0 to incorrectly return false when it should be true, failing to detect an out-of-bounds condition. The original code prevented this by explicitly checking isZero(concat(offset.hi(), size.hi())) first and treating non-zero high parts as immediate out-of-bounds.
| return CT_MAX_RDC; | ||
| public void setOutputs() { | ||
| final EWord sum = offset.add(size); | ||
| setRdcx(sum.compareTo(rds) > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing overflow check in RETURNDATACOPY bounds validation
The refactored setOutputs() method computes offset.add(size) and compares the result to rds. However, EWord uses modular arithmetic (mod 2^256), so if offset + size overflows, the result wraps around to a small value. The original implementation first checked if offset.hi() or size.hi() were non-zero (indicating values >= 2^128) and immediately flagged as out-of-bounds before performing the addition. Without this check, extreme values like offset = 2^256 - 1 and size = 2 would wrap to sum = 1, incorrectly passing the bounds check when rds > 1. This could cause the zkTracer to produce incorrect traces that don't match actual EVM semantics for RETURNDATACOPY.
DavePearce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR implements issue(s) #
Checklist
Note
Reworks the OOB module to self-contained zkasm-style operations (setInputs/setOutputs + traceOob), removes exo-calls, updates hub/IMC, CREATE flow, precompile OOBs and tests, and standardizes traceHub APIs; also bumps test timeout.
OobCallops (setInputs/setOutputs+traceOob); removeOobExoCallandOobOperation.Hubto usenew Oob()andImcFragmentto callhub.oob().call(f, hub).trace(...)totraceHub(...)across fragments; adjust all callers.CreateOobCall(remove London/Shanghai variants) and useXCreateOobCallon max code size; simplify section logic.CALL,CDL,RDC,JUMP(I),SSTORE,XCALL,XCREATE,DeploymentusingEWordtypes and internal comparisons.computeExponentLog), EC (ECADD/ECMUL/ECPAIRING/ECRECOVER), BLS (G1/G2 add/msm/pairing/check/map), SHA/RIP/ID to compute costs/flags internally and trace viainst(...).computeExponentLogfrom MODEXP OOB; adapt OOB accessors.Written by Cursor Bugbot for commit 47c8c2e. This will update automatically on new commits. Configure here.