Moved BitcoinTxBuilder property tests into ScalaTest context, fixed bug where opPushData was marking valid short P2SH scripts as invalid (#900)

core/src/main/scala/org/bitcoins/core/script/constant/ConstantInterpreter.scala

@@ -118,44 +118,31 @@ sealed abstract class ConstantInterpreter {
     */
   private def opPushData(
       program: ExecutionInProgressScriptProgram): StartedScriptProgram = {
-    //for the case when we have the minimal data flag and the bytes to push onto stack is represented by the
-    //constant telling OP_PUSHDATA how many bytes need to go onto the stack
-    //for instance OP_PUSHDATA1 OP_0
-    val scriptNumOp = if (program.script(1).bytes.nonEmpty) {
-      val h = program.script(1).bytes.head
-      ScriptNumberOperation.fromNumber(h.toInt)
-    } else {
-      None
+    //for the case where we have to push 0 bytes onto the stack, which is technically the empty byte vector
+    def emptyPush(): StartedScriptProgram = {
+      if (ScriptFlagUtil.requireMinimalData(program.flags)) {
+        program.failExecution(ScriptErrorMinimalData)
+      } else {
+        program.updateStackAndScript(ScriptNumber.zero :: program.stack,
+                                     program.script.tail.tail)
+      }
     }
 
-    if (ScriptFlagUtil.requireMinimalData(program.flags) && program
-          .script(1)
-          .bytes
-          .size == 1 &&
-        scriptNumOp.isDefined) {
-      logger.error(
-        "We cannot use an OP_PUSHDATA operation for pushing " +
-          "a script number operation onto the stack, scriptNumberOperation: " + scriptNumOp)
-      program.failExecution(ScriptErrorMinimalData)
-    } else if (ScriptFlagUtil.requireMinimalData(program.flags) && program.script.size > 2 &&
-               !BitcoinScriptUtil.isMinimalPush(program.script.head,
-                                                program.script(2))) {
-      logger.error(
-        "We are not using the minimal push operation to push the bytes onto the stack for the constant")
-      program.failExecution(ScriptErrorMinimalData)
-    } else {
-      //for the case where we have to push 0 bytes onto the stack, which is technically the empty byte vector
-      program.script(1) match {
-        case OP_0 | BytesToPushOntoStack.zero | ScriptNumber.zero |
-            ScriptNumber.negativeZero =>
-          if (ScriptFlagUtil.requireMinimalData(program.flags))
-            program.failExecution(ScriptErrorMinimalData)
-          else
-            program.updateStackAndScript(ScriptNumber.zero :: program.stack,
-                                         program.script.tail.tail)
-        case _: ScriptToken =>
-          pushScriptNumberBytesToStack(program.updateScript(program.script))
-      }
+    program.script(1) match {
+      case OP_0 | BytesToPushOntoStack.zero | ScriptNumber.zero |
+          ScriptNumber.negativeZero =>
+        emptyPush()
+      case token: ScriptConstant if token.bytes.toSeq.forall(_ == 0.toByte) =>
+        emptyPush()
+      case _: ScriptToken =>
+        if (ScriptFlagUtil.requireMinimalData(program.flags) && program.script.size > 2 && !BitcoinScriptUtil
+              .isMinimalPush(program.script.head, program.script(2))) {
+          logger.error(
+            s"Non-minimal push where minimal push was required: $program")
+          program.failExecution(ScriptErrorMinimalData)
+        } else {
+          pushScriptNumberBytesToStack(program)
+        }
     }
   }
 

testkit/src/main/scala/org/bitcoins/testkit/util/BitcoinSAsyncTest.scala

@@ -6,9 +6,12 @@ import akka.util.Timeout
 import org.bitcoins.core.config.{NetworkParameters, RegTest}
 import org.bitcoins.core.protocol.blockchain.ChainParams
 import org.bitcoins.core.util.BitcoinSLogger
+import org.scalacheck.Shrink
+import org.scalactic.anyvals.PosInt
 import org.scalatest._
 import org.scalatest.concurrent.AsyncTimeLimitedTests
 import org.scalatest.time.Span
+import org.scalatestplus.scalacheck.ScalaCheckPropertyChecks
 
 import scala.concurrent.ExecutionContext
 import scala.concurrent.duration.DurationInt
@@ -21,6 +24,7 @@ trait BaseAsyncTest
     extends BeforeAndAfter
     with BeforeAndAfterAll
     with MustMatchers
+    with ScalaCheckPropertyChecks
     with AsyncTimeLimitedTests
     with BitcoinSLogger { this: AsyncTestSuite =>
 
@@ -43,9 +47,49 @@ trait BaseAsyncTest
 
   override lazy val timeLimit: Span = 5.minutes
 
+  /** This def ensures that shrinks are disabled for all calls to forAll.
+    *
+    * If you want to enable shrinking for a specific test, introduce an
+    * implicit val into that scope with type Shrink[T] where T is the type
+    * of the generator you want to enable shrinking on.
+    */
+  implicit def noShrink[T]: Shrink[T] = Shrink.shrinkAny[T]
+
   override def afterAll: Unit = {
     TestKit.shutdownActorSystem(system, verifySystemShutdown = true)
   }
+
+  /** The configuration for property based tests in our testing suite
+    * @see http://www.scalatest.org/user_guide/writing_scalacheck_style_properties
+    */
+  implicit override val generatorDrivenConfig: PropertyCheckConfiguration = {
+    generatorDriveConfigOldCode
+  }
+
+  /** Sets the generator driven tests to perform the given amount of execs */
+  def customGenDrivenConfig(executions: Int): PropertyCheckConfiguration = {
+    PropertyCheckConfiguration(
+      minSuccessful = PosInt.from(executions).get,
+      minSize = PosInt.from(executions).get,
+      workers = 2
+    )
+  }
+
+  /** Property based tests that have been around a long time
+    * have a less of a chance failing, so execute them less
+    * @return
+    */
+  def generatorDriveConfigOldCode: PropertyCheckConfiguration = {
+    customGenDrivenConfig(BitcoinSUnitTest.OLD_CODE_EXECUTIONS)
+  }
+
+  /** Property based tests that are new have a higher chance of failing
+    * so execute them more
+    * @return
+    */
+  def generatorDrivenConfigNewCode: PropertyCheckConfiguration = {
+    customGenDrivenConfig(BitcoinSUnitTest.NEW_CODE_EXECUTIONS)
+  }
 }
 
 /** A trait that uses [[AsyncFlatSpec]] to execute tests

testkit/src/main/scala/org/bitcoins/testkit/util/BitcoinSUnitTest.scala

@@ -1,11 +1,13 @@
 package org.bitcoins.testkit.util
 
 import org.bitcoins.core.util.BitcoinSLogger
+import org.scalacheck.Shrink
 import org.scalactic.anyvals.PosInt
 import org.scalatest.concurrent.TimeLimitedTests
 import org.scalatest.time.Span
 import org.scalatestplus.scalacheck.ScalaCheckPropertyChecks
 import org.scalatest.{FlatSpec, MustMatchers}
+
 import scala.concurrent.duration.DurationInt
 
 /** A wrapper for boiler plate testing procesures in bitcoin-s */
@@ -18,6 +20,14 @@ abstract class BitcoinSUnitTest
 
   override val timeLimit: Span = 120.seconds
 
+  /** This def ensures that shrinks are disabled for all calls to forAll.
+    *
+    * If you want to enable shrinking for a specific test, introduce an
+    * implicit val into that scope with type Shrink[T] where T is the type
+    * of the generator you want to enable shrinking on.
+    */
+  implicit def noShrink[T]: Shrink[T] = Shrink.shrinkAny[T]
+
   /** The configuration for property based tests in our testing suite
     * @see http://www.scalatest.org/user_guide/writing_scalacheck_style_properties
     */
@@ -52,7 +62,7 @@ abstract class BitcoinSUnitTest
 
 }
 
-private object BitcoinSUnitTest {
+object BitcoinSUnitTest {
 
   /** The number of times new code
     * should be executed in a property based test