Rework P2SHScriptSignature.isStandardNonP2SH() (#2963)

* Take ben's suggestion for parsing the entire tx hex

* Add assertion that the secondSPK is NOT a P2SHScriptPubKey, revert the classification of P2SHScriptPubKey in P2SHScriptSignature.isStandardNonP2SH()

* Revert conditionalSPK match logic

* Fixed ConditionalScriptPubKeyTest.scala (#32)

Co-authored-by: Nadav Kohen <nadavk25@gmail.com>

core-test/src/test/scala/org/bitcoins/core/protocol/script/ConditionalScriptPubKeyTest.scala

@@ -1,5 +1,6 @@
 package org.bitcoins.core.protocol.script
 
+import org.bitcoins.core.protocol.transaction.Transaction
 import org.bitcoins.core.script.constant.ScriptConstant
 import org.bitcoins.testkitcore.util.BitcoinSJvmTest
 
@@ -27,10 +28,22 @@ class ConditionalScriptPubKeyTest extends BitcoinSJvmTest {
       "632103184b16f5d6c01e2d6ded3f8a292e5b81608318ecb8e93aa3747bc88b8dbf256cac67a914f5862841f254a1483eab66909ae588e45d617c5e8768"
     val scriptPubKey = RawScriptPubKey.fromAsmHex(hex)
 
-    assert(scriptPubKey.isInstanceOf[IfConditionalScriptPubKey])
+    scriptPubKey match {
-
+      case conditional: IfConditionalScriptPubKey =>
+        assert(
+          conditional.secondSPK.isInstanceOf[NonStandardScriptPubKey],
+          s"Although we have same op codes as p2sh spk, " +
+            s"this combination isn't used in a standalone output, rather a redeemScript"
+        )
+      case x => fail(s"Incorrect type for the spk, got=$x")
+    }
     val constant = ScriptConstant.fromHex(hex)
 
     assert(P2SHScriptSignature.isRedeemScript(constant))
+
+    //we should be able to parse the tx that contains it with no problem
+    val txHex =
+      "010000000193983cb2f1f3d83615f10e1ca33f49c72250acbebd840793aaa8bc7a6b28b76b000000008848304502207ffb30631d837895ac1b415408b70a809090b25d4070198043299fe247f62d2602210089b52f0d243dea1e4313ef67723b0d0642ff77bb6ca05ea85296b2539c797f5c81513d632103184b16f5d6c01e2d6ded3f8a292e5b81608318ecb8e93aa3747bc88b8dbf256cac67a914f5862841f254a1483eab66909ae588e45d617c5e8768ffffffff01a0860100000000001976a914a07aa8415d34d0db44f220ff9522609641c0bfbf88ac00000000"
+    assert(Transaction.fromHexT(txHex).isSuccess)
   }
 }

core/src/main/scala/org/bitcoins/core/protocol/script/ScriptSignature.scala

@@ -6,6 +6,7 @@ import org.bitcoins.core.util._
 import org.bitcoins.core.wallet.utxo.ConditionalPath
 import org.bitcoins.crypto.{ECDigitalSignature, ECPublicKey}
 
+import scala.annotation.tailrec
 import scala.util.Try
 
 /** Created by chris on 12/26/15.
@@ -265,50 +266,25 @@ object P2SHScriptSignature extends ScriptFactory[P2SHScriptSignature] {
     }
   }
 
+  @tailrec
   private def isStandardNonP2SH(
       spk: ScriptPubKey,
       isRecursiveCall: Boolean): Boolean = {
     spk match {
       case _: P2PKHScriptPubKey | _: MultiSignatureScriptPubKey |
           _: P2PKScriptPubKey | _: P2PKWithTimeoutScriptPubKey |
-          _: WitnessScriptPubKeyV0 | _: UnassignedWitnessScriptPubKey =>
+          _: UnassignedWitnessScriptPubKey | _: WitnessScriptPubKeyV0 |
-        true
+          _: ConditionalScriptPubKey => // Conditional SPKs are not recursively checked
-      case _: P2SHScriptPubKey =>
         true
       case EmptyScriptPubKey => isRecursiveCall // Fine if nested
-      case conditional: ConditionalScriptPubKey =>
-        val first =
-          isStandardNonP2SH(conditional.firstSPK, isRecursiveCall = true)
-
-        val second =
-          isStandardNonP2SH(conditional.secondSPK, isRecursiveCall = true)
-
-        //we need to see if we have a p2sh scriptpubkey nested
-        //inside of the conditional spk. This can actually happen
-        //when you literally just want to use the script OP_HASH160 <bytes> OP_EQUAL
-        //independent of the normal p2sh flow
-        //see: https://github.com/bitcoin-s/bitcoin-s/issues/2962
-        (first, second) match {
-          case (true, true) => true
-          case (true, false) =>
-            P2SHScriptPubKey.isValidAsm(conditional.secondSPK.asm)
-          case (false, true) =>
-            P2SHScriptPubKey.isValidAsm(conditional.firstSPK.asm)
-          case (false, false) =>
-            val isP2SHFirst =
-              P2SHScriptPubKey.isValidAsm(conditional.firstSPK.asm)
-            val isP2SHSecond =
-              P2SHScriptPubKey.isValidAsm(conditional.secondSPK.asm)
-
-            isP2SHFirst && isP2SHSecond
-        }
       case locktime: LockTimeScriptPubKey =>
         if (Try(locktime.locktime).isSuccess) {
           isStandardNonP2SH(locktime.nestedScriptPubKey, isRecursiveCall = true)
         } else {
           false
         }
-      case _: NonStandardScriptPubKey | _: WitnessCommitment =>
+      case _: NonStandardScriptPubKey | _: WitnessCommitment |
+          _: P2SHScriptPubKey =>
         false
     }
   }