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

Integrate aggregate signature + bit-vector QC validation trait + public hotshot-primitive repo + change signature scheme #1293

Merged

Conversation

dailinsubjam
Copy link
Contributor

Trying PR, a minor change on the signature scheme: from BLSSignatureScheme to BLSOverBN254CurveSignatureScheme.

@CLAassistant
Copy link

CLAassistant commented Jun 22, 2023

CLA assistant check
All committers have signed the CLA.

@dailinsubjam dailinsubjam linked an issue Jun 22, 2023 that may be closed by this pull request
15 tasks
@dailinsubjam dailinsubjam marked this pull request as draft June 22, 2023 17:57
@dailinsubjam dailinsubjam linked an issue Jun 22, 2023 that may be closed by this pull request
15 tasks
@dailinsubjam dailinsubjam changed the title Sishan/signature aggregation with hsp Integrating public hotshot-primitive repo and change signature scheme for QC aggregation Jun 22, 2023
@philippecamacho
Copy link
Contributor

I have got this error when I try to run the tests:

> nix-shell
> RUST_LOG=$ERROR_LOG_LEVEL RUST_LOG_FORMAT=$ERROR_LOG_FORMAT cargo test --verbose --release --lib --bins --tests --benches --features=full-ci --workspace -- --nocapture --test-threads=1
...
error[E0432]: unresolved import `hotshot::traits::election::vrf::VrfImpl`
 --> testing/tests/centralized_server.rs:5:51
  |
5 |     election::{static_committee::StaticCommittee, vrf::VrfImpl},
  |                                                   ^^^^^^^^^^^^ no `VrfImpl` in `traits::election::vrf`

error[E0432]: unresolved import `hotshot_testing::test_types::VrfTestTypes`
  --> testing/tests/centralized_server.rs:10:44
   |
10 |     test_types::{StaticCommitteeTestTypes, VrfTestTypes},
   |                                            ^^^^^^^^^^^^ no `VrfTestTypes` in `test_types`

warning: unused import: `signatures::BLSSignatureScheme`
  --> testing/tests/centralized_server.rs:21:21
   |
21 | use jf_primitives::{signatures::BLSSignatureScheme, vrf::blsvrf::BLSVRFS...
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `KeyPair`
  --> testing/tests/centralized_server.rs:23:83
   |
23 | ...OverBN254CurveSignatureScheme, KeyPair};
   |                                   ^^^^^^^

For more information about this error, try `rustc --explain E0432`.
warning: `hotshot-testing` (test "centralized_server") generated 2 warnings
error: could not compile `hotshot-testing` due to 2 previous errors; 2 warnings emitted

@dailinsubjam dailinsubjam linked an issue Jun 29, 2023 that may be closed by this pull request
15 tasks
@dailinsubjam dailinsubjam changed the title Integrating public hotshot-primitive repo and change signature scheme for QC aggregation Integrate aggregate signature + bit-vector QC validation trait + public hotshot-primitive repo + change signature scheme Jul 20, 2023
@dailinsubjam dailinsubjam changed the base branch from paper_benchmarking to run_view_refactor July 20, 2023 04:09
@dailinsubjam dailinsubjam marked this pull request as ready for review July 20, 2023 04:10
Copy link
Member

@shenkeyao shenkeyao left a comment

Choose a reason for hiding this comment

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

Looks good! Only minor comments.

centralized_server/benchmark_client/src/main.rs Outdated Show resolved Hide resolved
centralized_server/Cargo.toml Outdated Show resolved Hide resolved
src/traits/election/static_committee.rs Outdated Show resolved Hide resolved
types/src/certificate.rs Outdated Show resolved Hide resolved
@shenkeyao
Copy link
Member

Oh, one more minor thing: you could remove your name from comments that are notes rather than todos. I thought Sishan NOTE were all TODOs, then realized they were just code comments. 😄

@dailinsubjam
Copy link
Contributor Author

Oh, one more minor thing: you could remove your name from comments that are notes rather than todos. I thought Sishan NOTE were all TODOs, then realized they were just code comments. 😄

Agree! I've removed them and also update the comments.

Copy link
Contributor

@DieracDelta DieracDelta left a comment

Choose a reason for hiding this comment

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

This looks great! Phenomenal job! Couple of minor thoughts,

consensus/src/leader.rs Outdated Show resolved Hide resolved
src/traits/election/vrf.rs Outdated Show resolved Hide resolved
src/traits/election/vrf.rs Outdated Show resolved Hide resolved
src/traits/election/vrf.rs Outdated Show resolved Hide resolved
types/src/traits/election.rs Outdated Show resolved Hide resolved
types/src/data.rs Outdated Show resolved Hide resolved
types/src/vote.rs Show resolved Hide resolved
types/src/data.rs Show resolved Hide resolved
consensus/src/next_leader.rs Show resolved Hide resolved
types/src/vote.rs Show resolved Hide resolved
@dailinsubjam dailinsubjam changed the base branch from run_view_refactor to run_view_refactor_sig_agg July 20, 2023 20:11
@alxiong
Copy link
Contributor

alxiong commented Jul 21, 2023

@dailinsubjam I have some more API changes to stake table (EspressoSystems/hotshot-primitives#66 soon to be merged). and I feel that maybe it's time to move the entire stake_table code from hotshot-primitives to HotShot now. what's your opinion?
(do you prefer to merge this first, and then move stake_table later; OR move now and you modify this PR further for the updated API?)

@dailinsubjam
Copy link
Contributor Author

dailinsubjam commented Jul 21, 2023

@dailinsubjam I have some more API changes to stake table (EspressoSystems/hotshot-primitives#66 soon to be merged). and I feel that maybe it's time to move the entire stake_table code from hotshot-primitives to HotShot now. what's your opinion? (do you prefer to merge this first, and then move stake_table later; OR move now and you modify this PR further for the updated API?)

Thanks for letting me know! I'm working along the merge, and also using one branch of hotshot-primitives which has some of my minor changes (basically changing the version dependency and adding traits like clone, and debug), it may need some time (but not too much) to merge my changes with your changes if I transfer to the newest version of hotshot-primitives.

I might move on at this point (hopefully finish the merge on hotshot today or this weekend) and once you finished the stake_table merge we can do these at the same time: integrate my changes to hotshot-primitives or merge your changes to my hotshot-primitives branch, and dealing with the remaining merging issues on hotshot. Does that sound good to you?

Edit: This is not in a hurry until our branch on hotshot be merged into main, since now I'm also merging my changes to a branch of hotshot. But will definitely add it ASAP once it's ready.

@dailinsubjam dailinsubjam merged commit 58f0068 into run_view_refactor_sig_agg Jul 25, 2023
@alxiong alxiong deleted the sishan/signature_aggregation_with_hsp branch July 25, 2023 14:53
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.

Integrate aggregate signature + bit-vector QC validation trait
6 participants