-
Notifications
You must be signed in to change notification settings - Fork 0
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
feature:add-merkle-proof-circuit-and-tests #14
Conversation
crates/zk-por-core/src/account.rs
Outdated
let mut sum_equity = F::ZERO; | ||
let mut sum_debt = F::ZERO; | ||
|
||
self.equity.iter().for_each(|x| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can use fold for consistency
STANDARD_CONFIG can set zk to false; only last layer can use true |
|
||
/// Assert 0 <= x <= MAX_POSITIVE_AMOUNT | ||
/// MAX_POSITIVE_AMOUNT = (1 << MAX_POSITIVE_AMOUNT_LOG) - 1 | ||
pub fn assert_non_negative_unsigned<F: RichField + Extendable<D>, const D: usize>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion]: can make F to the GoldilocksField, rather than generic; MAX_POSITIVE_AMOUNT_LOG is tied to 64 GL, rather than any generic field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can remove generics as much as possible
} | ||
|
||
/// Computes `if b { h0 } else { h1 }`. | ||
pub fn select_hash( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also not needed right now
} | ||
|
||
/// Hash 2 hashout targets by splitting it into its individual component elements | ||
pub fn hash_2_subhashes_circuit( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might not be necessary
crates/zk-por-core/src/account.rs
Outdated
|
||
let sum_debt = self.debt.iter().fold(F::ZERO, |acc, x| acc + *x); | ||
|
||
let hash = PoseidonHash::hash_no_pad(vec![sum_equity, sum_debt].as_slice()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the account hash should includes user_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would we represent the user id in a field?
…o feature/add-merkle-proof-circuit
}); | ||
let num_assets = 50; | ||
let accounts = gen_accounts_with_random_data(batch_size, num_assets); | ||
let prover = MerkleSumTreeProver { accounts }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea of prove_with_circuit is to avoid build multiple times. copy is lighter compared to build circuit again. can have a comparison between copy and build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah will add a version for this
} | ||
|
||
pub fn set_account_targets(&self, account_info: &Account, pw: &mut PartialWitness<F>) { | ||
assert_eq!(self.equity.len(), account_info.equity.len()); | ||
assert_eq!(self.debt.len(), account_info.debt.len()); | ||
|
||
println!("{:?}", account_info.get_user_id_in_field()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this, or use trace::debug
/// Given children nodes, generate the MerkleSumNodeTarget | ||
pub fn get_parent_from_children<const N: usize>( | ||
builder: &mut CircuitBuilder<F, D>, | ||
children: [&MerkleSumNodeTarget; N], | ||
children: &Vec<MerkleSumNodeTarget>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the same? what is the reason changing to a vector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because in the old code we go from arr -> vec but if we just use a vec we avoid the conversion
for i in 0..self.accounts.len() { | ||
account_targets[i].set_account_targets(self.accounts.get(i).unwrap(), &mut pw); | ||
} | ||
let mut timing = TimingTree::new("prove", Level::Debug); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need use Level::Info for release mode
} | ||
} | ||
|
||
pub fn prove_n_subproofs< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is unused; the recursive circuit building takes a lot of time; we'd better copy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
|
||
builder.print_gate_counts(0); | ||
|
||
let mut timing = TimingTree::new("prove", Level::Debug); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need use Level::Info for release mode
let mut timing = TimingTree::new("prove_merkle_sum_tree", Level::Debug); | ||
let proof = | ||
prove(&prover_only, &common, pw, &mut timing).map_err(|_| ProofError::InvalidProof)?; | ||
println!("Started Proving"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm this
Ok(proof) | ||
match proof_res { | ||
Ok(proof) => { | ||
println!("Finished Proving"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use tracing
|
||
match proof_res { | ||
Ok(proof) => { | ||
println!("Finished Proving"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use tracing
|
||
circuit_data.verify(proof.clone()).unwrap(); | ||
let proof_res = prove(&prover_only, &common, pw.clone(), &mut timing); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pw.clone() is not necessary
Cargo.toml
Outdated
@@ -34,8 +34,8 @@ once_cell = "1.14" | |||
static_assertions = { version = "1.1.0", default-features = false } | |||
unroll = { version = "0.1.5", default-features = false } | |||
# zkp | |||
plonky2 = { git = "https://github.com/okx/plonky2"} | |||
plonky2_field = { git = "https://github.com/okx/plonky2"} | |||
plonky2 = { git = "https://github.com/okx/plonky2", branch="clone-circuit"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it has already been merged to main; i moved
Added the merkle tree proof of inclusion circuit and prover along with tests.