From 62bb36928ecc6eded329c68e7ae21d084d8e256e Mon Sep 17 00:00:00 2001 From: Chris Stewart Date: Tue, 15 Oct 2019 09:57:47 -0500 Subject: [PATCH] =?UTF-8?q?Update=20script=5Ftests.json,=20fix=20bugs=20th?= =?UTF-8?q?at=20were=20unveiled=20with=20that=20updat=E2=80=A6=20(#799)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Update script_tests.json, fix bugs that were unveiled with that update. Specifically around handling negative zero in Script, and cleanstack behavior for segwit scripts. We were not checking for equality at the byte level for negative zero. With cleanstack, we were failing the script, but not failing with the correct error. If you look at interpreter.cpp in bitcoin core, cleanstack check is done before checking if the stacktop is true or false * Address code review --- .../interpreter/ScriptInterpreterTest.scala | 31 +++++-------------- .../arithmetic/ArithmeticInterpreter.scala | 13 ++++---- .../script/constant/ScriptNumberUtil.scala | 10 ++++++ .../interpreter/ScriptInterpreter.scala | 7 +++-- 4 files changed, 29 insertions(+), 32 deletions(-) diff --git a/core-test/src/test/scala/org/bitcoins/core/script/interpreter/ScriptInterpreterTest.scala b/core-test/src/test/scala/org/bitcoins/core/script/interpreter/ScriptInterpreterTest.scala index 7ca23ad473..539d0020a0 100644 --- a/core-test/src/test/scala/org/bitcoins/core/script/interpreter/ScriptInterpreterTest.scala +++ b/core-test/src/test/scala/org/bitcoins/core/script/interpreter/ScriptInterpreterTest.scala @@ -1,21 +1,15 @@ package org.bitcoins.core.script.interpreter -import org.bitcoins.core.crypto.{ - BaseTxSigComponent, - WitnessTxSigComponentP2SH, - WitnessTxSigComponentRaw -} +import org.bitcoins.core.crypto.{BaseTxSigComponent, WitnessTxSigComponentP2SH, WitnessTxSigComponentRaw} import org.bitcoins.core.currency.CurrencyUnits import org.bitcoins.core.protocol.script._ -import org.bitcoins.core.protocol.transaction.{ - TransactionOutput, - WitnessTransaction -} +import org.bitcoins.core.protocol.transaction.{TransactionOutput, WitnessTransaction} import org.bitcoins.core.script.PreExecutionScriptProgram import org.bitcoins.core.script.flag.ScriptFlagFactory import org.bitcoins.core.script.interpreter.testprotocol.CoreTestCase import org.bitcoins.core.script.interpreter.testprotocol.CoreTestCaseProtocol._ import org.bitcoins.core.util._ +import org.bitcoins.testkit.util.BitcoinSUnitTest import org.scalatest.{FlatSpec, MustMatchers} import org.slf4j.LoggerFactory import spray.json._ @@ -26,27 +20,16 @@ import scala.util.Try /** * Created by chris on 1/6/16. */ -class ScriptInterpreterTest extends FlatSpec with MustMatchers { - private val logger = LoggerFactory.getLogger(this.getClass) +class ScriptInterpreterTest extends BitcoinSUnitTest { + "ScriptInterpreter" must "evaluate all the scripts from the bitcoin core script_tests.json" in { val source = Source.fromURL(getClass.getResource("/script_tests.json")) //use this to represent a single test case from script_valid.json - /* val lines = +/* val lines = """ - | [[ - | [ - | "304402200929d11561cd958460371200f82e9cae64c727a495715a31828e27a7ad57b36d0220361732ced04a6f97351ecca21a56d0b8cd4932c1da1f8f569a2b68e5e48aed7801", - | "0479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8", - | 0.00000001 - | ], - | "0x16 0x001491b24bf9f5288532960ac687abb035127b1d28a5", - | "HASH160 0x14 0x17743beb429c55c942d2ec703b98c4d57c2df5c6 EQUAL", - | "P2SH,WITNESS", - | "OK", - | "Basic P2SH(P2WPKH)" - |]] + | [["0x01 0x80", "DUP BOOLOR", "P2SH,STRICTENC", "EVAL_FALSE", "negative-0 negative-0 BOOLOR"]] """.stripMargin*/ val lines = try source.getLines.filterNot(_.isEmpty).map(_.trim) mkString "\n" diff --git a/core/src/main/scala/org/bitcoins/core/script/arithmetic/ArithmeticInterpreter.scala b/core/src/main/scala/org/bitcoins/core/script/arithmetic/ArithmeticInterpreter.scala index f420d00a82..516c887057 100644 --- a/core/src/main/scala/org/bitcoins/core/script/arithmetic/ArithmeticInterpreter.scala +++ b/core/src/main/scala/org/bitcoins/core/script/arithmetic/ArithmeticInterpreter.scala @@ -98,11 +98,12 @@ sealed abstract class ArithmeticInterpreter { program: ExecutionInProgressScriptProgram): StartedScriptProgram = { require(program.script.headOption.contains(OP_BOOLAND), "Script top must be OP_BOOLAND") - performBinaryBooleanOperation(program, (x, y) => { - val xIsFalse = (x == ScriptNumber.zero || x == OP_0) - val yIsFalse = (y == ScriptNumber.zero || y == OP_0) - if (xIsFalse || yIsFalse) false else true - }) + performBinaryBooleanOperation( + program, + (x, y) => { + !ScriptNumberUtil.isZero(x) && !ScriptNumberUtil.isZero(y) + } + ) } /** If a or b is not 0, the output is 1. Otherwise 0. */ @@ -111,7 +112,7 @@ sealed abstract class ArithmeticInterpreter { require(program.script.headOption.contains(OP_BOOLOR), "Script top must be OP_BOOLOR") performBinaryBooleanOperation(program, (x, y) => { - if (x == y && (x == ScriptNumber.zero || x == OP_0)) false else true + !ScriptNumberUtil.isZero(x) || !ScriptNumberUtil.isZero(y) }) } diff --git a/core/src/main/scala/org/bitcoins/core/script/constant/ScriptNumberUtil.scala b/core/src/main/scala/org/bitcoins/core/script/constant/ScriptNumberUtil.scala index 798d5bef8e..86d7ddbd56 100644 --- a/core/src/main/scala/org/bitcoins/core/script/constant/ScriptNumberUtil.scala +++ b/core/src/main/scala/org/bitcoins/core/script/constant/ScriptNumberUtil.scala @@ -161,6 +161,16 @@ trait ScriptNumberUtil { def firstByteAllZeros(hex: String): Boolean = firstByteAllZeros(BitcoinSUtil.decodeHex(hex)) + /** Checks if the two given [[ScriptNumber numbers]] are equivalent to zero + * in Script. Unfortunatey Script is one's complement which means we have + * things like negative zero, and also there isn't an enforcement of a + * minimal representation of zero, which means 0x00 = 0x0000 = 0x0000000.. == OP_0*/ + def isZero(x: ScriptNumber): Boolean = { + val xIsFalse = x == ScriptNumber.zero || x == OP_0 + val isNegZero = x == ScriptNumber.negativeZero + val isZero = x.toLong == 0 + xIsFalse || isNegZero || isZero + } } object ScriptNumberUtil extends ScriptNumberUtil diff --git a/core/src/main/scala/org/bitcoins/core/script/interpreter/ScriptInterpreter.scala b/core/src/main/scala/org/bitcoins/core/script/interpreter/ScriptInterpreter.scala index 0cefdb1615..e52f7fa7b2 100644 --- a/core/src/main/scala/org/bitcoins/core/script/interpreter/ScriptInterpreter.scala +++ b/core/src/main/scala/org/bitcoins/core/script/interpreter/ScriptInterpreter.scala @@ -74,7 +74,6 @@ sealed abstract class ScriptInterpreter extends BitcoinSLogger { executeProgram(scriptPubKeyProgram) logger.trace( s"scriptPubKeyExecutedProgram $scriptPubKeyExecutedProgram") - if (scriptSigExecutedProgram.error.isDefined) { scriptSigExecutedProgram } else if (scriptPubKeyExecutedProgram.error.isDefined || scriptPubKeyExecutedProgram.stackTopIsFalse) { @@ -382,7 +381,11 @@ sealed abstract class ScriptInterpreter extends BitcoinSLogger { evaluated: ExecutedScriptProgram): ExecutedScriptProgram = { logger.trace("Stack after evaluating witness: " + evaluated.stack) if (evaluated.error.isDefined) evaluated - else if (evaluated.stack.size != 1 || evaluated.stackTopIsFalse) + else if (evaluated.stack.size != 1) { + // Scripts inside witness implicitly require cleanstack behaviour + //https://github.com/bitcoin/bitcoin/blob/561a7d30478b82f5d46dcf0f16e864a9608004f4/src/script/interpreter.cpp#L1464 + evaluated.failExecution(ScriptErrorCleanStack) + } else if (evaluated.stackTopIsFalse) evaluated.failExecution(ScriptErrorEvalFalse) else evaluated }