Risc0 circuit #98

Avi-D-coder · 2024-05-13T08:36:14Z

This is the risc0 circuit that me and @jonas089 worked on, the branch is based on @marijanp's nixified risc0 toolchain.

TODO (not necessarily before merge):

groth16 wrapping from jonas089's branch.
~~tests.~~
Switch to bincode2.0rc3 from json.
Merge the logic in the server with the logic in kairos-circuit-logic.
The logic in the server does a bunch of prechecks that are important in that setting.
get asn1 compiling in risc0-guest for signature verification.
~~use patched sha256 crate from risc0~~

Co-authored-by: jonas pauli <[email protected]>

jonas089 · 2024-05-13T11:15:19Z

I personally liked my initial structure a lot better.
It was much easier to work with and easier to read.
kairos-prover-server is an unnecessary crate and all of this should happen inside the kairos-server.

It also doesn't look like there are any tests for the proving process.
If I just overlooked them please point them out.

Avi-D-coder · 2024-05-13T11:28:52Z

No tests yet.
kairos-prover-server is needed, it could be a CLI instead as marijanp has suggested.

But they compile with different toolchains so some separation is required.
I made it a microservice because I expect proving would occur on machines with GPUs.

The bare minimum logic is in the risc0 workspace since that workspace is still broken for RA.

jonas089 · 2024-05-13T11:31:16Z

Also how am I supposed to construct a Snapshot when there are no tests?

jonas089 · 2024-05-13T11:33:32Z

No tests yet. kairos-prover-server is needed, it could be a CLI instead as marijanp has suggested.

But they compile with different toolchains so some separation is required. I made it a microservice because I expect proving would occur on machines with GPUs.

The bare minimum logic is in the risc0 workspace since that workspace is still broken for RA.

Proving a Batch should not be an API endpoint. It should happen automatically on the kairos-server every n seconds / era / transactions.

jonas089

Without tests this can't be merged. We need an integration test for a scenario where a few Transactions have occurred and the batch proof is generated. Then the proof should be verified.

Such a tests is a requirement to

see that the circuit code works
develop the contract entry point that verifies the proof

Avi-D-coder · 2024-05-13T11:44:10Z

Also how am I supposed to construct a Snapshot when there are not tests?

fine grained tests will come soon, snapshot is constructed by existing code in the server (batch manager) if you want to take a crack at constructing one take a look at that, and or the property tests i have in the trie. We need a good property test generator like i have in the ops trie tests.

with that being said if you look at the signature of the trait I introduced in the prover server and implement in the prover, that is what you should build the contract end point around.

it should provide snapshot, ProofInputs and handle the output.

jonas089 · 2024-05-13T11:49:31Z

Also how am I supposed to construct a Snapshot when there are not tests?

fine grained tests will come soon, snapshot is constructed by existing code in the server (batch manager) if you want to take a crack at constructing one take a look at that, and or the property tests i have in the trie. We need a good property test generator like i have in the ops trie tests.

with that being said if you look at the signature of the trait I introduced in the prover server and implement in the prover, that is what you should build the contract end point around.

it should provide snapshot, ProofInputs and handle the output.

I can't begin to write the contract entry point before this PR has been merged and this PR can't be merged without tests. The entry point will depend on types exposed by this PR so therefore we need to merge it first and it must be obvious how a proof is constructed and we must know for sure that the circuit code works as is.

Avi-D-coder · 2024-05-13T12:04:09Z

i'd suggest pulling this branch into a branch based on the refactored deposit contract and following the types from there.

alternatively just copy the ProofInputs/Outputs types directly into your file and don't worry about the function that turns one to the other. just make the body todo and make the logic all type check and lint.

I can write an initial prop test later today or tomorrow.

but don't let that block you the proof Inputs and Outputs types are fixed. if you write the logic around those types it will work.

jonas089 · 2024-05-13T12:06:30Z

i'd suggest pulling this branch into a branch based on the refactored deposit contract and following the types from there.

alternatively just copy the ProofInputs/Outputs types directly into your file and don't worry about the function that turns one to the other. just make the body todo and make the logic all type check and lint.

I can write an initial prop test later today or tomorrow.

but don't let that block you the proof Inputs and Outputs types are fixed. if you write the logic around those types it will work.

This workflow is incredibly messy, we already have too many branches and I'm sure that most of us have lost track of what's happening on them. Merging all of this will be a huge pain since Marijan is also currently trying to merge something from his deposit contract into my deposit contract.

Avi-D-coder · 2024-05-13T12:13:57Z

This workflow is incredibly messy

it is indeed, maybe one day pijul or stacked diffs will save us, but until then we have to live with the trade offs. complete features in a single pr are going to result in branches that live a while. on the other hand merging incomplete code results in churn and quality issues.

copying two types is about the minimum amount of coupling between branches you could do. but yes it's always a mess, until the rebase.

github-actions · 2024-05-13T13:22:00Z

File	Coverage
All files	`43%`	❌
kairos-server/src/state/transactions/batch_state.rs	`42%`	❌
kairos-test-utils/src/cctl.rs	`87%`	✅
kairos-crypto/src/implementations/casper.rs	`6%`	❌
kairos-prover-server/src/lib.rs	`0%`	❌
kairos-server/src/config.rs	`0%`	❌
kairos-server/src/errors.rs	`12%`	❌
kairos-server/src/lib.rs	`95%`	✅
kairos-server/src/state.rs	`90%`	✅
kairos-server/src/utils.rs	`22%`	❌
kairos-circuit-logic/src/account_trie.rs	`0%`	❌
kairos-tx/src/asn.rs	`51%`	❌
kairos-tx/src/error.rs	`0%`	❌
kairos-test-utils/src/cctl/parsers.rs	`66%`	✅
kairos-server/src/state/trie.rs	`35%`	❌
kairos-server/src/routes/deposit.rs	`88%`	✅
kairos-server/src/routes/transfer.rs	`90%`	✅
kairos-server/tests/transactions.rs	`85%`	✅

Minimum allowed coverage is 60%

Generated by 🐒 cobertura-action against 3bcc368

Co-Authored-By: Marijan Petričević <[email protected]>

marijanp · 2024-05-31T07:34:11Z

kairos-prover/kairos-tx/Cargo.toml

-sha2 = "0.10"
+num-traits = { version = "0.2", default-features = false }
+rasn = { version = "0.14", default-features = false, features = ["macros"] }
+sha2 = { version = "0.10", default-features = false }


Does that mean we have to add the patch to the workspace or is it enough if one crate patches it?

kairos-prover/kairos-circuit-logic/src/transactions.rs

marijanp · 2024-05-31T07:52:11Z

kairos-prover/kairos-circuit-logic/src/transactions.rs

+    impl Default for AccountsState {
+        fn default() -> Self {
+            unreachable!("AccountsState should always be created with AccountsState::new()");
+        }
+    }


Is this considered good practice?

Never seen it before, but it successfully caught usage of proptest::any instead of any_with.

Proptest calls default if you don't provide a the argument.

I don't think I use AccountsState as a Arbitrary parameter any more but it's easy to accidentally do when writing proptests with the test-strategy macros.

marijanp · 2024-05-31T07:55:16Z

kairos-prover/kairos-circuit-logic/src/transactions.rs

+
+    #[derive(Debug, Clone)]
+    pub struct Accounts<A: Debug> {
+        pub pub_keys: Vec<Rc<PublicKey>>,


Why do we maintain an additional list of pub_keys here? isn't it a duplication of the hashmap below?

we sample keys from the vector. hashmaps can't have an efficient random sample function. usize -> PublicKey

Why do you want random samples? You could just hardcode test vectors.

Random samples are part of property testing.
In proptest or quickcheck you write code to generate pseudo random instances of your type and then test your code works as expected.

This is better than fixed testing because you cover more of your functions domain.

If your code fails the your test will be run again with a semantically smaller input. This is called shrinking. It allows you to find a minimal example that breaks your assumptions.

kairos-prover/kairos-circuit-logic/src/transactions.rs

kairos-prover/kairos-circuit-logic/src/account_trie.rs

marijanp · 2024-05-31T08:22:38Z

kairos-prover/kairos-circuit-logic/src/account_trie.rs

+        Ok(())
+    }
+
+    pub fn deposit(&mut self, deposit: &L1Deposit) -> Result<(), TxnErr> {


Why the choice to make this associated function take the L1Deposit struct directly but in withdraw and transfer you decompose the types. We could make it uniform and use the structs in withdraw and transfer too

L1Deposit contains the recipient public key, and amount, it's only a payload.
The transfer and withdraw take a payload and the sender public key and a nonce.
The sender account will have the fund removed, and nonce checked/bumped.
A deposit has no sender account, since it comes from the L1 so it's only a payload.

fn transfer(&mut self, sender: &PublicKey, transfer: &Transfer, nonce: u64) -> Result<(), TxnErr>
fn withdraw(&mut self, withdrawer: &PublicKey, withdraw: &Withdraw, nonce: u64) -> Result<(), TxnErr>

kairos-prover/kairos-circuit-logic/src/account_trie.rs

koxu1996 · 2024-06-03T08:46:58Z

I noticed that kairos-tx was moved to the prover directory. It should not be there, as it is a common module used by other root modules (cli, crypto, server) and it's not specifically related to the prover itself.

As clarified with @marijanp, this change was implemented to resolve an issue with Nix sourcing. I am really unhappy about such changes. Nix should be an extra feature, not something that compromises the quality of our codebase.

Update: fixed in #112.

kairos-prover/kairos-prover-risc0-server/src/main.rs

kairos-prover/default.nix

kairos-prover/kairos-prover-risc0-server/src/main.rs

koxu1996 · 2024-06-03T09:44:43Z

kairos-prover/kairos-circuit-logic/src/lib.rs

+    /// TODO consider replacing with a count and hash of the processed deposits
+    pub deposits: Box<[L1Deposit]>,


For future optimization we need hashing deposits, that's for sure. Well, that could be solved later with kairos-tx used for deposit - #88.

We could just make this a Vec of Strings and use hex::encode(deposit.hash())

This is something different.
By hashing here I mean hashing over all the deposits in the batch.
That gives a single hash to serialize, but comes at the cost of an additional sha256 in and outside the vm.

Not sure if it's worth it, but it's a possible future optimization.

koxu1996 · 2024-06-03T10:47:04Z

TODO (not necessarily before merge):

...

get asn1 compiling in risc0-guest for signature verification.

If we want (I think we should) to verify signatures inside ZK VM, then I am not sure why we have duplicate types, e.g. KairosTransaction and Transaction, but we can resolve this post PR.

koxu1996

@Avi-D-coder Great work, especially trie integration!

I am requesting one change - use explicit prover to avoid Bonsai, then I would be happy to approve this PR.

kairos-tx: move to project root

Risc0 dev when testing

jonas089

Some of the suggestions are good, but I think that we can merge this PR already and update bits of it soon after. I don't see any major issues with it and therefore think that it's ready to be made available in the main workspace.

jonas089 · 2024-05-22T13:08:41Z

kairos-circuit-logic/src/account_trie.rs

+        for batch in batches.into_iter() {
+            let mut account_trie = AccountTrie::new_try_from_db(db.clone(), prior_root_hash)
+                .expect("Failed to create account trie");
+            account_trie
+                .apply_batch(batch.iter().cloned())
+                .expect("Failed to apply batch");
+
+            let new_root_hash = account_trie
+                .txn
+                .commit(&mut DigestHasher::<sha2::Sha256>::default())
+                .expect("Failed to commit transaction");
+
+            let trie_snapshot = account_trie.txn.build_initial_snapshot();
+
+            let proof_inputs = ProofInputs {
+                transactions: batch.into_boxed_slice(),
+                trie_snapshot,
+            };
+
+            let ProofOutputs {
+                pre_batch_trie_root,
+                post_batch_trie_root,
+                deposits: _,
+                withdrawals: _,
+            } = proving_hook(proof_inputs).expect("Failed to prove execution");
+
+            assert_eq!(pre_batch_trie_root, prior_root_hash);
+            assert_eq!(post_batch_trie_root, new_root_hash);
+            prior_root_hash = new_root_hash;
+        }


db is not updated in this loop => the test fails for the second Batch when using the real proving backend.

Suggestion for improvement: update the db variable or only test with a single Batch.

I have tested this and got an account does not exist error since the second Batch doesn't start with a Deposit.

@Avi-D-coder Have you seen this?

commit updates the db.
Not sure what error you are hitting.

I don't get the error and CI passes.
Let's debug on your machine after our meeting.

jonas089 · 2024-06-03T12:17:27Z

kairos-prover/kairos-circuit-logic/src/transactions.rs

+
+    #[derive(Debug, Clone)]
+    pub struct Accounts<A: Debug> {
+        pub pub_keys: Vec<Rc<PublicKey>>,


Why do you want random samples? You could just hardcode test vectors.

jonas089 · 2024-06-03T14:28:01Z

kairos-prover/kairos-circuit-logic/src/lib.rs

+    /// TODO consider replacing with a count and hash of the processed deposits
+    pub deposits: Box<[L1Deposit]>,


We could just make this a Vec of Strings and use hex::encode(deposit.hash())

kairos-prover/kairos-prover-risc0-server/src/main.rs

jonas089

lgtm

marijanp and others added 9 commits April 29, 2024 14:47

flake: don't follow our nixpkgs revision for risc0pkgs to speedup builds

0d5b7d2

kairos-prover: sync with upstream risc0pkgs template

f8122c3

Merge branch 'main' of github.com:cspr-rad/kairos into ci-optim

6c125b6

Create kairos-types crate splitting out rust native transaction types

aa8396a

Add serde to kairos-types

0e6ce51

Co-authored-by: jonas pauli <[email protected]>

Enable risc0 metal acceleration support

0b836bd

Rebase, finish prover server and move .env to root

d65496a

Merge branch 'main' of github.com:cspr-rad/kairos into risc0-circuit-avi

71e04a0

Fix nix build

3bcc368

Avi-D-coder requested review from marijanp, koxu1996, jonas089 and Rom3dius May 13, 2024 10:03

jonas089 requested changes May 13, 2024

View reviewed changes

Avi-D-coder force-pushed the risc0-circuit-avi branch from 66b841a to 3bcc368 Compare May 13, 2024 12:49

Avi-D-coder added 5 commits May 13, 2024 08:36

Rename crates to better reflect their purpose

5c5bd40

Commit to more of the input, and add no_std risc0 verifier

72d5dde

Fix nix build, credit to Marijan

2a2bd45

Co-Authored-By: Marijan Petričević <[email protected]>

Fix error handling in kairos-prover-server-lib NativeProver

c862305

Work towards a proptest generator for the core circuit logic

2cff279

Avi-D-coder added 4 commits May 30, 2024 10:48

Address @marijanp's review

45fa5a8

Address Review

79140f1

Update prover cargo profiles, and remove old unused host crate

f5f3ea7

Disable risc0 real proving in nix build, increase proptest cases

036a049

marijanp reviewed May 31, 2024

View reviewed changes

Address @marijanp's review

0e0582a

marijanp approved these changes Jun 3, 2024

View reviewed changes

koxu1996 reviewed Jun 3, 2024

View reviewed changes

kairos-prover/kairos-prover-risc0-server/src/main.rs Show resolved Hide resolved

koxu1996 reviewed Jun 3, 2024

View reviewed changes

kairos-prover/default.nix Outdated Show resolved Hide resolved

koxu1996 reviewed Jun 3, 2024

View reviewed changes

kairos-prover/kairos-prover-risc0-server/src/main.rs Outdated Show resolved Hide resolved

koxu1996 reviewed Jun 3, 2024

View reviewed changes

kairos-tx: move to project root

41cc1b5

kairos-prover: only enable dev mode when testing

3b21e44

koxu1996 requested changes Jun 3, 2024

View reviewed changes

This was referenced Jun 3, 2024

kairos-tx: move to project root #112

Merged

Risc0 dev when testing #113

Merged

Avi-D-coder added 2 commits June 3, 2024 08:24

Merge pull request #112 from cspr-rad/move-kairos-tx

40a392a

kairos-tx: move to project root

Merge pull request #113 from cspr-rad/risc0-dev-when-testing

992acab

Risc0 dev when testing

jonas089 approved these changes Jun 3, 2024

View reviewed changes

Avi-D-coder added 2 commits June 3, 2024 10:15

Change prover backend based on feature flags

ee01693

Fix nix build

28fc5bc

koxu1996 approved these changes Jun 3, 2024

View reviewed changes

Avi-D-coder added 2 commits June 3, 2024 10:44

Fix nix build

7ba8948

Fix nix build

edd50b9

jonas089 approved these changes Jun 3, 2024

View reviewed changes

marijanp approved these changes Jun 3, 2024

View reviewed changes

marijanp merged commit 66a3537 into main Jun 3, 2024
4 checks passed

marijanp deleted the risc0-circuit-avi branch June 3, 2024 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Risc0 circuit #98

Risc0 circuit #98

Avi-D-coder commented May 13, 2024 •

edited

Loading

jonas089 commented May 13, 2024

Avi-D-coder commented May 13, 2024

jonas089 commented May 13, 2024 •

edited

Loading

jonas089 commented May 13, 2024

jonas089 left a comment

Avi-D-coder commented May 13, 2024

jonas089 commented May 13, 2024

Avi-D-coder commented May 13, 2024

jonas089 commented May 13, 2024 •

edited

Loading

Avi-D-coder commented May 13, 2024

github-actions bot commented May 13, 2024

marijanp May 31, 2024

marijanp May 31, 2024

Avi-D-coder May 31, 2024

marijanp May 31, 2024

Avi-D-coder May 31, 2024

jonas089 Jun 3, 2024

Avi-D-coder Jun 3, 2024

marijanp May 31, 2024

Avi-D-coder May 31, 2024

koxu1996 commented Jun 3, 2024 •

edited

Loading

koxu1996 Jun 3, 2024 •

edited

Loading

jonas089 Jun 3, 2024

Avi-D-coder Jun 3, 2024

koxu1996 commented Jun 3, 2024

koxu1996 left a comment

jonas089 left a comment

jonas089 May 22, 2024

jonas089 May 22, 2024

jonas089 May 29, 2024

Avi-D-coder Jun 3, 2024

jonas089 Jun 3, 2024

jonas089 Jun 3, 2024

jonas089 left a comment

		/// TODO consider replacing with a count and hash of the processed deposits
		pub deposits: Box<[L1Deposit]>,

Risc0 circuit #98

Risc0 circuit #98

Conversation

Avi-D-coder commented May 13, 2024 • edited Loading

TODO (not necessarily before merge):

jonas089 commented May 13, 2024

Avi-D-coder commented May 13, 2024

jonas089 commented May 13, 2024 • edited Loading

jonas089 commented May 13, 2024

jonas089 left a comment

Choose a reason for hiding this comment

Avi-D-coder commented May 13, 2024

jonas089 commented May 13, 2024

Avi-D-coder commented May 13, 2024

jonas089 commented May 13, 2024 • edited Loading

Avi-D-coder commented May 13, 2024

github-actions bot commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koxu1996 commented Jun 3, 2024 • edited Loading

koxu1996 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koxu1996 commented Jun 3, 2024

koxu1996 left a comment

Choose a reason for hiding this comment

jonas089 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas089 left a comment

Choose a reason for hiding this comment

Avi-D-coder commented May 13, 2024 •

edited

Loading

jonas089 commented May 13, 2024 •

edited

Loading

jonas089 commented May 13, 2024 •

edited

Loading

koxu1996 commented Jun 3, 2024 •

edited

Loading

koxu1996 Jun 3, 2024 •

edited

Loading