feat: add block rewards to altair and centrifuge runtimes #1342

wischli · 2023-05-11T12:41:01Z

Description

Adds block rewards implementation introduced in feat: impl block rewards #1198 to the Altair and Centrifuge runtimes
~~Reduces kick threshold for collators to 2 hours from 6 hours~~ UPDATE: In order for this to become effective, we would have to change the session duration as kicking is only applied on session transitions
- Automatically removes collators which fail to produce a block within that threshold from the set of block authors
Fixes issue of of unfunded sovereign pallet_rewards accounts which receive rewards
- Without the fix:
  - Rewards are not be minted into that account if they are below the ExistentialDeposit
  - Rewards can not be claimed by all parties as this would leave the account without balance
- Adds funding of 1 CFG or AIR to the pallet_rewards sovereign account via
  - GenesisConfig for ephemeral networks
  - FundExistentialDeposit migration for running networks
Adds required migration for SessionKeys
Removes migrations for dev runtime as a result of the agreement made at the offsite

About the rewards values

Altair

Based on this RFC, Altair rewards are currently 12000 AIR per month. Looking at the time calculation of the Centrifuge block rewards proposal, a year is counted as 365 days. Thus, on average, a month equals 30.4166666667 days.

Collator rewards: 12000 AIR per month / 30.417 days per month / 4 sessions per day = 98.63 AIR = 98630 MILLI_AIR
Total rewards: In Altair, ~~we transfer the rewards from the treasury and do not want to mint extra rewards into the treasury as the latter is already sufficiently funded~~.
- UPDATE 2023-05-15: We actually want to mint the rewards here as well to be closer to Centrifuge. There will be a follow-up RFC to burn some of the treasury.
- We could set this to infinity and it would have no effect because pallet_block_rewards::Config::Beneficiary is set to (). We only need to ensure that TotalRewards will always be at least CollatorReward * num_collators. Of course, TotalRewards can also be adjusted via RootOrHalfOfCouncil. However, I do not see any reason to set it to CollatorRewards * MaxCandidates for now as the multiplication with MaxCandidates just serves as a max bound. The remainder of TotalRewards - CollatorRewards will be dropped in Altair.

Centrifuge

In Centrifuge we mint rewards both for the collators as well as the treasury. These values have been pre-approved by https://gov.centrifuge.io/t/rfc-add-a-block-reward-and-improve-collators-cycle/4766

Collator rewards = 16.65 CFG per epoch / 2 sessions per epoch = 8.325 CFG = 8_325 * MILLI_CFG
Total rewards = 20,096 CFG per epoch / 2 sessions per epoch = 10048 CFG

About the migrations

Unfortunately, adding block rewards requires three migrations to Altair and Centrifuge. Here's a short summary:

Rewards

While testing block rewards on a local chain environment, I stumbled into a minor issue which needs to be fixed: When claiming rewards, we currently transfer from the pallet id of the base pallet_rewards instance to the recipient with keep_alive enabled. This blocks any reward claims which would reduce the balance of the pallet's sovereign account below the existential deposit (ED). The keep_alive should stay enabled. Thus, we need to fund the pallet_rewards sovereign account (at least) with the ED which is done by the migration added here.

Without this migration, not all rewards can be claimed from the reward recipients.

BlockRewards

This migration was introduced in #1198. It sets up the required block rewards storage for our current collators.

Without this migration, none of the collators would be eligible to claim rewards.

SessionKeys

Without this migration, all of our collators would have to update their session keys manually. E.g., none of the collators would actually be recognized by the session transition inside pallet_block_rewards because the block_rewards entry of SessionKeys is unset.

TODO

Migration for SessionKeys: Add new block_rewards entry
In separate PR: Remove concept of Domain in pallet-rewards #1246 --> refactor: remove rewards domain concept #1344
Maybe: Implement RewardsApi for Tuple --> Can be done later when LiquidityRewards are pushed to prod

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works

This reverts commit afc4d54.

lemunozm

Very clean PR @wischli. I really like it! 🚀
I left some comments, but most of them are questions

pallets/block-rewards/src/migrations.rs

pallets/rewards/src/lib.rs

pallets/rewards/src/migrations.rs

pallets/rewards/src/mock.rs

runtime/development/src/lib.rs

runtime/centrifuge/src/lib.rs

src/chain_spec.rs

NunoAlexandre · 2023-05-12T08:40:47Z

@wischli looking good already but I will give it a proper review once the ToDos are addressed 👍

pallets/rewards/src/migrations.rs

runtime/integration-tests/src/rewards/block/env.rs

src/chain_spec.rs

thea-leake

LGTM! 🚀

…-prod

mustermeiszer

Only one question regarding the ED minting in the migration, where I am not sure, it is working as we intend it to. But might be due to my improper understanding. If cleared, will approve.

pallets/rewards/src/migrations/new_instance.rs

NunoAlexandre

Some comments and suggestions but great work and looking great overall 🙌

pallets/rewards/src/lib.rs

runtime/altair/src/lib.rs

NunoAlexandre · 2023-05-15T13:12:16Z

src/chain_spec.rs

+		},
+		block_rewards_base: altair_runtime::BlockRewardsBaseConfig {
+			currency_id: CurrencyId::Native,
+			amount: 1 * MICRO_AIR,


Is this correct that the reward for a block is 1 micro AIR?

That's not the block reward but the funding of the pallet_rewards::Instance sovereign account which holds all the rewards until they are claimed. As long as this account does not have the ED, not all rewards can be claimed at the same time as it would kill this account.

Just out of interest, we could use the ED defined in the runtime instead of hardcoding, right?

wischli · 2023-05-16T05:12:59Z

There was a last-minute change to the Altair configuration: Lucas and Cassidy proposed to actually mint AIR instead of transferring from the treasury. There will be a follow-up proposal to burn some/all of the Altair treasury. The main reason is that we want to be more close to Centrifuge.

* refactor: remove rewards domain concept * refactor: rm dev migrations * refactor: remove DOM * bench: fix block rewards * fix: RewardsApi

lemunozm

LGTM!

mustermeiszer

Would prefer to panic instead of logging an error in the genesis build. This is at least what Substrate is doing, IIRC and easier to spot. But no blocker.

Looks clean and nice!

mustermeiszer · 2023-05-16T07:20:20Z

pallets/block-rewards/src/lib.rs

-			.map_err(|e| log::error!("Failed to attach currency to collator group: {:?}", e))
-			.ok();
+			T::Rewards::attach_currency(T::StakeCurrencyId::get(), T::StakeGroupId::get())
+				.map_err(|e| log::error!("Failed to attach currency to collator group: {:?}", e))


Genesis builds usually panic!

pallets/rewards/src/lib.rs

runtime/centrifuge/src/lib.rs

mustermeiszer · 2023-05-16T07:25:23Z

src/chain_spec.rs

+		},
+		block_rewards_base: altair_runtime::BlockRewardsBaseConfig {
+			currency_id: CurrencyId::Native,
+			amount: 1 * MICRO_AIR,


Just out of interest, we could use the ED defined in the runtime instead of hardcoding, right?

Co-authored-by: Frederik Gartenmeister <mustermeiszer@posteo.de>

lemunozm

LGTM!

NunoAlexandre

Solid 🤘

wischli added 8 commits May 10, 2023 17:41

feat: add ED funding to rewards pallet

0328dd0

feat: apply reward genesis config to dev runtime

aad3cd7

tests: apply rewards ED to block rewards

793797e

tests: add sanity intgr block rewards check

e320b01

feat: add block rewards to altair runtime

cd36e63

feat: add block rewards to cfg runtime

1c64212

docs: improve blk rw migraton com

7a4b92f

feat: reduce collator kick threshold to 2 hours

afc4d54

wischli added I9-release A specific release. P7-asap Issue should be addressed in the next days. D8-migration Pull request touches storage and needs migration code. labels May 11, 2023

wischli self-assigned this May 11, 2023

wischli requested review from branan, mikiquantum, mustermeiszer, NunoAlexandre, hieronx and lemunozm as code owners May 11, 2023 12:41

wischli added 2 commits May 11, 2023 14:48

Revert "feat: reduce collator kick threshold to 2 hours"

0301952

This reverts commit afc4d54.

fix: re-enable transfer keep alive

9b4dc2a

lemunozm reviewed May 11, 2023

View reviewed changes

mustermeiszer reviewed May 12, 2023

View reviewed changes

pallets/rewards/src/migrations.rs Outdated Show resolved Hide resolved

runtime/integration-tests/src/rewards/block/env.rs Outdated Show resolved Hide resolved

thea-leake reviewed May 12, 2023

View reviewed changes

src/chain_spec.rs Outdated Show resolved Hide resolved

wischli added 3 commits May 12, 2023 14:48

fix: chain_spec

d2aeaa2

refactor: move rewards migration

d0aa485

feat: add session key migration

315bb78

wischli mentioned this pull request May 12, 2023

refactor: remove rewards domain concept #1344

Merged

4 tasks

thea-leake previously approved these changes May 12, 2023

View reviewed changes

wischli requested a review from lemunozm May 12, 2023 13:20

wischli added 2 commits May 15, 2023 12:07

bench: fix block rewards

b87cd00

Merge remote-tracking branch 'origin/main' into feat/block-rewards-to…

a995225

…-prod

mustermeiszer reviewed May 15, 2023

View reviewed changes

pallets/rewards/src/migrations/new_instance.rs Show resolved Hide resolved

NunoAlexandre reviewed May 15, 2023

View reviewed changes

wischli added 3 commits May 15, 2023 21:33

refactor: move migrations to sep file

2b43d2b

refactor: block rewards pallet id

310f2b5

refactor: improve pallet genesis build err log

635a6af

wischli requested review from mustermeiszer and NunoAlexandre May 16, 2023 05:09

wischli added 6 commits May 16, 2023 07:24

refactor: remove rewards domain concept (#1344)

e3047d2

* refactor: remove rewards domain concept * refactor: rm dev migrations * refactor: remove DOM * bench: fix block rewards * fix: RewardsApi

refactor: rename LiquidityRewardsBase

24616cf

tests: add block rewards RAPI int

7d322e4

clippy: remove unused

9957c73

fmt: taplo

238ddc9

fix: dev chainspec after renaming

4be7404

lemunozm previously approved these changes May 16, 2023

View reviewed changes

mustermeiszer previously approved these changes May 16, 2023

View reviewed changes

refactor: apply suggestion from code review

1fb344e

wischli dismissed stale reviews from mustermeiszer and lemunozm via 1fb344e May 16, 2023 08:12

Update runtime/centrifuge/src/lib.rs

7bf4a40

Co-authored-by: Frederik Gartenmeister <mustermeiszer@posteo.de>

lemunozm approved these changes May 16, 2023

View reviewed changes

wischli enabled auto-merge (squash) May 16, 2023 08:38

NunoAlexandre approved these changes May 16, 2023

View reviewed changes

Merge branch 'main' into feat/block-rewards-to-prod

0fcf91c

wischli merged commit 629dcfd into main May 16, 2023

NunoAlexandre deleted the feat/block-rewards-to-prod branch May 16, 2023 13:03

wischli mentioned this pull request May 16, 2023

refactor: Drop tranche multiLocation #1340

Merged

6 tasks

wischli mentioned this pull request Aug 18, 2023

altair: Add LiquidityRewards & LiquidityRewardsBase pallets #1507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add block rewards to altair and centrifuge runtimes #1342

feat: add block rewards to altair and centrifuge runtimes #1342

wischli commented May 11, 2023 •

edited

Loading

lemunozm left a comment

NunoAlexandre commented May 12, 2023

thea-leake left a comment

mustermeiszer left a comment

NunoAlexandre left a comment

NunoAlexandre May 15, 2023

wischli May 15, 2023

mustermeiszer May 16, 2023

wischli commented May 16, 2023

lemunozm left a comment

mustermeiszer left a comment

mustermeiszer May 16, 2023

wischli May 16, 2023

mustermeiszer May 16, 2023

lemunozm left a comment

NunoAlexandre left a comment

feat: add block rewards to altair and centrifuge runtimes #1342

feat: add block rewards to altair and centrifuge runtimes #1342

Conversation

wischli commented May 11, 2023 • edited Loading

Description

About the rewards values

Altair

Centrifuge

About the migrations

Rewards

BlockRewards

SessionKeys

TODO

Checklist:

lemunozm left a comment

Choose a reason for hiding this comment

NunoAlexandre commented May 12, 2023

thea-leake left a comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

NunoAlexandre left a comment

Choose a reason for hiding this comment

NunoAlexandre May 15, 2023

Choose a reason for hiding this comment

wischli May 15, 2023

Choose a reason for hiding this comment

mustermeiszer May 16, 2023

Choose a reason for hiding this comment

wischli commented May 16, 2023

lemunozm left a comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

mustermeiszer May 16, 2023

Choose a reason for hiding this comment

wischli May 16, 2023

Choose a reason for hiding this comment

mustermeiszer May 16, 2023

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

NunoAlexandre left a comment

Choose a reason for hiding this comment

wischli commented May 11, 2023 •

edited

Loading