From: Chris Duncan Date: Thu, 27 Aug 2026 23:50:57 +0000 (-0700) Subject: Relax verify for donna. X-Git-Url: https://git.codecow.com/?a=commitdiff_plain;h=ff780f68875d3001b6cb90b5a3ce6379d2251caa;p=nano25519.git Relax verify for donna. --- diff --git a/AGENTS.md b/AGENTS.md index 3017260..23035ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,15 +14,33 @@ boundary. Everything is synchronous and single-threaded. derive(prv[, out]) // 32-byte private key -> 32-byte public key sign(msg, prv, pub[, out]) // 32-byte block hash -> 64-byte signature verify(sig, msg, pub) // -> boolean -verify_blocks(pub, blocks) // { hash, signature }[], up to 256 -> boolean[] +verify_blocks(pub, blocks) // { hash, signature }[], up to 32 -> boolean[] ``` Inputs are `Uint8Array` or hex strings, and the output type follows the input type. `derive` and `sign` take an optional preallocated output buffer as a -trailing argument. `verify_blocks` is the Nano account-chain case — many block -hashes against one public key. It is correct across the full range as of -`914c83c`, but not yet any faster than N sequential `verify()` calls — the -per-key work is still redone per block. +trailing argument. + +`verify_blocks` is the Nano account-chain case — many block hashes against one +public key. It is correct across the full range and has vector coverage. The +public-key decompression is hoisted into `crypto_verify_pubkey` and run once per +batch, which saves `(1 - 1/n) * k` where `k` is the key work's share of one +verify: **measured k ≈ 8.5%** (byte inputs) or **≈9.7%** (hex). At the cap of 32 +that realises **~8.1% / ~9.6%**, i.e. 97% of everything available. + +**Do not raise the cap to chase throughput.** `(1 - 1/n)` is 96.9% at n=32 and +99.7% at n=341 — the remaining 2.8% of an 8.5% prize is under a quarter of a +percent of runtime, and n=341 blocks the thread for ~24 ms against ~2.2 ms. The +cap is a latency decision. More headroom would have to come from attacking the +per-block double scalar multiplication, which is a different algorithm. + +**`sign` costs two fixed-base scalar multiplications, deliberately.** It derives +the public key from the private key and throws `Invalid public key` if the one +passed in disagrees. That is RFC 8032 §5.1.6's definition of signing, and the +comparison buys keypair integrity. It roughly halves signing throughput +(~26 µs → ~49 µs). **This is the committed shape — do not "optimise" it away.** +Removing the derivation would break RFC conformance; removing the comparison +would silently accept a mismatched keypair. ## Commands @@ -57,24 +75,52 @@ stale `dist/` will silently test the previous revision. Nothing crosses the boundary as arguments. The host writes into fixed static buffers, calls an export that takes only a length or a count, and reads results -back out of `OUTPUT_BUFFER`. `src/lib/wasm.ts` resolves every pointer and byte -length once at module load and re-exports them. +back out of the output buffer for that primitive. `src/lib/wasm.ts` resolves +every pointer and byte length once at module load and re-exports them. | Buffer | Purpose | | --- | --- | -| `MESSAGE_BUFFER` | 32 KiB. A message for `sign`/`verify`, or packed 96-byte `hash \|\| signature` records for `verify_blocks` | -| `PRV_BUFFER`, `PUB_BUFFER` | 32 bytes each | -| `SIGNATURE_BUFFER` | one signature for single `verify` | -| `OUTPUT_BUFFER` | 256 B. A public key, a signature, or one result byte per verified block | +| `INPUT_MSG` | 32 KiB. A message for `sign`/`verify`, or packed 96-byte records for `verify_blocks` | +| `INPUT_PRV`, `INPUT_PUB` | 32 bytes each | +| `INPUT_SIG` | 64 B, one signature for single `verify` | +| `OUTPUT_DERIVE` | 32 B, a public key | +| `OUTPUT_SIGN` | 64 B, a signature | +| `OUTPUT_VERIFY` | 32 B, one result byte per verified block (= `MAX_VERIFY_BLOCKS`) | -`verify_blocks` writes one result byte per block, so its cap **is** -`OUTPUT_BUFFER_BYTELENGTH` — both the wasm guard and the host guard derive from -it rather than hardcoding 256, and they cannot drift when the buffer is resized. -Preserve that coupling, and note it only holds while one byte per block does. +Pointer getters are `ptrInputMsg`, `ptrInputPrv`, `ptrInputPub`, `ptrInputSig`, +`ptrOutputDerive`, `ptrOutputSign`, `ptrOutputVerify`. + +**One buffer per role, and keep it that way.** These replace a single shared +`OUTPUT_BUFFER` that had to satisfy two different constraints — at least one +byte per block, *and* at least 64 bytes for a signature. Only the first was ever +written down, so cutting the batch cap to 32 made `sign` overrun it. If you ever +merge them again, both constraints have to be stated together. + +Records in `INPUT_MSG` are **`signature` then `hash`** (64 B + 32 B), libsodium +combined-mode order. Nothing validates this; both sides just have to agree. + +The batch cap is `MAX_VERIFY_BLOCKS` (32); the wasm guard, the host guard and +`OUTPUT_VERIFY` all read it, so they cannot drift. + +⚠️ **`MAX_MESSAGE_BYTELENGTH` is currently derived from the wrong constant:** + +```ts +export const MAX_VERIFY_BLOCKS: i32 = 32 +export const MAX_MESSAGE_BYTELENGTH: i32 = MAX_VERIFY_BLOCKS << 10 // 32,768 +``` + +This is right only because both happen to be 32. A batch needs +`MAX_VERIFY_BLOCKS * SIGNEDBLOCK_BYTELENGTH` = 3,072 B, not 32 KiB. Change the +cap and the **maximum signable message length changes with it**: at 16 it halves +to 16 KiB (a silent API regression), at 64 it doubles and pushes the module from +2 pages to 4. If you touch `MAX_VERIFY_BLOCKS`, check what happened to +`MAX_MESSAGE_BYTELENGTH` and to `memory.size()`. Exported byte-length globals (`KEY_BYTELENGTH`, `MESSAGE_BUFFER_BYTELENGTH`, …) are the single source of truth for these sizes — read them rather than -hardcoding, on both sides of the boundary. +hardcoding, on both sides of the boundary. `test/index.html` currently +hardcodes the cap as `32` in two places and is the one file known to violate +this — it tracked the last cap change by hand. ## Build constraints that break normal assumptions @@ -84,19 +130,23 @@ ordinary AssemblyScript: - **`runtime: "stub"`** — bump allocator, no collector. Any allocation on a call path is a permanent leak. Call paths must allocate nothing; all state is - module-level `StaticArray` allocated once at init. + module-level `StaticArray` allocated once at init. Unused statics are **not** + tree-shaken either: a `StaticArray(341)` that nothing reads still costs + 368 B of permanent heap. Delete declarations when you delete their last use. - **`noAssert: true`, `uncheckedBehavior: "always"`** — bounds and null checks are compiled out. An out-of-range index silently corrupts neighbouring statics instead of trapping. Validate every untrusted length or count as the **first** statement of an exported function, before any indexing. - **`disable: ["mutable-globals"]`** — a global with a non-constant initialiser - cannot be exported. Only compile-time constants survive as exported globals. + cannot be exported. Only compile-time constants survive as exported globals; + arithmetic over other constants is fine and still const-folds. - **`enable: ["simd"]`** — field elements are 12 `i32` limbs, not 10. The two trailing limbs are padding; respect the stride. -- **`initialMemory: 2`** — pins the module at 2 pages. Without it the stub - allocator doubles on growth (1→2→4→8), so page counts are always powers of - two. Re-derive this whenever a buffer is resized rather than leaving it - stale; it has been wrong in both directions already. +- **`initialMemory: 2`** — pins the module at 2 pages (131,072 B); heap at rest + is 103,456 B. Without the setting the stub allocator doubles on growth + (1→2→4→8), so page counts are always powers of two. Re-derive this whenever a + buffer is resized rather than leaving it stale; it has been wrong in both + directions already. Memory growth *during* a call would detach every `Uint8Array` view the host holds, so keeping call paths allocation-free is a correctness requirement, not @@ -117,6 +167,15 @@ col)` against static string data — the wat shows `call $abort` and store, and makes AssemblyScript pass a null message pointer, so the error text is lost before it reaches the host. +**Raw memory intrinsics are unguarded in every build.** `memory.copy`, +`memory.fill` and friends are not covered by `noAssert` or `uncheckedBehavior` — +turning the safety net back on does **not** make them bounds-checked. A wrong +length handed to one of them is checked by nobody, and it will corrupt the next +object's header rather than trapping. The symptom then appears inside an +unrelated function on a later call, when something reads that object's clobbered +length. If a trap points at a function that looks innocent, suspect whatever ran +before it. + **Scrub buffers on every path, unscoped.** Every exported wasm function must leave its input buffers fully zeroed when it returns, including the throw path. Use whole-buffer `fill(0)`, never a length-scoped fill: the buffer pointers are @@ -124,6 +183,20 @@ public exports, so a caller can write more bytes than the length it then declares, and a scoped fill leaves the rest behind. A 32 KiB fill costs ~0.1 µs — roughly 0.4% of one signature — so this is never worth optimising away. +**Establish fail-closed defaults before the first early exit.** `verify_blocks` +runs `OUTPUT_VERIFY.fill(255)` *before* validating the public key, so a batch +that bails out reports every block as `false` rather than leaving stale bytes. +Setting the default in the failure branch instead is the bug this shape avoids: +a result buffer is only fail-closed if the default is written before anything +can skip past it. + +**`crypto_verify_relaxed` requires a prepared key.** It does not validate the +public key; it reuses the module-level `A` that `crypto_verify_pubkey` left +behind, which is what makes the batch hoist possible. Call `crypto_verify_pubkey` +first, or you will verify a signature against whichever key ran last, with no +error. `crypto_verify_strict` does both itself. Neither actually relaxes any +rule despite the name — see the `PROBLEM_VECTOR` note under Testing. + **Counts, lengths, and end offsets are not interchangeable, and confusing them is silent here.** This project has produced the same bug three separate times: a byte length passed where an element count was wanted, and a length passed where @@ -155,29 +228,49 @@ best-of-N with N≥7. A short warmup with one timing pass once reported an 18.6% gain that was entirely JIT warmup; re-measured properly it was under 2%, with the sign of the delta flipping between runs. Treat anything under ~2% as noise. +**Time a whole unit of N operations, not each call.** One `performance.now()` +pair around a run of N is markedly less noisy than averaging N individually +timed calls, and the noise does not cancel: timed per call, the batch-vs-loop +advantage measured ~0%; timed as whole units, the same builds measured 3–5%. +Do it the same way on both sides of any comparison. + +**Repeated inputs are not neutral — the code is variable-time.** +`ge_double_scalarmult_vartime` costs what the bit pattern of its scalars costs, +so verifying one block N times is ~4% cheaper per block than verifying N +distinct blocks. That is small enough to read as noise and large enough to move +a conclusion: it is most of the difference between the ≈10% and 13–16% figures +that both circulate for `verify_blocks`. Build inputs the way a caller would, +and keep the same distinctness on both sides. + Useful scale anchors: a wasm call boundary is ~2 ns, a 32 KiB `memory.fill` is -~0.1 µs, and one `verify` is ~85 µs. Buffer scrubbing and boundary crossing -never show up on the clock; only the point arithmetic does. +~0.1 µs, one `verify` is ~73 µs with `Uint8Array` inputs and ~80 µs with hex +strings, one `sign` is ~49 µs, and the hoisted per-key work is ~6.2 µs. Buffer scrubbing and boundary crossing never +show up on the clock; only the point arithmetic does. ## Testing -`node ./test/node.mjs` after a build. Current state is **6168 passing, 1 +`node ./test/node.mjs` after a build. Current state is **6176 passing, 1 failing**, and that one failure is expected. -`verify_blocks` has **no vector coverage yet** — a green suite says nothing -about it. Exercise it directly, and across the whole range rather than at one -size: its bugs have twice been invisible below a boundary (64, then 256) and -only appeared past it. +`verify_blocks` now has vector coverage — a single block, a full 32-block +batch, both range errors, and the three block-shape `TypeError`s. Coverage still +runs thin in the middle: its bugs have twice been invisible below a boundary +(64, then 256) and appeared only past it, so exercise new changes across the +whole range rather than at one size, and always corrupt at least one signature +in the batch — the bug this feature has actually shipped is reporting invalid +signatures as valid, which an all-valid batch cannot catch. The failing case is `PROBLEM_VECTOR`: a live cemented Nano block from a small-order account (`nano_11a11…`, public key `0100…00`) that network consensus accepted but strict Ed25519 rejects. The test asserts `true` to match the ledger; `verify()` returns `false` to match the spec. Do not resolve this by weakening the small-order check in strict verification. The intended shape is a -relaxed variant for Nano blocks alongside a strict one for general use; -`crypto_verify_relaxed` and `crypto_verify_strict` both exist, but the relaxed -one is still byte-identical to the strict one, so the split does not yet buy -anything. +relaxed variant for Nano blocks alongside a strict one for general use. +`crypto_verify_relaxed` and `crypto_verify_strict` are no longer identical — +relaxed now means "the key was already validated" — but **neither relaxes the +small-order rule**, so the ledger-versus-spec decision is still open. Both paths +reach it through the single `crypto_verify_pubkey`, so whichever way it goes, +it changes in one place. ## Style diff --git a/src/assembly/crypto_verify.ts b/src/assembly/crypto_verify.ts index 402b9af..da31e18 100644 --- a/src/assembly/crypto_verify.ts +++ b/src/assembly/crypto_verify.ts @@ -3,9 +3,11 @@ import { BLOCKHASH_BYTELENGTH, KEY_BYTELENGTH } from '.' import { Blake2b } from './blake2b' +import { fe_0 } from './fe' import { ge_double_scalarmult_vartime_to_p3, ge_frombytes, ge_frombytes_negate_vartime, ge_has_small_order, ge_is_canonical } from './ge' -import { ge_p3, ge_sub_p3 } from './p' +import { ge_p3, ge_p3_tobytes, ge_sub_p3 } from './p' import { sc_is_canonical, sc_reduce } from './sc' +import { equalbytes } from './utils' // crypto_hash function const blake2b = new Blake2b() @@ -17,35 +19,40 @@ const expected_r = new ge_p3() const h = new StaticArray(64) const sb_ah = new ge_p3() const check = new ge_p3() +const check_r = new StaticArray(32) /** - * Verify public key `pub` is canonical, non-malleable, and correct. - * @returns -1 if public key fails to verify, else return 0 if public key is good + * Verify public key `pub` can be decoded. + * @returns -1 if public key fails to decode, else return 0 */ -export function crypto_verify_pubkey (pub: StaticArray): i32 { - - // fail if public key `k` is non-canonical (`p = 2²⁵⁵-19 ≤ k`) - if (!ge_is_canonical(pub)) return -1 - - if (ge_frombytes_negate_vartime(A, pub) != 0) return -1 - if (ge_has_small_order(A) != 0) return -1 - - return 0 +export function crypto_verify_decodepubkey (pub: StaticArray): i32 { + fe_0(A.X) + fe_0(A.Y) + fe_0(A.Z) + fe_0(A.T) + // fail if public key cannot be decoded + return ge_frombytes_negate_vartime(A, pub) } /** * Verify signature `s` was made by signing block hash `M` using public key - * `pub`. + * `pub`. Based on ed25519-donna to align with nano-node. Differences from + * `crypto_verify_strict`: + * + * - S < 2²⁵³, instead of S < L + * - Skip canonical and small-order checks on public key + * - Skip checks on r and sb_ah * - * IMPORTANT: Callers MUST call `crypto_verify_pubkey` first in order to set `A` - * for the scalar multiplication step before checking `sB = R + hA`. + * IMPORTANT: Callers MUST call `crypto_verify_decodepubkey` first in order to + * set `A` for the scalar multiplication step before checking `sB = R + hA`. * @returns -1 if signature fails to verify, else return 0 if signature is good */ export function crypto_verify_relaxed (s: StaticArray, M: StaticArray, pub: StaticArray): i32 { - // fail if private scalar `S` is non-canonical (`L ≤ S`) + // fail if private scalar `S` is out of range (`2²⁵³ ≤ S`) + if ((s[63] & 224) != 0) return -1 + memory.copy(changetype(S), changetype(s) + 32, 32) - if (!sc_is_canonical(S)) return -1 if (ge_frombytes(expected_r, s) != 0) return -1 if (ge_has_small_order(expected_r) != 0) return -1 @@ -58,9 +65,9 @@ export function crypto_verify_relaxed (s: StaticArray, M: StaticArray, p sc_reduce(h) ge_double_scalarmult_vartime_to_p3(sb_ah, h, A, S) - ge_sub_p3(check, expected_r, sb_ah) + ge_p3_tobytes(check_r, sb_ah) - return ge_has_small_order(check) - 1 + return equalbytes(s, check_r) - 1 } /** @@ -72,8 +79,15 @@ export function crypto_verify_relaxed (s: StaticArray, M: StaticArray, p * @returns -1 if signature fails to verify, else return 0 if signature is good */ export function crypto_verify_strict (s: StaticArray, M: StaticArray, mlen: i32, pub: StaticArray): i32 { - // Check public key is valid - if (crypto_verify_pubkey(pub) != 0) return -1 + + // fail if public key `k` is non-canonical (`p = 2²⁵⁵-19 ≤ k`) + if (!ge_is_canonical(pub)) return -1 + + // fail if public key cannot be decoded + if (crypto_verify_decodepubkey(pub) != 0) return -1 + + // fail if public key `k` is small order + if (ge_has_small_order(A) != 0) return -1 // fail if private scalar `S` is non-canonical (`L ≤ S`) memory.copy(changetype(S), changetype(s) + 32, 32) diff --git a/src/assembly/index.ts b/src/assembly/index.ts index 79a3712..c67eaba 100644 --- a/src/assembly/index.ts +++ b/src/assembly/index.ts @@ -3,12 +3,12 @@ import { crypto_derive } from './crypto_derive' import { crypto_sign } from './crypto_sign' -import { crypto_verify_pubkey, crypto_verify_relaxed, crypto_verify_strict } from './crypto_verify' +import { crypto_verify_decodepubkey, crypto_verify_relaxed, crypto_verify_strict } from './crypto_verify' export const BLOCKHASH_BYTELENGTH: i32 = 32 export const KEY_BYTELENGTH: i32 = 32 export const MAX_VERIFY_BLOCKS: i32 = 32 -export const MAX_MESSAGE_BYTELENGTH: i32 = 1 << 10 +export const MAX_MESSAGE_BYTELENGTH: i32 = 1 << 15 export const SIGNATURE_BYTELENGTH: i32 = 64 export const SIGNEDBLOCK_BYTELENGTH: i32 = BLOCKHASH_BYTELENGTH + SIGNATURE_BYTELENGTH @@ -214,7 +214,7 @@ export function verify_blocks (count: i32): void { INPUT_PUB.fill(0) // Verify public key before proceeding with signature verification - if (crypto_verify_pubkey(pub) == 0) { + if (crypto_verify_decodepubkey(pub) == 0) { // Iterate over block signature/hash pairs for (let i = 0, ptr = changetype(INPUT_MSG); i < count; i++, ptr += 96) { diff --git a/src/assembly/utils.ts b/src/assembly/utils.ts index 985c2cb..f17565b 100644 --- a/src/assembly/utils.ts +++ b/src/assembly/utils.ts @@ -24,6 +24,22 @@ export function equal (b: i8, c: i8): u8 { return u8((y - 1) >> 31) /* 1: yes; 0: no */ } +/** + * Constant-time byte comparison. + * @param {StaticArray} f + * @param {StaticArray} g + * @returns 1 if all bytes are equal, else 0 + */ +//@ts-expect-error +@inline +export function equalbytes (f: StaticArray, g: StaticArray): u8 { + let s: u8 = 0 + for (let i = 0; i < 12; i++) { + s |= f[i] ^ g[i] + } + return ((s - 1) >> 8) & 1 +} + //@ts-expect-error @inline export function load_3 (input: StaticArray, i: u8): u64 {