txscript: reject OP_CODESEPARATOR in unexecuted branches for non-segwit

[Roasbeef]

In this commit, we fix a policy-level divergence with Bitcoin Core when handling OP_CODESEPARATOR inside unexecuted OP_IF branches in non-segwit scripts with the ScriptVerifyConstScriptCode flag.

Bitcoin Core's EvalScript (interpreter.cpp:474-476) places the SCRIPT_VERIFY_CONST_SCRIPTCODE check for OP_CODESEPARATOR before the fExec branch-execution gate, causing it to fire unconditionally on every OP_CODESEPARATOR encountered during script iteration -- even inside OP_FALSE OP_IF ... OP_ENDIF envelopes.

Previously, btcd's equivalent check lived inside the opcodeCodeSeparator handler, which was never reached for opcodes in unexecuted branches due to the early return in executeOpcode that skips non-conditional opcodes when isBranchExecuting() is false. This meant a script like:

OP_FALSE OP_IF OP_CODESEPARATOR OP_ENDIF

would be rejected by Bitcoin Core's mempool but accepted by btcd's.

The fix moves the check before the branch-execution gate in executeOpcode, matching Bitcoin Core's structure. This follows the existing pattern in btcd where isOpcodeDisabled and isOpcodeAlwaysIllegal checks already fire regardless of branch execution state.

Note: SCRIPT_VERIFY_CONST_SCRIPTCODE is purely a policy flag (included in STANDARD_SCRIPT_VERIFY_FLAGS but not MANDATORY_SCRIPT_VERIFY_FLAGS), so this was not a consensus divergence. Both implementations would accept such transactions if mined in a block.

Found via differential fuzzing by Bruno from bitcoinfuzz.

[coveralls]

Pull Request Test Coverage Report for Build 22081381425

Details

• 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
• 75 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.04%) to 54.91%

---

Files with Coverage Reduction	New Missed Lines	%
txscript/opcode.go	5	97.99%
rpcclient/infrastructure.go	70	42.43%

Totals
Change from base Build 20942501138:	-0.04%
Covered Lines:	31146
Relevant Lines:	56722

---

💛 - Coveralls

[brunoerg]

ACK 3e17ea4

[saubyk]

cc: @gijswijs for review

[kcalvinalvin]

Bit weird that we check for non-segwitness by asserting witnessProgram and taprootCtx is nil but this works.

[kcalvinalvin]

Checked against Core at commit ff7cdf633e375f151cccbcc78c7add161b3d29b8 and verified that the behavior matches.

But can we add a segwit case? It's not tested here and would be good to have. Something like this maybe:

diff --git a/txscript/engine_test.go b/txscript/engine_test.go
index dea9f2f7..311a9038 100644
--- a/txscript/engine_test.go
+++ b/txscript/engine_test.go
@@ -6,6 +6,7 @@
 package txscript
 
 import (
+       "crypto/sha256"
        "testing"
 
        "github.com/btcsuite/btcd/chaincfg/chainhash"
@@ -466,6 +467,7 @@ func TestCodeSepUnexecutedBranch(t *testing.T) {
                name    string
                script  string
                flags   ScriptFlags
+               segwit  bool
                wantErr bool
                errCode ErrorCode
        }{
@@ -503,13 +505,37 @@ func TestCodeSepUnexecutedBranch(t *testing.T) {
                        wantErr: true,
                        errCode: ErrCodeSeparator,
                },
+               {
+                       // OP_CODESEPARATOR in a segwit P2WSH witness script
+                       // should succeed even with const scriptcode, since
+                       // the flag only applies to non-segwit scripts.
+                       name:    "codesep in segwit P2WSH with const scriptcode",
+                       script:  "CODESEPARATOR TRUE",
+                       flags: ScriptVerifyConstScriptCode | ScriptVerifyWitness | ScriptBip16,
+                       segwit:  true,
+                       wantErr: false,
+               },
        }
 
        for _, test := range tests {
                t.Run(test.name, func(t *testing.T) {
-                       pkScript := mustParseShortForm(test.script)
+                       scriptBytes := mustParseShortForm(test.script)
+
+                       pkScript := scriptBytes
+                       testTx := tx
+                       if test.segwit {
+                               hash := sha256.Sum256(scriptBytes)
+                               pkScript, _ = NewScriptBuilder().
+                                       AddOp(OP_0).AddData(hash[:]).Script()
+                               testTx = tx.Copy()
+                               testTx.TxIn[0].SignatureScript = nil
+                               testTx.TxIn[0].Witness = wire.TxWitness{
+                                       scriptBytes,
+                               }
+                       }
+
                        vm, err := NewEngine(
-                               pkScript, tx, 0, test.flags, nil, nil,
+                               pkScript, testTx, 0, test.flags, nil, nil,
                                -1, nil,
                        )
                        if err != nil {

[Roasbeef]

@kcalvinalvin PTAL.

One other thing we can do is remove the old check. Since we know check it each time, we no longer need that logic within the op code invocation itself.

[kcalvinalvin]

Nothing changed since I reviewed it..? Not sure what I should look at.

I mean the code change itself is still ok. I just think it'd be good to have the non segwit case

[allocz]

@kcalvinalvin

I agree with the concept of adding a test to cover OP_CODESEPARATOR on segwit, but maybe instead of adding custom behavior in the test execution loop just for the segwit variant of this test wouldn't be cleaner to add another test TestCodeSepExecutedBranchSegwit?