From 2d5509aa3f5b4791e60fdb482cf32854b5ceaed7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Duarte?= Date: Wed, 22 Jan 2025 13:05:11 +0000 Subject: [PATCH] fix: successfully_precommited_no_deals (#689) --- pallets/market/src/lib.rs | 34 ++++++++- pallets/storage-provider/src/lib.rs | 55 ++++----------- .../src/tests/pre_commit_sector_hook.rs | 40 ++++++----- .../src/tests/pre_commit_sectors.rs | 70 +++++++++---------- .../src/tests/prove_commit_sectors.rs | 14 ++-- primitives/src/pallets.rs | 8 ++- 6 files changed, 111 insertions(+), 110 deletions(-) diff --git a/pallets/market/src/lib.rs b/pallets/market/src/lib.rs index b1c9c6405..21c0dac44 100644 --- a/pallets/market/src/lib.rs +++ b/pallets/market/src/lib.rs @@ -121,10 +121,10 @@ pub mod pallet { pub struct BalanceEntry { /// Amount of Balance that has been deposited for future deals/earned from deals. /// It can be withdrawn at any time. - pub(crate) free: Balance, + pub free: Balance, /// Amount of Balance that has been staked as Deal Collateral /// It's locked to a deal and cannot be withdrawn until the deal ends. - pub(crate) locked: Balance, + pub locked: Balance, } #[derive(Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug, TypeInfo, MaxEncodedLen)] @@ -788,6 +788,24 @@ pub mod pallet { /// Functions exposed by the pallet impl Pallet { + /// Retrieve the locked balance for the given account. + pub fn locked(who: &T::AccountId) -> Option> { + // try_get is required because the StorageMap has ValueQuery instead of OptionQuery + match BalanceTable::::try_get(who) { + Ok(entry) => Some(entry.locked), + Err(_) => None, + } + } + + /// Retrieve the locked balance for the given account. + pub fn free(who: &T::AccountId) -> Option> { + // try_get is required because the StorageMap has ValueQuery instead of OptionQuery + match BalanceTable::::try_get(who) { + Ok(entry) => Some(entry.free), + Err(_) => None, + } + } + /// Account Id of the Market /// /// This actually does computation. @@ -1156,7 +1174,17 @@ pub mod pallet { } } - impl Market> for Pallet { + impl Market, BalanceOf> for Pallet { + fn lock_pre_commit_funds(who: &T::AccountId, amount: BalanceOf) -> DispatchResult { + // NOTE(@jmg-duarte,21/1/25): unsure if this should emit an event + lock_funds::(who, amount) + } + + fn slash_pre_commit_funds(who: &T::AccountId, amount: BalanceOf) -> DispatchResult { + // NOTE(@jmg-duarte,21/1/25): unsure if this should emit an event + slash_and_burn::(who, amount) + } + /// Verifies a given set of storage deals is valid for sectors being PreCommitted. /// Computes UnsealedCID (CommD) for each sector or None for Committed Capacity sectors. /// Currently UnsealedCID is hardcoded as we `compute_commd` remains unimplemented because of #92. diff --git a/pallets/storage-provider/src/lib.rs b/pallets/storage-provider/src/lib.rs index 979356c5e..650e907be 100644 --- a/pallets/storage-provider/src/lib.rs +++ b/pallets/storage-provider/src/lib.rs @@ -45,10 +45,7 @@ pub mod pallet { ensure, fail, pallet_prelude::*, sp_runtime::traits::{CheckedAdd, CheckedSub, One}, - traits::{ - Currency, ExistenceRequirement::KeepAlive, Imbalance, Randomness, ReservableCurrency, - WithdrawReasons, - }, + traits::{Currency, Randomness, ReservableCurrency}, }; use frame_system::{ ensure_signed, @@ -110,13 +107,13 @@ pub mod pallet { /// More information about libp2p peer ids: https://docs.libp2p.io/concepts/fundamentals/peers/ type PeerId: Clone + Debug + Decode + Encode + Eq + TypeInfo; - /// Currency mechanism, used for collateral + /// Currency mechanism, used for the Market balances. type Currency: ReservableCurrency; - /// Market trait implementation for activating deals - type Market: Market>; + /// Market trait implementation for activating deals. + type Market: Market, BalanceOf>; - /// Proof verification trait implementation for verifying proofs + /// Proof verification trait implementation for verifying proofs. type ProofVerification: ProofVerification; /// Trait for AuthorVRF querying. @@ -533,15 +530,17 @@ pub mod pallet { deal_amounts, )?; - // Check balance for deposit - let balance = T::Currency::total_balance(&owner); - ensure!(balance >= total_deposit, Error::::NotEnoughFunds); - T::Currency::reserve(&owner, total_deposit)?; + // Lock the pre-commit funds in the market account + T::Market::lock_pre_commit_funds(&owner, total_deposit)?; + StorageProviders::::try_mutate(&owner, |maybe_sp| -> DispatchResult { let sp = maybe_sp .as_mut() .ok_or(Error::::StorageProviderNotFound)?; + + // NOTE(@jmg-duarte,21/1/25): Not sure if this is still needed sp.add_pre_commit_deposit(total_deposit)?; + for sector_on_chain in on_chain_sectors { sp.put_pre_committed_sector(sector_on_chain) .map_err(|e| Error::::GeneralPalletError(e))?; @@ -1285,7 +1284,8 @@ pub mod pallet { state.pre_commit_deposits = slashed_deposits; // PRE-COND: currency was previously reserved in pre_commit - let Ok(()) = slash_and_burn::(&storage_provider, slash_amount) else { + let Ok(()) = T::Market::slash_pre_commit_funds(&storage_provider, slash_amount) + else { log::error!(target: LOG_TARGET, "failed to slash.. amount: {:?}, storage_provider: {:?}", slash_amount, storage_provider); continue; }; @@ -1600,35 +1600,6 @@ pub mod pallet { BalanceOf::::one() // TODO(@aidan46, #106, 2024-06-24): Set a logical value or calculation } - /// Slashes **reserved* currency, burns it completely and settles the token amount in the chain. - /// - /// Preconditions: - /// - `slash_amount` needs to be previously reserved via `T::Currency::reserve()` on `account`, - fn slash_and_burn( - account: &T::AccountId, - slash_amount: BalanceOf, - ) -> Result<(), DispatchError> { - let (imbalance, balance) = T::Currency::slash_reserved(account, slash_amount); - - log::debug!(target: LOG_TARGET, "imbalance: {:?}, balance: {:?}", imbalance.peek(), balance); - ensure!(balance == BalanceOf::::zero(), { - log::error!(target: LOG_TARGET, "could not slash_reserved entirely, precondition violated"); - Error::::SlashingFailed - }); - - // slash_reserved returns NegativeImbalance, we need to get a concrete value and burn it to level out the circulating currency - let imbalance = T::Currency::burn(imbalance.peek()); - - // TODO(@jmg-duarte,20/11/2024): we'll probably need to review this, - // we're slashing an account (makes sense) - // burning the imbalance (maybe we could stash it in an account for rewards) - // and settling it??? — this part makes less sense since it's similar to a withdraw - T::Currency::settle(account, imbalance, WithdrawReasons::RESERVE, KeepAlive) - .map_err(|_| Error::::SlashingFailed)?; - - Ok(()) - } - fn validate_seal_proof( owner: &T::AccountId, precommit: &SectorPreCommitOnChainInfo, BlockNumberFor>, diff --git a/pallets/storage-provider/src/tests/pre_commit_sector_hook.rs b/pallets/storage-provider/src/tests/pre_commit_sector_hook.rs index 5f10f1b57..8976929eb 100644 --- a/pallets/storage-provider/src/tests/pre_commit_sector_hook.rs +++ b/pallets/storage-provider/src/tests/pre_commit_sector_hook.rs @@ -7,7 +7,7 @@ use crate::{ pallet::{Event, StorageProviders}, sector::ProveCommitSector, tests::{ - account, events, publish_deals, register_storage_provider, run_to_block, Balances, + account, events, publish_deals, register_storage_provider, run_to_block, Market, RuntimeEvent, RuntimeOrigin, SectorPreCommitInfoBuilder, StorageProvider, System, Test, CHARLIE, }, @@ -21,13 +21,15 @@ use crate::{ #[test] fn pre_commit_hook_slashed_deal() { new_test_ext().execute_with(|| { + // TODO(@aidan46, #106, 2024-06-24): Set a logical value or calculation + const DEAL_PRECOMMIT_DEPOSIT: u64 = 1; + const DEAL_COLLATERAL: u64 = 25; + let storage_provider = CHARLIE; register_storage_provider(account(storage_provider)); publish_deals(storage_provider); let first_deal = 0; let second_deal = 1; - // TODO(@aidan46, #106, 2024-06-24): Set a logical value or calculation - let deal_precommit_deposit = 1; let first_sector = SectorPreCommitInfoBuilder::default() .sector_number(1.into()) @@ -49,6 +51,12 @@ fn pre_commit_hook_slashed_deal() { bounded_vec![second_sector.clone()], ) .unwrap(); + // 2 deals = (collateral + precommit) * 2 + assert_eq!( + Market::locked(&account(storage_provider)), + // The cast is kind of an hack but we know it is safe + Some((2 * (DEAL_COLLATERAL + DEAL_PRECOMMIT_DEPOSIT) as u32).into()) + ); StorageProvider::prove_commit_sectors( RuntimeOrigin::signed(account(storage_provider)), @@ -71,10 +79,12 @@ fn pre_commit_hook_slashed_deal() { // First sector removed from here because it was slashed, second one because it was proven. assert!(sp.pre_committed_sectors.is_empty()); // Pre-commit from the second deal is still there, as pre-commit deposits are until sector expired. - assert_eq!(sp.pre_commit_deposits, deal_precommit_deposit); + assert_eq!(sp.pre_commit_deposits, DEAL_PRECOMMIT_DEPOSIT); + // 1 deal got slashed so the respective locked funds *vanished* assert_eq!( - Balances::reserved_balance(account(storage_provider)), - deal_precommit_deposit + Market::locked(&account(storage_provider)), + // The cast is kind of an hack but we know it is safe + Some(((2 * DEAL_COLLATERAL + DEAL_PRECOMMIT_DEPOSIT) as u32).into()) ); let mut expected_faulty_sectors = BoundedBTreeSet::new(); expected_faulty_sectors @@ -91,22 +101,14 @@ fn pre_commit_hook_slashed_deal() { owner: account(storage_provider), faulty_partitions: expected_faulty_partitions, }), - // the slash -> withdraw is related to the usage of slash_and_burn - // when slashing the SP for a failed pre_commit - // this usage may need review for a proper economic balance - RuntimeEvent::Balances(pallet_balances::Event::::Slashed { - who: account(storage_provider), - amount: deal_precommit_deposit, - }), RuntimeEvent::Balances(pallet_balances::Event::::Rescinded { - amount: deal_precommit_deposit + amount: DEAL_PRECOMMIT_DEPOSIT }), RuntimeEvent::Balances(pallet_balances::Event::::Withdraw { - who: account(storage_provider), - amount: deal_precommit_deposit, - }), - RuntimeEvent::Balances(pallet_balances::Event::::Rescinded { - amount: deal_precommit_deposit + // The money was removed from the balance table, as such, + // the money is actually removed from the "pallet account" + who: Market::account_id(), + amount: DEAL_PRECOMMIT_DEPOSIT, }), RuntimeEvent::StorageProvider(Event::::SectorsSlashed { owner: account(storage_provider), diff --git a/pallets/storage-provider/src/tests/pre_commit_sectors.rs b/pallets/storage-provider/src/tests/pre_commit_sectors.rs index e92021de9..f2c9f7aee 100644 --- a/pallets/storage-provider/src/tests/pre_commit_sectors.rs +++ b/pallets/storage-provider/src/tests/pre_commit_sectors.rs @@ -8,9 +8,9 @@ use super::new_test_ext; use crate::{ pallet::{Error, Event, StorageProviders}, tests::{ - account, events, publish_deals, register_storage_provider, run_to_block, Balances, + account, events, publish_deals, register_storage_provider, run_to_block, Market, MaxProveCommitDuration, MaxSectorExpiration, RuntimeEvent, RuntimeOrigin, - SectorPreCommitInfoBuilder, StorageProvider, Test, ALICE, CHARLIE, INITIAL_FUNDS, + SectorPreCommitInfoBuilder, StorageProvider, System, Test, ALICE, CHARLIE, }, }; @@ -29,10 +29,8 @@ fn successfully_precommited() { .build(); // Check starting balance - assert_eq!( - Balances::free_balance(account(storage_provider)), - INITIAL_FUNDS - 70 - ); + // 70 (initial SP funds) - 25 * 2 (deals) = 20 + assert_eq!(Market::free(&account(storage_provider)), Some(20)); // Run pre commit extrinsic assert_ok!(StorageProvider::pre_commit_sectors( @@ -43,17 +41,13 @@ fn successfully_precommited() { // Check that the events were triggered assert_eq!( events(), - [ - RuntimeEvent::Balances(pallet_balances::Event::::Reserved { - who: account(storage_provider), - amount: 1 - },), - RuntimeEvent::StorageProvider(Event::::SectorsPreCommitted { + [RuntimeEvent::StorageProvider( + Event::::SectorsPreCommitted { block: 1, owner: account(storage_provider), sectors: bounded_vec![sector], - }) - ] + } + )] ); let sp = StorageProviders::::get(account(storage_provider)) @@ -63,8 +57,8 @@ fn successfully_precommited() { assert_eq!(sp.pre_committed_sectors.len(), 1); assert_eq!(sp.pre_commit_deposits, 1); assert_eq!( - Balances::free_balance(account(storage_provider)), - INITIAL_FUNDS - 70 - 1 // 1 for pre-commit deposit + Market::free(&account(storage_provider)), + Some(20 - 1) // 1 for pre-commit deposit ); }); } @@ -72,10 +66,20 @@ fn successfully_precommited() { #[test] fn successfully_precommited_no_deals() { new_test_ext().execute_with(|| { + const MARKET_BALANCE: u32 = 1000; + // Register CHARLIE as a storage provider. let storage_provider = CHARLIE; register_storage_provider(account(storage_provider)); + Market::add_balance( + RuntimeOrigin::signed(account(storage_provider)), + MARKET_BALANCE.into(), + ) + .unwrap(); + + System::reset_events(); + // Sector to be pre-committed. let sector = SectorPreCommitInfoBuilder::default() // No sectors -> No CommD verification @@ -91,17 +95,13 @@ fn successfully_precommited_no_deals() { // Check that the events were triggered assert_eq!( events(), - [ - RuntimeEvent::Balances(pallet_balances::Event::::Reserved { - who: account(storage_provider), - amount: 1 - },), - RuntimeEvent::StorageProvider(Event::::SectorsPreCommitted { + [RuntimeEvent::StorageProvider( + Event::::SectorsPreCommitted { block: 1, owner: account(storage_provider), sectors: bounded_vec![sector], - }) - ] + } + )] ); let sp = StorageProviders::::get(account(storage_provider)) @@ -111,10 +111,8 @@ fn successfully_precommited_no_deals() { assert_eq!(sp.pre_committed_sectors.len(), 1); assert_eq!(sp.pre_commit_deposits, 1); - assert_eq!( - Balances::free_balance(account(storage_provider)), - INITIAL_FUNDS - 1 - ); + assert_eq!(Market::locked(&account(storage_provider)), Some(1)); // Single pre-commit = price is 1 + assert_eq!(Market::free(&account(storage_provider)), Some(999)); }); } @@ -156,17 +154,13 @@ fn successfully_precommited_batch() { // Check that the events were triggered assert_eq!( events(), - [ - RuntimeEvent::Balances(pallet_balances::Event::::Reserved { - who: account(storage_provider), - amount: SECTORS_TO_PRECOMMIT - },), - RuntimeEvent::StorageProvider(Event::::SectorsPreCommitted { + [RuntimeEvent::StorageProvider( + Event::::SectorsPreCommitted { block: 1, owner: account(storage_provider), sectors, - }) - ] + } + )] ); let sp = StorageProviders::::get(account(storage_provider)) @@ -179,8 +173,8 @@ fn successfully_precommited_batch() { ); assert_eq!(sp.pre_commit_deposits, SECTORS_TO_PRECOMMIT); assert_eq!( - Balances::free_balance(account(storage_provider)), - INITIAL_FUNDS - 70 - SECTORS_TO_PRECOMMIT + Market::free(&account(storage_provider)), + Some(20 - SECTORS_TO_PRECOMMIT) ); }); } diff --git a/pallets/storage-provider/src/tests/prove_commit_sectors.rs b/pallets/storage-provider/src/tests/prove_commit_sectors.rs index 409d6d64f..c7498d826 100644 --- a/pallets/storage-provider/src/tests/prove_commit_sectors.rs +++ b/pallets/storage-provider/src/tests/prove_commit_sectors.rs @@ -10,9 +10,9 @@ use crate::{ pallet::{Error, Event, StorageProviders}, sector::{ProveCommitResult, ProveCommitSector}, tests::{ - account, events, publish_deals, register_storage_provider, run_to_block, Balances, + account, events, publish_deals, register_storage_provider, run_to_block, Market, RuntimeEvent, RuntimeOrigin, SectorPreCommitInfoBuilder, StorageProvider, System, Test, - ALICE, BOB, CHARLIE, INITIAL_FUNDS, + ALICE, BOB, CHARLIE, }, }; @@ -84,9 +84,9 @@ fn successfully_prove_sector() { // check that the funds are still locked assert_eq!( - Balances::free_balance(account(storage_provider)), + Market::free(&account(storage_provider)), // Provider reserved 70 tokens in the market pallet and 1 token is used for the pre-commit - INITIAL_FUNDS - 70 - 1 + Some(20 - 1) ); let sp_state = StorageProviders::::get(account(storage_provider)) .expect("Should be able to get providers info"); @@ -192,9 +192,9 @@ fn successfully_prove_multiple_sectors() { // check that the funds are still locked assert_eq!( - Balances::free_balance(account(storage_provider)), - // Provider reserved 70 tokens in the market pallet and 1 token is used per the pre-commit - INITIAL_FUNDS - 70 - SECTORS_TO_COMMIT as u64 + Market::free(&account(storage_provider)), + // 70 - 25 * 2 = 20 + Some(20 - SECTORS_TO_COMMIT as u64) ); let sp_state = StorageProviders::::get(account(storage_provider)) .expect("Should be able to get providers info"); diff --git a/primitives/src/pallets.rs b/primitives/src/pallets.rs index 3cd3ab9ef..998707772 100644 --- a/primitives/src/pallets.rs +++ b/primitives/src/pallets.rs @@ -47,7 +47,13 @@ pub trait ProofVerification { } /// Represents functions that are provided by the Market Provider Pallet -pub trait Market { +pub trait Market { + /// Locks funds for pre-commit purposes. + fn lock_pre_commit_funds(who: &AccountId, amount: Balance) -> DispatchResult; + + /// Slashes funds locked for pre-commit purposes. + fn slash_pre_commit_funds(who: &AccountId, amount: Balance) -> DispatchResult; + /// Verifies a given set of storage deals is valid for sectors being PreCommitted. /// Computes UnsealedCID (CommD) for each sector or None for Committed Capacity sectors. fn verify_deals_for_activation(