initial implementation of the groth16 verifier by zz-sol · Pull Request #4 · solana-program/groth16-verifier-program

zz-sol · 2026-09-17T12:18:32Z

#2 got merged/closed because of a forced push. reopen this PR

…uction builders

…fy instruction

… arkworks, registry and malformed-input tests

… tests

… budget notes

samkim-crypto

Okay, some comments from my pass through the program.

samkim-crypto · 2026-09-17T14:28:57Z

+| `3` | `Verify`            | vk PDA (r)                                        | none              | `proof ‖ public_inputs`; the hot path |
+| `4` | `CloseStaging`      | authority (s,w), staging (w)                      | stored authority  | Refunds staging rent. Canonical accounts cannot be closed |
+
+### Registration


It would be good to document how an application should distribute the matching proving key and how users can check that it corresponds to the registered verifying key. I think keeping the proving key off-chain makes sense, but the example workflow should cover obtaining it so users can generate proofs independently.

Fixed in 21236dd, and some clarification in d895da5

samkim-crypto · 2026-09-17T14:30:05Z

+One client-side rule keeps the staging account private: **`create_account` and
+`InitializeStaging` go in the same transaction.** `InitializeStaging` can only
+check that the account is program-owned and blank; it has no way to know who
+paid for it. An account created in one transaction and initialized in the next
+can be initialized by a third party in between, who then owns its rent through
+`CloseStaging`. The subsequent `Write`s may be spread over as many transactions
+as the key needs. Small keys fit `create_account ‖ InitializeStaging ‖ Write ‖
+Publish` in one transaction; large ones take several, with no window in which a
+third party can affect the outcome.


Could we add a runnable registration-and-verification example, with a helper that constructs staging account creation and initialization together? It would be useful to show chunked uploads, publication, and a consuming program checking the expected key address before verification.

Fixed in 21236dd

samkim-crypto · 2026-09-17T14:36:09Z

+    if !payload.is_empty() {
+        return Err(ProgramError::InvalidInstructionData);
+    }
+    let [authority, payer, staging, key, _system] = accounts else {


I might be misunderstanding how Pinocchio handles MAX_ACCOUNTS, but is this check actually needed? It looks like the entrypoint only deserializes five accounts, so any additional accounts would be discarded before reaching this check anyway.

Addressed in 5575269

samkim-crypto · 2026-09-17T14:37:42Z

+| --- | ------------------- | ------------------------------------------------- | ----------------- | ----- |
+| `0` | `InitializeStaging` | authority (s), staging (w)                        | authority         | Data `num_public_inputs: u16`, rejected if `n > 151`. Requires the account be owned by the program, uninitialized, and exactly `40 + 448 + 64·(n+1)` bytes. Writes the header. **Must be in the same transaction as the `create_account` that made the staging account** — see [Registration](#registration) |
+| `1` | `Write`             | authority (s), staging (w)                        | stored authority  | Data `offset: u32 ‖ bytes`. `offset` is relative to the **body**; the write must satisfy `offset + len ≤ body_len` with overflow-checked arithmetic. The header is never writable |
+| `2` | `Publish`           | authority (s,w), payer (s,w), staging (w), vk PDA (w), system | stored authority | No instruction data. Validates the staging body, derives the canonical PDA from `sha256(body)`, brings it into existence at its exact final size, copies the body, writes the header — all in one instruction. Closes staging, refunding its rent to authority (which is why authority is writable) |


Publish requires both the authority and payer to sign, but the “Signer” column seems to only list the authority. Should we include the payer here too?

Fixed in 21236dd

samkim-crypto · 2026-09-17T14:41:45Z

+pub fn process_instruction(
+    program_id: &Address,
+    accounts: &mut [AccountView],
+    data: &[u8],
+) -> ProgramResult {
+    let (&tag, payload) = data
+        .split_first()
+        .ok_or(ProgramError::InvalidInstructionData)?;
+    match Tag::from_u8(tag).ok_or(ProgramError::InvalidInstructionData)? {
+        Tag::Verify => processor::verify::process(program_id, accounts, payload),
+        Tag::InitializeStaging => {
+            processor::initialize_staging::process(program_id, accounts, payload)
+        }
+        Tag::Write => processor::write::process(program_id, accounts, payload),
+        Tag::Publish => processor::publish::process(program_id, accounts, payload),
+        Tag::CloseStaging => processor::close_staging::process(program_id, accounts, payload),
+    }
+}


Nit: I think moving this process_instruction function to processor/mod.rs and just keeping lib.rs focused on declarations and entrypoint setup seem more natural to me. This is more of a stylistic preference, so I will leave it up to you though.

Fixed in e35b573

samkim-crypto · 2026-09-17T14:42:44Z

+//! Verification and layout errors from [`solana_groth16_verify`] are exposed as
+//! `ProgramError::Custom(code)` with the codes below, so a CPI caller can tell
+//! "proof did not verify" apart from "malformed input". Registry-level
+//! failures use the standard `ProgramError` variants that describe them.


Nit: The module introduction says registry failures use standard ProgramError variants, but several use custom codes 100–104. Could we maybe distinguish standard account/signature failures from custom registry failures?

Fixed in b58e289

samkim-crypto · 2026-09-17T14:44:23Z

+}
+
+#[test]
+fn republishing_fails_and_leaves_the_first_intact() {


Maybe we can add tests that pass a published key account to Write, InitializeStaging, and CloseStaging, asserting rejection and unchanged account state? Published-key immutability is central to Verify’s security assumptions, so I think it would be useful to cover it explicitly.

Fixed in bef975b

samkim-crypto · 2026-09-17T14:45:41Z

+            accounts.push(payer.clone());
+        }
+
+        let result = self


Could we add a test using process_transaction_instructions for CreateAccount + InitializeStaging, including rollback when initialization fails? The current instruction-chain helper executes each instruction in a separate transaction context, whereas clients must submit these two instructions atomically.

Fixed in bef975b

samkim-crypto · 2026-09-17T14:46:59Z

+    let vk = VerifyingKey::from_body_with_len(body, n).map_err(map_groth16)?;
+
+    // --- 2. validate every point ------------------------------------------------
+    vk.validate_for_publish().map_err(map_groth16)?;


It might be better to perform the key-address and existing-account checks before validate_for_publish(). That would avoid the expensive point validation for duplicate publications or an incorrect target address.

Fixed in b430308

samkim-crypto

Starting to dig into the verify library, but some initial comments below.

samkim-crypto · 2026-09-17T15:32:04Z

+//!
+//! let vk = VerifyingKey::from_body(key_body)?;
+//! let (proof, public_inputs) = Proof::split_from(instruction_data)?;
+//! verify(&vk, &proof, public_inputs)?;


Could we make the key-validation precondition explicit here and the crate-level example? VerifyingKey::from_body only checks the layout, so the example should either show validate_for_publish when accepting a key or explain where the key has already been validated.

Fixed in d895da5

samkim-crypto · 2026-09-17T15:34:13Z

+mod proof;
+#[cfg(feature = "verify")]
+pub mod scalar;
+#[cfg(feature = "verify")]


I am wondering if we should expose the account-size helpers without requiring the verify feature? An instruction-only client still needs staging_account_len to create the staging account, and shouldn’t need the host verification dependencies just to calculate its size. But maybe we can address this in a follow-up.

Fixed in 21236dd

zz-sol added 11 commits September 12, 2026 15:03

Initial repository skeleton: workspace, toolchain, Makefile and CI

dc62038

Add solana-groth16-verify: stateless BN254 Groth16 verifier and instr…

95fbbfd

…uction builders

Add solana-groth16-program: Pinocchio verifying-key registry and Veri…

1edd2d6

…fy instruction

Add groth16-convert arkworks bridge and the Mollusk test harness with…

555a5f4

… arkworks, registry and malformed-input tests

Add gnark key and proof parsers, the gnark fixture and its end-to-end…

0dae425

… tests

Add groth16-bench: stage-isolating SBF program, CU breakdown test and…

b105c8f

… budget notes

Clean up: full README, design notes and host coverage target

e2c3d7b

recfactor

f2910ec

Update design.md

61a75be

Update design.md

5f23907

linter

6179aed

zz-sol marked this pull request as ready for review September 17, 2026 12:18

zz-sol mentioned this pull request Sep 17, 2026

initial implementation of the groth16 verifier #2

Merged

samkim-crypto reviewed Sep 17, 2026

View reviewed changes

zz-sol added 9 commits September 18, 2026 15:26

Merge remote-tracking branch 'upstream'

ace6a9b

Add registration flow utilities and docs

21236dd

improve docs for errors

b58e289

update docs w.r.t. key validation

d895da5

Check PDA address before point validation

b430308

Clarify account-count handling

5575269

Add atomic staging and key immutability tests

bef975b

Move entrypoint into processor module

e35b573

Merge branch 'main' into main

d5ed85f

Conversation

zz-sol commented Sep 17, 2026

Uh oh!

samkim-crypto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zz-sol Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samkim-crypto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zz-sol Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

samkim-crypto left a comment •

edited

Loading

zz-sol Sep 19, 2026 •

edited

Loading

samkim-crypto left a comment •

edited

Loading

zz-sol Sep 19, 2026 •

edited

Loading