fix(compiler): Rabin verify compares (sig^2 + padding) mod n numerically, not byte-wise - #146
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a consensus-critical Rabin verification bug where (sig^2 + padding) mod n was compared to SHA256(msg) byte-wise (OP_EQUAL) even though OP_MOD returns a minimally-encoded Script number (sometimes 33 bytes), causing valid signatures to fail about half the time on real Script VMs. The fix normalizes the digest into a minimal Script-number form and switches to a numeric comparison (OP_NUMEQUAL).
Changes:
- Update Rabin
verifyRabinSigcodegen tail to:OP_SHA256 <push 0x00> OP_CAT OP_BIN2NUM OP_NUMEQUAL(TS/Go/Rust). - Update byte-frozen opcode-sequence tests to assert the new 18-op sequence and the raw
0x00push. - Update the oracle-price conformance
expected-script.hexgolden to match the new script bytes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/runar-compiler/src/passes/rabin-codegen.ts | Updates TS Rabin Script emission to normalize digest encoding and compare numerically (BUG-011). |
| packages/runar-compiler/src/tests/rabin-codegen.test.ts | Updates TS unit test to pin the new 18-op sequence and verify the 0x00 bytes push. |
| conformance/tests/oracle-price/expected-script.hex | Updates conformance golden script hex impacted by the Rabin opcode tail change. |
| compilers/rust/src/codegen/rabin.rs | Updates Rust Rabin emission and its byte-frozen test for the new numeric-compare tail. |
| compilers/go/codegen/rabin.go | Updates Go Rabin emission to push 0x00, OP_CAT, OP_BIN2NUM, and OP_NUMEQUAL. |
| compilers/go/codegen/rabin_test.go | Updates Go byte-frozen golden test to include the bytes push and new opcodes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+14
to
+18
| * The opcode sequence is a fixed 18 ops: | ||
| * OP_SWAP | ||
| * OP_DUP OP_0 <push 65536> OP_WITHIN OP_VERIFY // 0 <= padding < 65536 (BUG-010) | ||
| * OP_ROT OP_DUP OP_MUL OP_ADD OP_SWAP OP_MOD OP_SWAP OP_SHA256 OP_EQUAL | ||
| * OP_ROT OP_DUP OP_MUL OP_ADD OP_SWAP OP_MOD | ||
| * OP_SWAP OP_SHA256 <push 0x00> OP_CAT OP_BIN2NUM OP_NUMEQUAL |
icellan
force-pushed
the
fix/rabin-digest-encoding-bug011
branch
from
August 28, 2026 13:04
69dce6f to
baa95bd
Compare
icellan
added a commit
to E-Jacko/runar
that referenced
this pull request
Aug 28, 2026
…ining 4 tiers PR icellan#146 fixed verifyRabinSig in TypeScript, Go and Rust and moved the oracle-price golden with them — but Python, Zig, Ruby and Java each own a separate Rabin codegen module and were not touched. Those four then emitted the old OP_EQUAL tail against a golden built for the new one, which is why the PR showed nine failing checks including four tier compilers. CLAUDE.md requires any language feature change in all seven tiers; this completes it. Each tier now emits the same tail as the reference: OP_SHA256 <push 0x00> OP_CAT OP_BIN2NUM OP_NUMEQUAL OP_MOD leaves a MINIMAL Script number, which carries a trailing 0x00 sign byte whenever the digest's most-significant byte has its high bit set (~50% of messages, measured 198/400), while OP_SHA256 pushes exactly 32 raw bytes. A bare OP_EQUAL is a BYTE compare and refused about half of all honest signatures on a real consensus VM. OP_NUMEQUAL never aborts, so the any-of-N pattern still yields false rather than killing the script. Each tier's byte-frozen test pinned the old 15-op sequence and is updated to 18. Both Zig pins and the Java one assumed the ONLY null slot was the push_int at index 3; there is now a push_data at 14, so they discriminate by index rather than being loosened. The Zig contiguous-hex pin is set from the compiler's ACTUAL output for that contract, not inferred. I first wrote 9d (OP_NUMEQUALVERIFY) reasoning that runar.assert would fuse the compare, as it does in oracle-price; compiling the contract shows 9c — the fusion is context-dependent. The comment records that so the next reader does not repeat the inference. Verified: * conformance oracle-price — PASS across all 7 tiers (the fixture that was failing python/zig/ruby/java) * conformance suite — 72/72, zero FAIL * zig 754/754 · java BUILD SUCCESSFUL · ruby 72/72 conformance goldens, 0 failures · python rabin codegen test green * conformance/rabin_execution_test.go — still green: the pre-fix tail rejects an honest signature, the post-fix tail accepts it, and forged / wrong-message signatures are still rejected, on the real go-sdk interpreter with a real signature from the shipped signer.
…lly, not byte-wise OP_MOD's minimal Script-number result is 33 bytes when the digest high bit is set (~50% of SHA-256 digests) vs OP_SHA256's raw 32; the bare OP_EQUAL then rejects honest signatures ~half the time on a real VM (fails-closed). Compare numerically via 0x00 || OP_BIN2NUM || OP_NUMEQUAL. Patches TS/Go/Rust emitters + TS test + oracle-price golden.
`verifyRabinSig` shipped with byte goldens across all seven tiers (fixture
`oracle-price`), a discharged Lean theorem, and NO execution coverage —
conformance/witnesses/ has no entry for the fixture. Seven tiers agreeing on
bytes proves agreement, not correctness; that is the same gap that hid the
SLH-DSA fund bug.
Five tests, real signatures from the shipped signer (packages/runar-go
RabinSign), real go-sdk interpreter with Genesis + Chronicle + ForkID:
* pre-fix tail REJECTS an honest signature (the RED proof)
* post-fix tail ACCEPTS it — the fix fails closed, so only an accept
demonstrates the repair
* post-fix still rejects a forged signature and a signature over a
different message, so the repair is not vacuous
* the compiled contract actually carries the numeric tail
On severity: the PR body says honest signatures fail 'about half the time',
and that is right — measured 198/400 = 49.5%. The signer reads the digest
little-endian (leBytesToBigInt), which already matches script-number byte
order, so the operands agree for most messages. The defect is the SIGN BYTE
alone: OP_MOD leaves a minimal script number, which carries an explicit 0x00
whenever the value's most-significant byte (h[31], read LE) has its high bit
set — 33 bytes against OP_SHA256's 32, and OP_EQUAL is a byte comparison.
A ~50% rate is worse for discovery than 100%: the first hand test of a Rabin
contract passes about half the time, so the primitive looks to work. The test
therefore pins a message whose digest triggers the sign byte
(BSV/USD=50003, h[31]=0x83) instead of a random one, which would be ~50%
flaky.
The codegen link is checked against the Go CLI, not the node compileRúnar
helper: that helper coerces a constructor arg to BigInt only when the declared
type is literally 'bigint', and oracle-price declares RabinPubKey, so the
modulus stays a string and baking fails. Confirmed non-vacuous — swapping in a
main-built compiler makes it fail with 'does not carry the numeric Rabin tail';
restoring gives 5/5.
…ining 4 tiers PR icellan#146 fixed verifyRabinSig in TypeScript, Go and Rust and moved the oracle-price golden with them — but Python, Zig, Ruby and Java each own a separate Rabin codegen module and were not touched. Those four then emitted the old OP_EQUAL tail against a golden built for the new one, which is why the PR showed nine failing checks including four tier compilers. CLAUDE.md requires any language feature change in all seven tiers; this completes it. Each tier now emits the same tail as the reference: OP_SHA256 <push 0x00> OP_CAT OP_BIN2NUM OP_NUMEQUAL OP_MOD leaves a MINIMAL Script number, which carries a trailing 0x00 sign byte whenever the digest's most-significant byte has its high bit set (~50% of messages, measured 198/400), while OP_SHA256 pushes exactly 32 raw bytes. A bare OP_EQUAL is a BYTE compare and refused about half of all honest signatures on a real consensus VM. OP_NUMEQUAL never aborts, so the any-of-N pattern still yields false rather than killing the script. Each tier's byte-frozen test pinned the old 15-op sequence and is updated to 18. Both Zig pins and the Java one assumed the ONLY null slot was the push_int at index 3; there is now a push_data at 14, so they discriminate by index rather than being loosened. The Zig contiguous-hex pin is set from the compiler's ACTUAL output for that contract, not inferred. I first wrote 9d (OP_NUMEQUALVERIFY) reasoning that runar.assert would fuse the compare, as it does in oracle-price; compiling the contract shows 9c — the fusion is context-dependent. The comment records that so the next reader does not repeat the inference. Verified: * conformance oracle-price — PASS across all 7 tiers (the fixture that was failing python/zig/ruby/java) * conformance suite — 72/72, zero FAIL * zig 754/754 · java BUILD SUCCESSFUL · ruby 72/72 conformance goldens, 0 failures · python rabin codegen test green * conformance/rabin_execution_test.go — still green: the pre-fix tail rejects an honest signature, the post-fix tail accepts it, and forged / wrong-message signatures are still rejected, on the real go-sdk interpreter with a real signature from the shipped signer.
The golden-provenance gate demanded an independent cross-check for the moved
oracle-price golden. The strong option — a witness that EXECUTES the fixture —
was previously impossible, and that impossibility is why BUG-011 reached RC:
the 72-fixture corpus pinned oracle-price's BYTES across seven tiers, but
nothing ever RAN the emitted script.
It had no witness because `settle` calls checkSig and the differential harness
could only carry literal bytes. This lifts that limit with a {"signWith": key}
marker.
The subtlety is that the two oracles verify a signature against DIFFERENT
messages, so one signature cannot satisfy both:
* interpreter — real ECDSA over the fixed TEST_MESSAGE (crypto/ecdsa.ts).
NOT a mock, despite CLAUDE.md still describing checkSig as 'always true';
interpreter.ts:765 calls verifyTestMessageSig.
* ScriptVM — real secp256k1 over the BIP-143 sighash of the synthetic
spend context.
A marker is therefore resolved per side, each valid in its own domain. Handing
one side the other's signature reports a signing-convention mismatch as a
source-vs-script divergence.
script-vm.ts now exports SYNTHETIC_SPEND_CONTEXT so the signer derives the
preimage from the object the VM actually builds its Spend from. That is
load-bearing: ScriptVM uses transactionVersion 1 while
real-crypto-execution.ts uses 2, and version is part of the BIP-143 preimage,
so a copied constant that drifted would emit signatures that fail to verify and
read as a codegen defect.
The witness pins price 50003 deliberately: its digest's most-significant byte
(h[31], read little-endian) has the high bit set, which is the ~50% of messages
where OP_MOD's minimal Script number carries a 0x00 sign byte the raw 32-byte
digest does not. That is exactly the case the pre-fix OP_EQUAL byte compare
REJECTED. A price without that property would pass against the buggy code and
pin nothing. The Rabin signature is real, from packages/runar-go RabinSign.
Three spends: accept, forged-signature reject, and valid-signature-over-a-
different-price reject — so the repair is pinned as non-vacuous, not merely
present.
script-size-baseline oracle-price 44 -> 48, read off the golden rather than
hardcoded; the fix adds exactly 4 bytes.
Verified: differential witness 3/3 · script-size gate ok=72 warn=0 fail=0.
oracle-price 44 -> 48 bytes, the mechanical consequence of the BUG-011 Rabin fix (four added bytes; OP_EQUAL -> OP_NUMEQUAL is 1:1). The entry already existed and its sha256 self-invalidated when the file changed, which is the pin working as designed — this re-signs it for the new content rather than adding a new opt-out. The underlying golden is justified by the STRONG path, not this entry: it is content-pinned in conformance/witnesses/oracle-price.json, which now executes the fixture end to end.
The witness files have a SECOND consumer: fold-equivalence.test.ts carries its
own decodeArg, so adding the marker to differential.test.ts alone broke it with
Error: unencodable witness arg: {"signWith":"alice"}
Caught by the full vitest run, which reported 1 failed FILE and 0 failed tests
— a collection error, not an assertion.
runFoldEquivalence delegates to runDifferentialExecution once per fold mode, so
markers resolve per mode automatically. That is what makes signed contracts
work here at all: the sighash subscript is the locking script and folding
changes those bytes, so fold-OFF and fold-ON need signatures over their OWN
scripts. A single pre-resolved signature would fail on whichever mode it was
not built for and read as a fold divergence.
Verified: fold-equivalence 14/14 · full vitest 477 files / 9262 tests, 0
failures.
I ported Ruby's Rabin CODEGEN in ec15494 but missed its own byte-frozen test, which still pinned the pre-BUG-011 15-op sequence: TestRabinCodegen#test_rabin_module_emits_byte_frozen_golden Expected: 15 Actual: 18 My miss, and I compounded it by reporting Ruby as green at the time: rake test runs each file in a subprocess and prints a summary per file, so 40-odd "0 failures" lines scrolled past while the one failing file's summary was buried. The aggregate line "1 test file(s) failed" was right there and I read past it. The conformance goldens genuinely did pass (72/72) because the EMITTED bytes were already correct -- only the tier's own unit pin was stale, which is exactly the kind of gap a per-file summary hides. Same treatment as the Python, Zig and Java pins: the sequence goes to 18 and the nil slot is discriminated by index, since there is now a push_data at 14 alongside the push_int at 3, rather than loosening the assertion to accept any push. Verified: rake test exits 0, all 66 test files pass.
icellan
force-pushed
the
fix/rabin-digest-encoding-bug011
branch
from
August 29, 2026 08:32
2059700 to
33e2265
Compare
The Lean model still emitted the pre-fix OP_EQUAL tail, so oracle-price dropped out of byte-exact once the compilers moved: NOTICE: 1 baselineMatches fixture(s) NO LONGER byte-exact: oracle-price FAIL: byte-exact match regressed: 56 < 57 Ported the 4-op normalization into `rabinBodyOps` and the model's `lowerVerifyRabinSigOpsLive`, then re-proved the codegen-to-spec theorem `runOps_rabinBodyOps_eq` across the new steps (push 0x00 / OP_CAT / OP_BIN2NUM / OP_NUMEQUAL). Three local opcode lemmas were needed because `Stack.Sim` is outside this module's import closure, which is why every opcode lemma here is already restated locally; OP_BIN2NUM is INLINED in runOpcode rather than routed through liftBytesUnary, so its lemma unfolds the match directly. The SPEC had to change too, and that is the substantive part. It previously read decide ((encodeMinimalLE lhs).toList = (sha256 msg).toList) - a faithful model of the OLD byte compare. That is exactly why a DISCHARGED theorem proved this codegen correct while the emitted script rejected ~50% of honest signatures on a real VM: the proof was sound and the spec encoded the defect. A codegen-to-spec equivalence can only say "the compiler computes what this predicate says", never "this predicate is what anyone wanted". The proof then caught an error in my first attempt at the new spec. I wrote decide (lhs = decodeMinimalLE (sha256 msg)) and the goal reduced to decodeMinimalLE (sha256 msg ++ 0x00) = decodeMinimalLE (sha256 msg) which is FALSE precisely when the digest's top byte has its high bit set -- the bug's own mechanism, restated. The `++ 0x00` is load-bearing and the spec now carries it, matching the bytes the script actually compares. Crypto anchors re-derived (RUNAR_VERIFICATION_REGEN=1), not re-attested: the model fingerprint moved to v1:63f:7119136b:48a17cf09a3966cb and the same 8 anchors come back byte-exact. Verified: pipelineGolden 57/72, zero FAIL, exit 0 (8 regen-live / 0 inherited / 12 inert) - lean-verify.sh 71 jobs OK - check-tcb-drift inventory sum 71 OK.
…manifest Both remaining CI failures on icellan#146 were REAL, not stale. 1. Script Execution Oracle — TestRabinVerify_CompiledContractCarriesTheNumericTail shelled out to a hardcoded "../compilers/go/runar-go". CI downloads that binary as an artifact into the REPO ROOT, so the path only exists after a local `go build`: green on my machine, red in CI. This is the SAME mistake I made on the cross-tier rejection gate earlier and fixed there by moving onto the runner's own finders. I reintroduced it in a different language a few commits later. The test now probes ../runar-go first, then ../compilers/go/runar-go, and FAILS rather than skips when neither exists — a silent skip would leave the codegen link unmade while the file still reported green. Verified under both layouts: passes with the binary in compilers/go/, and passes with it moved to the repo root to mimic CI. 2. Decompiler templates manifest — the manifest embeds each example's compiled hex, so the BUG-011 tail moved oracle-price 44 -> 48 bytes. Checked before committing: exactly ONE entry changed, oracle-price, 77 entries in and 77 out, every other hex byte-identical. That is the manifest tracking a real byte change, not a blanket restamp. Verified: decompiler tests 130 passed / 7 skipped, fingerprint drift check clean, templates check clean, script-execution oracle `ok` (70s).
…test The script-execution-oracle job builds no Go binary — it only needs the TS workspace — so neither ../runar-go nor ../compilers/go/runar-go exists there and the test failed on a missing binary rather than on codegen. Take a prebuilt binary when one is staged, otherwise build from source into the test's temp dir. Still fails rather than skips if that build fails: the test compares real codegen, and a skip would read as a pass.
`bip143-crosstier` is the only integration test that asserts block inclusion rather than mempool acceptance, and it fails intermittently in CI with "accepted to the mempool but never selected into a block" — a message that names the symptom and nothing else. On bitcoin-sv, mempool membership does not imply mining eligibility: the block assembler builds from the journal, and `getmempoolinfo` reports the two sizes separately. Reproduced on a live regtest node by running the Go suite first (CI's order) and then the TS suite: the node reaches a state with `journalsize: 0` while `getrawmempool` holds four zero-fee 7618-byte transactions, and from then on nothing confirms — not even a wallet transaction paying 20 000 sat/kB. Rebroadcasting one returns `txn-already-known`, so the node has it and is simply not mining it. Report that state on failure — pool membership, fee, and each parent's confirmation count — so the next occurrence names its own cause. The assertion is unchanged; only the error message gained evidence. Also confirm the funding transaction before spending it. An unconfirmed parent makes the spend a mempool descendant, which cannot enter the journal ahead of its ancestor, so a lagging parent would surface as the child never being mined — not what this test is asserting.
…hans txs `bip143-crosstier` is the only integration test that asserts block inclusion rather than mempool acceptance, and it failed on ~60% of full-suite runs (3 of 5 locally, and on CI's last two runs of main) with a transaction that the node had accepted and would not mine. The cause is the suite mining against itself. Vitest runs test files in parallel, every file mines via `generatetoaddress` — directly or through RPCProvider's autoMine — and two workers generating on the same tip build competing blocks. The loser's transactions are dropped from the block that wins, so a valid transaction can miss block after block. Measured on a live node: four concurrent workers, each funding an address and mining up to five blocks, left 10 of 16 funding transactions unconfirmed. Putting a mutex around just the mining calls dropped that to 0 of 16. The mutex cannot live on the test side, because autoMine mines from inside the SDK, so serialize at the file level instead: 3 further full-suite runs, 40 files and 229 tests green each time, against 3 failures in the 5 runs before. Costs ~2.5 min of wall time on this suite and removes chain reorgs as a source of nondeterminism for every test in it. Also mine before funding in `bip143-crosstier`, so the node wallet selects confirmed coins. Without it the wallet chains onto its own unconfirmed change — measured parents at 0 confirmations, against 1 when funding right after a block — which puts the group in the secondary mempool rather than the journal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
verifyRabinSigcodegen emitted… OP_MOD OP_SWAP OP_SHA256 OP_EQUAL.OP_MODpushes a minimal Script number — 33 bytes (trailing0x00sign byte) whenever the digest's top byte has its high bit set (~50% of SHA-256 digests) — whileOP_SHA256pushes 32 raw bytes. Same value, different bytes, soOP_EQUALis false and honest signatures are rejected by any consensus VM about half the time (fails-closed; no forgery risk; the pure-bigint test interpreter masks it).Fix: append an explicit
0x00sign byte to the digest (OP_CAT), collapse to minimal form (OP_BIN2NUM), compare withOP_NUMEQUAL(fuses toOP_NUMEQUALVERIFYunderassert). The numeric compare is deliberate — width-normalizing withOP_NUM2BINaborts (rather than returning false) for out-of-width values: a wrong-key mod result is uniform in[0, n), n > 2^256, soNUM2BIN(·, 32)would break the any-of-N patternverifyRabinSig(k1) || verifyRabinSig(k2). The BUG-010padding ∈ [0, 65536)bound is untouched.Verified on
@bsv/sdk2.1.6Spend(consensus interpreter): pre-fix 0/60 high-bit-digest honest signatures validate (60/60 for high-bit-clear); post-fix 60/60 in both classes, with every tamper / wrong-key /sig=0forgery / out-of-band case still rejected (120 fragment + 100 full-covenant BIP-143 samples).Patches the TS, Go and Rust emitters + the TS sequence test + the oracle-price conformance golden. The Python/Zig/Ruby/Java emitters carry the identical tail and need the same one-op change.
Cost: +4 script bytes per
verifyRabinSigcall.