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

[wip] feat: Halo2 compatible vanilla Hashers #1560

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DrPeterVanNostrand
Copy link
Contributor

@DrPeterVanNostrand DrPeterVanNostrand commented Feb 3, 2022

  • Pasta compatible vanilla hashers
  • Pasta compatible vanilla proofs
  • GPU code generic over a field F

@DrPeterVanNostrand DrPeterVanNostrand changed the title [wip] feat(filecoin-hashsers): add Halo2 compatible Sha256 and Poseidon Hashers [wip] feat(filecoin-hashers): add Halo2 compatible Sha256 and Poseidon Hashers Feb 3, 2022
@DrPeterVanNostrand DrPeterVanNostrand changed the title [wip] feat(filecoin-hashers): add Halo2 compatible Sha256 and Poseidon Hashers [wip] feat(filecoin-hashers): add Halo2 compatible Sha256 and Poseidon (vanilla) Hashers Feb 3, 2022
@DrPeterVanNostrand DrPeterVanNostrand force-pushed the halo-hashers branch 7 times, most recently from 137c053 to ad5955e Compare February 11, 2022 14:34
@DrPeterVanNostrand DrPeterVanNostrand force-pushed the halo-hashers branch 5 times, most recently from f1af449 to a8be7e8 Compare February 25, 2022 18:25
@DrPeterVanNostrand DrPeterVanNostrand changed the title [wip] feat(filecoin-hashers): add Halo2 compatible Sha256 and Poseidon (vanilla) Hashers [wip] feat(filecoin-hashers): Halo2 compatible (non-circuit) Hashers Feb 25, 2022
cargo +$(cat rust-toolchain) test -p storage-proofs-update --features isolated-testing --release test_empty_sector_update_circuit_8kib
cargo +$(cat rust-toolchain) test -p storage-proofs-update --features isolated-testing --release test_empty_sector_update_circuit_16kib
cargo +$(cat rust-toolchain) test -p storage-proofs-update --features isolated-testing --release test_empty_sector_update_circuit_32kib
cargo +$(cat rust-toolchain) test -p storage-proofs-update --features isolated-testing --release circuit_1kib
Copy link
Contributor Author

@DrPeterVanNostrand DrPeterVanNostrand Feb 28, 2022

Choose a reason for hiding this comment

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

Note: this change was necessary because I changed the ESU and ESU-Poseidon test names.

@DrPeterVanNostrand DrPeterVanNostrand force-pushed the halo-hashers branch 3 times, most recently from f992e1b to 220308f Compare March 1, 2022 18:06
fn cache_porep_params<Tree>(porep_config: PoRepConfig)
where
Tree: 'static + MerkleTreeTrait,
<Tree::Hasher as Hasher>::Domain: Domain<Field = Fr>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Groth16 params can only be generated for the field Fr.

) {
fn cache_empty_sector_update_params<Tree>(porep_config: PoRepConfig)
where
Tree: 'static + MerkleTreeTrait<Hasher = TreeRHasher<Fr>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ESU can only use TreeRs whose hasher is TreeRHasher.

) -> anyhow::Result<((u64, u64), (u64, u64), (u64, u64))>
where
Tree: 'static + MerkleTreeTrait,
<Tree::Hasher as Hasher>::Domain: Domain<Field = Fr>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: for now, only bench using the field Fr.

fn get_porep_info<Tree>(porep_config: PoRepConfig) -> CircuitInfo
where
Tree: 'static + MerkleTreeTrait,
<Tree::Hasher as Hasher>::Domain: Domain<Field = Fr>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Groth16 circuits can only be created using the field Fr.

) -> Result<()>
where
Tree: 'static + MerkleTreeTrait,
<Tree::Hasher as Hasher>::Domain: Domain<Field = Fr>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: for now, only generate parent cache for the production field Fr.

@@ -32,7 +34,7 @@ opencl = ["bellperson/opencl", "neptune/opencl"]

# available hashers
blake2s = ["blake2s_simd"]
poseidon = ["neptune", "lazy_static"]
poseidon = ["neptune", "lazy_static", "typemap"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: typemap allows us to lookup the Poseidon constants for a field and arity.

@@ -26,6 +30,60 @@ lazy_static! {
pub static ref POSEIDON_CONSTANTS_11: PoseidonConstants::<Fr, U11> = PoseidonConstants::new();
pub static ref POSEIDON_MD_CONSTANTS: PoseidonConstants::<Fr, PoseidonMDArity> =
PoseidonConstants::new();
pub static ref POSEIDON_CONSTANTS_2_PALLAS: PoseidonConstants::<Fp, U2> =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: create Poseidon constants for the three hasher fields; put the constants into a lookup table keyed by (field, arity)

@@ -27,21 +28,49 @@ pub trait Domain:
+ Eq
+ Send
+ Sync
// TODO (halo): remove once we have Pasta GPU support.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Domain: From<Fr> + Into<Fr> is used as a stopgap to cause GPU code to panic when using Pasta fields; these bounds should be removed once the GPU work has finished.

fr32/Cargo.toml Outdated
@@ -20,6 +20,8 @@ blstrs = "0.4.0"
bitvec = "0.17"
criterion = "0.3"
itertools = "0.9"
# pasta_curves = "0.3.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: change before merging

@DrPeterVanNostrand DrPeterVanNostrand force-pushed the halo-hashers branch 10 times, most recently from 3f4d23b to ab090d7 Compare March 7, 2022 19:39
# neptune = { version = "5.1.0", optional = true, features = ["arity2", "arity4", "arity8", "arity11", "arity16", "arity24", "arity36"] }
# pasta_curves = "0.3.0"
neptune = { git = "https://github.com/filecoin-project/neptune", branch = "wip-fr-as-trait", optional = true, features = ["arity2", "arity4", "arity8", "arity11", "arity16", "arity24", "arity36"] }
pasta_curves = { git = "https://github.com/vmx/pasta_curves", branch = "ec-gpu", features = ["gpu"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is status on dep branches here? I saw this one updated (https://github.com/filecoin-project/neptune/pull/135/files)?

@storojs72
Copy link
Contributor

storojs72 commented Mar 9, 2022

I'm not enough qualified to judge on the essence of the introduced changes... But this PR seems to be too monstrous (+4,912 −2,710). If this huge change is going to be merged, probably it is better to split this single PR on multiply PRs and introduce / review / merge them incrementally. WDYT, @DrPeterVanNostrand?

@DrPeterVanNostrand DrPeterVanNostrand force-pushed the halo-hashers branch 2 times, most recently from 069954d to e769deb Compare March 9, 2022 19:04
@DrPeterVanNostrand
Copy link
Contributor Author

@storojs72 You're right, it's an ugly PR (which I'm also not happy about). I'm totally in favor of small precise PRs, just this one is kind of difficult to merge incrementally.

The reason why it hasn't been split into multiple features/PRs is because this PR essentially adds a single feature to rust-fil-proofs (i.e. to make our hash functions compatible with both the BLS12-381 and Pasta scalar fields). Almost everything in rust-fil-proofs is downstream from the hash functions; making a small change to the hash function interface requires updating the interface of everything that uses it.

Also, much of this PR repeats the same 2 or 3 interface changes. For example, adding this one additional trait bound probably accounts for 500 additional lines in the diff.

impl<Tree> SomeType<Tree>
where
    <Tree::Hasher as Hasher>::Domain: Domain<Field = Fr>,

@cryptonemo
Copy link
Collaborator

I'm not enough qualified to judge on the essence of the introduced changes... But this PR seems to be too monstrous (+4,912 −2,710). If this huge change is going to be merged, probably it is better to split this single PR on multiply PRs and introduce / review / merge them incrementally. WDYT, @DrPeterVanNostrand?

@storojs72 Appreciate the feedback, and thanks for taking a look! This particular PR we knew was going to be a monster and the team had a sync review session for calibration on it not that long ago. These types of PRs don't happen necessarily all that often (thankfully), but we are in the middle of a sort of large re-factor for pending future work.

@cryptonemo
Copy link
Collaborator

@DrPeterVanNostrand Still reviewing, but I expect to be complete by tomorrow (just FYI)

@DrPeterVanNostrand DrPeterVanNostrand force-pushed the halo-hashers branch 2 times, most recently from 2fd142d to 84496a9 Compare March 10, 2022 19:19
cryptonemo
cryptonemo previously approved these changes Mar 10, 2022
Copy link
Collaborator

@cryptonemo cryptonemo left a comment

Choose a reason for hiding this comment

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

This generally all looks good to me. The only concern is how we're handling the dependencies that haven't been merged/released yet.

ff = "0.11.0"
anyhow = "1.0.34"
serde = "1.0.117"
rand = "0.8.0"

neptune = { version = "5.1.0", optional = true, features = ["arity2", "arity4", "arity8", "arity11", "arity16", "arity24", "arity36"] }
neptune = { git = "https://github.com/filecoin-project/neptune", branch = "fr-as-trait", optional = true, features = ["bls", "pasta", "arity2", "arity4", "arity8", "arity11", "arity16", "arity24", "arity36"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: change neptune dependencies once this PR is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

We decided to keep that branch for now until there's a proper pasta_curves release.

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.

4 participants