feat: add tool to split phase2 parameters #1235

vmx · 2020-07-30T23:22:23Z

The phase2 CLI has a new command called split-keys which allows to
split phase2 parameter output into parameters that can be used for
rust-fil-proofs.

I'm not done yet, the path handling is not robust enough. Though it would
be good to get early review.

vmx · 2020-07-31T11:52:33Z

It already works like the other commands. It creates the .params and .vk file in the directory the program was run from. The log file is created in the directory of the source file.

I'm now looking for ideas on how to verify that it does the right thing.

DrPeterVanNostrand

Nice, everything looks good to me.

The only question that I have is whether or not it's ok to set porep_id = [0u8; 32] when generating the identifier.

And maybe whether or not the .participants file is going to be written at the end of split-keys.

filecoin-proofs/src/bin/phase2.rs

DrPeterVanNostrand · 2020-08-03T19:07:28Z

filecoin-proofs/src/bin/phase2.rs

+            let porep_config = PoRepConfig {
+                sector_size: SectorSize(sector_size),
+                partitions: PoRepProofPartitions(n_partitions),
+                porep_id: [0; 32],


I'm not certain that setting the porep-id to all zeroes will produce the correct identifier for mainnet params which have a nonzero porep-id. It may, but idk

I got them from

rust-fil-proofs/filecoin-proofs/src/bin/phase2.rs

Line 277 in 5e760a7

porep_id: [0; 32],

. I used the parameter of the initial files.

The phase2 initial parameter gen won't be affected by the porep-id, that's why it's set to all zeros. But, I didn't know whether or not the porep-id will affect the identifier

rust-fil-proofs/storage-proofs/porep/src/stacked/vanilla/params.rs

Lines 84 to 91 in 5e760a7

fn identifier(&self) -> String {

format!(

"layered_drgporep::PublicParams{{ graph: {}, challenges: {:?}, tree: {} }}",

self.graph.identifier(),

self.layer_challenges,

Tree::display()

)

}

is called on the public parameters.

Example output: layered_drgporep::PublicParams{ graph: stacked_graph::StackedGraph{expansion_degree: 8 base_graph: drgraph::BucketGraph{size: 1073741824; degree: 6; hasher: poseidon_hasher} }, challenges: LayerChallenges { layers: 11, max_count: 18 }, tree: merkletree-poseidon_hasher-8-8-0 }

So it doesn't look like the porep_id is part of it.

It's possible that there is a bug (no reason to expect it, and a scan of code suggests not), but for reference:

The identifier used to derive the working file for the parameters should not depend on porep_id.

There should be a 1-1 relationship between distinct groth params and file names.

porep_id is intentionally decoupled from the concrete circuit (by design, it can be used to change the graphs without changing the circuit).

This explains both why phase2 does not require porep_id and also why it must not affect the generated filename.

If the generated filename does depend on porep_id, that is a bug — and we should fix it.

Here is the code which I think shows that it does not (even though it 'could'): https://github.com/filecoin-project/rust-fil-proofs/blob/master/storage-proofs/porep/src/stacked/vanilla/params.rs#L84-L91

So I think it should be safe to assume an arbitrary porep_id can be supplied. However, it would be worth verifying that choice of porep_id doesn't affect the final filenames — as an independent check on this important logic.

filecoin-proofs/src/bin/phase2.rs

vmx · 2020-08-03T20:30:35Z

Even if this would work, I want to make one more change, which is extracting the contributions into yet another file (that's purely for the verification process).

porcuquine · 2020-08-03T20:37:47Z

One quick note, bearing in mind that I haven't had a chance to review this overall yet:

Ideally, this tool would be general-purpose in the sense that it could be used for any circuits created by phase2. That can and will change in the future when we perform more trusted setups.

I think (based on cursory discussion) it's the case that you currently extract the circuit parameters by parsing the filenames. In a perfect world, you would get all the circuit specification in the same way the initial phase2 parameters were created. Then, as long as the commands used to create initial parameters for a circuit were saved, they could be used verbatim to also perform this final splitting. I think, if designed correctly, that will allow for the final phase to happen correctly without need of any manual intervention or decision-making, and even with new, not-yet-supported circuits. In other words, we can/should reuse the circuit-selection logic used for creating initial phase2 params. This might require more refactoring than is convenient now, though — in which case it could be added as a follow-up but prior to the next trusted setup.

vmx · 2020-08-03T20:51:42Z

I think (based on cursory discussion) it's the case that you currently extract the circuit parameters by parsing the filenames.

No I don't, I copy and pasted the parameters from earlier in the file.

In a perfect world, you would get all the circuit specification in the same way the initial phase2 parameters were created.

I actually thought about refactoring the code to do that. Though I hesitated as I don't know how much we want to refactor the original trusted setup code. It's a matter of e.g. taking

rust-fil-proofs/filecoin-proofs/src/bin/phase2.rs

Lines 338 to 355 in 5e760a7

    
           fn blank_winning_post_poseidon_circuit<Tree: 'static + MerkleTreeTrait>( 
        
               sector_size: u64, 
        
           ) -> FallbackPoStCircuit<Tree> { 
        
               let post_config = PoStConfig { 
        
                   sector_size: SectorSize(sector_size), 
        
                   challenge_count: WINNING_POST_CHALLENGE_COUNT, 
        
                   sector_count: WINNING_POST_SECTOR_COUNT, 
        
                   typ: PoStType::Winning, 
        
                   priority: false, 
        
               }; 
        
               let public_params = winning_post_public_params::<Tree>(&post_config).unwrap(); 
        
               <FallbackPoStCompound<Tree> as CompoundProof< 
        
                   FallbackPoSt<Tree>, 
        
                   FallbackPoStCircuit<Tree>, 
        
               >>::blank_circuit(&public_params) 
        
           }

and splitting it into two functions, one that generates the parameters (which will then be re-used by my code) and the <FallbackPoStCompound<Tree> as CompoundProof< FallbackPoSt<Tree>, FallbackPoStCircuit<Tree>, >>::blank_circuit(&public_params) call.

vmx · 2020-08-03T20:53:08Z

If I should do the refactoring mentioned above, let me know.

porcuquine · 2020-08-03T20:59:51Z

@vmx I think what you're describing is the same thing I have believed is the 'right thing'. If you think this can be done practically in such a way as to be usable for the current trusted setup, then I think it's probably worth doing now. Unless @DrPeterVanNostrand sees a problem with the original code being refactored in what will be used in the last step of this trusted setup, I would proceed. If we decide we don't want to touch it now, we should still do it in a separate step for next time. This all makes me think it's probably better to do it now, fwiw.

porcuquine · 2020-08-04T05:53:00Z

filecoin-proofs/src/bin/phase2.rs

+                    // Writing the contributions to disk is not needed for the final parameters,
+                    // they won't be published, they are only there for verification purpose.
+                    let contribs_path =
+                        format!("v{}-{}.contribs", parameter_cache::VERSION, &identifier);


There is a subtlety here. If we use the current value of VERSION when running the tool, then we will end up with the version corresponding to the current (untrusted) parameters.

That would imply a sequence like the following:

Bump VERSION.

Run split tool to create trusted parameters.

Publish parameters.

Release.

Note that in this sequence, the version must be bumped before the tool is run. We could bump the version here/now, but then the trusted parameters would need to be included in the very next release. For that reason, something like the sequence above is probably better. It's also possible that I'm seeing something wrong, so please let me know if so or explicitly confirm below otherwise (cc: @cryptonemo @vmx ).

I would use the sequence then @cryptonemo would use when he publishes new parameters. I'd expect it to be what @porcuquine outlined above, but @cryptonemo would know best.

@cryptonemo Can you confirm that we're not missing anything here and that my sequence above is workable (or propose an alternative)?

vmx · 2020-08-04T15:24:59Z

@DrPeterVanNostrand I've made the refactoring that @porcuquine proposed. If you look at the commit in isolation a6b2f94 it shows quite nicely that is is just a basic refactoring for the existing code, but makes the new code more robust as there's less duplicated code.

porcuquine

Approved contingent on @cryptonemo's confirmation that we don't need any more sophistication here to best handle the version bump.

vmx · 2020-08-04T22:30:58Z

@DrPeterVanNostrand could you please approve or request changes? I especially would like to know if you are OK with the refactoring that also changes other parts of the code. I'd then make any needed changes (and the rebase that's needed anyway) my tomorrow and then we can hopefully merge it.

vmx · 2020-08-05T17:45:12Z

@DrPeterVanNostrand just confirmed through a side-channel that the refactoring is OK. So I'll go with that.

Those functions will be used by subsequent commits for splitting the trusted setup results into the correct parameter files.

porcuquine

Looks good.

The `phase2` CLI has a new command called `split-keys` which allows to split phase2 parameter output into parameters that can be used for rust-fil-proofs. Some auxiliary files for verification purpose are created as well.

vmx requested a review from DrPeterVanNostrand July 30, 2020 23:22

vmx marked this pull request as ready for review July 31, 2020 11:47

vmx requested review from cryptonemo, dignifiedquire and porcuquine as code owners July 31, 2020 11:47

DrPeterVanNostrand reviewed Aug 3, 2020

View reviewed changes

vmx force-pushed the phase2-split-keys branch from 45cca9f to 0de46b9 Compare August 3, 2020 20:46

porcuquine reviewed Aug 4, 2020

View reviewed changes

cryptonemo mentioned this pull request Aug 4, 2020

feat: add a check parameter command that maps parameter files #1238

Merged

porcuquine previously approved these changes Aug 4, 2020

View reviewed changes

cryptonemo previously approved these changes Aug 4, 2020

View reviewed changes

porcuquine added the trusted-setup label Aug 5, 2020

DrPeterVanNostrand previously approved these changes Aug 5, 2020

View reviewed changes

vmx dismissed stale reviews from DrPeterVanNostrand, cryptonemo, and porcuquine via 0f6f28f August 5, 2020 18:46

refactor: phase2: created dedicated functions for public params

4db8e3c

Those functions will be used by subsequent commits for splitting the trusted setup results into the correct parameter files.

porcuquine previously approved these changes Aug 5, 2020

View reviewed changes

vmx dismissed porcuquine’s stale review via 4db8e3c August 5, 2020 19:10

vmx force-pushed the phase2-split-keys branch from 0f6f28f to 4db8e3c Compare August 5, 2020 19:10

feat: phase2: split-keys command

06a1564

The `phase2` CLI has a new command called `split-keys` which allows to split phase2 parameter output into parameters that can be used for rust-fil-proofs. Some auxiliary files for verification purpose are created as well.

cryptonemo approved these changes Aug 5, 2020

View reviewed changes

vmx merged commit 3b01736 into master Aug 5, 2020

vmx deleted the phase2-split-keys branch August 5, 2020 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add tool to split phase2 parameters #1235

feat: add tool to split phase2 parameters #1235

vmx commented Jul 30, 2020

vmx commented Jul 31, 2020

DrPeterVanNostrand left a comment •

edited

Loading

DrPeterVanNostrand Aug 3, 2020

vmx Aug 3, 2020

DrPeterVanNostrand Aug 3, 2020

vmx Aug 3, 2020

porcuquine Aug 3, 2020

vmx commented Aug 3, 2020

porcuquine commented Aug 3, 2020

vmx commented Aug 3, 2020

vmx commented Aug 3, 2020

porcuquine commented Aug 3, 2020

porcuquine Aug 4, 2020

vmx Aug 4, 2020

porcuquine Aug 4, 2020

cryptonemo Aug 4, 2020

vmx commented Aug 4, 2020

porcuquine left a comment

vmx commented Aug 4, 2020

vmx commented Aug 5, 2020

porcuquine left a comment

	fn identifier(&self) -> String {
	format!(
	"layered_drgporep::PublicParams{{ graph: {}, challenges: {:?}, tree: {} }}",
	self.graph.identifier(),
	self.layer_challenges,
	Tree::display()
	)
	}

feat: add tool to split phase2 parameters #1235

feat: add tool to split phase2 parameters #1235

Conversation

vmx commented Jul 30, 2020

vmx commented Jul 31, 2020

DrPeterVanNostrand left a comment • edited Loading

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

vmx commented Aug 3, 2020

porcuquine commented Aug 3, 2020

vmx commented Aug 3, 2020

vmx commented Aug 3, 2020

porcuquine commented Aug 3, 2020

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

vmx commented Aug 4, 2020

porcuquine left a comment

Choose a reason for hiding this comment

vmx commented Aug 4, 2020

vmx commented Aug 5, 2020

porcuquine left a comment

Choose a reason for hiding this comment

DrPeterVanNostrand left a comment •

edited

Loading