Merge pull request #147 from eigerco/fix/117/verify-deals-pre-commit

fix(storage-provider-pallet): call verify_deals when precommiting

pallets/storage-provider/Cargo.toml

@@ -29,6 +29,7 @@ frame-support = { workspace = true, default-features = false }
 frame-system = { workspace = true, default-features = false }
 
 [dev-dependencies]
+env_logger = { workspace = true }
 multihash-codetable = { workspace = true, features = ["blake2b"] }
 pallet-balances = { workspace = true, default-features = false }
 pallet-market = { workspace = true, default-features = false }

pallets/storage-provider/src/lib.rs

@@ -35,7 +35,7 @@ pub mod pallet {
     use codec::{Decode, Encode};
     use frame_support::{
         dispatch::DispatchResult,
-        ensure,
+        ensure, fail,
         pallet_prelude::*,
         traits::{Currency, ReservableCurrency},
     };
@@ -165,6 +165,12 @@ pub mod pallet {
         /// Emitted when a prove commit is sent after the deadline.
         /// These pre-commits will be cleaned up in the hook.
         ProveCommitAfterDeadline,
+        /// Emitted when Market::verify_deals_for_activation fails for an unexpected reason.
+        /// Verification happens in pre_commit, to make sure a sector is precommited with valid deals.
+        CouldNotVerifySectorForPreCommit,
+        /// Declared unsealed_cid for pre_commit is different from the one calcualated by `Market::verify_deals_for_activation`.
+        /// unsealed_cid === CommD and is calculated from piece ids of all of the deals in a sector.
+        InvalidUnsealedCidForSector,
     }
 
     #[pallet::call]
@@ -215,6 +221,7 @@ pub mod pallet {
                 .map_err(|_| Error::<T>::StorageProviderNotFound)?;
             let sector_number = sector.sector_number;
             let current_block = <frame_system::Pallet<T>>::block_number();
+
             ensure!(
                 sector_number <= SECTORS_MAX.into(),
                 Error::<T>::InvalidSector
@@ -228,7 +235,8 @@ pub mod pallet {
                     && !sp.sectors.contains_key(&sector_number),
                 Error::<T>::SectorNumberAlreadyUsed
             );
-            validate_cid::<T>(&sector.unsealed_cid[..])?;
+
+            let unsealed_cid = validate_cid::<T>(&sector.unsealed_cid[..])?;
             let balance = T::Currency::total_balance(&owner);
             let deposit = calculate_pre_commit_deposit::<T>();
             Self::validate_expiration(
@@ -237,18 +245,48 @@ pub mod pallet {
                 sector.expiration,
             )?;
             ensure!(balance >= deposit, Error::<T>::NotEnoughFunds);
+
+            let sector_on_chain = SectorPreCommitOnChainInfo::new(
+                sector.clone(),
+                deposit,
+                <frame_system::Pallet<T>>::block_number(),
+            );
+
+            let mut sector_deals = BoundedVec::new();
+            sector_deals.try_push((&sector_on_chain).into())
+                .map_err(|_| {
+                    log::error!(target: LOG_TARGET, "pre_commit_sector: failed to push into sector deals, shouldn't ever happen");
+                    Error::<T>::CouldNotVerifySectorForPreCommit
+                })?;
+            let calculated_commds = T::Market::verify_deals_for_activation(&owner, sector_deals)?;
+
+            ensure!(calculated_commds.len() == 1, {
+                log::error!(target: LOG_TARGET, "pre_commit_sector: failed to verify deals, invalid calculated_commd length: {}", calculated_commds.len());
+                Error::<T>::CouldNotVerifySectorForPreCommit
+            });
+
+            // We need to verify CommD only if there are deals in the sector, otherwise it's a Committed Capacity sector.
+            if sector.deal_ids.len() > 0 {
+                // PRE-COND: verify_deals_for_activation is called with a single sector, so a single CommD should always be returned
+                let Some(calculated_commd) = calculated_commds[0] else {
+                    log::error!(target: LOG_TARGET, "pre_commit_sector: commd from verify_deals is None...");
+                    fail!(Error::<T>::CouldNotVerifySectorForPreCommit)
+                };
+
+                ensure!(calculated_commd == unsealed_cid, {
+                    log::error!(target: LOG_TARGET, "pre_commit_sector: calculated_commd != sector.unsealed_cid, {:?} != {:?}", calculated_commd, unsealed_cid);
+                    Error::<T>::InvalidUnsealedCidForSector
+                });
+            }
+
             T::Currency::reserve(&owner, deposit)?;
             StorageProviders::<T>::try_mutate(&owner, |maybe_sp| -> DispatchResult {
                 let sp = maybe_sp
                     .as_mut()
                     .ok_or(Error::<T>::StorageProviderNotFound)?;
                 sp.add_pre_commit_deposit(deposit)?;
-                sp.put_pre_committed_sector(SectorPreCommitOnChainInfo::new(
-                    sector.clone(),
-                    deposit,
-                    <frame_system::Pallet<T>>::block_number(),
-                ))
-                .map_err(|_| Error::<T>::MaxPreCommittedSectorExceeded)?;
+                sp.put_pre_committed_sector(sector_on_chain)
+                    .map_err(|_| Error::<T>::MaxPreCommittedSectorExceeded)?;
                 Ok(())
             })?;
             Self::deposit_event(Event::SectorPreCommitted { owner, sector });
@@ -345,7 +383,7 @@ pub mod pallet {
     }
 
     // Adapted from filecoin reference here: https://github.com/filecoin-project/builtin-actors/blob/54236ae89880bf4aa89b0dba6d9060c3fd2aacee/actors/miner/src/commd.rs#L51-L56
-    fn validate_cid<T: Config>(bytes: &[u8]) -> Result<(), Error<T>> {
+    fn validate_cid<T: Config>(bytes: &[u8]) -> Result<cid::Cid, Error<T>> {
         let c = Cid::try_from(bytes).map_err(|e| {
             log::error!(target: LOG_TARGET, "failed to validate cid: {:?}", e);
             Error::<T>::InvalidCid
@@ -359,7 +397,8 @@ pub mod pallet {
                 && c.hash().size() == 32,
             Error::<T>::InvalidCid
         );
-        Ok(())
+
+        Ok(c)
     }
 
     /// Calculate the required pre commit deposit amount

pallets/storage-provider/src/tests/mod.rs

@@ -115,12 +115,14 @@ type ClientDealProposalOf<Test> = ClientDealProposal<
 
 const ALICE: &'static str = "//Alice";
 const BOB: &'static str = "//Bob";
+const CHARLIE: &'static str = "//Charlie";
 
 /// Initial funds of all accounts.
 const INITIAL_FUNDS: u64 = 100;
 
 // Build genesis storage according to the mock runtime.
 fn new_test_ext() -> sp_io::TestExternalities {
+    let _ = env_logger::try_init();
     let mut t = frame_system::GenesisConfig::<Test>::default()
         .build_storage()
         .unwrap()
@@ -130,6 +132,7 @@ fn new_test_ext() -> sp_io::TestExternalities {
         balances: vec![
             (account(ALICE), INITIAL_FUNDS),
             (account(BOB), INITIAL_FUNDS),
+            (account(CHARLIE), INITIAL_FUNDS),
         ],
     }
     .assimilate_storage(&mut t)
@@ -209,6 +212,44 @@ fn register_storage_provider(account: AccountIdOf<Test>) {
     System::reset_events();
 }
 
+/// Publish deals to Market Pallet for the sectors to be properly pre-committed and proven.
+/// Pre-commit requires it as it calls [`Market::verify_deals_for_activation`].
+///
+/// It adds balance to the Market Pallet and publishes 2 deals to match default values in [`SectorPreCommitInfoBuilder`].
+/// It also resets events to not interfere with [`events()`] assertions.
+/// Deal 1: Client = Alice, Provider = provided
+/// Deal 2: Client = Bob, Provider = provided
+/// Balances: Alice = 60, Bob = 70, Provider = 70
+fn publish_deals(storage_provider: &str) {
+    // Add balance to the market pallet
+    assert_ok!(Market::add_balance(
+        RuntimeOrigin::signed(account(ALICE)),
+        60
+    ));
+    assert_ok!(Market::add_balance(RuntimeOrigin::signed(account(BOB)), 60));
+    assert_ok!(Market::add_balance(
+        RuntimeOrigin::signed(account(storage_provider)),
+        70
+    ));
+
+    // Publish the deal proposal
+    Market::publish_storage_deals(
+        RuntimeOrigin::signed(account(storage_provider)),
+        bounded_vec![
+            DealProposalBuilder::default()
+                .client(ALICE)
+                .provider(storage_provider)
+                .signed(ALICE),
+            DealProposalBuilder::default()
+                .client(BOB)
+                .provider(storage_provider)
+                .signed(BOB)
+        ],
+    )
+    .expect("publish_storage_deals needs to work in order to call verify_deals_for_activation");
+    System::reset_events();
+}
+
 struct SectorPreCommitInfoBuilder {
     seal_proof: RegisteredSealProof,
     sector_number: SectorNumber,
@@ -229,7 +270,8 @@ impl Default for SectorPreCommitInfoBuilder {
                 .expect("hash is always 32 bytes"),
             deal_ids: bounded_vec![0, 1],
             expiration: YEARS,
-            unsealed_cid: cid_of("unsealed_cid")
+            // TODO(@th7nder,#92,19/07/2024): compute_commd not yet implemented.
+            unsealed_cid: cid_of("placeholder-to-be-done")
                 .to_bytes()
                 .try_into()
                 .expect("hash is always 32 bytes"),
@@ -253,6 +295,11 @@ impl SectorPreCommitInfoBuilder {
         self
     }
 
+    pub fn unsealed_cid(mut self, unsealed_cid: SectorId) -> Self {
+        self.unsealed_cid = unsealed_cid;
+        self
+    }
+
     pub fn build(self) -> SectorPreCommitInfo<u64> {
         SectorPreCommitInfo {
             seal_proof: self.seal_proof,

pallets/storage-provider/src/tests/pre_commit_sector.rs

@@ -1,29 +1,32 @@
 use frame_support::{assert_noop, assert_ok};
+use sp_core::bounded_vec;
 use sp_runtime::{BoundedVec, DispatchError};
 
 use super::new_test_ext;
 use crate::{
     pallet::{Error, Event, StorageProviders},
     sector::SECTORS_MAX,
     tests::{
-        account, events, register_storage_provider, run_to_block, Balances, MaxProveCommitDuration,
-        MaxSectorExpirationExtension, RuntimeEvent, RuntimeOrigin, SectorPreCommitInfoBuilder,
-        StorageProvider, Test, ALICE,
+        account, cid_of, events, publish_deals, register_storage_provider, run_to_block, Balances,
+        MaxProveCommitDuration, MaxSectorExpirationExtension, RuntimeEvent, RuntimeOrigin,
+        SectorPreCommitInfoBuilder, StorageProvider, Test, ALICE, CHARLIE,
     },
 };
 
 #[test]
 fn successfully_precommited() {
     new_test_ext().execute_with(|| {
-        // Register ALICE as a storage provider.
-        let storage_provider = ALICE;
+        // Register CHARLIE as a storage provider.
+        let storage_provider = CHARLIE;
         register_storage_provider(account(storage_provider));
+        // Publish deals for verification before pre-commit.
+        publish_deals(storage_provider);
 
         // Sector to be pre-committed.
         let sector = SectorPreCommitInfoBuilder::default().build();
 
         // Check starting balance
-        assert_eq!(Balances::free_balance(account(storage_provider)), 100);
+        assert_eq!(Balances::free_balance(account(storage_provider)), 30);
 
         // Run pre commit extrinsic
         StorageProvider::pre_commit_sector(
@@ -47,6 +50,57 @@ fn successfully_precommited() {
             ]
         );
 
+        let sp_alice = StorageProviders::<Test>::get(account(storage_provider))
+            .expect("SP Alice should be present because of the pre-check");
+
+        assert!(sp_alice.sectors.is_empty()); // not yet proven
+        assert!(!sp_alice.pre_committed_sectors.is_empty());
+        assert_eq!(sp_alice.pre_commit_deposits, 1);
+        assert_eq!(Balances::free_balance(account(storage_provider)), 29);
+    });
+}
+
+#[test]
+fn successfully_precommited_no_deals() {
+    new_test_ext().execute_with(|| {
+        // Register CHARLIE as a storage provider.
+        let storage_provider = CHARLIE;
+        register_storage_provider(account(storage_provider));
+
+        // Sector to be pre-committed.
+        let sector = SectorPreCommitInfoBuilder::default()
+            // No sectors -> No CommD verification
+            .deals(bounded_vec![])
+            .unsealed_cid(
+                cid_of("cc-unsealed-cid")
+                    .to_bytes()
+                    .try_into()
+                    .expect("hash is always 32 bytes"),
+            )
+            .build();
+
+        // Run pre commit extrinsic
+        StorageProvider::pre_commit_sector(
+            RuntimeOrigin::signed(account(storage_provider)),
+            sector.clone(),
+        )
+        .expect("Pre commit failed");
+
+        // Check that the events were triggered
+        assert_eq!(
+            events(),
+            [
+                RuntimeEvent::Balances(pallet_balances::Event::<Test>::Reserved {
+                    who: account(storage_provider),
+                    amount: 1
+                },),
+                RuntimeEvent::StorageProvider(Event::<Test>::SectorPreCommitted {
+                    owner: account(storage_provider),
+                    sector,
+                })
+            ]
+        );
+
         let sp_alice = StorageProviders::<Test>::get(account(storage_provider))
             .expect("SP Alice should be present because of the pre-check");
 
@@ -91,9 +145,10 @@ fn fails_storage_provider_not_found() {
 #[test]
 fn fails_sector_number_already_used() {
     new_test_ext().execute_with(|| {
-        // Register ALICE as a storage provider.
-        let storage_provider = ALICE;
+        // Register CHARLIE as a storage provider.
+        let storage_provider = CHARLIE;
         register_storage_provider(account(storage_provider));
+        publish_deals(storage_provider);
 
         // Sector to be pre-committed
         let sector = SectorPreCommitInfoBuilder::default().build();
@@ -114,6 +169,34 @@ fn fails_sector_number_already_used() {
     });
 }
 
+#[test]
+fn fails_declared_commd_not_matching() {
+    new_test_ext().execute_with(|| {
+        // Register CHARLIE as a storage provider.
+        let storage_provider = CHARLIE;
+        register_storage_provider(account(storage_provider));
+        publish_deals(storage_provider);
+
+        // Sector to be pre-committed
+        let sector = SectorPreCommitInfoBuilder::default()
+            .unsealed_cid(
+                cid_of("different-unsealed-cid")
+                    .to_bytes()
+                    .try_into()
+                    .expect("hash is always 32 bytes"),
+            )
+            .build();
+
+        assert_noop!(
+            StorageProvider::pre_commit_sector(
+                RuntimeOrigin::signed(account(storage_provider)),
+                sector
+            ),
+            Error::<Test>::InvalidUnsealedCidForSector,
+        );
+    });
+}
+
 #[test]
 fn fails_invalid_sector() {
     new_test_ext().execute_with(|| {