Skip to content

Make OP_CODESEPERATOR work in an arkade script context#92

Open
msinkec wants to merge 1 commit into
masterfrom
feat/op-codeseperator
Open

Make OP_CODESEPERATOR work in an arkade script context#92
msinkec wants to merge 1 commit into
masterfrom
feat/op-codeseperator

Conversation

@msinkec
Copy link
Copy Markdown
Contributor

@msinkec msinkec commented Jun 1, 2026

Summary

OP_CODESEPARATOR was recognized and executed inside arkade scripts but had no observable effect: the opcode's update of the tapscript sighash's codesep_pos was gated behind a trackCodeSep flag that ArkadeScript.Execute set to false. As a result the code-separator position was never folded into the signature hash, diverging from BIP342.

This PR makes OP_CODESEPARATOR commit its opcode position into the arkade tapscript sighash, matching BIP342 semantics, and updates the signing helper so signers can produce matching digests.

Changes

  • Engine (opcode.go, engine.go, script.go): opcodeCodeSeparator now unconditionally records vm.tokenizer.OpcodePosition() into taprootCtx.codeSepPos. Removed the trackCodeSep flag and the line in ArkadeScript.Execute that disabled tracking. The position is tracked against the executing (emulator-packet) script per BIP342; the separately-committed spending-tapleaf hash is unaffected.
  • Signer helper (sigvalidate.go): CalcTapscriptSignaturehash → renamed to CalcArkadeScriptSignatureHash and gained a codeSepPos uint32 parameter so callers can compute the digest the engine will verify against. Exposed BlankCodeSepValue (the 0xffffffff sentinel) for the common no-separator case. Updated repo call sites to the clearer Arkade-specific name.
  • Doc fixes (opcode.go:1754): Updated the stale OP_CHECKSIG / OP_CODESEPARATOR comments that still described btcd's legacy script-subscript signing process; they now describe the arkade tapscript digest (spending-tapleaf hash + last executed separator position).
  • Dead-code removal (engine.go, opcode.go): Removed the lastCodeSep field, 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 the codesep_pos opcode-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 — observes codesep_pos through the real Execute path: blank initially, then the separator's opcode position after it runs.
  • TestArkadeScriptExecuteOpSighashUsesCodeSeparatorPosition (script_test.go:228) — checks OP_SIGHASH (not just OP_CHECKSIG) uses the executed separator position and that it differs from the blank-sentinel path.
  • Replaces the old TestArkadeScriptExecuteDoesNotUsePacketCodeSeparatorForSighash, which asserted the previous (disabled) behavior.

@msinkec msinkec self-assigned this Jun 1, 2026
Copy link
Copy Markdown

@arkanaai arkanaai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = 0xFFFFFFFF regardless of OP_CODESEPARATOR in the script.
  • After: codesep_pos reflects the last executed OP_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:

  1. opcodeCodeSeparator (opcode.go:1762-1765) — Clean. Unconditionally sets vm.taprootCtx.codeSepPos = uint32(vm.tokenizer.OpcodePosition()). Correctly uses opcode position (not byte offset), matching BIP342. ✅

  2. CalcArkadeScriptSignatureHash (sigvalidate.go:79-109) — Good rename from CalcTapscriptSignaturehash. New codeSepPos uint32 parameter correctly plumbed: creates a taprootCtx, sets codeSepPos, then calls computeArkadeSighash. Passes 0 for witness size in the helper context, which is correct since sig ops budgeting is irrelevant for digest computation. ✅

  3. BlankCodeSepValue export (engine.go:33-36) — Correctly exported with explicit uint32 type so callers can specify the no-separator default. ✅

  4. Dead code removal (engine.go, opcode.go)lastCodeSep was only written, never read (btcd legacy for pre-segwit OP_CODESEPARATOR byte-offset subscripting). trackCodeSep is no longer needed. Both cleanly removed. ✅

  5. script.go:184 removal — The trackCodeSep = false line was the root cause of the bug. Removing it is the correct fix. ✅

  6. OP_SIGHASH pathopcodeSighashcomputeArkadeSighash reads vm.taprootCtx.codeSepPos directly, so it automatically picks up the updated position. No changes needed. ✅

  7. 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

  • bancod and solver depend on emulator/pkg/arkade but neither calls CalcTapscriptSignaturehash / CalcArkadeScriptSignatureHash. No breakage.
  • No other repos import emulator/pkg/arkade.
  • The internal/application/finalization.go call to txscript.CalcTapscriptSignaturehash is 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 correct codeSepPos=0 verifies. ✅
  • Negative case (script_test.go:133-143): Signature with BlankCodeSepValue is rejected when OP_CODESEPARATOR was executed. ✅
  • State observation (script_test.go:147-189): codeSepPos transitions from BlankCodeSepValue → opcode position 0 via debug callback. ✅
  • OP_SIGHASH path (script_test.go:191-258): Confirms OP_SIGHASH also 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): signArkadeInput helper updated to new API with BlankCodeSepValue. ✅

Nits (non-blocking)

None. The PR is clean.


🤖 Reviewed by Arkana (Claude Opus) · Protocol-critical flag set · Human sign-off required before merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant