num256 sub overflow protection doesn't work

[pdaian]

viper Version: 0.0.2
pyethereum Version: 2.1.0
OS: linux
Python Version (python --version): 3.6

What's your issue about?

num256_sub overflow protection appears to be very broken (fails in cases that should succeed, succeeds in cases that should fail); at first glance, perhaps it is flipping the order of the arguments to the sub function in the overflow check (failing when y - x would overflow, not x - y).

Minimal example:

from ethereum.tools import tester as t
from ethereum.slogging import configure_logging

#configure_logging(':trace')
s = t.Chain()
from viper import compiler

code = """
def foo(x:num256) -> num256:
    return num256_sub(x, as_num256(1))

def foo2(x:num256) -> num256:
    return x

def foo3(x:num256) -> num256:
    return num256_sub(as_num256(1), x)
"""

t.languages['viper'] = compiler.Compiler() 

c = s.contract(code, language='viper')

NUM256_MAX = 115792089237316195423570985008687907853269984665640564039457584007913129639935  # Max possible num256 value
print(c.foo2(NUM256_MAX)) # Should/does return NUM256_MAX
print(c.foo3(NUM256_MAX)) # Should fail; instead returns 2
print(c.foo(NUM256_MAX)) # Should return NUM256_MAX - 1; instead fails

How can it be fixed?

Have not looked into this yet.

Cute Animal Picture

             |       :     . |
             | '  :      '   |
             |  .  |   '  |  |
   .--._ _...:.._ _.--. ,  ' |
  (  ,  `        `  ,  )   . |
   '-/              \-'  |   |
     |  o   /\   o  |       :|
     \     _\/_     / :  '   |
     /'._   ^^   _.;___      |
   /`    `""""""`      `\=   |
 /`                     /=  .|
;             '--,-----'=    |
|                 `\  |    . |
\                   \___ :   |
/'.                     `\=  |
\_/`--......_            /=  |
            |`-.        /= : |
            | : `-.__ /` .   |
            |jgs .   ` |    '|
            |  .  : `   . |  |

[DavidKnott]

@pdaian I'll fix this now

[pdaian]

I looked at the LLL changes and they look correct, closing.

[denis-bogdanas]

Hi, I have a related question. Is num256 overflow protected or not?

This document says:
https://github.com/ethereum/vyper/tree/master/examples/tokens/ERC20_solidity_compatible
"Notably, num256 is not overflow protected".

Viper reference says nothing on the topic. This issue suggests it is protected. Conflicting data.

thanks,

[DavidKnott]

@denis-bogdanas Thanks a lot for bringing this up! num256 overflow protection has been implemented and is tested here. Would you mind making a PR for the ERC20_solidity_compatible Readme correcting this?

[denis-bogdanas]

I guess it's better for someone more knowledgeable to review that document. There are probably more things to update.