Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Staking module: approximate fraction into perbill to avoid overflow #2889

Merged
merged 10 commits into from
Jun 24, 2019

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Jun 17, 2019

previous strategy: #2863
related to #706 and #1572

in this PR we solve the potential overflow by approximating the fraction (nominator.value/total_exposure) into a Perbill.

This approximation is visible in tests as you can see a -1 in some reward in some test.

TODO:

  • maybe implement from rational_approximation for other Perthings
  • test for perthing
  • do we accept such approximation everytime or should we approximate only when it overflows ?
  • maybe use a more precise approximation like PerU64 (it is the maximum we can do as otherwise we don't have double size arithmetic in our hands) ?

@gui1117 gui1117 added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 17, 2019
@gui1117 gui1117 requested a review from kianenigma June 17, 2019 15:49
@gui1117 gui1117 added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. A4-gotissues labels Jun 18, 2019
@kianenigma
Copy link
Contributor

This is good in terms of replacing the two potentially dangerous multiplication closures. I wonder if we have anywhere else in runtime such cases? (relating to the topic of the issue: sweep runtime..).

Furthermore:

I think this is enough to replace the whole ACCURACY constant in phragmen. I can do it myself by pushing here or you can take over.

if *c != n.who {
-	let ratio = {
-		// Full support. No need to calculate.
-		if *n.load == *e.load { Perbill::one() }
-		else {
-			// This should not saturate. Safest is to just check
-			if let Some(r) = ACCURACY.checked_mul(*e.load) {
-				r / n.load.max(1)
-			} else {
-				// Just a simple trick.
-				*e.load / (n.load.max(1) / ACCURACY)
-			}
-		}
-	};
+      let ratio = Perbill::from_rational_approximation(*e.load() / *n.load)
	e.ratio = ratio;
	assignment.1.push((e.who.clone(), ratio));
}

Since here the ACCURACY is already u32 it should be good to fit in Perbill. Some minor changes are then needed to the lib.rs file as well.

I also thought about what to do with U128. I think unfortunately it is simply impossible to approximate it with Perbill/mill due to notorious

// tries to approximate `1/approval_stake`
c.score = Fraction::from_xth(c.approval_stake);

where c.approval_stake can, in theory (not in practice thou') be more than the max balance type and this is will be in an issue:

Perbill::from_rational_approximation(1, 3*u64::max_value()), 

(^^ actually overflows, I was expecting it to collapse to zero)

core/sr-primitives/src/lib.rs Show resolved Hide resolved
@gui1117
Copy link
Contributor Author

gui1117 commented Jun 19, 2019

Yes I think introducing Persomething in phragmen worth it.

we can introduce Perbill or create PerU32 or even create PerU64 (we can't do more easily as we need double size arithmetic, and u128 is kind of the limit.

@gui1117
Copy link
Contributor Author

gui1117 commented Jun 19, 2019

though in another PR

@gui1117 gui1117 added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jun 19, 2019
@gavofyork
Copy link
Member

would be nice to see a test for the overflow case (that fails on the original code)

@gui1117
Copy link
Contributor Author

gui1117 commented Jun 20, 2019

yes done

gui1117 and others added 2 commits June 21, 2019 15:47
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
@gavofyork gavofyork merged commit 7f61107 into master Jun 24, 2019
@gavofyork gavofyork deleted the gui-fix-overflow-2 branch June 24, 2019 11:23
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Jul 10, 2019
…aritytech#2889)

* approximate fraction into perbill

* test

* fix comment

* line width

* bump impl version

* rename test for better naming

* test overflow

* Apply suggestions from code review

Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants