paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.81k stars 660 forks source link

Rent based PVF registration #1796

Closed eskimor closed 1 month ago

eskimor commented 1 year ago

We should make sure the deposit for registering a PVF is significant and reflects storage costs properly. It is better to make this smaller later, if need be than the other way round.

eskimor commented 11 months ago

PVF deposit seems to be quite large already. In fact probably too large for on-demand. We should think of an alternative model in addition to the deposit one, where you have to pay "rent": A relatively small amount, but on a regular basis and if you don't pay the storage gets cleared. Pruning of unpaid storage can be triggered off-chain via an extrinsic.

bkchr commented 11 months ago

Pruning of unpaid storage can be triggered off-chain via an extrinsic.

Whoever triggers this, should get the initial deposit.

Szegoo commented 11 months ago

I'd like to propose a solution for implementing a "rental system" without the need for any storage migrations.

The following describes the steps of my suggested solution:

/// Defines how frequently the rent needs to be paid.
#[pallet::constant]
type RentDuration: Get<BlockNumberFor<Self>>;

/// The initial 'base' deposit of registering a parathread.
#[pallet::constant]
type RentalParaDeposit: Get<BalanceOf<Self>>;

/// The recurring rental cost as a percentage of the initial rental registration payment.
#[pallet::constant]
type RecurringRentCost: Get<Perbill>;
pub struct RentInfo<Balance, BlockNumber> {
    // Stores information about the last time the rent was paid.
    last_rent_payment: BlockNumber,
    // The amount that needs to be paid every `T::RentDuration` blocks.
    rent_cost: Balance,
}
pub fn register_rental(
    origin: OriginFor<T>,
    id: ParaId,
    genesis_head: HeadData,
    validation_code: ValidationCode,
) -> DispatchResult {
    let who = ensure_signed(origin)?;

    Self::do_register(who, None, id, genesis_head, validation_code, true, ParaKind::Parathread)?;

    Ok(())
}

/* -- snip -- */
fn do_register(
    who: T::AccountId,
    deposit_override: Option<BalanceOf<T>>,
    id: ParaId,
    genesis_head: HeadData,
    validation_code: ValidationCode,
    ensure_reserved: bool,
    para_kind: ParaKind, // <- new `para_kind` parameter.
) -> DispatchResult {
    /* -- snip -- */

    let (genesis, deposit) = 
        Self::validate_onboarding_data(genesis_head, validation_code, para_kind)?;
    // ^^^ Based on the `para_kind` parameter it will figure out the proper deposit.

    /* -- snip -- */
}

We would also need to add other two extrinsics:

If no prior work has been done on this, and if this solution seems sensible, I would like to implement it.

eskimor commented 11 months ago

Removing the PVF of a parachain that has opened channels and stuff might be problematic/not possible. What we could do, is have the hash of the PVF registered for a very small deposit. Then we can safely remove the actual PVF, because if the parachain wants to produce a block again, it can just re-register the PVF (with same hash) and produce the block. Not even pre-checking would be required, as long as the hash stays the same.

Szegoo commented 11 months ago

True, that can make things more trickier. I think storing the hash of the PVF is a good solution and it shouldn't be hard to implement.

Probably not necessary since this would increase complexity, but if we really want to minimize the cost of registering a PVF, the additional deposit for storing the hash could be accounted for when opening a channel.

eskimor commented 11 months ago

If no prior work has been done on this, and if this solution seems sensible, I would like to implement it.

Awesome! At a glance the solution seems sensible. I have two more concerns though:

  1. Is it possible to end the rent willingly - and get the deposit back? Or does the owner always race with others? If the deposit is small enough and we communicate it as part of the rent (consider it lost). The latter should be fine.
  2. For the hash solution, there are also some things to consider:

It must never happen that the PVF exists in backing, but no longer in approval/disputes. This would be really bad. In general we need to handle the case that the actual PVF does not exist.

Both should be resolvable by a two-phase process:

  1. Rent is over - parachain moves into "hibernated" state. - Still exists, but is no longer allowed to author any blocks.
  2. After a full session has passed, we actually remove the PVF from storage. It should be cheap enough, to just check rent status before scheduling any para. Then we only need a transaction for (2).
Szegoo commented 11 months ago
  1. Is it possible to end the rent willingly - and get the deposit back? Or does the owner always race with others? If the deposit is small enough and we communicate it as part of the rent (consider it lost). The latter should be fine.

Yeah, It should be fairly simple to add a separate extrinsic to end the rent and claim the deposit back, but as you said there is probably no need for that.

Then we only need a transaction for (2).

I am not sure if I correctly understand what you are referring to here.

After a full session has passed, we actually remove the PVF from storage.

What would trigger this? I assume we don't want the scheduler to trigger this and not sure if putting this logic in a hook like on_initialize is ideal, but I might be wrong.

If both of these removal phases are triggered by users the prune_unpaid extrinsic would need to be called twice. Once to move the parachain to the "hibernated" state and once to actually remove it from state. This would mean that for users calling prune_unpaid would only be worth it when the parachain is already in a "hibernated" state assuming that is when the 'reward' is received. A solution to this might be to split the 'reward'. The caller that moves the parachain into a "hibernated" state would receive half of the deposit and whoever manages to call prune_unpaid the second time will get the other half.

eskimor commented 11 months ago

What would trigger this? I assume we don't want the scheduler to trigger this and not sure if putting this logic in a hook like on_initialize is ideal, but I might be wrong.

The prune transaction would be invalid, if the block where the rent expired has not been a complete session ago. As sessions are time based and not block based it might make more sense to specify the rent in terms of sessions. Then this check would be super simple.

The hibernation state could be calculated lazily by accessing code - so no actual state change needs to occur. We just check whether the rent was paid for the current session, if not - we won't schedule that para.

Does this make sense?

Szegoo commented 11 months ago

@eskimor Yeah, this makes sense. Thanks.

eskimor commented 1 month ago

Closing, we will likely go with off-chain runtime upgrades directly.