Non Interactive Proof of Replication #1537

NemanjaLu92 · 2024-04-13T20:18:07Z

See also filecoin-project/ref-fvm#1999

anorth

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

runtime/src/runtime/fvm.rs

actors/miner/src/lib.rs

anorth · 2024-04-16T23:09:06Z

actors/miner/src/lib.rs

+            state.put_sectors(store, sectors_to_add).map_err(|e| {
+                e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to put new sectors")
+            })?;


Avoid map_err and downcast_default. Use with_context_code and similar instead for new code.

anorth · 2024-04-16T23:10:58Z

actors/miner/src/lib.rs

+            state
+                .add_initial_pledge(&total_pledge)
+                .map_err(|e| actor_error!(illegal_state, "failed to add initial pledge: {}", e))?;


Check for sufficient balance first!

anorth

I'd defer thorough review until this is complete (especially handling aggregate proofs), but still looking good so far.

anorth · 2024-05-10T01:01:46Z

Cargo.toml

-frc46_token = "9.0.0"
+fvm_actor_utils = "10.0.0"
+frc42_dispatch = "6.0.0"
+frc46_token = "10.0.0"


Please make these and other dependency updates against main as a separate PR, that you can create and land now. Remove this noise from the semantic changes introduced in this PR.

anorth · 2024-05-10T01:08:45Z

actors/miner/src/lib.rs

+                    miner: miner_id,
+                    number: sector.sector_number,
+                },
+                randomness: Randomness(Vec::new()),


This one needs to be initialised properly, though the interactive_randomness below doesn't.

Actually, even interactive_randomness should be initialized to commr right? https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0090.md#specification

I would have expected that to happen inside the proofs library. I think we need to ask someone else.

It's internally ignored, so it can be the same as comm_r. I believe if you pass all zeroes, it should fail as that's marked as invalid.

anorth · 2024-05-10T01:09:59Z

actors/miner/src/lib.rs

+                randomness: Randomness(Vec::new()),
+                proof: proof.into(),
+                sealed_cid: sector.sealed_cid,
+                unsealed_cid: CompactCommD::empty().get_cid(params.seal_proof_type).unwrap(),


Calculate empty CommD just once outside the iteration

anorth · 2024-05-10T01:15:39Z

actors/miner/src/lib.rs

+                expected_storage_pledge: storage_pledge.clone(),
+                power_base_epoch: activation_epoch,
+                replaced_day_reward: TokenAmount::zero(),
+                sector_key_cid: None,


@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.

anorth · 2024-05-10T01:17:20Z

actors/miner/src/lib.rs

+
+        rt.transaction(|state: &mut State, rt| {
+            let current_balance = rt.current_balance();
+            if current_balance < total_pledge {


You need to check the available balance. Use state.get_unlocked_balance(&current_balance)

anorth · 2024-05-10T01:24:15Z

actors/miner/src/lib.rs

+                .with_context_code(ExitCode::USR_ILLEGAL_STATE, || "failed to put new sectors")?;
+
+            state
+                .assign_sectors_to_deadlines(


I've proposed in Slack that we allow the caller to specify the deadline, which would be a change here.

anorth · 2024-05-10T01:28:21Z

actors/miner/src/policy.rs

@@ -158,6 +162,10 @@ pub fn qa_power_for_sector(size: SectorSize, sector: &SectorOnChainInfo) -> Stor
    qa_power_for_weight(size, duration, &sector.deal_weight, &sector.verified_deal_weight)
 }

+pub fn base_power_for_sector(size: SectorSize) -> StoragePower {
+    BigInt::from(size as u64) * &*QUALITY_BASE_MULTIPLIER


I don't think this multiplication is valid. My interpretation of QUALITY_BASE_MULTIPLIER is that it's supposed to be a denominator, when multiplying by either DEAL_WEIGHT_MULTIPLIER or VERIFIED_DEAL_WEIGHT_MULTIPLIER. In any case, multiplying size by 10 isn't what we want here.

Just do StoragePower::from(size) at the call sites.

You are right. My math is invalid. I'd still like to keep the function base_power_for_sector so we have an clear way for getting CC sector power consistent with QA power function.

anorth · 2024-05-10T01:29:38Z

actors/miner/src/types.rs

@@ -135,6 +135,34 @@ pub struct ProveCommitSectorParams {
    pub proof: RawBytes,
 }

+// Note no UnsealedCID because it must be "zero" data.
+#[derive(Clone, Debug, Eq, PartialEq, Serialize_tuple, Deserialize_tuple)]
+pub struct SectorNIActivationInfo {


This has changed in the FIP since you wrote this. Add sealing_number separate from sector_number.

anorth · 2024-05-10T01:32:46Z

test_vm/tests/suite/prove_commit3_test.rs

@@ -2,7 +2,7 @@ use fil_actors_integration_tests::tests::prove_commit_sectors2_test;
 use fil_actors_runtime::test_blockstores::MemoryBlockstore;
 use test_vm::TestVM;

-#[test]
+#[test_log::test]


I assume you will revert the test_log usage and dependencies when preparing for merge. If we want to use test_log, I think that's a separate consideration that we should execute in main ahead of this PR, and uniformly across all tests. Feel free to start an issue/discussion about doing that.

rvagg · 2024-05-13T09:47:43Z

actors/miner/src/types.rs

+    pub sector_number: SectorNumber,
+    pub sealed_cid: Cid, // CommR


these two are out of order from the FIP; as it's tuple encoding, order matters

rvagg · 2024-05-13T09:49:09Z

actors/miner/src/types.rs

+// Note no UnsealedCID because it must be "zero" data.
+#[derive(Clone, Debug, Eq, PartialEq, Serialize_tuple, Deserialize_tuple)]
+pub struct SectorNIActivationInfo {
+    pub sealer_id: Option<ActorID>,


this is not nullable in the FIP, does this assume filecoin-project/FIPs#993 is accepted?

In fact, this isn't even used in this PR; and I see no mention of SealerID / sealer_id in the text of https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0090.md other than in the description of this struct. The only thing it says about it is "Must be set to ID of receiving actor for now", so I guess it's a placeholder waiting for filecoin-project/FIPs#993? If so, I guess it needs to be checked against the receiving actor in prove_commit_sectors_ni.

Yes it's a placeholder that must be checked. Shouldn't be optional.

Should we fail a whole batch if sealer_id of some sector is different that receiving miner ID, or just filter them out?

The question is relevant only when not using aggregate proof. I think having aggregate proof for sectors intended for different receiver doesn't make sense and should fail.

Agree, just fail if sealer_id doesn't match receiver.

rvagg · 2024-05-17T07:43:21Z

runtime/src/runtime/fvm.rs

+    #[cfg(feature = "fake-proofs")]
+    fn batch_verify_ni_seals(&self, batch: &[NISealVerifyInfo]) -> anyhow::Result<Vec<bool>> {
+        Ok(batch.iter().map(|_| true).collect())
+    }
+


I think this can go, right? we're just using batch_verify_seals now with the NI proof types

rvagg · 2024-05-24T05:19:12Z

runtime/src/runtime/fvm.rs

+    AggregateSealVerifyProofAndInfos, NISealVerifyInfo, RegisteredSealProof, ReplicaUpdateInfo,
+    SealVerifyInfo, WindowPoStVerifyInfo,


Suggested change

AggregateSealVerifyProofAndInfos, NISealVerifyInfo, RegisteredSealProof, ReplicaUpdateInfo,

SealVerifyInfo, WindowPoStVerifyInfo,

AggregateSealVerifyProofAndInfos, RegisteredSealProof, ReplicaUpdateInfo, SealVerifyInfo,

WindowPoStVerifyInfo,

rvagg · 2024-05-24T09:50:23Z

runtime/src/runtime/policy.rs

+    pub prove_commit_ni_sector_batch_max_size: usize,
+    pub max_prove_commit_ni_randomness_lookback: ChainEpoch,
+    pub valid_prove_commit_ni_proof_type: ProofSet,
+


these need documenting and should be moved below pre_commit_challenge_delay so they are consistently placed with the policy_constants (and we can match their placement in go-state-types so it's easier to look them up)

rvagg · 2024-05-24T09:57:02Z

runtime/src/runtime/policy.rs

+    pub const PROVE_COMMIT_NI_SECTOR_BATCH_MAX_SIZE: usize = 256;
+    pub const MAX_PROVE_COMMIT_NI_LOOKBACK: ChainEpoch = 365 * EPOCHS_IN_DAY + CHAIN_FINALITY;


needs commenting, and I believe this should be 30 and not 365 as per FIP-0090

as per shared doc, current proposal is to change FIP to 180 days

rvagg · 2024-05-24T10:04:27Z

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.

rvagg · 2024-05-25T10:54:44Z

actors/miner/src/lib.rs

+            .collect::<Vec<SectorOnChainInfo>>();
+
+        let total_pledge = BigInt::from(sectors_to_add.len()) * initial_pledge;
+


Suggested change

let mut needs_cron = false;

rvagg · 2024-05-25T10:55:06Z

actors/miner/src/lib.rs

+            state
+                .check_balance_invariants(&rt.current_balance())
+                .map_err(balance_invariants_broken)?;
+


Suggested change

needs_cron = !state.deadline_cron_active;

state.deadline_cron_active = true;

rvagg · 2024-05-25T10:55:26Z

actors/miner/src/lib.rs

+
+            Ok(())
+        })?;
+


Suggested change

if needs_cron {

let new_dl_info = state.deadline_info(rt.policy(), curr_epoch);

enroll_cron_event(

rt,

new_dl_info.last(),

CronEventPayload { event_type: CRON_EVENT_PROVING_DEADLINE },

)?;

}

NemanjaLu92 added 5 commits April 12, 2024 16:02

Added verify NI to fvm

6473c89

Update integration tests

6b0cc72

Added integration test for niporep that does not do anything sensible

874742f

sectors added to state

e9198d5

add pledge calculation and expiration

aedc94c

anorth reviewed Apr 16, 2024

View reviewed changes

rjan90 mentioned this pull request Apr 18, 2024

Implement FIP-0090: Non-Interactive PoRep #1534

Open

NemanjaLu92 added 3 commits April 23, 2024 11:55

Add basic checks, precalculate pledge, introduce randomness lookback

e141961

Remove ni verify

868c39d

Add more checks and mock expectations to pass unit test

f121bc4

anorth reviewed May 10, 2024

View reviewed changes

rvagg reviewed May 13, 2024

View reviewed changes

NemanjaLu92 added 3 commits May 15, 2024 21:52

Review comments

8a8d180

Add aggregated proof and unit test

92f7c27

Add aggregate proof integration test

cf2afe9

rvagg reviewed May 17, 2024

View reviewed changes

rvagg mentioned this pull request May 24, 2024

test: miner: add niporep params cbor forms vector tests #1543

Open

rvagg reviewed May 24, 2024

View reviewed changes

rvagg mentioned this pull request May 25, 2024

feat: miner: max prove commit sector batch size #1544

Closed

rvagg reviewed May 25, 2024

View reviewed changes

rvagg mentioned this pull request May 27, 2024

Ni-PoREP (temp, working branch for testing) #1546

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non Interactive Proof of Replication #1537

Non Interactive Proof of Replication #1537

NemanjaLu92 commented Apr 13, 2024 •

edited by anorth

anorth left a comment

anorth Apr 16, 2024

anorth Apr 16, 2024

anorth left a comment

anorth May 10, 2024

anorth May 10, 2024

NemanjaLu92 May 15, 2024

anorth May 16, 2024

cryptonemo May 16, 2024

anorth May 10, 2024

anorth May 10, 2024

anorth May 10, 2024

anorth May 10, 2024

anorth May 10, 2024

NemanjaLu92 May 15, 2024

anorth May 10, 2024

anorth May 10, 2024

rvagg May 13, 2024

rvagg May 13, 2024

rvagg May 13, 2024

anorth May 13, 2024

NemanjaLu92 May 15, 2024

NemanjaLu92 May 15, 2024 •

edited

anorth May 16, 2024

rvagg May 17, 2024

rvagg May 24, 2024

rvagg May 24, 2024

rvagg May 24, 2024

rvagg May 25, 2024

rvagg commented May 24, 2024

rvagg May 25, 2024

rvagg May 25, 2024

rvagg May 25, 2024

		pub sector_number: SectorNumber,
		pub sealed_cid: Cid, // CommR

		AggregateSealVerifyProofAndInfos, NISealVerifyInfo, RegisteredSealProof, ReplicaUpdateInfo,
		SealVerifyInfo, WindowPoStVerifyInfo,

		pub const PROVE_COMMIT_NI_SECTOR_BATCH_MAX_SIZE: usize = 256;
		pub const MAX_PROVE_COMMIT_NI_LOOKBACK: ChainEpoch = 365 * EPOCHS_IN_DAY + CHAIN_FINALITY;

		.collect::<Vec<SectorOnChainInfo>>();

		let total_pledge = BigInt::from(sectors_to_add.len()) * initial_pledge;


	needs_cron = !state.deadline_cron_active;
	state.deadline_cron_active = true;

+        if needs_cron {
+            let new_dl_info = state.deadline_info(rt.policy(), curr_epoch);
+            enroll_cron_event(
+                rt,
+                new_dl_info.last(),
+                CronEventPayload { event_type: CRON_EVENT_PROVING_DEADLINE },
+            )?;
+        }

Non Interactive Proof of Replication #1537

Are you sure you want to change the base?

Non Interactive Proof of Replication #1537

Conversation

NemanjaLu92 commented Apr 13, 2024 • edited by anorth

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NemanjaLu92 May 15, 2024 • edited

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

Choose a reason for hiding this comment

rvagg commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NemanjaLu92 commented Apr 13, 2024 •

edited by anorth

NemanjaLu92 May 15, 2024 •

edited