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

[WIP] fix overflow in staking #2863

Closed
wants to merge 5 commits into from
Closed

[WIP] fix overflow in staking #2863

wants to merge 5 commits into from

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Jun 14, 2019

Introduce a new struct Rational (that we might want to move to sr-primitves).

Rational allow to have a number represented as int + num/den with num < den and den*num <= max_value

This allows us to multiply whatever number with this rational it will only overflow if the actual result doesn't fits into N.

The property den*num <= max_value is done by lowering num/den precision, by deviding them by 2 as long as it is superior.

TODO:

  • see todo in the code
  • maybe some phragmen stuff as well.
  • tests
  • is the approximation OK ? how to evaluate that because I'm not confident at all with the current approximation actually

related to #706 and #1572

EDIT: another solution would be to ask simplearithmetic to have doublesize operation.

EDIT: another solution is to have an associated type which is ConvertDoubleSize, or DoOperationMultiplyDivide kind of things and let user do his operation

@gui1117 gui1117 added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 14, 2019
@gui1117
Copy link
Contributor Author

gui1117 commented Jun 14, 2019

well, we might be able to improve precision:

all in all what we want is doing a * b / c so if we can afford it the best seems is:
(great common divisor == gcd)
we compute gcd(a,c)
we reduce
we compute gcd(b, c)
we reduce
then we can decrease precision of d * e / f by shifting f and max(d,e) until we can compute result.

.try_into().unwrap_or(u32::max_value());

let err = (approx as i64 - res as i64).abs() as f64 / q as f64;
assert!(err < 0.0001);
Copy link
Contributor Author

@gui1117 gui1117 Jun 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

despite being small relatively to q, this error can be quite big if q is big. evaluate the error is difficult.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for instance if reward = 7, total_exposure = 0xffff_ffff and other_value is 1073741822 then the approximation is 0 and actual result is 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if reward is small then it is better to have the rational computed for other_value/total_exposure, but that implies compute it for each nominator.

Another solution is just to reduce precision of total_exposure so total_exposure^2 doesn't overflow. (let's by shifting X time)

then we do shift(max(reward, other_value)) X time, and it would works and might be better for approximation.

@gui1117
Copy link
Contributor Author

gui1117 commented Jun 14, 2019

Another idea is to convert exposure_other_value/exposure_total to a Permill, but I don't really know how to do that without potential overflow as well.

@burdges
Copy link

burdges commented Jun 14, 2019

There are some clever schemes for rational like http://www.cs.toronto.edu/~hehner/ratno.pdf but not sure if any provide any real advantages on standard hardware, and none look designed for implementations.

Yes a*y/z = (a / gcd(z,a)) * (y / gcd(z,a)) and you can do a/gcd(z,a) quickly.

I'm not sure about any fast tricks for addition in this representation, maybe you just need to reduce
(a + b/c) + (x+y/z) = (a+x) + (bz + yc)/cz.

@gui1117
Copy link
Contributor Author

gui1117 commented Jun 14, 2019

@burdges thanks, I take a look.

all in all a reward (a), for each validator a total_exposure(c) and for each nominator a part(b). and we want to compute ab/c but ab can overflow.

my PR does an unsounded approximation of a/c with the property ac doesn't overflow and then we compute ab/c with some euclidian division and remainder that works with this property. but the approximation worry me.

Something I want would be to convert a/c or b/c to a per u64::max representation and then I'm ok.

@burdges
Copy link

burdges commented Jun 14, 2019

I'd miss the overflow issue. If these values are u64 then use u128 when working with products before reduction. I'd expect reduction to be slow, so don't be afraid to waste some bits collecting some faster operations and then doing only one reduction. It's likely faster to put 3 u32 into a u128 and then reduce only once.

@gui1117
Copy link
Contributor Author

gui1117 commented Jun 14, 2019

yes true, but I can't have any guarantee on the size of the arithmetic provided, a user could theoritically have balance which is u1024.

That's why a good solution would be something like BigInt in num_big_int or requiring simpleArithmetic trait to provide a doublesize space to do operation in.

EDIT: but at the same time we might not want to have this BigInt stuff in the runtime.

@burdges
Copy link

burdges commented Jun 14, 2019

I see. Yes, maybe some trait that leaves space, not sure if SimpleArithmetic makes sense.

pub struct Expanding<N>(pub N)
pub trait ExpandingArithmetic :
        ... + Mul<Self, Output = Self::Doube> + ...
{
    type Double;
}

I'd suggest leaving some bound so that addition does not need to go to the wider type though.

@gui1117
Copy link
Contributor Author

gui1117 commented Jun 17, 2019

ok I tried a new direction, I will open a new PR to avoid confusion

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants