Skip to content

Commit

Permalink
fix: successfully_precommited_no_deals (#689)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jmg-duarte
jmg-duarte authored Jan 22, 2025
1 parent 0833738 commit 2d5509a
Show file tree
Hide file tree
Showing 6 changed files with 111 additions and 110 deletions.
34 changes: 31 additions & 3 deletions pallets/market/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,10 @@ pub mod pallet {
pub struct BalanceEntry<Balance> {
/// 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)]
Expand Down Expand Up @@ -788,6 +788,24 @@ pub mod pallet {

/// Functions exposed by the pallet
impl<T: Config> Pallet<T> {
/// Retrieve the locked balance for the given account.
pub fn locked(who: &T::AccountId) -> Option<BalanceOf<T>> {
// try_get is required because the StorageMap has ValueQuery instead of OptionQuery
match BalanceTable::<T>::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<BalanceOf<T>> {
// try_get is required because the StorageMap has ValueQuery instead of OptionQuery
match BalanceTable::<T>::try_get(who) {
Ok(entry) => Some(entry.free),
Err(_) => None,
}
}

/// Account Id of the Market
///
/// This actually does computation.
Expand Down Expand Up @@ -1156,7 +1174,17 @@ pub mod pallet {
}
}

impl<T: Config> Market<T::AccountId, BlockNumberFor<T>> for Pallet<T> {
impl<T: Config> Market<T::AccountId, BlockNumberFor<T>, BalanceOf<T>> for Pallet<T> {
fn lock_pre_commit_funds(who: &T::AccountId, amount: BalanceOf<T>) -> DispatchResult {
// NOTE(@jmg-duarte,21/1/25): unsure if this should emit an event
lock_funds::<T>(who, amount)
}

fn slash_pre_commit_funds(who: &T::AccountId, amount: BalanceOf<T>) -> DispatchResult {
// NOTE(@jmg-duarte,21/1/25): unsure if this should emit an event
slash_and_burn::<T>(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.
Expand Down
55 changes: 13 additions & 42 deletions pallets/storage-provider/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<Self::AccountId>;

/// Market trait implementation for activating deals
type Market: Market<Self::AccountId, BlockNumberFor<Self>>;
/// Market trait implementation for activating deals.
type Market: Market<Self::AccountId, BlockNumberFor<Self>, BalanceOf<Self>>;

/// Proof verification trait implementation for verifying proofs
/// Proof verification trait implementation for verifying proofs.
type ProofVerification: ProofVerification;

/// Trait for AuthorVRF querying.
Expand Down Expand Up @@ -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::<T>::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::<T>::try_mutate(&owner, |maybe_sp| -> DispatchResult {
let sp = maybe_sp
.as_mut()
.ok_or(Error::<T>::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::<T>::GeneralPalletError(e))?;
Expand Down Expand Up @@ -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::<T>(&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;
};
Expand Down Expand Up @@ -1600,35 +1600,6 @@ pub mod pallet {
BalanceOf::<T>::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<T: Config>(
account: &T::AccountId,
slash_amount: BalanceOf<T>,
) -> 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::<T>::zero(), {
log::error!(target: LOG_TARGET, "could not slash_reserved entirely, precondition violated");
Error::<T>::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::<T>::SlashingFailed)?;

Ok(())
}

fn validate_seal_proof<T: Config>(
owner: &T::AccountId,
precommit: &SectorPreCommitOnChainInfo<BalanceOf<T>, BlockNumberFor<T>>,
Expand Down
40 changes: 21 additions & 19 deletions pallets/storage-provider/src/tests/pre_commit_sector_hook.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -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())
Expand All @@ -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)),
Expand All @@ -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
Expand All @@ -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::<Test>::Slashed {
who: account(storage_provider),
amount: deal_precommit_deposit,
}),
RuntimeEvent::Balances(pallet_balances::Event::<Test>::Rescinded {
amount: deal_precommit_deposit
amount: DEAL_PRECOMMIT_DEPOSIT
}),
RuntimeEvent::Balances(pallet_balances::Event::<Test>::Withdraw {
who: account(storage_provider),
amount: deal_precommit_deposit,
}),
RuntimeEvent::Balances(pallet_balances::Event::<Test>::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::<Test>::SectorsSlashed {
owner: account(storage_provider),
Expand Down
70 changes: 32 additions & 38 deletions pallets/storage-provider/src/tests/pre_commit_sectors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
};

Expand All @@ -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(
Expand All @@ -43,17 +41,13 @@ fn successfully_precommited() {
// 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>::SectorsPreCommitted {
[RuntimeEvent::StorageProvider(
Event::<Test>::SectorsPreCommitted {
block: 1,
owner: account(storage_provider),
sectors: bounded_vec![sector],
})
]
}
)]
);

let sp = StorageProviders::<Test>::get(account(storage_provider))
Expand All @@ -63,19 +57,29 @@ 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
);
});
}

#[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
Expand All @@ -91,17 +95,13 @@ fn successfully_precommited_no_deals() {
// 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>::SectorsPreCommitted {
[RuntimeEvent::StorageProvider(
Event::<Test>::SectorsPreCommitted {
block: 1,
owner: account(storage_provider),
sectors: bounded_vec![sector],
})
]
}
)]
);

let sp = StorageProviders::<Test>::get(account(storage_provider))
Expand All @@ -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));
});
}

Expand Down Expand Up @@ -156,17 +154,13 @@ fn successfully_precommited_batch() {
// Check that the events were triggered
assert_eq!(
events(),
[
RuntimeEvent::Balances(pallet_balances::Event::<Test>::Reserved {
who: account(storage_provider),
amount: SECTORS_TO_PRECOMMIT
},),
RuntimeEvent::StorageProvider(Event::<Test>::SectorsPreCommitted {
[RuntimeEvent::StorageProvider(
Event::<Test>::SectorsPreCommitted {
block: 1,
owner: account(storage_provider),
sectors,
})
]
}
)]
);

let sp = StorageProviders::<Test>::get(account(storage_provider))
Expand All @@ -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)
);
});
}
Expand Down
Loading

0 comments on commit 2d5509a

Please sign in to comment.