Opcode count compute limits

[msinkec]

Summary

Adds a per-input execution-count brake for the VM's heavy opcodes and removes the BIP-342 sigops budget, replacing both with one simple lookup table. This bounds worst-case single-input CPU and unblocks the heavy-opcode PRs (#79 EC ops, #80 OP_MODEXP), which add opcodes that are otherwise charged nothing and only limited by the 10 KB script-size cap.

Motivation

The VM enforces script size and stack size but has no op-count limit. BIP-342 dropped it deliberately, which is fine for cheap opcodes.

Several Arkade opcodes (OP_CHECKSIGFROMSTACK, OP_ECADD, OP_ECMUL, OP_ECPAIRING, OP_ECMULSCALARVERIFY, OP_TWEAKVERIFY, OP_MODEXP) cost orders of magnitude more per call, so an adversarial 10 KB script could drive multi-second CPU per input. See #81.

What changed

• Added ComputeLimits lookup table (pkg/arkade/compute_limits.go):

  • type ComputeLimits map[byte]int
  • Opcode → max executions per input
  • Absent opcode ⇒ unlimited
  • DefaultComputeLimits() returns a fresh copy

• Added engine charging (engine.go):

  • One chargeOpcode call in executeOpcode
  • Runs after the dead-branch return, so opcodes in unexecuted branches don't count
  • Runs before the handler
  • Per-input counts live on the tapscript context
  • Exhaustion returns txscript.ErrScriptTooBig, naming the opcode and limit

• Removed the BIP-342 sigops budget:

  • Deleted sigOpsBudget, tallysigOp, sigOpsDelta
  • Deleted witness-size scaling
  • Deleted both tallysigOp calls in the sig handlers
  • That budget models L1 block-weight/fee economics, which don't apply to the emulator
  • Sig opcodes are now entries in the same count-limit table

• Added admin override (internal/config):

  • Example: EMULATOR_COMPUTE_LIMITS="OP_ECPAIRING=8,OP_MODEXP=128"
  • Applied on top of defaults
  • Threaded via WithComputeLimits into the three script.Execute call sites
  • Unknown opcode, non-integer limit, or negative limit fails config load

• Retained existing per-call caps:

  • maxECPairingCount = 16
  • maxModexpOperandLen = 64
  • These bound a single call's cost
  • The count limit bounds the number of calls
  • They compose

Default limits

Opcode(s)	Limit	~cost/call
OP_CHECKSIG, OP_CHECKSIGVERIFY, OP_CHECKSIGADD, OP_CHECKSIGFROMSTACK	50 each	84 µs
OP_ECADD	1000	3.7 µs
OP_ECMUL, OP_ECMULSCALARVERIFY, OP_TWEAKVERIFY	50 each	84 µs
OP_ECPAIRING	2	2.04 ms
OP_MODEXP	64	60 µs

Behavior change

Dropping the sigops budget removes the BIP-342 empty-signature exemption: an empty-signature OP_CHECKSIG / OP_CHECKSIGADD in the unselected-branch multisig pattern now counts toward the opcode's limit.

This only over-counts cheap non-verifying calls. It never permits extra real verifications, and 50 is generous for realistic multisig.

---

Closing #81

[arkanaai]

Code Review — Opcode Count Compute Limits

Good design. The flat per-opcode count table is simpler and more appropriate for Arkade than the BIP-342 witness-size-proportional sigops budget. The implementation is clean, well-tested, and the config-override path is solid. A few issues to address:

---

🔴 MEDIUM — Shared mutable map in defaultComputeLimits (pkg/arkade/compute_limits.go:47, pkg/arkade/engine.go:835)

NewEngine assigns the shared defaultComputeLimits map directly to vm.limits:

var defaultComputeLimits = DefaultComputeLimits()  // one map for the process

// engine.go:835
limits: defaultComputeLimits,  // every engine gets the SAME map reference

ComputeLimits is map[byte]int — a reference type. Today no code writes through vm.limits[op], so this is safe. But it's fragile: a future change that does vm.limits[op] = X on any engine would silently corrupt every engine's limits for the rest of the process lifetime.

Fix: Either:

• Copy the map in NewEngine (safe, tiny cost — 10 entries), or
• Keep the shared reference but add a prominent // READ-ONLY — do not mutate comment on both the var and the field, and add a test that verifies defaultComputeLimits is not mutated after engine creation.

Copying is the safer choice for protocol-critical code.

---

🟡 LOW — WithComputeLimits replaces the entire map (pkg/arkade/script.go:40-43)

func WithComputeLimits(c ComputeLimits) ExecuteOption {
    return func(engine *Engine) {
        engine.limits = c
    }
}

If a caller passes a partial map (e.g., ComputeLimits{OP_ECPAIRING: 8}), all other heavy opcodes become unlimited. The config parser handles this correctly by starting from DefaultComputeLimits() and overriding, but a direct API caller of WithComputeLimits could easily make this mistake.

Fix: Add a godoc warning:

// WithComputeLimits overrides the per-input opcode-execution compute brake for
// this execution. c replaces the default limits entirely — opcodes absent from
// c become unlimited. Callers that want to override a subset should start from
// DefaultComputeLimits() and modify the returned map.

---

🟡 LOW — ErrScriptTooBig reuse for semantically different error (pkg/arkade/engine.go:340)

The compute-limit exhaustion reuses ErrScriptTooBig:

return scriptError(txscript.ErrScriptTooBig,
    fmt.Sprintf("opcode %s exceeded execution limit of %d", ...))

This is the same error code used at engine.go:842 for "script size exceeds max allowed size". Callers that match on ErrScriptTooBig will conflate "script too large" with "opcode executed too many times." The descriptive string distinguishes them, but error-code consumers won't see the string.

Not blocking, but consider whether a dedicated ErrComputeLimitExceeded code would be cleaner for downstream error handling.

---

🟢 INFO — Observations (no action needed)

1. Dead-branch skip is correct — chargeOpcode is called after the !vm.isBranchExecuting() early return in executeOpcode, so unexecuted-branch opcodes are never charged. Confirmed by TestChargeOpcodeIgnoresDeadBranch.

2. Zero limit — Validate() accepts limit=0, meaning an admin can disable an opcode entirely via config (OP_ECPAIRING=0). This is a reasonable safety valve. Tested implicitly via TestComputeLimitPairingTripsBeforePerCallCap (limit=2 trips at 3rd call, so limit=0 would trip at 1st).

3. No cross-repo breakage — solver, bancod, and other consumers only use opcode constants, NewPacket, and ArkadeScriptHash from the emulator package. None call script.Execute or application.New directly. The introspector-review repo references an older module path and isn't affected.

4. Test coverage is good — limit enforcement, dead-branch exemption, override, no-taproot-context bypass, pairing integration, sig counting, and config parsing are all tested. The config tests cover empty/single/multi/unknown/negative/malformed cases.

5. BIP-342 sigops removal — Removing the witness-size-proportional budget is correct for Arkade. The L1 budget models block-weight economics that don't apply here. The new flat-50-per-sig limit is more predictable and sufficient for realistic multisig patterns.

---

⚠️ Protocol-Critical — Human Review Required

This PR modifies the script VM execution semantics (opcode limiting, sigops budget removal). While the changes are a security improvement (bounding worst-case CPU), they are protocol-critical — they change what scripts are valid. Requires human sign-off before merge.

---

tl;dr: Fix the shared-map reference (copy in NewEngine), improve WithComputeLimits godoc, and optionally add a dedicated error code. Otherwise solid work. Requesting changes for the shared-map issue as it's a latent correctness risk in protocol-critical code.

[arkanaai]

Code Review — Opcode Count Compute Limits

Good design. The flat per-opcode count table is simpler and more appropriate for Arkade than the BIP-342 witness-size-proportional sigops budget. The implementation is clean, well-tested, and the config-override path is solid. A few issues to address:

---

🔴 MEDIUM — Shared mutable map in defaultComputeLimits (pkg/arkade/compute_limits.go:47, pkg/arkade/engine.go:835)

NewEngine assigns the shared defaultComputeLimits map directly to vm.limits:

var defaultComputeLimits = DefaultComputeLimits()  // one map for the process

// engine.go:835
limits: defaultComputeLimits,  // every engine gets the SAME map reference

ComputeLimits is map[byte]int — a reference type. Today no code writes through vm.limits[op], so this is safe. But it's fragile: a future change that does vm.limits[op] = X on any engine would silently corrupt every engine's limits for the rest of the process lifetime.

Fix: Either:

• Copy the map in NewEngine (safe, tiny cost — 10 entries), or
• Keep the shared reference but add a prominent // READ-ONLY — do not mutate comment on both the var and the field, and add a test that verifies defaultComputeLimits is not mutated after engine creation.

Copying is the safer choice for protocol-critical code.

---

🟡 LOW — WithComputeLimits replaces the entire map (pkg/arkade/script.go:40-43)

func WithComputeLimits(c ComputeLimits) ExecuteOption {
    return func(engine *Engine) {
        engine.limits = c
    }
}

If a caller passes a partial map (e.g., ComputeLimits{OP_ECPAIRING: 8}), all other heavy opcodes become unlimited. The config parser handles this correctly by starting from DefaultComputeLimits() and overriding, but a direct API caller of WithComputeLimits could easily make this mistake.

Fix: Add a godoc warning:

// WithComputeLimits overrides the per-input opcode-execution compute brake for
// this execution. c replaces the default limits entirely — opcodes absent from
// c become unlimited. Callers that want to override a subset should start from
// DefaultComputeLimits() and modify the returned map.

---

🟡 LOW — ErrScriptTooBig reuse for semantically different error (pkg/arkade/engine.go:340)

The compute-limit exhaustion reuses ErrScriptTooBig:

return scriptError(txscript.ErrScriptTooBig,
    fmt.Sprintf("opcode %s exceeded execution limit of %d", ...))

This is the same error code used at engine.go:842 for "script size exceeds max allowed size". Callers that match on ErrScriptTooBig will conflate "script too large" with "opcode executed too many times." The descriptive string distinguishes them, but error-code consumers won't see the string.

Not blocking, but consider whether a dedicated ErrComputeLimitExceeded code would be cleaner for downstream error handling.

---

🟢 INFO — Observations (no action needed)

1. Dead-branch skip is correct — chargeOpcode is called after the !vm.isBranchExecuting() early return in executeOpcode, so unexecuted-branch opcodes are never charged. Confirmed by TestChargeOpcodeIgnoresDeadBranch.

2. Zero limit — Validate() accepts limit=0, meaning an admin can disable an opcode entirely via config (OP_ECPAIRING=0). This is a reasonable safety valve. Tested implicitly via TestComputeLimitPairingTripsBeforePerCallCap (limit=2 trips at 3rd call, so limit=0 would trip at 1st).

3. No cross-repo breakage — solver, bancod, and other consumers only use opcode constants, NewPacket, and ArkadeScriptHash from the emulator package. None call script.Execute or application.New directly. The introspector-review repo references an older module path and isn't affected.

4. Test coverage is good — limit enforcement, dead-branch exemption, override, no-taproot-context bypass, pairing integration, sig counting, and config parsing are all tested. The config tests cover empty/single/multi/unknown/negative/malformed cases.

5. BIP-342 sigops removal — Removing the witness-size-proportional budget is correct for Arkade. The L1 budget models block-weight economics that don't apply here. The new flat-50-per-sig limit is more predictable and sufficient for realistic multisig patterns.

---

⚠️ Protocol-Critical — Human Review Required

This PR modifies the script VM execution semantics (opcode limiting, sigops budget removal). While the changes are a security improvement (bounding worst-case CPU), they are protocol-critical — they change what scripts are valid. Requires human sign-off before merge.

---

tl;dr: Fix the shared-map reference (copy in NewEngine), improve WithComputeLimits godoc, and optionally add a dedicated error code. Otherwise solid work. Requesting changes for the shared-map issue as it's a latent correctness risk in protocol-critical code.

[msinkec]

Note on compute-limit granularity:

The current implementation intentionally uses a very simple opcode -> max executions per input lookup table. Limits are independent per opcode, so a worst-case script can spend the full allowance for each configured heavy opcode rather than sharing one grouped budget.

Measured on an Apple M4 Pro, that implemented worst case is roughly 37 ms per input when all configured heavy-opcode counts are pushed to their limits. A grouped-counter model would bring the same calculation closer to ~24 ms, but the current bound is still far below the multi-second unbounded cases this PR is meant to prevent.

This is a deliberate simplicity tradeoff for now: the table is easy to read, configure, validate, and reason about. If we later need a tighter aggregate CPU bound, we can move from per-opcode counters to grouped counters or a weighted budget without changing the basic charging hook in the engine.

Opcode / allowance	Measured cost
50 OP_CHECKSIG	5.27 ms
50 OP_CHECKSIGVERIFY	4.72 ms
50 OP_CHECKSIGADD	4.62 ms
50 OP_CHECKSIGFROMSTACK	4.50 ms
1000 OP_ECADD	2.85 ms
50 OP_ECMUL	2.30 ms
50 OP_ECMULSCALARVERIFY	3.62 ms
50 OP_TWEAKVERIFY	1.63 ms
2 OP_ECPAIRING calls, 16 pairs each	5.05 ms
64 OP_MODEXP calls, 64-byte operands	3.15 ms
Total implemented worst case	~37 ms/input

[arkanaai]

Follow-up Review — New Commits 74275cf2, 0cec6efc

All three issues from the previous review are resolved:

✅ Shared mutable map (was 🔴 MEDIUM)

NewEngine now calls DefaultComputeLimits() for a fresh copy each time. The defaultComputeLimits package-level var is gone. cloneComputeLimits helper added for WithExactComputeLimits. TestNewEngineDefaultLimitsAreIndependent confirms engines don't share state. Clean fix.

✅ WithComputeLimits partial-map footgun (was 🟡 LOW)

Split into two functions:

• WithComputeLimits(c) — merges c on top of DefaultComputeLimits(), nil is a no-op. Safe for partial overrides.
• WithExactComputeLimits(c) — exact replacement with clone. Used by the config path (which already resolved from defaults).

Godoc on both is clear. TestWithComputeLimitsMergesWithDefaults and TestWithExactComputeLimitsCanRemoveDefaultLimit cover the distinction. TestWithComputeLimitsNilKeepsDefaultLimits covers the nil edge case.

✅ Config refinements

• Empty value (OP_ECADD=) now deletes the limit (makes it unlimited) — tested by TestParseComputeLimitsEmptyValueRemovesLimit.
• Empty pair and trailing comma now error — tested.
• Aggregate-cost comment added to DefaultComputeLimits().
• Call sites correctly use WithExactComputeLimits since the config path already merges with defaults.

Data flow verification

parseComputeLimits("") → returns full DefaultComputeLimits() (never nil) → stored in Config.ComputeLimits → passed to application.New → s.computeLimits → WithExactComputeLimits(s.computeLimits). Correct at every step.

---

⚠️ Protocol-critical reminder still applies — this changes VM execution semantics. Human sign-off required before merge.

[arkanaai]

Follow-up Review — New Commits 74275cf2, 0cec6efc

All three issues from the previous review are resolved:

✅ Shared mutable map (was 🔴 MEDIUM)

NewEngine now calls DefaultComputeLimits() for a fresh copy each time. The defaultComputeLimits package-level var is gone. cloneComputeLimits helper added for WithExactComputeLimits. TestNewEngineDefaultLimitsAreIndependent confirms engines don't share state. Clean fix.

✅ WithComputeLimits partial-map footgun (was 🟡 LOW)

Split into two functions:

• WithComputeLimits(c) — merges c on top of DefaultComputeLimits(), nil is a no-op. Safe for partial overrides.
• WithExactComputeLimits(c) — exact replacement with clone. Used by the config path (which already resolved from defaults).

Godoc on both is clear. TestWithComputeLimitsMergesWithDefaults and TestWithExactComputeLimitsCanRemoveDefaultLimit cover the distinction. TestWithComputeLimitsNilKeepsDefaultLimits covers the nil edge case.

✅ Config refinements

• Empty value (OP_ECADD=) now deletes the limit (makes it unlimited) — tested by TestParseComputeLimitsEmptyValueRemovesLimit.
• Empty pair and trailing comma now error — tested.
• Aggregate-cost comment added to DefaultComputeLimits().
• Call sites correctly use WithExactComputeLimits since the config path already merges with defaults.

Data flow verification

parseComputeLimits("") → returns full DefaultComputeLimits() (never nil) → stored in Config.ComputeLimits → passed to application.New → s.computeLimits → WithExactComputeLimits(s.computeLimits). Correct at every step.

---

⚠️ Protocol-critical reminder still applies — this changes VM execution semantics. Human sign-off required before merge.

[chris-ricketts]

Wondering if the opCounts should be exposed in the StepInfo, as well as an option to count but not error on limit exceeded.

E.g. I could imagine a scenario where someone is building a script and they want to count the limited opcodes in the non-dead paths for debugging/optimisation.

However, this could be in a future PR if deemed necessary.

Otherwise looks solid - LGTM

[msinkec]

Wondering if the opCounts should be exposed in the StepInfo, as well as an option to count but not error on limit exceeded.

E.g. I could imagine a scenario where someone is building a script and they want to count the limited opcodes in the non-dead paths for debugging/optimisation.

However, this could be in a future PR if deemed necessary.

Otherwise looks solid - LGTM

Let's do this in a seperate PR should the need arise. In the end a debugging tool that would do this besides other things such as stepping through the execution etc would be ideal.