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

LFVM: Refactor overflow checks, add tests #838

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

LuisPH3
Copy link
Contributor

@LuisPH3 LuisPH3 commented Oct 3, 2024

part of #751

This PR removes checkSizeOffsetUint64Overflow and rounding done to delay overflow checks
This is done by moving range check for offset and length Op parameters in the memory expansion or getData calls instead of it doing at the beginning of the instructions.
This change simplifies many instructions control flow, and facilitates testing

LFVM has 3 kinds of memory management in the VM, in the sense of manipulating slices that could suffer an out of range access:

  • Memory: where every access Read/Write is preceded by a memory expansion, with a gas check.
  • input/code accesses: where memory is read-only and out-of-range scenarios are handled by allocating the requested data and right padding with zeroes.
  • result Data: although close related to the latter input buffers, It returns errors on any kind of out-of-range errors because it is accessed to write into it.

Regarding error reporting, these functions behave as follows:

  • Memory shall return Out of gas for any large access. One could argue that overflows are just a limitation of the architecture, and given a 256 bits addressable space, memory accesses would only produce out of gas errors.
  • input/code data accesses are zeroed in any out-of-bounds access, except for having a size value that can not be allocated because it exceeds the maximum addressable space. This is the only error possible.
  • read Data will produce errors whenever, the offset, size, or the sum of both are larger than max uint64. Additionally, will produce an error if the result Data buffer was not allocated large enough for the values computed during execution.

other changes:

  • jump overflow errors use now invalid jump to denote overflows in the program counter.

Copy link
Collaborator

@HerbertJordan HerbertJordan left a comment

Choose a reason for hiding this comment

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

Looks good, and I think it is a clear improvement.
Did you encounter any major trade-offs you needed to make during the refactoring?

facuMH
facuMH previously approved these changes Oct 4, 2024
@LuisPH3 LuisPH3 marked this pull request as ready for review October 4, 2024 08:24
@LuisPH3
Copy link
Contributor Author

LuisPH3 commented Oct 4, 2024

Looks good, and I think it is a clear improvement. Did you encounter any major trade-offs you needed to make during the refactoring?

TL;DR: getData avoids checks because it correlates to memory expansions, which are gas-guarded.

There are 2 small details that may require discussion:

  • getData relies on being called after memory expansion checks: Overflow of the size parameter is a non-recoverable error. Reason being that the architecture cannot address 'MaxUint64' + 1 buffer. Adding this error report to getData generated a number of non-reachable regions in the op functions. For this reason, the length argument is uin64 in the getData function and not uint256 as int mem.getSlice which does perform the check on size.
  • There is an unguarded check that I would have liked to do, but it is not executable: getData when offset and size are smaller than MaxUint64 but (offset + size) > MaxUint64. This scenario should return a right-padded buffer containing the overlap of the original buffer and the offset:offset+size. Nevertheless, the original buffer would require a large amount of memory, which is not allocatable. CT cannot trigger this error neither because the required memory consumption is gas guarded.

HerbertJordan
HerbertJordan previously approved these changes Oct 8, 2024
Copy link
Collaborator

@HerbertJordan HerbertJordan left a comment

Choose a reason for hiding this comment

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

Great improvement, thanks!

go/interpreter/lfvm/memory_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/memory_test.go Outdated Show resolved Hide resolved
delay range check for offset and length op parameters instead of doing at the beginning of the instructions to simplify instructions control flow.
@LuisPH3 LuisPH3 force-pushed the luis/refactor_overflow_check branch from 7ab6449 to 0dd63cc Compare October 8, 2024 08:33
Remove superfluous tests
Copy link
Contributor

@facuMH facuMH left a comment

Choose a reason for hiding this comment

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

I have a couple questions/comments before fully approving

go/interpreter/lfvm/memory.go Show resolved Hide resolved
go/interpreter/lfvm/memory.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/memory_test.go Show resolved Hide resolved
go/interpreter/lfvm/instructions.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/instructions.go Show resolved Hide resolved
go/interpreter/lfvm/instructions_test.go Outdated Show resolved Hide resolved
go/interpreter/lfvm/super_instructions.go Outdated Show resolved Hide resolved
- remove dead code
-  move word cost after get slice to guard size cast
- move global variable to local scope
- fix variable naming
@LuisPH3 LuisPH3 requested a review from facuMH October 8, 2024 11:28
facuMH
facuMH previously approved these changes Oct 8, 2024
Copy link
Contributor

@facuMH facuMH left a comment

Choose a reason for hiding this comment

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

thanks for the great changes !!

@LuisPH3
Copy link
Contributor Author

LuisPH3 commented Oct 8, 2024

  • ct

@LuisPH3 LuisPH3 merged commit 563210c into main Oct 8, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants