Catch stack underflow (#33)

* Stack underflow: Add failing test to catch #31

* ensurePop, use proc instead of template + add comment for future refactoring of popInternal

* Check stack underflows before popping values

* run json tests again

VMTests.md

@@ -132,7 +132,7 @@ VMTests
 + mul5.json                                                       OK
 + mul6.json                                                       OK
 + mul7.json                                                       OK
-- mulUnderFlow.json                                               Fail
++ mulUnderFlow.json                                               OK
 + mulmod0.json                                                    OK
 + mulmod1.json                                                    OK
 + mulmod1_overflow.json                                           OK
@@ -163,7 +163,7 @@ VMTests
 + sdivByZero0.json                                                OK
 + sdivByZero1.json                                                OK
 + sdivByZero2.json                                                OK
-- sdiv_dejavu.json                                                Fail
++ sdiv_dejavu.json                                                OK
 + sdiv_i256min.json                                               OK
 + sdiv_i256min2.json                                              OK
 + sdiv_i256min3.json                                              OK
@@ -198,7 +198,7 @@ VMTests
 + sub3.json                                                       OK
 + sub4.json                                                       OK
 ```
-OK: 186/195 Fail: 8/195 Skip: 1/195
+OK: 188/195 Fail: 6/195 Skip: 1/195
 ## vmBitwiseLogicOperation
 ```diff
 + and0.json                                                       OK

src/vm/stack.nim

@@ -75,28 +75,31 @@ proc push*(stack: var Stack, value: Bytes) =
   stack.values.add(value.toType(UInt256))
 
 proc internalPop(stack: var Stack, numItems: int): seq[UInt256] =
-  if len(stack) < numItems:
+  # TODO: it is very inefficient to allocate a seq
-    result = @[]
+  assert  numItems <= stack.len
-  else:
   result = stack.values[^numItems .. ^1]
   stack.values = stack.values[0 ..< ^numItems]
 
 proc internalPop(stack: var Stack, numItems: int, T: typedesc): seq[T] =
+  # TODO: it is very inefficient to allocate a seq
+
+  assert  numItems <= stack.len
   result = @[]
-  if len(stack) < numItems:
-    return
 
   for z in 0 ..< numItems:
     var value = stack.values.pop()
     result.add(toType(value, T))
 
-template ensurePop(elements: untyped, a: untyped): untyped =
+proc ensurePop(elements: seq|Stack, a: int) =
-  if len(`elements`) < `a`:
+  let num = elements.len
-    raise newException(InsufficientStack, "No stack items")
+  let expected = a
+  if num < expected:
+    raise newException(InsufficientStack,
+      &"Stack underflow: expected {expected} elements, got {num} instead.")
 
 proc popInt*(stack: var Stack): UInt256 =
+  ensurePop(stack, 1)
   var elements = stack.internalPop(1, UInt256)
-  ensurePop(elements, 1)
   result = elements[0]
 
 macro internalPopTuple(numItems: static[int]): untyped =
@@ -110,6 +113,8 @@ macro internalPopTuple(numItems: static[int]): untyped =
   result = quote:
     proc `name`*(`stackNode`: var Stack, `t`: typedesc): `typ`
   result[^1] = nnkStmtList.newTree()
+  result[^1].add quote do:
+    ensurePop(`stackNode`, `numItems`)
   for z in 0 ..< numItems:
     var zNode = newLit(z)
     var element = quote:

tests/test_stack.nim

@@ -93,3 +93,12 @@ suite "stack":
     var stack = newStack()
     expect(InsufficientStack):
       stack.dup(0)
+
+  test "binary operations raises InsufficientStack appropriately":
+    # https://github.com/status-im/nimbus/issues/31
+    # ./tests/fixtures/VMTests/vmArithmeticTest/mulUnderFlow.json
+
+    var stack = newStack()
+    stack.push(123)
+    expect(InsufficientStack):
+      discard stack.popInt(2)