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

add price for ASSERTMSG and ABORTMSG opcodes #2877

Merged
merged 7 commits into from
Aug 9, 2023
Merged

add price for ASSERTMSG and ABORTMSG opcodes #2877

merged 7 commits into from
Aug 9, 2023

Conversation

ixje
Copy link
Contributor

@ixje ixje commented Jul 20, 2023

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

It should be the same as POP

shargon
shargon previously approved these changes Jul 21, 2023
@ixje
Copy link
Contributor Author

ixje commented Jul 21, 2023

@shargon that price sounds a bit excessive if we're going to give it a price other than the regular ABORT, ASSERT anyway.

ABORTMSG has just 1 Pop() in its implementation, all JMPIF* instructions have 1 POP and a lot more logic in the ExecuteJump() function and those only cost 1 << 1.

Similar, ASSERTMSG has 2 POPs, like all of the jump instructions with a comparison (JMPEQ, JMPNE, JMPGT etc), they also all cost 1 << 1.

This price is basically the equivalent of INITSLOT which has a for-loop pop'ing n items and what not. Even all arithmetic operators cost less at 1 << 3.

@roman-khimov
Copy link
Contributor

@shargon, we have 0 for RET for a long long time. RET does some processing that can cost more than POP and there can be 1024 RETs invoked in a row without any cost. Then in #1875 (comment) ABORT was set to 0 as well because it's always the last instruction to perform. ASSERT was defined to be 0 as well in c6c009a of #2045, even though technically it already pops something off the stack. But it also is the last instruction to execute, so it can be free.

Now ABORTMSG is the same computationally as the old ASSERT, so it should be 0. And ASSERTMSG should just follow the suite, an additional POP is negligible and it's also the last thing to execute.

@ixje
Copy link
Contributor Author

ixje commented Jul 25, 2023

@shargon ☝️

1 similar comment
@Jim8y
Copy link
Contributor

Jim8y commented Jul 27, 2023

@shargon ☝️

@shargon
Copy link
Member

shargon commented Jul 27, 2023

@shargon, we have 0 for RET for a long long time. RET does some processing that can cost more than POP and there can be 1024 RETs invoked in a row without any cost. Then in #1875 (comment) ABORT was set to 0 as well because it's always the last instruction to perform. ASSERT was defined to be 0 as well in c6c009a of #2045, even though technically it already pops something off the stack. But it also is the last instruction to execute, so it can be free.

Now ABORTMSG is the same computationally as the old ASSERT, so it should be 0. And ASSERTMSG should just follow the suite, an additional POP is negligible and it's also the last thing to execute.

we must learn from the mistakes of the past, I think that if there are pop operations internally, the minimum should have that cost plus another, otherwise the cost is miscalculated, however, I see that the only one who disagrees is me so I will accept the decision 😊

shargon
shargon previously approved these changes Jul 27, 2023
@shargon
Copy link
Member

shargon commented Jul 27, 2023

It seems that the nuget package doesn't work :S

@ixje
Copy link
Contributor Author

ixje commented Jul 27, 2023

It seems that the nuget package doesn't work :S

You can see it was working when I did my initial PR. Perhaps it gets auto deleted after some time?

@roman-khimov
Copy link
Contributor

the minimum should have that cost plus another, otherwise the cost is miscalculated

Yeah, that was my thinking in #1875, but I can find another argument for zero-cost ASSERT and alike. Suppose you're executing a script and there is 0.00000001 GAS left for its execution. The next instruction is some ASSERT/ABORTMSG/ASSERTMSG. If they're to have some cost, you'd get "out of GAS" exception, but when they cost zero you get whatever is appropriate for the instruction. I think in most cases you'd prefer getting ASSERT/ABORT message.

@shargon
Copy link
Member

shargon commented Jul 30, 2023

It seems that the nuget package doesn't work :S

You can see it was working when I did my initial PR. Perhaps it gets auto deleted after some time?

I know, but it doesn't work also in my local...

@ixje
Copy link
Contributor Author

ixje commented Aug 1, 2023

It seems that the nuget package doesn't work :S

You can see it was working when I did my initial PR. Perhaps it gets auto deleted after some time?

I know, but it doesn't work also in my local...

If the package had something like a 7-day retention then of course it will also not work locally as it has long expired by now. I guess you have to ask @erikzhang as he's the one that has setup that myget publishing so he should be able to see what happend to the package on the myget website. Or you can perhaps rerun the workflow to have it publish again (just for the sake of your testing)

@shargon
Copy link
Member

shargon commented Aug 9, 2023

Or you can perhaps rerun the workflow to have it publish again (just for the sake of your testing)

Done

@shargon shargon merged commit 9464107 into neo-project:master Aug 9, 2023
@ixje ixje deleted the opcode-prices branch August 9, 2023 17:38
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.

5 participants