From b70580de5aa321634c4ae0cbc13784405c0cefa9 Mon Sep 17 00:00:00 2001 From: Chris Stewart Date: Tue, 2 Feb 2016 17:14:39 -0600 Subject: [PATCH] Fixed control operations issue where OP_ELSE expression wasn't being removed if the previous OP_ELSE was evaluated --- .../ControlOperationsInterpreter.scala | 64 ++++++++++----- .../ControlOperationsInterpreterTest.scala | 81 ++++++++++++++----- .../interpreter/ScriptInterpreterTest.scala | 2 + 3 files changed, 107 insertions(+), 40 deletions(-) diff --git a/src/main/scala/org/scalacoin/script/control/ControlOperationsInterpreter.scala b/src/main/scala/org/scalacoin/script/control/ControlOperationsInterpreter.scala index fb8b9bd1b3..a009ccd8b3 100644 --- a/src/main/scala/org/scalacoin/script/control/ControlOperationsInterpreter.scala +++ b/src/main/scala/org/scalacoin/script/control/ControlOperationsInterpreter.scala @@ -59,7 +59,24 @@ trait ControlOperationsInterpreter { */ def opElse(stack : List[ScriptToken], script : List[ScriptToken]) : (List[ScriptToken], List[ScriptToken]) = { require(script.headOption.isDefined && script.head == OP_ELSE, "First script opt must be OP_ELSE") - (stack,script.tail) + val tree = parseBinaryTree(script) + println("Parsed tree: " + tree) + val treeWithNextOpElseRemoved = tree match { + case Empty => Empty + case leaf : Leaf[ScriptToken] => leaf + case node : Node[ScriptToken] => + if (node.r.value == Some(OP_ELSE)) { + val replacementTree = node.r.left.getOrElse(Empty).findFirstDFS[ScriptToken](OP_ENDIF)().getOrElse(Empty) + val replacementNode = replacementTree match { + case Empty => Empty + case leaf : Leaf[ScriptToken] => Node(leaf.v, Empty, node.r.right.getOrElse(Empty)) + case node1 : Node[ScriptToken] => Node(node1.v,node1.l,node.r.right.getOrElse(Empty)) + } + Node(node.v,node.l,replacementNode) + } + else node + } + (stack,treeWithNextOpElseRemoved.toList.tail) } @@ -122,8 +139,8 @@ trait ControlOperationsInterpreter { */ @tailrec private def loop(script : List[ScriptToken], tree : BinaryTree[ScriptToken]) : BinaryTree[ScriptToken] = { - logger.debug("Script : " + script) - logger.debug("Tree: " + tree) +/* logger.debug("Script : " + script) + logger.debug("Tree: " + tree)*/ script match { case OP_IF :: t => val (newTail, parsedTree) = parseOpIf(script, Empty) @@ -131,7 +148,6 @@ trait ControlOperationsInterpreter { loop(newTail, newTree) case OP_ELSE :: t => val (newTail, parsedTree) = parseOpElse(script, Empty) - println("parsed tree: " + parsedTree) val newTree = insertSubTree(tree,parsedTree) loop(newTail, newTree) case OP_ENDIF :: t => @@ -156,7 +172,7 @@ trait ControlOperationsInterpreter { */ private def insertSubTree(tree : BinaryTree[ScriptToken],subTree : BinaryTree[ScriptToken]) : BinaryTree[ScriptToken] = { //TODO: Optimize this to a tailrec function - logger.debug("Inserting subTree: " + subTree + " into tree: " + tree) + //logger.debug("Inserting subTree: " + subTree + " into tree: " + tree) tree match { case Empty => subTree case leaf : Leaf[ScriptToken] => Node(leaf.v, subTree, Empty) @@ -339,22 +355,28 @@ trait ControlOperationsInterpreter { * @return */ def removeFirstOpElse(tree : BinaryTree[ScriptToken]) : BinaryTree[ScriptToken] = { - //need to traverse the tree to see if there is an OP_ENDIF on the left hand side - val leftBranchContainsOpElse = if (tree.left.isDefined) tree.left.get.contains[ScriptToken](OP_ELSE)() else false - val leftBranchContainsOpIf = if (tree.left.isDefined) tree.left.get.contains[ScriptToken](OP_IF)() else false - logger.debug("Tree contains OP_ENDIF: " + leftBranchContainsOpElse) - if (leftBranchContainsOpElse && !leftBranchContainsOpIf) { - //if the left branch contains an OP_ELSE but no OP_IF - //then we need to delete the OP_ELSE in the left branch - val subTree: Option[BinaryTree[ScriptToken]] = tree.left.get.findFirstDFS[ScriptToken](OP_ELSE)() - logger.debug("Sub tree: " + subTree) - //need to remove the subtree for the OP_ELSE - //need to insert the right branch of the subtree into the original place of the OP_ELSE - if (subTree.isDefined) tree.replace(subTree.get, subTree.get.right.getOrElse(Empty)) - else tree - } else if (tree.right.isDefined && tree.right.get.value == Some(OP_ELSE)) { - Node(tree.value.get,tree.left.getOrElse(Empty),tree.right.get.right.getOrElse(Empty)) - } else tree + + tree match { + case Empty => Empty + case leaf : Leaf[ScriptToken] => leaf + case node : Node[ScriptToken] => + //need to traverse the tree to see if there is an OP_ENDIF on the left hand side + val leftBranchContainsOpElse = node.l.contains[ScriptToken](OP_ELSE)() + val leftBranchContainsOpIf = node.l.contains[ScriptToken](OP_IF)() + if (leftBranchContainsOpElse && !leftBranchContainsOpIf) { + //if the left branch contains an OP_ELSE but no OP_IF + //then we need to delete the OP_ELSE in the left branch + val subTree: Option[BinaryTree[ScriptToken]] = node.l.findFirstDFS[ScriptToken](OP_ELSE)() + logger.debug("Sub tree: " + subTree) + //need to remove the subtree for the OP_ELSE + //need to insert the right branch of the subtree into the original place of the OP_ELSE + if (subTree.isDefined) tree.replace(subTree.get, subTree.get.right.getOrElse(Empty)) + else tree + } else if (node.r.value == Some(OP_ELSE)) { + logger.debug("============================**********************************") + Node(node.v,node.l,node.r.right.getOrElse(Empty)) + } else tree + } } diff --git a/src/test/scala/org/scalacoin/script/control/ControlOperationsInterpreterTest.scala b/src/test/scala/org/scalacoin/script/control/ControlOperationsInterpreterTest.scala index d1f84bdee2..5f5970c52b 100644 --- a/src/test/scala/org/scalacoin/script/control/ControlOperationsInterpreterTest.scala +++ b/src/test/scala/org/scalacoin/script/control/ControlOperationsInterpreterTest.scala @@ -4,7 +4,7 @@ import org.scalacoin.script.arithmetic.OP_ADD import org.scalacoin.script.bitwise.OP_EQUAL import org.scalacoin.script.constant._ import org.scalacoin.script.reserved.{OP_VER, OP_RESERVED} -import org.scalacoin.util.Leaf +import org.scalacoin.util.{Empty, Node, Leaf} import org.scalatest.{MustMatchers, FlatSpec} /** @@ -85,10 +85,10 @@ class ControlOperationsInterpreterTest extends FlatSpec with MustMatchers with C removeFirstOpElse(List(OP_IF, OP_1,OP_ELSE, OP_2, OP_ELSE, OP_3, OP_ENDIF)) must be (List(OP_IF, OP_1, OP_ELSE, OP_3, OP_ENDIF)) } -/* it must "remove the first OP_ELSE in a binary tree" in { + it must "remove the first OP_ELSE in a binary tree" in { val script1 = List(OP_IF,OP_ELSE,OP_ENDIF) val bTree1 = parseBinaryTree(script1) - removeFirstOpElse(bTree1).toSeq must be (List(OP_IF,OP_ENDIF)) + removeFirstOpElse(bTree1).toSeq must be (List(OP_IF)) val script2 = List(OP_IF,OP_ENDIF) val bTree2 = parseBinaryTree(script2) @@ -97,7 +97,7 @@ class ControlOperationsInterpreterTest extends FlatSpec with MustMatchers with C val script3 = List(OP_IF, OP_1,OP_ELSE, OP_2, OP_ELSE, OP_3, OP_ENDIF) val bTree3 = parseBinaryTree(script3) removeFirstOpElse(bTree3).toSeq must be (List(OP_IF, OP_1, OP_ELSE, OP_3, OP_ENDIF)) - }*/ + } it must "find a matching OP_ENDIF for an OP_IF" in { //https://gist.github.com/Christewart/381dc1dbbb07e62501c3 @@ -229,6 +229,8 @@ class ControlOperationsInterpreterTest extends FlatSpec with MustMatchers with C } + + it must "evaluate an OP_IF correctly" in { val stack = List(OP_0) val script = List(OP_IF, OP_RESERVED, OP_ENDIF, OP_1) @@ -237,7 +239,6 @@ class ControlOperationsInterpreterTest extends FlatSpec with MustMatchers with C newScript must be (List(OP_ENDIF,OP_1)) } - it must "evaluate an OP_IF OP_ELSE OP_ENDIF block" in { val stack = List(OP_0) val script = List(OP_IF, OP_VER, OP_ELSE, OP_1, OP_ENDIF) @@ -259,8 +260,6 @@ class ControlOperationsInterpreterTest extends FlatSpec with MustMatchers with C checkMatchingOpIfOpEndIf(script3) must be (true) } - - it must "evaluate an OP_IF block correctly if the stack top is true" in { val stack = List(OP_1) val script = List(OP_IF, OP_1, OP_ELSE, OP_0, OP_ENDIF) @@ -270,17 +269,6 @@ class ControlOperationsInterpreterTest extends FlatSpec with MustMatchers with C newScript must be (List(OP_1)) } - it must "remove the first OP_ELSE if the stack top is true for an OP_IF" in { - val stack = List(ScriptNumberImpl(1)) - val script = List(OP_IF, OP_1, OP_ELSE, OP_RETURN, OP_ELSE, OP_1, OP_ENDIF, OP_ELSE, OP_RETURN, OP_ENDIF, - OP_ADD, OP_2, OP_EQUAL) - - val (newStack,newScript) = opIf(stack,script) - - newStack.isEmpty must be (true) - newScript must be (List(OP_1,OP_ELSE, OP_1, OP_ENDIF, OP_ELSE, OP_RETURN, OP_ENDIF, OP_ADD, OP_2, OP_EQUAL)) - } - it must "evaluate a weird case using multiple OP_ELSEs" in { val stack = List(ScriptNumberImpl(1)) val script = List(OP_IF, OP_ELSE, OP_0, OP_ELSE, OP_1, OP_ENDIF) @@ -320,12 +308,67 @@ class ControlOperationsInterpreterTest extends FlatSpec with MustMatchers with C newStack.isEmpty must be (true) newScript must be (List(OP_ELSE, OP_1,OP_IF,OP_1,OP_ELSE, - OP_RETURN,OP_ELSE,OP_1,OP_ENDIF, OP_ELSE,OP_RETURN,OP_ENDIF,OP_ADD,OP_2,OP_EQUAL)) + OP_RETURN,OP_ELSE,OP_1,OP_ENDIF,OP_ELSE, OP_RETURN,OP_ENDIF,OP_ADD,OP_2,OP_EQUAL)) + + + val stack1 = newStack + val script1 = newScript + + val (newStack1,newScript1) = opElse(stack1,script1) + newStack1.isEmpty must be (true) + newScript1 must be (List(OP_1,OP_IF,OP_1,OP_ELSE, + OP_RETURN,OP_ELSE,OP_1,OP_ENDIF,OP_ENDIF,OP_ADD,OP_2,OP_EQUAL)) } + it must "remove the first OP_ELSE if the stack top is true for an OP_IF" in { + val stack = List(ScriptNumberImpl(1)) + val script = List(OP_IF, OP_1, OP_ELSE, OP_RETURN, OP_ELSE, OP_1, OP_ENDIF, OP_ELSE, OP_RETURN, OP_ENDIF, + OP_ADD, OP_2, OP_EQUAL) + + val (newStack,newScript) = opIf(stack,script) + + newStack.isEmpty must be (true) + newScript must be (List(OP_1,OP_ELSE, OP_1, OP_ENDIF, OP_ELSE, OP_RETURN, OP_ENDIF, OP_ADD, OP_2, OP_EQUAL)) + } + + it must "evaluate an OP_ENDIF correctly" in { + val stack = List(ScriptNumberImpl(1), ScriptNumberImpl(1)) + val script = List(OP_ENDIF, OP_ELSE, OP_RETURN, OP_ENDIF, OP_ADD, OP_2, OP_EQUAL) + + val (newStack,newScript) = opEndIf(stack,script) + + newStack must be (stack) + newScript must be (script.tail) + } + + + it must "parse a partial script correctly" in { + val script = List(OP_IF, OP_1, OP_ELSE, OP_RETURN, OP_ELSE, OP_1, OP_ENDIF, OP_ELSE, OP_RETURN, OP_ENDIF, + OP_ADD, OP_2, OP_EQUAL) + + val bTree = parseBinaryTree(script) + bTree.value must be (Some(OP_IF)) + + bTree.left.get.value must be (Some(OP_1)) + + + bTree.right.get.value must be (Some(OP_ELSE)) + + bTree.right.get.left.get.value must be (Some(OP_RETURN)) + + bTree.right.get.right.get.value must be (Some(OP_ELSE)) + + bTree.right.get.right.get.left.get.value must be (Some(OP_1)) + + bTree.right.get.right.get.left.get.left.get.value must be (Some(OP_ENDIF)) + + bTree.right.get.right.get.left.get.left.get.left.get.value must be (Some(OP_ELSE)) + + } + } diff --git a/src/test/scala/org/scalacoin/script/interpreter/ScriptInterpreterTest.scala b/src/test/scala/org/scalacoin/script/interpreter/ScriptInterpreterTest.scala index 48e6f48daa..868e0463ef 100644 --- a/src/test/scala/org/scalacoin/script/interpreter/ScriptInterpreterTest.scala +++ b/src/test/scala/org/scalacoin/script/interpreter/ScriptInterpreterTest.scala @@ -38,6 +38,8 @@ class ScriptInterpreterTest extends FlatSpec with MustMatchers with ScriptInterp } + + it must "evaluate all valid scripts from the bitcoin core script_valid.json" in { import CoreTestCaseProtocol._