Non Interactive Proof of Replication #1537
Conversation
There was a problem hiding this comment.
This looks to be on the right track, though missing some considerations. I assume it's just WIP, but thought I'd take a look anyway.
- Validation of anything that would have been checked at PreCommit and is still relevant (e.g. sector number, expiration epoch, ...)
- Different path for aggregate vs batched proofs
- Fetch chain randomness according to the epoch specified for each sector
| expected_storage_pledge: storage_pledge.clone(), | ||
| power_base_epoch: activation_epoch, | ||
| replaced_day_reward: TokenAmount::zero(), | ||
| sector_key_cid: None, |
There was a problem hiding this comment.
@Kubuxu this is how current CC sectors are represented, just confirming we want to keep doing the same thing. Whether a sector can be updated is determined by [verified_]deal_weight == 0, and if so the sealed_cid is copied into this sector key cid upon the first update.
|
I had a go at documenting the constants in go-state-types here: filecoin-project/go-state-types#270 I'd appreciate feedback on them to make sure I'm understanding and describing them well. |
|
For anyone watching this PR, the latest status discussions have been happening in https://filecoinproject.slack.com/archives/C06EZQ2DTD0/p1718256559056139 |
NemanjaLu92
force-pushed
the
nlucic/niporep
branch
from
5a3661d
to
042360b
Compare
NemanjaLu92
force-pushed
the
nlucic/niporep
branch
from
ec44768
to
ee59fdf
Compare
| apply_ok( | ||
| v, | ||
| &worker, | ||
| &maddr, | ||
| &TokenAmount::zero(), | ||
| MinerMethod::ProveCommitSectorsNI as u64, | ||
| Some(params.clone()), | ||
| ); |
There was a problem hiding this comment.
| apply_ok( | |
| v, | |
| &worker, | |
| &maddr, | |
| &TokenAmount::zero(), | |
| MinerMethod::ProveCommitSectorsNI as u64, | |
| Some(params.clone()), | |
| ); | |
| let ret_br: BatchReturn = apply_ok( | |
| v, | |
| &worker, | |
| &maddr, | |
| &TokenAmount::zero(), | |
| MinerMethod::ProveCommitSectorsNI as u64, | |
| Some(params.clone()), | |
| ) | |
| .deserialize() | |
| .unwrap(); | |
| assert_eq!(ret_br.len(), 2); | |
| assert!(ret_bf.ok()); | |
| assert_eq!(ret_bf.size(), 4); | |
| // something more to inspect codes and/or successes |
would be really nice to check the return value in this, and the successful case in prove_commit_ni_whole_success_test (I'm ~guessing at the suggested code here based on the very few [unfortunately] instances of checking, such as in integration_tests/src/tests/replica_update_test.rs).
| } | ||
|
|
||
| #[test] | ||
| fn prove_too_much_sector_ni_fail() { |
There was a problem hiding this comment.
| fn prove_too_much_sector_ni_fail() { | |
| fn prove_too_many_sectors_ni_fail() { |
| ¶ms.aggregate_proof, | ||
| sector_numbers.len(), | ||
| policy, | ||
| false, |
| ¶ms.aggregate_proof, | ||
| params.sectors.len() as u64, | ||
| policy, | ||
| true, |
| false => (policy.min_aggregated_sectors, policy.max_aggregated_sectors), | ||
| true => (policy.min_aggregated_sectors_ni, policy.max_aggregated_sectors_ni), |
There was a problem hiding this comment.
| false => (policy.min_aggregated_sectors, policy.max_aggregated_sectors), | |
| true => (policy.min_aggregated_sectors_ni, policy.max_aggregated_sectors_ni), | |
| true => (policy.min_aggregated_sectors, policy.max_aggregated_sectors), | |
| false => (policy.min_aggregated_sectors_ni, policy.max_aggregated_sectors_ni), |
There was a problem hiding this comment.
parameter name still doesn't match usage; either change its name or change the usage in here and at callsites
| warn!( | ||
| "seal challenge epoch {} must be before now {}", | ||
| sector.seal_rand_epoch, curr_epoch | ||
| ); | ||
| fail_validation = true; |
There was a problem hiding this comment.
| warn!( | |
| "seal challenge epoch {} must be before now {}", | |
| sector.seal_rand_epoch, curr_epoch | |
| ); | |
| fail_validation = true; | |
| return Err(actor_error!( | |
| illegal_argument, | |
| "seal challenge epoch {} must be before now {}", | |
| sector.seal_rand_epoch, curr_epoch | |
| )); |
just fail the whole lot in this case I think
| fail_validation = true; | ||
| } | ||
|
|
||
| if sector.seal_rand_epoch >= curr_epoch { |
There was a problem hiding this comment.
An explicit hard fail is probably the best option. But if we didn't do it explicitly here, it would abort anyway, either when attempting to fetch the randomness or when verifying the proof later.
|
|
||
| if params.sectors.len() > 5 { | ||
| let aggregate_fee = aggregate_prove_commit_network_fee( | ||
| params.sectors.len() - fail_count - 5, |
There was a problem hiding this comment.
Either the test at line 912 should be params.sectors.len() - fail_count > 5 or the fail count should not be subtracted here. As-is, this can give a negative (underflow) sector count, or expect the wrong fee. You should be able to hit this with a test (submit 6, fail 2), and then fix the expectation.
|
Should we close this PR now that #1559 is merged ? |
|
merged in #1559 🥳 thanks @NemanjaLu92 ! |
See also filecoin-project/ref-fvm#1999