feat: split up window post API into separate calls #1269
Conversation
|
Some comments: This is draft work because 1, winning post isn't addressed at all, 2 there is work to be done on prove_vanilla, which does not accept the challenges as directly as it could yet, and 3) while not discussed, all API changes are additive, to preserve the existing call (assuming we wanted that for now). Expect updates, but early feedback is welcome too. |
cryptonemo
force-pushed
the
split-window-post
branch
from
308b3b4
to
a05747a
Compare
cryptonemo
force-pushed
the
split-window-post
branch
from
a05747a
to
dcdef75
Compare
There was a problem hiding this comment.
@cryptonemo I left some general notes. If you need more clarification or question my suggestions, let's chat and make sure we're on the same page. This is going in the right direction, but I think the underlying structure is not quite right yet.
| } | ||
|
|
||
| /// Generates the vanilla proofs required for Window proof-of-spacetime. | ||
| pub fn generate_window_post_vanilla_proofs<Tree: 'static + MerkleTreeTrait>( |
There was a problem hiding this comment.
What we need is a function that generates the vanilla proofs only for a single sector, as described in the issue.
|
|
||
| /// Generates a Window proof-of-spacetime. | ||
| pub fn generate_window_post_circuit_proof<Tree: 'static + MerkleTreeTrait>( | ||
| challenges: &WindowPoStChallenges, |
There was a problem hiding this comment.
We don't need the challenges here — just the vanilla proofs. WindosPostChallenges is being used as a vehicle to carry arguments (randomness, prover_id), which should just be passed independently.
| deserialize = "Vec<VanillaProof<Tree>>: Deserialize<'de>" | ||
| ))] | ||
| pub vanilla_proofs: Vec<VanillaProof<Tree>>, | ||
| } |
There was a problem hiding this comment.
This struct should just contain the vanilla proofs for a single sector and nothing else. As above, I don't think you need to differentiate between Window and Winning PoSt. Something like:
pub struct FallbackPoStVanillaProofs<Tree: MerkleTreeTrait> {
pub sector_id: SectorId,
pub replica_info: PublicReplicaInfo,
pub vanilla_proofs: Vec<VanillaProof<Tree>>
}
I include SectorId and replica_info because these are sector-specific. What's not needed is general proof configuration parameters.
There was a problem hiding this comment.
What I think may be missing are the sector-specific private inputs: comm_c and comm_r_last. We could technically get the latter from the inclusion proofs, but the former is needed in order to prove the relationship between comm_r_last and comm_r.
Note that PublicReplicaInfo above is effectively just comm_r. We could either include PersistentAux here, create a new structure just for all the needed commitments, or just individually include comm_c, comm_r_last, and comm_r.
| pub_params: &PublicParams<'a, S>, | ||
| pub_in: &S::PublicInputs, | ||
| priv_in: &S::PrivateInputs, | ||
| sector_challenges: &BTreeMap<SectorId, Vec<u64>>, |
There was a problem hiding this comment.
It is à priori wrong to have sector_challenges here because the CompoundProof abstraction is more general than this. The concept of a challenge is foreign to this module. On that basis, I suspect this method is unneeded — or at least does not belong here.
| } | ||
|
|
||
| /// prove_vanilla and prove_circuit combined is equivalent to ProofScheme::prove. | ||
| fn prove_vanilla( |
There was a problem hiding this comment.
I would call this vanilla_proofs, to parallel the existing circuit_proofs method.
|
|
||
| // This will always run at least once, since there cannot be zero partitions. | ||
| ensure!(partition_count > 0, "There must be partitions"); | ||
|
|
There was a problem hiding this comment.
We should probably include a sanity check so we never bother trying to create circuit proofs for invalid vanilla proofs.
| } | ||
|
|
||
| /// prove_vanilla and prove_circuit combined is equivalent to ProofScheme::prove. | ||
| fn prove_circuit<'b>( |
There was a problem hiding this comment.
I would call this something like prove_with_vanilla.
| priv_inputs: &'b Self::PrivateInputs, | ||
| partition_count: usize, | ||
| sector_challenges: &BTreeMap<SectorId, Vec<u64>>, | ||
| ) -> Result<Vec<Self::Proof>> { |
There was a problem hiding this comment.
I think maybe this is not decomposed right or fully enough. I'm not sure partitions and challenges should be mixed like this. There should be two distinct actions:
- Generate vanillla proofs for a single sector, given the challenges for that sector.
- Group vanilla proofs into partitions.
When decomposed as planned, the vanilla proofs will have been generated separately, but not grouped into partitions. I think we will need a partition_vanilla_proofs (note 'partition' is a verb here) function which just takes all the vanilla proofs and creates partitions proofs from them. (It happens to be the case that both Window and Winning PoSt only use a single partition, but we don't need to and shouldn't rely on that.) This will include the padding logic from prove_all_partitions — for the case that fewer than the 'required' number of vanilla proofs are provided.
|
Closing in favor of #1278 |
This is a draft PR that is trying to address #854. This code affects Window PoSt only, and once that is nailed down, Winning PoSt will be updated after (using a similar split).