Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make staking inflation curve configurable. #3644

Merged
merged 11 commits into from Sep 21, 2019

Conversation

@thiolliere
Copy link
Contributor

commented Sep 18, 2019

Context:

currently the curve used to compute the reward of the validators and nominators is fixed in srml-staking. This PR makes it configurable at the runtime (node-runtime) level.

Done:

  • Definition of the curve has changed to just a list of points: (Perbill, Perbill), this allows to define any function from [0, 1] to [0, 1] even the curve is very steep. (The limitation for [0, 1] -> [0, 1] is to avoid difficulties with overflows)
  • definition of PiecewiseLinear is moved to sr-primitives, it uses its own function multiply_by_rational_saturating. Maybe we want to make this latter function available publicly.
  • to declare a curve easily a proc-macro crate is introduced at srml-staking-reward-curve.
    this crate generate a test for the precision alongside the declaration.
  • the generation of the curve has changed for efficiency reason because the time to compile is important.
    new algorithm just divide into variable segment with some maximum size.
@thiolliere thiolliere requested a review from kianenigma as a code owner Sep 18, 2019
@gavofyork

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Awesome :)

Will need a polkadot update PR but looking very good otherwise.

Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
// Compute value * p / q.
// This is guarantee not to overflow on whatever values nor loose precision.
// `q` must be superior to zero.
fn multiply_by_rational_saturating<N>(value: N, p: u32, q: u32) -> N

This comment has been minimized.

Copy link
@kianenigma

kianenigma Sep 19, 2019

Contributor

this stuff is okay for now but as me want to refactor more of the arithmatic stuff and move them into their own crate, I wonder:

What is the accuracy requirement here? if p and q are always u32 and we want to be able to accurately compute it, then perquintillion is the option to go atm.

On the other hand, that's a bit of on overkill and has some redundant bytes. If this pattern (a * p / q where p and q are u32) is common then actually making the per-u32 might be worthwhile. It will also be a challenge to see if the macros in the sr-arithmatic refactor actually work or now 😀

This comment has been minimized.

Copy link
@thiolliere

thiolliere Sep 19, 2019

Author Contributor

What is the accuracy requirement here? if p and q are always u32 and we want to be able to accurately compute it, then perquintillion is the option to go atm.

well doesn't perquintillion::from_rational_approximation does loose a bit because like 1/3 is 33333.../quintillion but maybe we can afford this.
Or maybe now that I think of it we should go the other way around make perthings use this function 🤔

This comment has been minimized.

Copy link
@kianenigma

kianenigma Sep 19, 2019

Contributor

I think what you mean is not what I meant by accuracy, but rather a rounding error of at most 1 unit which, imo, we can afford. What I mean is:

The most fine-grained approximation of this function is p = 1, q = u32::MAX. This will be roughly 1 divided by 4 billion. Naturally, per-1-billion is inherently incapable of representing this number while a per quintillion is. This is why to abstract this, something like per-billion is inaccurate, per quintillion is slightly wasteful while per-u32 is ideal.

This comment has been minimized.

Copy link
@thiolliere

thiolliere Sep 21, 2019

Author Contributor

no what I meant is that there is an inherent imprecision from converting from one fraction to one another in some cases: let's say a = 100billion, p=1, q=3 converting 1/3 into a perbillion is inaccurate and dependings on how much a is bigger than a billion the inaccuracy remains:

	let a = 100 * 1_000_000_000u64;
	let res = multiply_by_rational_saturating(a, 1, 3);
	assert_eq!(res, 33333333333);
	let res = super::Perbill::from_rational_approximation(1u32, 3u32) * a;
	assert_eq!(res, 33333333300);
/// - `test_precision`: The maximum error allowed in the generated test.
/// Expressed in millionth, must be between 0 and 1_000_000.
///
/// # Example

This comment has been minimized.

Copy link
@kianenigma
@@ -0,0 +1,67 @@
/// Return Per-million value.
pub fn log2(p: u32, q: u32) -> u32 {

This comment has been minimized.

Copy link
@kianenigma

kianenigma Sep 19, 2019

Contributor

this probably should be placed in sr-primitives?

This comment has been minimized.

Copy link
@kianenigma

kianenigma Sep 19, 2019

Contributor

(or now I see your assertions and comments below about how this is not a general-purpose log2. Maybe let it be here and at some point move to sr-primitives if it became general purpose.)

This comment has been minimized.

Copy link
@thiolliere

thiolliere Sep 19, 2019

Author Contributor

for now it is not part of runtime just compile-time.
thus I didn't completely checked that loop cannot loop forever in any situation and there is assert inside.
What I check is that if it produce something is it accurate.

But yes we could do. Although we should maybe precisely know the number of loop it corst in worst situation.

Copy link
Contributor

left a comment

Some refactor suggestions but overall seems to deliver what it intended to do --make the curve configurable-- perfectly :) LGTM

@kianenigma

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

Some refactor suggestions but overall seems to deliver what it intended to do --make the curve configurable-- perfectly :) LGTM

(just saw the label being in-progress -- ^^ given that it is actually done.)

thiolliere and others added 2 commits Sep 19, 2019
Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@gavofyork gavofyork merged commit de90fe7 into master Sep 21, 2019
10 checks passed
10 checks passed
continuous-integration/gitlab-cargo-check-benches Build stage: test; status: success
Details
continuous-integration/gitlab-cargo-check-subkey Build stage: test; status: success
Details
continuous-integration/gitlab-check-line-width Build stage: test; status: success
Details
continuous-integration/gitlab-check-runtime Build stage: test; status: success
Details
continuous-integration/gitlab-check-web-wasm Build stage: test; status: success
Details
continuous-integration/gitlab-check_warnings Build stage: build; status: success
Details
continuous-integration/gitlab-node-exits Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux-stable Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux-stable-int Build stage: test; status: success
Details
continuous-integration/gitlab-test-srml-staking Build stage: test; status: success
Details
@gavofyork gavofyork deleted the gui-gav-investigate_npos branch Sep 21, 2019
en added a commit to en/substrate that referenced this pull request Sep 24, 2019
* Draft for new design of NPoS rewards

* finish code

* fix test

* add tests

* improve log test

* version bump

* Update srml/staking/reward-curve/Cargo.toml

Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* u128 -> u64

* make conversion to smaller type safe

* Update core/sr-primitives/src/curve.rs

Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.