Shielded Inflation rewards #1025

mariari · 2023-01-16T08:31:01Z

A followup pr of #714 that adds the inflation rewards for the shielded pool.

The formulas follow from:

https://specs.namada.net/economics/inflation-system.html

mariari · 2023-01-17T11:11:24Z

core/src/ledger/storage/mod.rs

+                self.write(
+                    &token::balance_key(addr, &masp_addr),
+                    total_supply_in_masp.try_to_vec().unwrap(),
+                )


I'm not sure how this gets initialized on chain from the genesis config @brentstone @murisi would either of you know?

mariari · 2023-01-17T11:22:19Z

It seems that currently the e2e tests fail, I have to investigate why still

mariari · 2023-01-18T07:56:11Z

Currently 19 e2e tests fail:

ifailures:
    e2e::eth_bridge_tests::everything
    e2e::ibc_tests::run_ledger_ibc
    e2e::ledger_tests::double_signing_gets_slashed ; timeout
    e2e::ledger_tests::implicit_account_reveal_pk
    e2e::ledger_tests::invalid_transactions        ; timeout
    e2e::ledger_tests::ledger_many_txs_in_a_block
    e2e::ledger_tests::ledger_txs_and_queries

    ;; Must be updated to the new rewards
    ;; "Needle: NAM: 0.2", in the temp file "NAM: 467977.4\r\n"
    e2e::ledger_tests::masp_incentives

    e2e::ledger_tests::masp_pinned_txs       ; passes on rerun (timeout?)
    e2e::ledger_tests::masp_txs_and_queries  ; passes on rerun (timeout?)
    e2e::ledger_tests::pos_bonds
    e2e::ledger_tests::pos_init_validator
    e2e::ledger_tests::pos_rewards
    e2e::ledger_tests::proposal_offline

    ;; trying to connect: tcp connect error: OS error 111
    ;; Figure this might be due to a timeout somewhere
    e2e::ledger_tests::proposal_submission

    e2e::ledger_tests::run_ledger_load_state_and_reset
    e2e::ledger_tests::test_genesis_validators         ; timeout
    e2e::ledger_tests::test_node_connectivity_and_consensus
    e2e::wallet_tests::wallet_encrypted_key_cmds

Most of them are due to timeout, rerunning two of them locally has made it pass (likely again due to timeout). With two interesting e2e tests failing

the first is masp_incentives which should be updated to the new formula, however the contrast between the expected value and what was inflated seems a bit much. It goes from 0.2 NAM to 467977.4 NAM if I'm reading the tmp files correctly.

The other one is e2e::ledger_tests::proposal_submission which gives a tcp connection error, which may be due to timeouts on epoch, however that is unconfirmed.

Overall, I think the pr should be fit to merge outside of fine tuning masp_incentives, with timing issues handled in further prs

mariari · 2023-01-30T07:57:43Z

pls update wasm

murisi

Thanks for the changes! Overall the code looks good (though I've reviewed only your changes, not @brentstone 's). I'm mainly concerned about two and a half things:

Is the inflation that is minted exactly equal to what is stored in token::last_inflation(addr)? If it is, then maybe we can make this equivalence clearer in the code... If not, we need to fix the discrepancy lest it subtly affects the PID controller.
The conversion tree code was written under the assumption that no rewards are given for shielded NAM (and hence that no compounding takes place). This was my mistake. Since your code can potentially give rewards for locked NAM (meaning that compounding necessarily happens), the conversion tree code needs to be updated.
Now that compounding of rewards can take place, we need to add an E2E test for this compounding behavior.

murisi · 2023-02-13T12:35:55Z

core/src/types/token/parameters.rs

+pub const LOCKED_RATIO_TARGET_KEY: &str = "locked_ratio_target";
+
+/// The key for the max reward rate for a given asset
+pub const MAX_REWARD_RATE: &str = "max_reward_rate";


This could instead be named MAX_REWARD_RATE_KEY lest this variable be mistaken for an actual rate. This doesn't matter too much though since the variable's meaning should be clear from context.

I agree with @murisi here

Suggested change

pub const MAX_REWARD_RATE: &str = "max_reward_rate";

pub const MAX_REWARD_RATE_KEY: &str = "max_reward_rate";

murisi · 2023-02-13T14:36:46Z

core/src/ledger/storage/masp_conversions.rs

    // Reward all tokens according to above reward rates
-    for (addr, reward) in &masp_rewards {
+    for addr in masp_rewards.keys() {
+        let reward = calculate_masp_rewards(wl_storage, addr)


The following code assumes that the reward for locked NAM is 0, i.e. it assumes that compounding doesn't happen. What follows would need to be modified to correctly handle NAM compounding. Something along the lines of deflating NAM_0 rewards, and not handing out NAM_0 rewards for NAM. Probably a test should be added to ensure that compounding happens correctly.

murisi · 2023-02-13T15:16:24Z

core/src/types/token/parameters.rs

+    }
+}
+
+impl Default for Parameters {


Maybe a better default would be to have 0 rewards and 0 locked ratio target? I.e. behave like there is no incentive system by default. But I guess this is debatable...

murisi · 2023-02-13T15:35:47Z

core/src/types/token/parameters.rs

+    /// Shielded Pool nominal proportional gain for the given token
+    kp_gain_nom: Decimal,
+    /// Locked ratio for the given token
+    locked_ratio_target_key: Decimal,


Did you instead mean locked_ratio_target? This variable doesn't seem to be a key...

murisi · 2023-02-14T07:36:14Z

core/src/ledger/storage/masp_conversions.rs

        // Since floor(a) + floor(b) <= floor(a+b), there will always be
        // enough rewards to reimburse users
-        total_reward += (addr_bal * *reward).0;
+        total_reward += (addr_bal * reward).0;


I'm not sure that the right-hand-side, (addr_bal * reward).0, equals inflation as computed in calculate_masp_rewards. By the implementation of Mul for Amount, (addr_bal * reward).0=(addr_bal / reward.1)*reward.0=(addr_bal / 100)*(inflation * 100 / total_token_in_masp)=(addr_bal / 100)*(inflation * 100 / addr_bal). Algebraically this should equal inflation, but I suspect the first factor alone, (addr_bal / 100), will introduce rounding errors.

Overall it feels like we are recomputing a value that we already knew/computed before, but in a way that is vulnerable to overflows and rounding errors. Maybe we could move the mint_tokens call from this function into calculate_masp_rewards (where the inflation variable is available)? (Whatever we do with rounding errors, the MASP must always have exactly, or slightly more than enough account balance to redeem shielded conversions/withdrawals...)

…rewards products

…r e2e

mariari · 2023-06-05T05:24:53Z

Last commit before merging v0.16.0. Note that the e2e incetives fails, but everything else works

Namada 0.16.0

mariari · 2023-06-30T06:35:57Z

pls update wasms

Namada 0.17.5

…ewards

- _Changes_ + We offset the denominations of the tokens by their denomination relative to the nam token thus a 10^18 token gets offset to 10^6, which is the base systems denomination

bengtlofgren · 2023-07-23T10:14:16Z

genesis/dev.toml

 # Portion of a validator's stake that should be slashed on a light
 # client attack.
-light_client_attack_min_slash_rate = "0.001"
+light_client_attack_min_slash_rate = "0.1"


Why has this been changed in this PR? Not sure I follow?

mariari force-pushed the mariari/inflation-rewards branch 2 times, most recently from cdfe93e to ea7bf48 Compare January 16, 2023 10:44

mariari marked this pull request as ready for review January 17, 2023 06:57

mariari requested review from brentstone and murisi January 17, 2023 06:57

mariari commented Jan 17, 2023

View reviewed changes

tzemanovic changed the base branch from brent/pos-inflation-rewards to main January 18, 2023 10:14

mariari force-pushed the mariari/inflation-rewards branch 3 times, most recently from 831a49a to 84343fd Compare February 9, 2023 03:49

murisi requested changes Feb 14, 2023

View reviewed changes

mariari force-pushed the mariari/inflation-rewards branch 4 times, most recently from 13d918e to 8bf245e Compare February 22, 2023 11:17

brentstone added 12 commits February 22, 2023 17:06

inflation and rewards modules

4ac196d

storage types and keys for inflation

0e8b201

log block rewards with accumulator LazyMaps and apply inflation with …

94449a0

…rewards products

trigger 2-block countdown to new epoch for Tendermint

92609ca

configure genesis to set up any number of validators

33ce31f

tests for inflation

efffafd

Cargo lock and tomls

11fffae

update comments and documentation

074027b

WIP - current changes, debug printouts, etc stuff

b030fd8

fix e2e test ibc

a41952f

upgrade total token supply tracking and balance tracking at genesis

a13f673

fix e2e::ledger_tests::proposal_submission

6b0dc7f

mariari force-pushed the mariari/inflation-rewards branch from e5d165e to 9ea97ef Compare June 5, 2023 03:26

mariari added 4 commits June 5, 2023 11:30

Make the formula for the conversion rates reliable, also dump logs fo…

3db724a

…r e2e

Now handle native inflation rewards for native tokens.

efef60c

Corrected the compounding of the MASP transparent balance.

95357f3

Update the test with the correct inflation values

38bb6bb

mariari force-pushed the mariari/inflation-rewards branch from 9ea97ef to 38bb6bb Compare June 5, 2023 03:31

mariari force-pushed the mariari/inflation-rewards branch 4 times, most recently from 9bb003e to a46b699 Compare June 12, 2023 06:07

mariari added 2 commits June 14, 2023 12:22

Merge tag 'v0.16.0' into mariari/inflation-rewards

b0718fe

Namada 0.16.0

Fixup clippy issues

67d9ce5

mariari force-pushed the mariari/inflation-rewards branch from a46b699 to 67d9ce5 Compare June 14, 2023 04:33

Merge tag 'v0.17.5' into mariari/inflation-rewards

5fc78f6

Namada 0.17.5

mariari force-pushed the mariari/inflation-rewards branch from dd379ce to 5fc78f6 Compare June 30, 2023 06:37

Merge branch 'mariari/fixed-amounts-history' into mariari/inflation-r…

d92eb8f

…ewards

mariari force-pushed the mariari/inflation-rewards branch 5 times, most recently from 42998e2 to 31930e4 Compare July 5, 2023 15:45

fixup! Continue debugging

7b3442b

- _Changes_ + We offset the denominations of the tokens by their denomination relative to the nam token thus a 10^18 token gets offset to 10^6, which is the base systems denomination

mariari force-pushed the mariari/inflation-rewards branch from 31930e4 to 7b3442b Compare July 6, 2023 12:53

multiply and divide stuff

789603c

tzemanovic mentioned this pull request Jul 18, 2023

Draft 0.20 #1702

Closed

bengtlofgren reviewed Jul 23, 2023

View reviewed changes

murisi mentioned this pull request Oct 17, 2023

Implemented compounding inflation for native tokens. #1985

Merged

2 tasks

murisi closed this Oct 20, 2023

	pub const MAX_REWARD_RATE: &str = "max_reward_rate";
	pub const MAX_REWARD_RATE_KEY: &str = "max_reward_rate";

Shielded Inflation rewards #1025

Shielded Inflation rewards #1025

Conversation

mariari commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariari commented Jan 17, 2023

Uh oh!

mariari commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariari commented Jan 30, 2023

Uh oh!

murisi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariari commented Jun 5, 2023

Uh oh!

mariari commented Jun 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mariari commented Jan 16, 2023 •

edited

Loading

mariari commented Jan 18, 2023 •

edited

Loading

murisi left a comment •

edited

Loading