Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major rewrite of Solana crate which switches the programs over to #11

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Brando753
Copy link

@Brando753 Brando753 commented Dec 10, 2024

Anchor. Adds support for a Verifier Router which can support multiple routers. Supports Emergency Stop calls.

Work In Progress: Still Requires Rewrite of Code Comments, Documentation, Some Examples, and User Scripts.

Add support for Program Verifier Routing and Emergency Stops.

  • Groth16 Refactors
  • Solana Verifier Router
  • Solana Emeregency Stop
  • Utility crates for use with these contracts
  • A simple contract that verifies a RiscV proof using the main contracts.
  • Deployment scripts and management scripts for Solana contracts that can run on testnet.
  • A TransactionSigner class that supports Fireblocks for use with Deployment Scripts

Anchor. Adds support for a Verifier Router which can support multiple
routers. Supports Emergency Stop calls.

Work In Progress: Still Requires Rewrite of Code Comments,
Documentation, Some Examples, and User Scripts.
much work to calculate PDA's. Finished E2E test. Currently still needs
a transactionSigner for fireblocks, updated comments, and GROTH16 is not
verifier is not passing our example program, I must test if my
conversion to anchor caused this.
@Brando753
Copy link
Author

The main functionality is complete. The final step is documenting the code, adding comments, and cleaning up.

@Brando753
Copy link
Author

Code cleanup complete; Only waiting on Documentation of Readmes and updating/adding comments.

The example program is not working as the verifier is returning a VerificationError on the run; I tested the example code against the original Solana verifier program and got the same error; @hans, am I passing in the correct arguments to the groth16 verifier?

@Brando753 Brando753 marked this pull request as ready for review December 24, 2024 13:33
@Brando753
Copy link
Author

Documentation, code comments, and readmes have been added. The verifier still throws a verification error, but the router is complete. This is ready for code review.

@Brando753 Brando753 requested a review from hmrtn December 28, 2024 14:10
Copy link
Member

@hmrtn hmrtn left a comment

Choose a reason for hiding this comment

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

Overall, I think this PR is in the right direction. The groth16, estop, and router logic seems correct. Further testing and review is still needed to merge.

Aside from small fixes, there is one issue that I think must be investigated, namely the emergency_stop_with_proof commented on in the thread.

I also notice some large gaps in testing coverage:

  • While there are tests for the "bad verifier", there aren't comprehensive tests demonstrating successful verification of actual groth16 proofs
  • tests for correct pairing checks and public input validation
  • tests for invalid scalar values (exceeding BASE_FIELD_MODULUS_Q)
  • tests for incorrect number of public inputs
  • tests for the vk_ic length validation
  • general rust unit tests for all modules

As a side note, from here out lets have smaller and more focused PR's and commits.

solana-examples/hello-world/host/Cargo.toml Outdated Show resolved Hide resolved
solana-examples/hello-world/methods/Cargo.toml Outdated Show resolved Hide resolved
overflow-checks = true
lto = "fat"
codegen-units = 1
[profile.release.build-override]
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding breaks for all sections in TOML files

Suggested change
[profile.release.build-override]
[profile.release.build-override]

solana-examples/hello-world/host/src/main.rs Outdated Show resolved Hide resolved
solana-verifier/.eslintignore.swp Outdated Show resolved Hide resolved
solana-ownable/ownable/README.md Show resolved Hide resolved
solana-verifier/programs/groth_16_verifier/src/lib.rs Outdated Show resolved Hide resolved
solana-verifier/migrations/deploy.ts Outdated Show resolved Hide resolved
Copy link
Author

@Brando753 Brando753 left a comment

Choose a reason for hiding this comment

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

Aside from small fixes, there is one issue that I think must be investigated, namely the emergency_stop_with_proof commented on in the thread.

I commented specifically on that section, but as is, it's not an issue, though we could document it better.

I also notice some large gaps in testing coverage:
While there are tests for the "bad verifier", there aren't comprehensive tests demonstrating successful verification of actual groth16 proofs

Yes, this feature was to create a verifier router, and so all the tests were done with a mock verifier that always has simple known inputs that it will validate on to allow us to test if the router is correctly routing, passing in the data correctly, and handling emergency proofs correctly. The verifier I was given for this feature was presumed to be unit-tested and working, though the example program does test the verifier as an integration test.
Regarding the routing, emergency stops, ownable macro, and entire integration test, I do not believe we have any missing coverage. The verifier itself falls out of the scope of this PR.

general rust unit tests for all modules

Unit testing was done in typescript as many aspects of anchor make the smaller module testing difficult. Much of the router code heavily relies upon the strongly typed elements of anchor, and therefore, unit testing was done in tests/router-verifier.ts, and the integration test was done in the example program and the ownable test program.

solana-examples/hello-world/host/Cargo.toml Outdated Show resolved Hide resolved
solana-examples/hello-world/methods/Cargo.toml Outdated Show resolved Hide resolved
solana-examples/README.md Outdated Show resolved Hide resolved
solana-examples/hello-world/host/src/main.rs Outdated Show resolved Hide resolved
solana-examples/hello-world/host/src/main.rs Outdated Show resolved Hide resolved
solana-verifier/.eslintignore.swp Outdated Show resolved Hide resolved
solana-verifier/programs/groth_16_verifier/Cargo.toml Outdated Show resolved Hide resolved
solana-verifier/programs/groth_16_verifier/src/lib.rs Outdated Show resolved Hide resolved
@Brando753 Brando753 requested a review from hmrtn January 8, 2025 20:54
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