feat(blobstream): utilizes sdk.Dec type for power difference calculation #2719

staheri14 · 2023-10-18T22:18:36Z

Closes #2700
Closes #2699

staheri14 · 2023-10-18T22:22:11Z

x/blobstream/types/validator.go

@@ -112,7 +113,8 @@ func EVMAddrLessThan(e common.Address, o common.Address) bool {
 // increases by 1% due to inflation, we shouldn't have to generate a new
 // validator set, after all the validators retained their relative percentages
 // during inflation and normalized Blobstream power shows no difference.
-func (ibv InternalBridgeValidators) PowerDiff(c InternalBridgeValidators) float64 {
+func (ibv InternalBridgeValidators) PowerDiff(
+	c InternalBridgeValidators) sdk.Dec {
 	powers := map[string]int64{}


[note to the reviewers] Alternatively, we could perform all calculations using sdk.Dec from the outset. However, since there are no floating-point calculations involved until later in this function, I thought it might be more efficient to continue using integer operations until that point. Nevertheless, if it is preferable to use sdk.Dec arithmetic throughout, I can update the initial portion of the function accordingly.

I think switching to sdk.Dec would be also future-proof in case we wanted to make any changes not to give to future devs the ability to choose. So, I would go with switching everything to sdk.Dec

Thanks for your comment!
I'm unsure how this change would effectively enforce any constraints on future developers when the function signature remains unchanged, and only its internals are to be modified to utilize sdk.Dec. Could you please provide more insight into your perspective?

staheri14 · 2023-10-18T23:08:11Z

x/blobstream/types/validator.go

 	powers := map[string]int64{}
 	// loop over ibv and initialize the map with their powers
 	for _, bv := range ibv {
-		powers[bv.EVMAddress.Hex()] = int64(bv.Power)
+		powers[bv.EVMAddress.Hex()] = int64(bv.Power) // TODO: overflow?


The validator powers are of type uint64, and the current conversion could potentially lead to overflow. Upon reviewing the ValidateBasic function, it appears that there is no range check performed on the power value. It would be prudent to confirm whether there might be any validators with powers falling within the range of [math.MaxInt64, math.MaxUint64]. Such power values could indeed cause an overflow issue in the code line mentioned above. (For example the power value of math.MaxUint64 becomes -1 when converted to int64)

good catch 👍 But I think if the state machine got to a state where this overflows, then something is seriously messed up with the staking module. In fact, values like those will not be accepted by the staking module, then we normalize those values before this step. I think that we can use a validation at this level as a last line of defense, but reaching it is super unlikely if I understand corerctly

values like those will not be accepted by the staking module

So, does it mean that there will never be a case in which a validator's power reside in this range [math.MaxInt64, math.MaxUint64]? and it is guaranteed by the staking module?

yes, the validator power is using int64, so no overflow would happen. However, I found something interesting:
https://github.com/celestiaorg/cosmos-sdk/blob/main/x/staking/types/validator.go#L369-L372

// PotentialConsensusPower returns the potential consensus-engine power. func (v Validator) PotentialConsensusPower(r math.Int) int64 { return sdk.TokensToConsensusPower(v.Tokens, r) }

The power is being reduced by a factor of:

// DefaultPowerReduction is the default amount of staking tokens required for 1 unit of consensus-engine power var DefaultPowerReduction = NewIntFromUint64(1000000)

However, the tokens amount is represented by an int256... So, there might be overflow there when going from tokens to consensus-engine power unless there is the assumption that the staked tokens wouldn't be that much.

cc @evan-forbes any idea?

sorry mind pointing out where's the int256 to save me a sec 😅

this is good to keep in the back of our heads, especially for networks such as arabica where the supply and voting power is getting rather insane.

In the validator struct, the delegated tokens are represented using Tokens which are Ints which are sdkmath.Int.

Then, these are used when calculating the validator delegation and power in here after getting the consensus-engine power which would panic when converting from sdkmath.Int to int64 if the amount is out of bounds.

codecov-commenter · 2023-10-18T23:12:29Z

Codecov Report

Merging #2719 (9951654) into main (fc348bf) will increase coverage by 0.02%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##             main    #2719      +/-   ##
==========================================
+ Coverage   19.93%   19.95%   +0.02%     
==========================================
  Files         139      139              
  Lines       16702    16707       +5     
==========================================
+ Hits         3329     3334       +5     
  Misses      13051    13051              
  Partials      322      322

Files	Coverage Δ
x/blobstream/abci.go	`59.55% <100.00%> (ø)`
x/blobstream/types/validator.go	`42.71% <92.30%> (+2.92%)`	⬆️

staheri14 · 2023-10-18T23:22:52Z

It appears that this PR also resolves issue #2699. I couldn't locate any float calculations in validator.go except for those being handled by this PR. @sweexordious, could you please confirm whether this is the case? then I'll update the description of PR accordingly.

rach-id · 2023-10-18T23:40:24Z

It appears that this PR also resolves issue #2699. I couldn't locate any float calculations in validator.go except for those being handled by this PR. @sweexordious, could you please confirm whether this is the case? then I'll update the description of PR accordingly.

Apparently yes, this is also handling that issue, please do update. Thanks a lot for helping with this 🙏

rach-id

Awesome stuff 👍

I will approve after we switch all arithmetic operations to using sdk.Dec.

rach-id · 2023-10-18T23:42:54Z

x/blobstream/types/validator.go

@@ -112,7 +113,8 @@ func EVMAddrLessThan(e common.Address, o common.Address) bool {
 // increases by 1% due to inflation, we shouldn't have to generate a new
 // validator set, after all the validators retained their relative percentages
 // during inflation and normalized Blobstream power shows no difference.
-func (ibv InternalBridgeValidators) PowerDiff(c InternalBridgeValidators) float64 {
+func (ibv InternalBridgeValidators) PowerDiff(
+	c InternalBridgeValidators) sdk.Dec {
 	powers := map[string]int64{}


I think switching to sdk.Dec would be also future-proof in case we wanted to make any changes not to give to future devs the ability to choose. So, I would go with switching everything to sdk.Dec

evan-forbes · 2023-10-19T01:26:06Z

I think switching to sdk.Dec would be also future-proof in case we wanted to make any changes not to give to future devs the ability to choose. So, I would go with switching everything to sdk.Dec

imo I think we should do this, but in a different issue/bump. We still need to test this on IRL networks to make sure that we're doing this in a non consensus breaking way or reevaluate and the simpler we can make that process the better

evan-forbes

one quick question to remove another instance of floats right above these, but still not blocking on it

x/blobstream/types/validator.go

evan-forbes · 2023-10-19T01:35:31Z

before merging the backported version of this, we should sync from scratch and ideally perform other tests to ensure that this is not consensus breaking. If it is, then we can reevaluate our options from there

staheri14 · 2023-10-19T17:33:39Z

I think switching to sdk.Dec would be also future-proof in case we wanted to make any changes not to give to future devs the ability to choose. So, I would go with switching everything to sdk.Dec

imo I think we should do this, but in a different issue/bump. We still need to test this on IRL networks to make sure that we're doing this in a non consensus breaking way or reevaluate and the simpler we can make that process the better

Considering this comment of @evan-forbes, do we still want to refactor the initial section of the PowerDiff function to utilize sdk.Dec @sweexordious in this PR? In my opinion, since the initial section of this function exclusively involves integer operations, converting it to utilize sdk.Dec arithmetic wouldn't enhance computational precision; instead, it would primarily improve code readability and clarity.
PS: I have already started updating the first portion of the PowerDiff and it is in progress; there is one intricacy with the Abs method of sdk.Dec type, which converts zero values to nil which then causes the rest of computations to panic (and I am debugging and trying to find a fix). I can open a follow-up PR to refactor the first portion of PowerDiff function, if there is an agreement on whether refactoring is needed or not.

…in-significantpowerdiff

evan-forbes · 2023-10-19T18:26:12Z

In my opinion, since the initial section of this function exclusively involves integer operations, converting it to utilize sdk.Dec arithmetic wouldn't enhance computational precision; instead, it would primarily improve code readability and clarity.

my understanding was that this would potentially stop an overflow with large stake? correct me if I'm wrong @sweexordious

reguardless I think we should merge this or some version of it sooner rather than later as ideally this would be included in the version that is used for mainnet

…ec-in-significantpowerdiff

x/blobstream/types/validator.go

.github/workflows/ci-release.yml

rach-id

awesome stuff 👍 👍

just apply the comments and will approve

.goreleaser.yaml

x/blobstream/types/validator.go

…in-significantpowerdiff

rach-id

…ion (#2719) Closes #2700 Closes #2699 (cherry picked from commit 6192a5a) # Conflicts: # x/qgb/types/types_test.go # x/qgb/types/validator.go

…ion (backport #2719) (#2735) This is an automatic backport of pull request #2719 done by [Mergify](https://mergify.com). Cherry-pick of 6192a5a has failed: ``` On branch mergify/bp/v1.x/pr-2719 Your branch is up to date with 'origin/v1.x'. You are currently cherry-picking commit 6192a5a. (fix conflicts and run "git cherry-pick --continue") (use "git cherry-pick --skip" to skip this patch) (use "git cherry-pick --abort" to cancel the cherry-pick operation) Changes to be committed: modified: x/qgb/abci.go Unmerged paths: (use "git add <file>..." to mark resolution) both modified: x/qgb/types/types_test.go both modified: x/qgb/types/validator.go ``` To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details> --------- Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com> Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

staheri14 added 5 commits October 18, 2023 15:06

redefines SignificantPowerDifferenceThreshold as sdk.Dec

8751950

refactors PowerDiff to return sdk.Dec

db47d1d

reworks the tests with the new change

ff5851a

returns absolute value

20a1b59

adds the function name to the doc

cc712ab

staheri14 requested review from rach-id and evan-forbes as code owners October 18, 2023 22:18

celestia-bot requested a review from a team October 18, 2023 22:18

staheri14 commented Oct 18, 2023

View reviewed changes

staheri14 added 2 commits October 18, 2023 15:23

adds a todo

6683a04

fixes goimports complaints

0d49079

staheri14 self-assigned this Oct 18, 2023

brings all func signature into one line

73c7664

staheri14 commented Oct 18, 2023

View reviewed changes

staheri14 added 2 commits October 18, 2023 16:13

reorders imports

b3dcdcc

fixes another set of formatting issues

91080ff

rach-id reviewed Oct 18, 2023

View reviewed changes

evan-forbes previously approved these changes Oct 19, 2023

View reviewed changes

x/blobstream/types/validator.go Outdated Show resolved Hide resolved

evan-forbes added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Oct 19, 2023

Merge remote-tracking branch 'origin/main' into sanaz/use-of-sdk-dec-…

b061f0c

…in-significantpowerdiff

celestia-bot requested a review from a team October 19, 2023 17:35

staheri14 enabled auto-merge (squash) October 19, 2023 17:52

staheri14 requested a review from rach-id October 19, 2023 17:52

staheri14 added 2 commits October 19, 2023 16:04

Merge branch 'sanaz/use-sdkdec-for-powerdiff' into sanaz/use-of-sdk-d…

aa7a736

…ec-in-significantpowerdiff

removes todo

7e96c5b

staheri14 dismissed evan-forbes’s stale review via 7e96c5b October 19, 2023 23:06

celestia-bot requested a review from a team October 19, 2023 23:07

staheri14 added 2 commits October 19, 2023 16:07

deletes remnants of int64 calculations

c3cf767

uses equality check of Dec type

5fe16f0

staheri14 requested a review from evan-forbes October 19, 2023 23:14

resolves go linter issues

f7d07c7

staheri14 enabled auto-merge (squash) October 19, 2023 23:23

staheri14 requested review from rootulp and cmwaters as code owners October 19, 2023 23:39

rach-id reviewed Oct 19, 2023

View reviewed changes

x/blobstream/types/validator.go Outdated Show resolved Hide resolved

celestia-bot requested a review from a team October 19, 2023 23:40

staheri14 requested a review from rach-id October 19, 2023 23:40

rach-id reviewed Oct 19, 2023

View reviewed changes

.github/workflows/ci-release.yml Outdated Show resolved Hide resolved

rach-id reviewed Oct 19, 2023

View reviewed changes

.goreleaser.yaml Outdated Show resolved Hide resolved

staheri14 force-pushed the sanaz/use-of-sdk-dec-in-significantpowerdiff branch from efae962 to f7d07c7 Compare October 19, 2023 23:47

celestia-bot requested a review from a team October 19, 2023 23:47

converts uint64 to bigint

bddfcfa

rach-id reviewed Oct 19, 2023

View reviewed changes

x/blobstream/types/validator.go Outdated Show resolved Hide resolved

staheri14 added 2 commits October 19, 2023 16:52

adds another conversion from uint64 to bigint

d3c7ae0

Merge remote-tracking branch 'origin/main' into sanaz/use-of-sdk-dec-…

9951654

…in-significantpowerdiff

celestia-bot requested a review from a team October 19, 2023 23:53

rach-id approved these changes Oct 19, 2023

View reviewed changes

evan-forbes approved these changes Oct 20, 2023

View reviewed changes

staheri14 merged commit 6192a5a into main Oct 20, 2023

staheri14 deleted the sanaz/use-of-sdk-dec-in-significantpowerdiff branch October 20, 2023 10:12

mergify bot mentioned this pull request Oct 20, 2023

feat(blobstream): utilizes sdk.Dec type for power difference calculation (backport #2719) #2735

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(blobstream): utilizes sdk.Dec type for power difference calculation #2719

feat(blobstream): utilizes sdk.Dec type for power difference calculation #2719

staheri14 commented Oct 18, 2023 •

edited

Loading

staheri14 Oct 18, 2023

rach-id Oct 18, 2023

staheri14 Oct 19, 2023

staheri14 Oct 18, 2023 •

edited

Loading

rach-id Oct 18, 2023

staheri14 Oct 18, 2023

rach-id Oct 19, 2023

evan-forbes Oct 19, 2023 •

edited

Loading

rach-id Oct 19, 2023

codecov-commenter commented Oct 18, 2023 •

edited

Loading

staheri14 commented Oct 18, 2023

rach-id commented Oct 18, 2023

rach-id left a comment

rach-id Oct 18, 2023

evan-forbes commented Oct 19, 2023

evan-forbes left a comment •

edited

Loading

evan-forbes commented Oct 19, 2023

staheri14 commented Oct 19, 2023 •

edited

Loading

evan-forbes commented Oct 19, 2023

rach-id left a comment

rach-id left a comment

feat(blobstream): utilizes sdk.Dec type for power difference calculation #2719

feat(blobstream): utilizes sdk.Dec type for power difference calculation #2719

Conversation

staheri14 commented Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 18, 2023 • edited Loading

Codecov Report

staheri14 commented Oct 18, 2023

rach-id commented Oct 18, 2023

rach-id left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Oct 19, 2023

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

evan-forbes commented Oct 19, 2023

staheri14 commented Oct 19, 2023 • edited Loading

evan-forbes commented Oct 19, 2023

rach-id left a comment

Choose a reason for hiding this comment

rach-id left a comment

Choose a reason for hiding this comment

staheri14 commented Oct 18, 2023 •

edited

Loading

staheri14 Oct 18, 2023 •

edited

Loading

evan-forbes Oct 19, 2023 •

edited

Loading

codecov-commenter commented Oct 18, 2023 •

edited

Loading

evan-forbes left a comment •

edited

Loading

staheri14 commented Oct 19, 2023 •

edited

Loading