From a7a38881db410535bb73a9e6ef6c6bd74696196f Mon Sep 17 00:00:00 2001 From: Chris Stewart Date: Tue, 27 Dec 2016 20:58:06 -0600 Subject: [PATCH] Refacotring NULLFAIL logic into one place --- .../crypto/TransactionSignatureChecker.scala | 35 +++++++--- .../TransactionSignatureCheckerResult.scala | 7 ++ .../script/crypto/CryptoInterpreter.scala | 66 +++++++++---------- .../TransactionSignatureCreatorSpec.scala | 1 + .../interpreter/ScriptInterpreterTest.scala | 12 +--- 5 files changed, 65 insertions(+), 56 deletions(-) diff --git a/src/main/scala/org/bitcoins/core/crypto/TransactionSignatureChecker.scala b/src/main/scala/org/bitcoins/core/crypto/TransactionSignatureChecker.scala index 2dd577223c..6d9c2fae2a 100644 --- a/src/main/scala/org/bitcoins/core/crypto/TransactionSignatureChecker.scala +++ b/src/main/scala/org/bitcoins/core/crypto/TransactionSignatureChecker.scala @@ -68,9 +68,10 @@ trait TransactionSignatureChecker extends BitcoinSLogger { TransactionSignatureSerializer.hashForSignature(w.transaction,w.inputIndex,sigsRemovedScript, hashType, w.amount,w.sigVersion) } - logger.info("Hash for signature: " + BitcoinSUtil.encodeHex(hashForSignature.bytes)) + logger.debug("Hash for signature: " + BitcoinSUtil.encodeHex(hashForSignature.bytes)) val isValid = pubKey.verify(hashForSignature,signature) - if (isValid) SignatureValidationSuccess else SignatureValidationFailureIncorrectSignatures + if (isValid) SignatureValidationSuccess + else nullFailCheck(Seq(signature),SignatureValidationFailureIncorrectSignatures, flags) } } @@ -94,15 +95,15 @@ trait TransactionSignatureChecker extends BitcoinSLogger { if (sigs.size > pubKeys.size) { //this is how bitcoin core treats this. If there are ever any more //signatures than public keys remaining we immediately return - //false https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L955-L959 + //false https://github.com/bitcoin/bitcoin/blob/8c1dbc5e9ddbafb77e60e8c4e6eb275a3a76ac12/src/script/interpreter.cpp#L943-L945 logger.warn("We have more sigs than we have public keys remaining") - SignatureValidationFailureIncorrectSignatures + nullFailCheck(sigs,SignatureValidationFailureIncorrectSignatures,flags) } else if (requiredSigs > sigs.size) { //for the case when we do not have enough sigs left to check to meet the required signature threshold - //https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L914-915 + //https://github.com/bitcoin/bitcoin/blob/8c1dbc5e9ddbafb77e60e8c4e6eb275a3a76ac12/src/script/interpreter.cpp#L990-L991 logger.warn("We do not have enough sigs to meet the threshold of requireSigs in the multiSignatureScriptPubKey") - SignatureValidationFailureSignatureCount + nullFailCheck(sigs,SignatureValidationFailureSignatureCount,flags) } else if (sigs.nonEmpty && pubKeys.nonEmpty) { val sig = sigs.head @@ -111,18 +112,34 @@ trait TransactionSignatureChecker extends BitcoinSLogger { result match { case SignatureValidationSuccess => multiSignatureEvaluator(txSignatureComponent, script, sigs.tail,pubKeys.tail,flags, requiredSigs - 1) - case SignatureValidationFailureIncorrectSignatures => + case SignatureValidationFailureIncorrectSignatures | SignatureValidationErrorNullFail => + //notice we pattern match on 'SignatureValidationErrorNullFail' here, this is because + //'checkSignature' may return that result, but we need to continue evaluating the signatures + //in the multisig script, we don't check for nullfail until evaluation the OP_CHECKMULTSIG is completely done multiSignatureEvaluator(txSignatureComponent, script, sigs, pubKeys.tail,flags, requiredSigs) case x @ (SignatureValidationFailureNotStrictDerEncoding | SignatureValidationFailureSignatureCount | SignatureValidationFailurePubKeyEncoding | ScriptValidationFailureHighSValue | ScriptValidationFailureHashType | ScriptValidationFailureWitnessPubKeyType) => - x + nullFailCheck(sigs,x,flags) } } else if (sigs.isEmpty) { //means that we have checked all of the sigs against the public keys //validation succeeds SignatureValidationSuccess - } else SignatureValidationFailureIncorrectSignatures + } else nullFailCheck(sigs,SignatureValidationFailureIncorrectSignatures,flags) + + + } + /** If the NULLFAIL flag is set as defined in BIP146, it checks to make sure all failed signatures were an empty byte vector + * [[https://github.com/bitcoin/bips/blob/master/bip-0146.mediawiki#NULLFAIL]] + * */ + private def nullFailCheck(sigs: Seq[ECDigitalSignature], result: TransactionSignatureCheckerResult,flags: Seq[ScriptFlag]): TransactionSignatureCheckerResult = { + logger.info("Result before nullfail check:" + result) + val nullFailEnabled = ScriptFlagUtil.requireScriptVerifyNullFail(flags) + if (nullFailEnabled && !result.isValid) { + //we need to check that all signatures were empty byte vectors, else this fails because of BIP146 and nullfail + if (sigs.exists(_.bytes.nonEmpty)) SignatureValidationErrorNullFail else result + } else result } } diff --git a/src/main/scala/org/bitcoins/core/crypto/TransactionSignatureCheckerResult.scala b/src/main/scala/org/bitcoins/core/crypto/TransactionSignatureCheckerResult.scala index 90d24585ab..763090f1f4 100644 --- a/src/main/scala/org/bitcoins/core/crypto/TransactionSignatureCheckerResult.scala +++ b/src/main/scala/org/bitcoins/core/crypto/TransactionSignatureCheckerResult.scala @@ -64,3 +64,10 @@ case object ScriptValidationFailureHashType extends SignatureValidationError /** Fails the script if the given public key was not compressed and the [[org.bitcoins.core.script.flag.ScriptVerifyWitnessPubKeyType]] * flag was set */ case object ScriptValidationFailureWitnessPubKeyType extends SignatureValidationError + +/** + * Fails the script if a an invalid signature is not an empty byte vector + * See BIP146 + * [[https://github.com/bitcoin/bips/blob/master/bip-0146.mediawiki#nullfail]] + */ +case object SignatureValidationErrorNullFail extends SignatureValidationError diff --git a/src/main/scala/org/bitcoins/core/script/crypto/CryptoInterpreter.scala b/src/main/scala/org/bitcoins/core/script/crypto/CryptoInterpreter.scala index 4566004172..d226622cd3 100644 --- a/src/main/scala/org/bitcoins/core/script/crypto/CryptoInterpreter.scala +++ b/src/main/scala/org/bitcoins/core/script/crypto/CryptoInterpreter.scala @@ -77,34 +77,30 @@ trait CryptoInterpreter extends ControlOperationsInterpreter with BitcoinSLogger result match { case SignatureValidationSuccess => ScriptProgram(program, OP_TRUE :: restOfStack, program.script.tail) - case err : SignatureValidationError => - if (ScriptFlagUtil.requireScriptVerifyNullFail(flags) && signature.bytes.nonEmpty) { - ScriptProgram(executionInProgressScriptProgram, ScriptErrorSigNullFail) - } else { - err match { - case SignatureValidationFailureNotStrictDerEncoding => - logger.info("Signature validation failed: " + SignatureValidationFailureNotStrictDerEncoding) - ScriptProgram(program, ScriptErrorSigDer) - case SignatureValidationFailureIncorrectSignatures => - logger.info("Signature validation failed: " + SignatureValidationFailureIncorrectSignatures) - ScriptProgram(program, OP_FALSE :: restOfStack,program.script.tail) - case SignatureValidationFailureSignatureCount => - logger.info("Signature validation failed: " + SignatureValidationFailureSignatureCount) - ScriptProgram(program, OP_FALSE :: restOfStack,program.script.tail) - case SignatureValidationFailurePubKeyEncoding => - logger.info("Signature validation failed: " + SignatureValidationFailurePubKeyEncoding) - //means that a public key was not encoded correctly - ScriptProgram(program,ScriptErrorPubKeyType) - case ScriptValidationFailureHighSValue => - logger.info("Signature validation failed: " + ScriptValidationFailureHighSValue) - ScriptProgram(program,ScriptErrorSigHighS) - case ScriptValidationFailureHashType => - logger.info("Signature validation failed: " + ScriptValidationFailureHashType) - ScriptProgram(program,ScriptErrorSigHashType) - case ScriptValidationFailureWitnessPubKeyType => - ScriptProgram(program,ScriptErrorWitnessPubKeyType) - } - } + case SignatureValidationFailureNotStrictDerEncoding => + logger.info("Signature validation failed: " + SignatureValidationFailureNotStrictDerEncoding) + ScriptProgram(program, ScriptErrorSigDer) + case SignatureValidationFailureIncorrectSignatures => + logger.info("Signature validation failed: " + SignatureValidationFailureIncorrectSignatures) + ScriptProgram(program, OP_FALSE :: restOfStack,program.script.tail) + case SignatureValidationFailureSignatureCount => + logger.info("Signature validation failed: " + SignatureValidationFailureSignatureCount) + ScriptProgram(program, OP_FALSE :: restOfStack,program.script.tail) + case SignatureValidationFailurePubKeyEncoding => + logger.info("Signature validation failed: " + SignatureValidationFailurePubKeyEncoding) + //means that a public key was not encoded correctly + ScriptProgram(program,ScriptErrorPubKeyType) + case ScriptValidationFailureHighSValue => + logger.info("Signature validation failed: " + ScriptValidationFailureHighSValue) + ScriptProgram(program,ScriptErrorSigHighS) + case ScriptValidationFailureHashType => + logger.info("Signature validation failed: " + ScriptValidationFailureHashType) + ScriptProgram(program,ScriptErrorSigHashType) + case ScriptValidationFailureWitnessPubKeyType => + ScriptProgram(program,ScriptErrorWitnessPubKeyType) + case SignatureValidationErrorNullFail => + ScriptProgram(executionInProgressScriptProgram,ScriptErrorSigNullFail) + } } } @@ -252,14 +248,9 @@ trait CryptoInterpreter extends ControlOperationsInterpreter with BitcoinSLogger //https://github.com/bitcoin/bips/blob/master/bip-0066.mediawiki#specification ScriptProgram(executionInProgressScriptProgram, ScriptErrorSigDer) case SignatureValidationFailureIncorrectSignatures => - val nullFailEnabled = ScriptFlagUtil.requireScriptVerifyNullFail(flags) - if (nullFailEnabled && signatures.exists(_.bytes.nonEmpty)) { - ScriptProgram(executionInProgressScriptProgram,ScriptErrorSigNullFail) } - else { - //this means that signature verification failed, however all signatures were encoded correctly - //just push a ScriptFalse onto the stack - ScriptProgram(executionInProgressScriptProgram, OP_FALSE :: restOfStack, program.script.tail) - } + //this means that signature verification failed, however all signatures were encoded correctly + //just push a OP_FALSE onto the stack + ScriptProgram(executionInProgressScriptProgram, OP_FALSE :: restOfStack, program.script.tail) case SignatureValidationFailureSignatureCount => //means that we did not have enough signatures for OP_CHECKMULTISIG ScriptProgram(executionInProgressScriptProgram, ScriptErrorInvalidStackOperation) @@ -272,6 +263,9 @@ trait CryptoInterpreter extends ControlOperationsInterpreter with BitcoinSLogger ScriptProgram(executionInProgressScriptProgram, ScriptErrorSigHashType) case ScriptValidationFailureWitnessPubKeyType => ScriptProgram(executionInProgressScriptProgram,ScriptErrorWitnessPubKeyType) + case SignatureValidationErrorNullFail => + + ScriptProgram(executionInProgressScriptProgram,ScriptErrorSigNullFail) } } } diff --git a/src/test/scala/org/bitcoins/core/crypto/TransactionSignatureCreatorSpec.scala b/src/test/scala/org/bitcoins/core/crypto/TransactionSignatureCreatorSpec.scala index c6c0a1d2da..5f69fe9ea9 100644 --- a/src/test/scala/org/bitcoins/core/crypto/TransactionSignatureCreatorSpec.scala +++ b/src/test/scala/org/bitcoins/core/crypto/TransactionSignatureCreatorSpec.scala @@ -38,6 +38,7 @@ class TransactionSignatureCreatorSpec extends Properties("TransactionSignatureCr //run it through the interpreter val program = ScriptProgram(txSignatureComponent) val result = ScriptInterpreter.run(program) + logger.info("result: " + result) result == ScriptOk } diff --git a/src/test/scala/org/bitcoins/core/script/interpreter/ScriptInterpreterTest.scala b/src/test/scala/org/bitcoins/core/script/interpreter/ScriptInterpreterTest.scala index ed29a817e8..6e189cfb5c 100644 --- a/src/test/scala/org/bitcoins/core/script/interpreter/ScriptInterpreterTest.scala +++ b/src/test/scala/org/bitcoins/core/script/interpreter/ScriptInterpreterTest.scala @@ -31,17 +31,7 @@ class ScriptInterpreterTest extends FlatSpec with MustMatchers with ScriptInterp //use this to represent a single test case from script_valid.json /* val lines = """ - | [ [ - | [ - | "", - | 0.00000000 - | ], - | "0x47 0x304402200a5c6163f07b8d3b013c4d1d6dba25e780b39658d79ba37af7057a3b7f15ffa102201fd9b4eaa9943f734928b99a83592c2e7bf342ea2680f6a2bb705167966b742001", - | "0x41 0x0479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG", - | "P2SH,WITNESS", - | "WITNESS_UNEXPECTED", - | "P2PK with witness" - | ]] + | [ ["0 0x09 0x300602010102010101 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG,NULLFAIL", "NULLFAIL", "BIP66-compliant but not NULLFAIL-compliant"]] """.stripMargin*/ val lines = try source.getLines.filterNot(_.isEmpty).map(_.trim) mkString "\n" finally source.close() val json = lines.parseJson