Add support for Empty Sector Update proofs (for SnapDeals) #1519
Conversation
|
Important note: while all tests are passing locally, I have verified that the |
cryptonemo
force-pushed
the
empty-sector-update
branch
from
29ab5c5
to
01aa48c
Compare
cryptonemo
requested review from
DrPeterVanNostrand,
dignifiedquire and
Kubuxu
storage-proofs-update/src/circuit.rs
Outdated
| pub comm_r_old: Option<Fr>, | ||
| pub comm_d_new: Option<Fr>, | ||
| pub comm_r_new: Option<Fr>, | ||
| pub h_select: Option<Fr>, |
There was a problem hiding this comment.
@DrPeterVanNostrand this is not critical so we should figure out anything else there is. Could we pack k and h_select into one input? AFAIK proof-verification significantly scales with the number of public inputs, so this could be a measurable improvement.
There was a problem hiding this comment.
Yup, I will do it today.
There was a problem hiding this comment.
Done, but a final review would be appreciated. I added one public-input for the packed k and h_select, then perform a binary decomposition on that public-input, then take the first 4 bits of that as partition_bits and the next 6 bits as h_select_bits.
storage-proofs-update/src/vanilla.rs
Outdated
| let input_index_without_k = node_index & remove_k_from_node_index_mask; | ||
| let high = | ||
| input_index_without_k >> (node_index_bit_len - partition_bit_len - h); | ||
| let rho: Fr = <TreeRHasher as Hasher>::Function::hash2( |
There was a problem hiding this comment.
We should avoid computing this hash on a per-node basis, the core reason why we hash only the high bits is to avoid 2^30 hash computations.
There was a problem hiding this comment.
I believe that I've made the change, but a second review would be beneficial.
filecoin-proofs/src/api/update.rs
Outdated
|
|
||
| /// Encodes data into an existing replica. | ||
| /// Returns tuple of (comm_r_new, comm_r_last_new, comm_d_new) | ||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
Should document what happens to the old replica & gurantees
| staged_data_path: &Path, | ||
| h: usize, | ||
| ) -> Result<(TreeRDomain, TreeRDomain, TreeDDomain)> { | ||
| // Sanity check all input path types. |
There was a problem hiding this comment.
May be also ensure that the new_replica_path (and cache path) not equal to the old one here?
DrPeterVanNostrand
force-pushed
the
empty-sector-update
branch
2 times, most recently
from
eb21db7
to
6d79e6d
Compare
style: clippy updates style: rust fmt
* feat: re-factor tree_r_last building for external re-use * reduce conversions in encode (@dignifiedquire) * feat: use an enum return type to avoid transmute (unsafe) * fix: keep new crate version consistent with latest release * fix: add doc and additional type check * feat: wrap settings for tree builders in a method
This check was loosened since lotus may pad the data file
feat: add and expose an API to prove sector updates with vanilla proofs
From the lotus side, additional data may be stored after the nodes we want in the tree, so using a different builder method will allow that to happen without a panic
Signed-off-by: Jakub Sztandera <kubuxu@protocol.ai>
cryptonemo
force-pushed
the
empty-sector-update
branch
from
5527615
to
4c23b61
Compare
| )] | ||
| struct Opt { | ||
| #[structopt(long, help = "Only cache PoSt groth params.")] | ||
| only_post: bool, | ||
| #[structopt(long, help = "Only cache EmptySectorUpdate groth params.")] |
There was a problem hiding this comment.
--only-sector-update and --only-post are mutually exclusive. If you add a group = "sectortype" to each of them, you'll get an error message like this if you specify both:
error: The argument '--only-post' cannot be used with one or more of the other specified argument
|
|
||
| impl From<HSelect> for u64 { | ||
| fn from(x: HSelect) -> Self { | ||
| x.0 as u64 |
There was a problem hiding this comment.
This is a general note for new future code (no need to change it). as can also be lossy. Hence I think u64::from(x.0) should be preferred. See https://stackoverflow.com/questions/48795329/what-is-the-difference-between-fromfrom-and-as-in-rust for more information.
| @@ -0,0 +1,16 @@ | |||
| // Update Proof Partitions are the number of partitions used in the | |||
There was a problem hiding this comment.
Should be a doc comment (three slashes).
| match self.raw { | ||
| Some(..) => {} | ||
| None => { |
There was a problem hiding this comment.
Nitpick (no need to change): I'd fine a if self.raw.is_none() { … } clearer.
| @@ -171,6 +171,9 @@ where | |||
| } | |||
| } | |||
|
|
|||
| // Note: This method verifies that the tree can be build with the size | |||
There was a problem hiding this comment.
Should be a doc comment (three slashes).
| where | ||
| TreeR: MerkleTreeTrait<Hasher = TreeRHasher>, | ||
| { | ||
| pub _tree_r: PhantomData<TreeR>, |
There was a problem hiding this comment.
Why is this pub? I don't see a reason why PhantomData should every need public acces.
| pub struct PublicInputs { | ||
| pub k: usize, | ||
| pub comm_r_old: TreeRDomain, | ||
| pub comm_d_new: TreeDDomain, | ||
| pub comm_r_new: TreeRDomain, | ||
| // The number of high bits to take from each challenge's bits. Used to verify replica encoding | ||
| // in the vanilla proof. `h` is only a public-input for the vanilla proof; the circuit takes | ||
| // `h_select` as a public-input rather than `h`. | ||
| pub h: usize, | ||
| } | ||
|
|
||
| pub struct PrivateInputs { | ||
| pub comm_c: TreeRDomain, | ||
| pub tree_r_old_config: StoreConfig, | ||
| // Path to old replica. | ||
| pub old_replica_path: PathBuf, | ||
| pub tree_d_new_config: StoreConfig, | ||
| pub tree_r_new_config: StoreConfig, | ||
| // Path to new replica. | ||
| pub replica_path: PathBuf, | ||
| } |
There was a problem hiding this comment.
Do all members (also for the PublicParams above) need to be public? I try to keep as much out of the public API as possible. Once somrthing is in the public API, it's tricky to get it out again. E.g. If you want to change a type (let's say you change from PathBuf to Path) and it's public, it's a breaking change and you have to be very careful about it.
No description provided.