Skip to content

Implement OP_ECADD, OP_ECMUL, OP_ECPAIRING#79

Open
msinkec wants to merge 4 commits into
masterfrom
feat/ec-opcodes
Open

Implement OP_ECADD, OP_ECMUL, OP_ECPAIRING#79
msinkec wants to merge 4 commits into
masterfrom
feat/ec-opcodes

Conversation

@msinkec
Copy link
Copy Markdown
Contributor

@msinkec msinkec commented May 14, 2026

Summary

Adds three Arkade Script opcodes for elliptic curve arithmetic, selectable at runtime by curve_id:

Word Opcode Hex Stack signature Output
OP_ECADD 246 0xf6 x1 y1 x2 y2 curve_id x3 y3
OP_ECMUL 247 0xf7 x y k curve_id x2 y2
OP_ECPAIRING 248 0xf8 [g1_x g1_y g2_x_c1 g2_x_c0 g2_y_c1 g2_y_c0]… pair_count curve_id bool

Curves at launch:

Curve ID Curve Operations
0 secp256k1 add, mul
1 secp256r1 (NIST P-256) add, mul
2 alt_bn128 / BN254 add, mul, pairing

0xf9 stays reserved. Pairing only works for alt_bn128 at launch; the curve_id argument is kept so a future BLS12 curve can reuse the same opcode shape.

Stack interface

All numeric inputs and outputs — curve IDs, pair counts, affine coordinates, Fp2 components, scalars, result coordinates — are Arkade BigNums (minimally encoded sign-magnitude little-endian). A script can build a coordinate with OP_ADD/OP_SUB and feed it directly into OP_ECADD without an extra fixed-width encoding step.

  • (0, 0) is the wire representation of the G1 point at infinity. G2 infinity is all four Fp2 components zero.
  • OP_ECPAIRING follows EIP-197 coordinate order for alt_bn128 G2: each pair is six BigNums (g1_x, g1_y, g2_x_c1, g2_x_c0, g2_y_c1, g2_y_c0).
  • k = 0 for OP_ECMUL returns infinity. pair_count = 0 for OP_ECPAIRING returns true (matches EIP-197).
  • Scalars must be 0 ≤ k < group_order. Coordinates must be 0 ≤ x < field_modulus. This intentionally differs from EIP-196, where the scalar is a 32-byte calldata blob that can exceed the group order — Arkade Script has native arbitrary-precision arithmetic, so we reject non-canonical scalars at the opcode boundary.

Validation and failure model

Every structural or value error fails script execution; only a valid pairing-product that happens to evaluate to a non-identity element pushes canonical false. Failure cases:

  • stack underflow
  • unsupported curve_id
  • unsupported pairing curve (only alt_bn128 supported at launch)
  • non-minimal BigNum inputs
  • negative coordinates, field elements, scalars, or pair counts
  • coordinates ≥ field_modulus
  • scalars ≥ group_order
  • off-curve G1 / G2
  • G2 points outside the alt_bn128 r-subgroup (explicit subgroup check; G1 has cofactor 1 so on-curve suffices)
  • pair_count > 16

Errors map to txscript.ErrInvalidStackOperation except where existing stack/BigNum helpers already produce a more specific code (ErrMinimalData, ErrNumberTooBig, stack-underflow ErrInvalidStackOperation).

pair_count is hard-capped at 16. Arkade Script has no gas model, so pairing CPU work must be bounded deterministically.

Implementation

All curve logic and adapter code lives in pkg/arkade/ec_ops.go:

  • Curve metadata (id, name, field modulus, group order, supportsPairing) in one table.
  • BigNum-to-*big.Int validation helpers at the opcode boundary (popCurveID, popInFieldElement, popInGroupScalar).
  • Three opcode handlers: opcodeECAdd, opcodeECMul, opcodeECPairing.
  • Per-curve adapters that hide library types from the handlers and convert outputs back to canonical BigNums.

Backends:

  • alt_bn128 (G1, G2, pairing) and secp256k1 (G1) — github.com/consensys/gnark-crypto v0.19.2 (pinned).
  • secp256r1 / NIST P-256 — Go standard library crypto/elliptic.P256() (the same crypto/internal/nistec implementation used by crypto/tls, crypto/ecdsa, and crypto/ecdh).

@msinkec msinkec requested review from Kukks and louisinger May 14, 2026 20:50
@msinkec msinkec self-assigned this May 14, 2026
…/otel

   and CVE-2026-33811, -33814, -39820, -39836, -42499 in the Go stdlib
   shipped via the Dockerfile builder image.
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 — OP_ECADD / OP_ECMUL / OP_ECPAIRING

Verdict: Request Changes — protocol-critical consensus opcodes require human sign-off before merge. The implementation is high quality overall, but there are issues to address and downstream coordination required.


⚠️ PROTOCOL-CRITICAL — REQUIRES HUMAN REVIEW

This PR adds three new consensus opcodes to the Arkade Script VM. Any implementation divergence between nodes causes chain forks. This must not merge without explicit human sign-off from protocol leads.


Cross-Repo Breakage — Action Required

1. rust-sdk must be updated (blocking)
ark-script/src/opcodes.rs defines ARKADE_OPCODES ending at 0xf5. The comment reads:

"Arkade opcodes occupy byte values 0xb3 and 0xc4..=0xf5"

Any Rust consumer using ARKADE_OPCODES to validate scripts will reject 0xf60xf8 as unknown. This needs a coordinated update — ideally an open PR on rust-sdk before this merges.

2. Downstream Go consumers: bancod and layerzero-usdt0-arkade-demo both import github.com/ArkLabsHQ/introspector/pkg/arkade. They will pick up these opcodes on next go get. No breakage expected (new opcodes only, no API changes), but worth a heads-up.


Findings

1. [Medium] No activation gate for new opcodes

pkg/arkade/opcode.go:609-611 — The three opcodes go from opcodeInvalid → live handlers with no version/height/flag gate. If any node on the network is running the old code when a script uses OP_ECADD, it will reject the transaction as invalid while updated nodes accept it — chain split.

Is there an activation mechanism outside this repo (e.g., in arkd)? If not, this needs one, or the deployment plan needs to guarantee all nodes upgrade atomically.

2. [Low] crypto/elliptic.P256() Add/ScalarMult deprecation risk

pkg/arkade/ec_ops.go:627-657 — The deprecated crypto/elliptic API is used for secp256r1. The //nolint:staticcheck suppression is acknowledged in the comments, which is good. However, Go could change or remove the deprecated methods in a future release. Consider documenting the minimum Go version constraint, or migrating to crypto/ecdh with manual point serialization if feasible.

3. [Low] Missing edge-case test vectors

The test suite is solid but a few boundary vectors are missing:

  • k = order - 1 for each curve (last valid scalar — confirms the boundary check is < not <=)
  • pair_count = 16 (exactly at maxECPairingCount — confirms the boundary is > not >=)
  • OP_ECMUL with secp256r1 infinity input (P=(0,0), k>0) — tested for secp256k1 but not P-256
  • OP_ECADD with secp256r1 infinity + infinity — tested for secp256k1 but not P-256

These are consensus-critical boundaries — a fence-post error here means valid scripts fail or invalid scripts succeed.

4. [Info] Pairing CPU bound

maxECPairingCount = 16 is reasonable. BN254 pairing-product check with 16 pairs on gnark-crypto should complete in ~50-100ms. Worth benchmarking and documenting the expected worst-case latency to justify the cap in the spec.


What Looks Good ✅

  • On-curve validation on all inputs before computation — prevents invalid-point attacks
  • G2 subgroup check (IsInSubGroup()) for BN254 pairing — critical for soundness, correctly implemented
  • Field/scalar bounds checkingpopInFieldElement enforces 0 ≤ n < modulus, popInGroupScalar enforces 0 ≤ k < order
  • Minimal BigNum encoding enforced via existing PopBigNum — no non-canonical encoding ambiguity
  • Infinity point handling(0,0) convention consistently applied across all three curves with explicit special-casing for P-256's crypto/elliptic which rejects (0,0) from IsOnCurve
  • Fuzz test builders (ecCurveCaseBuilder, ecPairingFuzzCaseBuilder) — well-designed to penetrate past validation gates
  • gnark-crypto v0.19.2 pinned — correct for consensus-critical code
  • Error codes — all failures map to documented consensus error codes
  • Stack discipline — pop/push order verified against spec; alt-stack and cond-stack invariants checked in property tests
  • Pairing follows EIP-197 semantics — empty set returns true, coordinate order matches, but with stricter scalar validation (good deviation from EIP-196)
  • Comprehensive test coverage — off-curve, out-of-field, negative values, non-minimal encoding, subgroup violations all tested

Summary

The EC arithmetic implementation is well-engineered and the security-critical checks (on-curve, subgroup, field bounds) are all present. The main concerns are:

  1. Activation gating — how do we prevent chain splits during rollout?
  2. rust-sdk coordination — needs 0xf6-0xf8 added to ARKADE_OPCODES
  3. Missing boundary test vectorsk = order-1 and pair_count = 16

Once the activation strategy is clarified and the missing tests are added, this is ready for human protocol-lead sign-off.


🤖 Reviewed by Arkana · aggressive code review · model: opus

Copy link
Copy Markdown
Collaborator

@louisinger louisinger left a comment

Choose a reason for hiding this comment

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

concept ACK but we need a strategy for the liquid opcodes because they are confusing. I have the feeling we can "drop" them and replace by these new primitives ?

Comment thread README.md Outdated
Comment thread pkg/arkade/opcode.go Outdated
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.

2 participants