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

Feature/oog #74

Merged
merged 6 commits into from
Jun 2, 2022
Merged

Feature/oog #74

merged 6 commits into from
Jun 2, 2022

Conversation

laisolizq
Copy link
Contributor

  • check out of gas
  • check stack over flow (it will be necessary to update this)

main/opcodes.zkasm Outdated Show resolved Hide resolved
Copy link
Contributor

@ignasirv ignasirv left a comment

Choose a reason for hiding this comment

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

💯

@@ -12,13 +12,15 @@ opSTOP:
GAS + B => GAS
$ => SP :MLOAD(lastSP)
$ => PC :MLOAD(lastPC)
1 :MSTORE(SP++) ; // TODO: call to an EOA address, what is going on ?
1 :MSTORE(SP++)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comments at the beginning of the file

:JMP(readCode)

opSTOPend:
:JMP(endCode)

opADD:
SP - 2 :JMPN(stackOverflow)
Copy link
Contributor

Choose a reason for hiding this comment

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

This error should be renamed to: stackUnderflow
And just add the label stackUnderflow just above stackOverflow since the routine would be exactly the same.
This apply for the rest of this errors

:JMP(sendGasSeq)

stackOverflow:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ethereum yellow paper quotation:

if the execution halts in an exceptional
fashion (i.e. due to an exhausted gas supply, stack underflow, invalid jump destination or invalid instruction), then
no gas is refunded to the caller and the state is reverted to
the point immediately prior to balance transfer (i.e.

main/utils.zkasm Outdated
:JMP(stackOverflow)

stackOverflowEnd:
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@krlosMata krlosMata left a comment

Choose a reason for hiding this comment

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

😸

Copy link
Contributor

@krlosMata krlosMata left a comment

Choose a reason for hiding this comment

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

😸

@laisolizq laisolizq merged commit 1ee784d into main Jun 2, 2022
krlosMata added a commit that referenced this pull request Jan 26, 2024
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.

4 participants