Incentive vesting for validators#12210
Conversation
andreitrand
added
T2-pallets
|
/cmd bench --runtime asset-hub-westend --pallet pallet_vesting |
|
Command "bench --runtime asset-hub-westend --pallet pallet_vesting" has started 🚀 See logs here |
|
Command "bench --runtime asset-hub-westend --pallet pallet_vesting" has failed ❌! See logs here |
|
/cmd bench --runtime asset-hub-westend --pallet pallet_vesting |
|
Command "bench --runtime asset-hub-westend --pallet pallet_vesting" has started 🚀 See logs here |
…t-hub-westend --pallet pallet_vesting'
|
Command "bench --runtime asset-hub-westend --pallet pallet_vesting" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
|
/cmd bench --runtime westend --pallet pallet_vesting |
|
Command "bench --runtime westend --pallet pallet_vesting" has started 🚀 See logs here |
…end --pallet pallet_vesting'
|
Command "bench --runtime westend --pallet pallet_vesting" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
| }) | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
do you think it would be worth adding for each runtime configuring vesting pallet (or at least on WAH and similarly on Polkadot AH when time comes) a test checking that MAX_VESTING_SCHEDULE(Instance1) > ceil(VestingDuration / (BondingDuration x EraLength)) (+ safety margin?) or similar formula.
E.g. for WAH MAX_VESTING_SCHEDULE should be greater than 365d / (28eras x 1d) ≈ 13
Today we are comfortably satisfying the constraint but since we are talking of configurations coming from different pallets, if someone change one of the knobs (VestingDuration, BondingDuration etc), we would end up in vesting almost silently degraded (validators start hitting AtMaxVestingSchedules and get liquid payouts only a per-payout ForcedLiquid event that's good but probably easy to miss).
| @@ -0,0 +1,116 @@ | |||
| title: 'staking-async: vesting-based validator self-stake incentive payouts' | |||
There was a problem hiding this comment.
when this becomes the real prdoc, I would keep it much much shorter, just essential bits. A longer version is good for pull request.
| ## Migration | ||
|
|
||
| No storage migration required. `VestingEpochStart` starts as `None`; incentive payouts | ||
| are delivered as liquid transfers until the first bonding-duration boundary is crossed. |
There was a problem hiding this comment.
Do I get it right that we are "leaking" liquid incentive until we hit era % BondingDuration == 0 boundary - So potentially we would wait up to 28d? Is it acceptable? No strong opinion here. Just wondering if we have considered to have vesting active in the first era after runtime upgrade instead of up to 28 days later?
E.g. in session_rotations.rs something like
if bonding_duration != 0
&& (VestingEpochStart::<T>::get().is_none() || starting_era % bonding_duration == 0)
{
VestingEpochStart::<T>::put(now);
}| /// On the merge path `MinVestedTransfer` is not enforced, allowing sub-minimum era payouts | ||
| /// to accumulate into the epoch's schedule. | ||
| /// | ||
| /// # Warning — since `vested_transfer` is a permissionless extrinsic, an external actor can |
There was a problem hiding this comment.
Whereas I think the solution in this PR is more elegant than what suggested in the audit vs DoS (i.e. one storage item per schedule), I would probably add a note in the trait itself (around line 450 in this file) mentioning something like this:
/// This trait is **not** safe on its own. `pallet_vesting`'s `vested_transfer` is a permissionless
/// extrinsic, so on any instance that leaves it open an external actor can fill a target's schedule
/// slots (`MAX_VESTING_SCHEDULES`), making every subsequent [`add_to_vesting`](Self::add_to_vesting)
/// *create* fail with `AtMaxVestingSchedules`. Safety is
/// the **consumer's** responsibility: wire this trait to a dedicated `pallet_vesting` instance whose
/// permissionless `vested_transfer` is blocked (e.g. via a call filter), and/or fall back to a
/// liquid transfer plus an observable event on failure.
//// Example: the validator-incentive payout does both.
/// The default instance does neither and must not be used as a `VestedPayout` target while
/// `vested_transfer` is open on it.
| /// Transfer `amount` from `source` to `dest`, merging with the existing vesting schedule | ||
| /// whose `starting_block` equals `start_at`, or creating a new schedule if none exists. | ||
| /// On the merge path `MinVestedTransfer` is not enforced, allowing sub-minimum era payouts | ||
| /// to accumulate into the epoch's schedule. |
There was a problem hiding this comment.
Should we add something like this around On the merge path MinVestedTransfer is not enforced part:
/// This removes a protection that normally keeps every vesting schedule above a meaningful size,
/// and is safe **only** because the sole caller
/// is a trusted, internal path. Do not expose this to untrusted callers.
?
| match T::Currency::transfer( | ||
| // Use the current vesting window's start block as the merge key. If this is not yet set | ||
| // (i.e. before the first epoch boundary), the duration is forced to zero which will cause | ||
| // the adapter to deliver liquid. |
There was a problem hiding this comment.
From issue #11876 , "Eras 28..55 → all merge into the schedule whose starting_block is era 28's block." which I read as "each era's incentive belongs to that era's epoch" (but maybe I am getting it wrong cc @Ank4n).
Here in the implementation, we merge into the vesting schedule keyed by whatever epoch is current when payout_stakers is called. E.g. via payout_stakers I can claim era = current_era - 50, the funds come from era-50's pot, but they merge into the vesting schedule keyed by whatever epoch is current now.
Is this intentional / acceptable?
There was a problem hiding this comment.
I think ideally it should be based on the vesting epoch of the claimed era
There was a problem hiding this comment.
also, the fallback should be to start from now, no? not all liquid.
| @@ -551,6 +554,33 @@ impl pallet_vesting::Config for Runtime { | |||
| type RuntimeEvent = RuntimeEvent; | |||
| type WeightInfo = weights::pallet_vesting::WeightInfo<Runtime>; | |||
There was a problem hiding this comment.
should we update this one to point weights::pallet_vesting_vesting::… (the newly generated one after you run /cmd bench)? You probably need to add it in cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/mod.rs too.
| type Currency = Balances; | ||
| type MinVestedTransfer = MinVestedTransfer; | ||
| type RuntimeEvent = RuntimeEvent; | ||
| type WeightInfo = weights::pallet_vesting::WeightInfo<Runtime>; |
There was a problem hiding this comment.
should we update this one to point weights::pallet_vesting_validator_vesting::… (the newly generated one after you run /cmd bench)? You probably need to add it in cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/mod.rs too.
| /// Delegates delivery to [`Config::ValidatorIncentivePayout`]. On failure, falls back to a | ||
| /// direct liquid transfer and emits [`Event::ValidatorIncentiveForcedLiquid`] so operators | ||
| /// can detect vesting schedule slot exhaustion. | ||
| fn transfer_validator_incentive(era: EraIndex, stash: &T::AccountId, amount: BalanceOf<T>) { |
There was a problem hiding this comment.
are we charging in the payout extrinsic for the vesting work (read vesting, write_vesting, transfer, ...)? Should we add WeightInfo for add_to_vesting create/merge (worst case) ?
| // A schedule exists for "start_at", so we merge the incoming schedule with the existing | ||
| // one, while intentionally not enforcing "MinVestedTransfer" since per-era amounts | ||
| // may be sub-minimum. | ||
| T::Currency::transfer(source, dest, amount, ExistenceRequirement::AllowDeath)?; |
There was a problem hiding this comment.
in the merge path, we are moving funds before running fallible storage operations like exec_action (like 843) and write_vesting(line 844). Should we do the fallible work / pre-checks before the transfer or should we wrap it in a transaction or add a comment explaining why in the current code neither exec_action nor write_vesting can fail? I believe it's the case right now (but to be double-checked e.g. write_vesting only fails if > MAX_VESTING_SCHEDULES but merge path never grows the list etc). I am just worried that it's probably ok today and a refactor can break it.
Note that the create branch doesn't suffer of the same (potential / latent disease since it checks before transferring (and there is even a comment about it here
| .saturating_add(T::DbWeight::get().reads(4_u64)) | ||
| .saturating_add(T::DbWeight::get().writes(3_u64)) | ||
| } | ||
| /// NOTE: placeholder — re-run benchmarks after merging. |
There was a problem hiding this comment.
I guess run before merging :D
| } | ||
| } | ||
|
|
||
| impl pallet_vesting::Config<pallet_vesting::Instance1> for Runtime { |
There was a problem hiding this comment.
I know this was suggested elsewhere, but I'm wondering if adding a separate vesting instance is overkill, given:
- These new vestings wouldn't show up in existing UIs/dapps.
- There's no blocker with using only one instance of vesting (maybe there is and I'm unaware.. lmk).
There was a problem hiding this comment.
as we discussed - I think it's worth exploring
- single instance of the pallet
- modify the permissionless
vested_transferto charge a per-schedule deposit (as suggested by SRLabs too - now, these would be already wasted DOT in case we target account != myself, so pretty weak threat, maybe we can live w/o deposit...) - drop the liquid payment as fallback (and with that also kill the incentive to self-call
vested_transferfor validators) from this PR. We can think at a graceful merge into an existing incentive schedule on the create-fail path maybe.
This way we also don't break UI - which is nice.
One thing to take into account (see #11876 (comment) but there we are not taking validators-only into account) is to check how many validators are close to MAX_VESTING_SCHEDULES in practice - the median is promising but also true one account is close to 28 - so probably if we don't go for the isolation / multiple-instance approach, then we would have to raise the cap.
Again, this would be an exploration task. Might very well be that after that, we realize that the isolation / multiple instance approach is a lower-risk / cleaner approach (and if we go for that, then - as mentioned in another comment - let's go for a system/protocol instance of the vesting pallet and not a staking-incentive-specific one)
| AssetTxPayment: pallet_asset_conversion_tx_payment = 13, | ||
| Vesting: pallet_vesting = 14, | ||
| PgasAllowance: pallet_pgas_allowance = 15, | ||
| ValidatorVesting: pallet_vesting::<Instance1> = 16, |
There was a problem hiding this comment.
Even if we go the route of a new vesting pallet, maybe it should be for other system-related vesting in general, not specifically for validators only.
(Not saying anything is wrong, but we should really reason through why we're doing this and understand the tradeoffs before adding a new pallet.)
| UnexpectedKind::ValidatorIncentiveTransferFailed { era }, | ||
| )); | ||
| defensive!("Validator incentive liquid transfer failed"); | ||
| // Vested delivery failed (e.g. AtMaxVestingSchedules). We then attempt liquid |
There was a problem hiding this comment.
also, should never fallback to liquid transfer. If they can fill their vesting schedule, then they can get away with no vesting for the incentives. Or am I missing something?
| /// Set to `0` to deliver incentives as a liquid transfer (useful for test environments or | ||
| /// runtimes without `pallet-vesting`). | ||
| #[pallet::no_default] | ||
| type VestingDuration: Get<BlockNumberFor<Self>>; |
There was a problem hiding this comment.
Is this RC blocks or AH blocks?
There was a problem hiding this comment.
Also, rustdoc could mention how this is set? (governance?)
| /// Exposed as a public constant so runtime configs can reference it via | ||
| /// `parameter_types! { pub const VestingLockId: LockIdentifier = | ||
| /// pallet_vesting::DEFAULT_VESTING_LOCK_ID; }`. | ||
| pub const DEFAULT_VESTING_LOCK_ID: LockIdentifier = *b"vesting "; |
There was a problem hiding this comment.
Can come explicitly from the runtime. Default doesn't really make sense here (I think).
| /// Set to `0` to deliver incentives as a liquid transfer (useful for test environments or | ||
| /// runtimes without `pallet-vesting`). | ||
| #[pallet::no_default] | ||
| type VestingDuration: Get<BlockNumberFor<Self>>; |
There was a problem hiding this comment.
I was thinking a bit more about this. This can be represented in eras instead of blocks, and then we
define it in the runtime as something like 13 * validator unbonding period (13 * 28 = 364 days in PAH). For other chains with different unbonding periods, it can be set similarly. This would guarantee max 13 vests coming from validator incentive, which means the existing pallet-vesting becomes more than enough to serve this as well (assuming pallet-vesting isn't used much anyway) . We should look at existing data on how filled vesting schedules currently are.
3 participants
Validators receive a self-stake incentive that has so far been provided as an immediate liquid transfer. We now switch to a 1-year (configurable) vested incentive schedule.
To achieve this while not reaching an account's vesting schedule limit we group and merge all schedules that belong to the same bonding window into one. In addition, in order to prevent a malicious actor negatively affecting a validator by pushing too many distinct vesting schedules:
Fixes #11876