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

Add basic test for DPCTxnNote #1

Merged
merged 6 commits into from
May 14, 2022
Merged

Add basic test for DPCTxnNote #1

merged 6 commits into from
May 14, 2022

Conversation

alxiong
Copy link
Collaborator

@alxiong alxiong commented Apr 30, 2022

This PR:

  • fixed the DPCTxnBody::generate()API design to accept local_data_commitment_randomness since the predicates it accepts are assumed to finalized outside this function.
  • fixed a few error message and minor bug
  • add a few debug print for testing
  • add an integration test for DPCTxnNote

Outdated:

See notion page on the benchmark effort.

@alxiong alxiong self-assigned this Apr 30, 2022
@alxiong alxiong changed the title Add benchmark code Add integration test for DPCTxnNote May 13, 2022
@alxiong alxiong marked this pull request as ready for review May 13, 2022 15:15
@alxiong
Copy link
Collaborator Author

alxiong commented May 13, 2022

I will remove fee and fee change and collect detailed benchmark data on another branch, as discussed.

Copy link

@chancharles92 chancharles92 left a comment

Choose a reason for hiding this comment

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

LGTM, maybe @zhenfeizhang can take another quick look.

src/transaction.rs Outdated Show resolved Hide resolved
@alxiong
Copy link
Collaborator Author

alxiong commented May 13, 2022

This is pending an PR on upstream Jellyfish to support hash_to_group for Jubjub (which we need for deriving diversified_base from diversifier) to pass the CI test.

☝️ being done by @zhenfeizhang

Copy link
Contributor

@zhenfeizhang zhenfeizhang left a comment

Choose a reason for hiding this comment

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

LGTM

/// Get the number of constraints in the underlying `PredicateCircuit`
fn num_constraints(&self) -> usize {
// TODO: (alex) poor API design, can't get a immutable reference to the
// underlying `PlonkCircuit`/`Instance` in jf-plonk
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the padded version or the unpadded one? Shall we patch Jellyfish?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is padded version. the unpadded version num of constraint is printed here.

I think we don't need to patch Jellyfish, since our circuit is built and finalized outside of Jellyfish, so the downstream libraries can print out the raw/unpadded num_of_constraint in their code.

@alxiong alxiong changed the title Add integration test for DPCTxnNote Add basic test for DPCTxnNote May 14, 2022
@alxiong alxiong merged commit 2e8a20c into main May 14, 2022
@alxiong alxiong deleted the alex-benchmark branch May 14, 2022 13:14
@alxiong alxiong mentioned this pull request May 14, 2022
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.

3 participants