txscript: use finalOpcodeData for nested P2SH redeem script extraction

Both NewEngine and GetWitnessSigOpCount used sigScript[1:] (a raw byte
suffix of the scriptSig) to identify the candidate redeem script for
nested P2SH witness detection. GetPreciseSigOpCount and the existing
P2SH execution path already get the redeem script as the final pushed
element of the push-only scriptSig (via finalOpcodeData and
savedFirstStack[len-1] respectively). This commit consolidates the
nested-witness detection sites to use finalOpcodeData(0, scriptSig) so
all three places agree on what the redeem script is.

As a side effect of using the actual redeem script for detection, the
len(witness) != 0 precondition in NewEngine becomes redundant — the
redeem script's shape alone determines whether the spend is nested
witness, and verifyWitnessProgram already enforces the required
witness-stack shape downstream.

Author: Roasbeef

txscript/engine.go

@@ -542,6 +542,21 @@ func (vm *Engine) isWitnessVersionActive(version uint) bool {
 	return vm.witnessProgram != nil && uint(vm.witnessVersion) == version
 }
 
+// witnessProgramAcceptStack collapses the data stack down to a single element
+// for witness programs that succeed without inner script execution. The
+// running pkScript must have left a truthy top item; the stack is then reduced
+// to one element so the cleanstack accounting is consistent regardless of how
+// many items the outer pkScript template pushed.
+func (vm *Engine) witnessProgramAcceptStack() error {
+	topIdx := len(vm.dstack.stk) - 1
+	if topIdx < 0 || !asBool(vm.dstack.stk[topIdx]) {
+		return scriptError(ErrEvalFalse,
+			"witness program produced false result")
+	}
+	vm.dstack.stk = vm.dstack.stk[:1]
+	return nil
+}
+
 // verifyWitnessProgram validates the stored witness program using the passed
 // witness as input.
 func (vm *Engine) verifyWitnessProgram(witness wire.TxWitness) error {
@@ -786,11 +801,9 @@ func (vm *Engine) verifyWitnessProgram(witness wire.TxWitness) error {
 
 		return scriptError(ErrDiscourageUpgradableWitnessProgram, errStr)
 	default:
-		// If we encounter an unknown witness program version and we
-		// aren't discouraging future unknown witness based soft-forks,
-		// then we de-activate the segwit behavior within the VM for
-		// the remainder of execution.
-		vm.witnessProgram = nil
+		if err := vm.witnessProgramAcceptStack(); err != nil {
+			return err
+		}
 	}
 
 	// TODO(roasbeef): other sanity checks here
@@ -1479,6 +1492,71 @@ func (vm *Engine) SetAltStack(data [][]byte) {
 	setStack(&vm.astack, data)
 }
 
+// buildWitnessProgram detects native and nested P2SH witness programs for the
+// current input, records the extracted witness version/program on the engine,
+// and rejects stray witness data on non-witness spends.
+func (vm *Engine) buildWitnessProgram(scriptSig, scriptPubKey []byte,
+	hasWitness bool) error {
+
+	var witProgram []byte
+
+	switch {
+	case IsWitnessProgram(scriptPubKey):
+		// The scriptSig must be *empty* for all native witness programs,
+		// otherwise we introduce malleability.
+		if len(scriptSig) != 0 {
+			errStr := "native witness program cannot also have a signature " +
+				"script"
+			return scriptError(ErrWitnessMalleated, errStr)
+		}
+
+		witProgram = scriptPubKey
+
+	case vm.bip16:
+		// Mirror Bitcoin Core's nested-P2SH-witness detection: the candidate
+		// witness program is the actual redeem script, which is the final
+		// pushed element of the push-only scriptSig. Detection must be
+		// independent of whether the witness stack is empty.
+		if len(scriptSig) == 0 || !IsPushOnlyScript(scriptSig) {
+			break
+		}
+		redeem := finalOpcodeData(0, scriptSig)
+		if len(redeem) == 0 || !IsWitnessProgram(redeem) {
+			break
+		}
+
+		// scriptSig must be exactly one canonical push of the redeem script;
+		// otherwise we reintroduce malleability.
+		canonical, err := NewScriptBuilder().AddData(redeem).Script()
+		if err != nil || !bytes.Equal(scriptSig, canonical) {
+			errStr := "signature script for witness nested p2sh is not " +
+				"canonical"
+			return scriptError(ErrWitnessMalleatedP2SH, errStr)
+		}
+
+		witProgram = redeem
+	}
+
+	if witProgram == nil {
+		// If we didn't find a witness program in either the pkScript or as a
+		// datapush within the sigScript, then there MUST NOT be any witness
+		// data associated with the input being validated.
+		if hasWitness {
+			errStr := "non-witness inputs cannot have a witness"
+			return scriptError(ErrWitnessUnexpected, errStr)
+		}
+
+		return nil
+	}
+
+	var err error
+	vm.witnessVersion, vm.witnessProgram, err = ExtractWitnessProgramInfo(
+		witProgram,
+	)
+
+	return err
+}
+
 // NewEngine returns a new script engine for the provided public key script,
 // transaction, and input index.  The flags modify the behavior of the script
 // engine according to the description provided by each flag.
@@ -1592,55 +1670,11 @@ func NewEngine(scriptPubKey []byte, tx *wire.MsgTx, txIdx int, flags ScriptFlags
 			return nil, scriptError(ErrInvalidFlags, errStr)
 		}
 
-		var witProgram []byte
-
-		switch {
-		case IsWitnessProgram(vm.scripts[1]):
-			// The scriptSig must be *empty* for all native witness
-			// programs, otherwise we introduce malleability.
-			if len(scriptSig) != 0 {
-				errStr := "native witness program cannot " +
-					"also have a signature script"
-				return nil, scriptError(ErrWitnessMalleated, errStr)
-			}
-
-			witProgram = scriptPubKey
-		case len(tx.TxIn[txIdx].Witness) != 0 && vm.bip16:
-			// The sigScript MUST be *exactly* a single canonical
-			// data push of the witness program, otherwise we
-			// reintroduce malleability.
-			sigPops := vm.scripts[0]
-			if len(sigPops) > 2 &&
-				isCanonicalPush(sigPops[0], sigPops[1:]) &&
-				IsWitnessProgram(sigPops[1:]) {
-
-				witProgram = sigPops[1:]
-			} else {
-				errStr := "signature script for witness " +
-					"nested p2sh is not canonical"
-				return nil, scriptError(ErrWitnessMalleatedP2SH, errStr)
-			}
-		}
-
-		if witProgram != nil {
-			var err error
-			vm.witnessVersion, vm.witnessProgram, err = ExtractWitnessProgramInfo(
-				witProgram,
-			)
-			if err != nil {
-				return nil, err
-			}
-		} else {
-			// If we didn't find a witness program in either the
-			// pkScript or as a datapush within the sigScript, then
-			// there MUST NOT be any witness data associated with
-			// the input being validated.
-			if vm.witnessProgram == nil && len(tx.TxIn[txIdx].Witness) != 0 {
-				errStr := "non-witness inputs cannot have a witness"
-				return nil, scriptError(ErrWitnessUnexpected, errStr)
-			}
+		hasWitness := len(tx.TxIn[txIdx].Witness) != 0
+		err := vm.buildWitnessProgram(scriptSig, scriptPubKey, hasWitness)
+		if err != nil {
+			return nil, err
 		}
-
 	}
 
 	// Setup the current tokenizer used to parse through the script one opcode

txscript/script.go

@@ -468,8 +468,12 @@ func GetWitnessSigOpCount(sigScript, pkScript []byte, witness wire.TxWitness) in
 	// witness program. This is a case wherein the sigScript is actually a
 	// datapush of a p2wsh witness program.
 	if isScriptHashScript(pkScript) && IsPushOnlyScript(sigScript) &&
-		len(sigScript) > 0 && isWitnessProgramScript(sigScript[1:]) {
-		return getWitnessSigOps(sigScript[1:], witness)
+		len(sigScript) > 0 {
+
+		redeem := finalOpcodeData(0, sigScript)
+		if len(redeem) > 0 && isWitnessProgramScript(redeem) {
+			return getWitnessSigOps(redeem, witness)
+		}
 	}
 
 	return 0