Make OP_CODESEPERATOR work in an arkade script context#92
Conversation
There was a problem hiding this comment.
Arkana Code Review — #92
Verdict: APPROVE ✅ — but flagged for mandatory human review (protocol-critical: sighash digest change).
Summary
This PR fixes a real bug: OP_CODESEPARATOR was recognized but neutered inside arkade scripts because ArkadeScript.Execute set trackCodeSep = false, so codesep_pos was never folded into the signature hash. This diverged from BIP342. The fix is clean, correct, and well-tested.
Protocol-Critical Assessment ⚠️
This changes what signature digests the VM accepts. Specifically:
- Before: All arkade sighashes used
codesep_pos = 0xFFFFFFFFregardless ofOP_CODESEPARATORin the script. - After:
codesep_posreflects the last executedOP_CODESEPARATOR's opcode position per BIP342.
A bug in sighash computation means either funds locked (valid sigs rejected) or funds stolen (invalid sigs accepted). The implementation looks correct, but this requires a human cryptography-aware reviewer to sign off.
Migration note: Any deployed arkade scripts containing OP_CODESEPARATOR followed by OP_CHECKSIG that were signed against the old (blank) digest will now fail. Since the old behavior was broken (codesep was a no-op), there likely aren't any such scripts in production, but this should be verified.
Findings
No blocking issues found. Detailed notes:
-
opcodeCodeSeparator(opcode.go:1762-1765) — Clean. Unconditionally setsvm.taprootCtx.codeSepPos = uint32(vm.tokenizer.OpcodePosition()). Correctly uses opcode position (not byte offset), matching BIP342. ✅ -
CalcArkadeScriptSignatureHash(sigvalidate.go:79-109) — Good rename fromCalcTapscriptSignaturehash. NewcodeSepPos uint32parameter correctly plumbed: creates a taprootCtx, setscodeSepPos, then callscomputeArkadeSighash. Passes0for witness size in the helper context, which is correct since sig ops budgeting is irrelevant for digest computation. ✅ -
BlankCodeSepValueexport (engine.go:33-36) — Correctly exported with explicituint32type so callers can specify the no-separator default. ✅ -
Dead code removal (
engine.go,opcode.go) —lastCodeSepwas only written, never read (btcd legacy for pre-segwitOP_CODESEPARATORbyte-offset subscripting).trackCodeSepis no longer needed. Both cleanly removed. ✅ -
script.go:184removal — ThetrackCodeSep = falseline was the root cause of the bug. Removing it is the correct fix. ✅ -
OP_SIGHASH path —
opcodeSighash→computeArkadeSighashreadsvm.taprootCtx.codeSepPosdirectly, so it automatically picks up the updated position. No changes needed. ✅ -
Comment updates (
opcode.go:1771-1779) — Old comments described btcd's legacy script-subscript signing. New comments correctly describe the arkade tapscript digest (spending tapleaf hash + last executed separator position). ✅
Cross-Repo Impact
bancodandsolverdepend onemulator/pkg/arkadebut neither callsCalcTapscriptSignaturehash/CalcArkadeScriptSignatureHash. No breakage. ✅- No other repos import
emulator/pkg/arkade. - The
internal/application/finalization.gocall totxscript.CalcTapscriptSignaturehashis btcd's standard BIP342 function — unrelated to this rename. ✅
Test Coverage
Excellent. Four new/updated tests covering:
- Positive case (
script_test.go:77): Signature with correctcodeSepPos=0verifies. ✅ - Negative case (
script_test.go:133-143): Signature withBlankCodeSepValueis rejected whenOP_CODESEPARATORwas executed. ✅ - State observation (
script_test.go:147-189):codeSepPostransitions fromBlankCodeSepValue→ opcode position 0 via debug callback. ✅ - OP_SIGHASH path (
script_test.go:191-258): ConfirmsOP_SIGHASHalso uses the executed separator position and produces a different digest than the blank-sentinel path. ✅ - Existing test updated (
test/signed_pay_to_output_test.go:228-234):signArkadeInputhelper updated to new API withBlankCodeSepValue. ✅
Nits (non-blocking)
None. The PR is clean.
🤖 Reviewed by Arkana (Claude Opus) · Protocol-critical flag set · Human sign-off required before merge
Summary
OP_CODESEPARATORwas recognized and executed inside arkade scripts but had no observable effect: the opcode's update of the tapscript sighash'scodesep_poswas gated behind atrackCodeSepflag thatArkadeScript.Executeset tofalse. As a result the code-separator position was never folded into the signature hash, diverging from BIP342.This PR makes
OP_CODESEPARATORcommit its opcode position into the arkade tapscript sighash, matching BIP342 semantics, and updates the signing helper so signers can produce matching digests.Changes
opcode.go,engine.go,script.go):opcodeCodeSeparatornow unconditionally recordsvm.tokenizer.OpcodePosition()intotaprootCtx.codeSepPos. Removed thetrackCodeSepflag and the line inArkadeScript.Executethat disabled tracking. The position is tracked against the executing (emulator-packet) script per BIP342; the separately-committed spending-tapleaf hash is unaffected.sigvalidate.go):CalcTapscriptSignaturehash→ renamed toCalcArkadeScriptSignatureHashand gained acodeSepPos uint32parameter so callers can compute the digest the engine will verify against. ExposedBlankCodeSepValue(the0xffffffffsentinel) for the common no-separator case. Updated repo call sites to the clearer Arkade-specific name.opcode.go:1754): Updated the staleOP_CHECKSIG/OP_CODESEPARATORcomments that still described btcd's legacy script-subscript signing process; they now describe the arkade tapscript digest (spending-tapleaf hash + last executed separator position).engine.go,opcode.go): Removed thelastCodeSepfield, its write, and its per-script reset. It was a vestige of btcd's legacy pre-segwit sighash (which subscripted the script from the last separator's byte offset); BIP341/342 replaced that mechanism with thecodesep_posopcode-position commitment this engine uses, leaving the field with zero readers.Tests
TestArkadeScriptExecuteUsesCodeSeparatorForSighash— a signature committing to the executed codesep position verifies; one using the blank sentinel is now rejected (proves the position is actually committed).TestArkadeScriptExecuteUpdatesCodeSepPosOnCodeSeparator— observescodesep_posthrough the realExecutepath: blank initially, then the separator's opcode position after it runs.TestArkadeScriptExecuteOpSighashUsesCodeSeparatorPosition(script_test.go:228) — checksOP_SIGHASH(not justOP_CHECKSIG) uses the executed separator position and that it differs from the blank-sentinel path.TestArkadeScriptExecuteDoesNotUsePacketCodeSeparatorForSighash, which asserted the previous (disabled) behavior.