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.

Sign up for GitHub

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

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

Merged
merged 23 commits into from
Oct 20, 2023
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
8751950
redefines SignificantPowerDifferenceThreshold as sdk.Dec
staheri14 Oct 18, 2023
db47d1d
refactors PowerDiff to return sdk.Dec
staheri14 Oct 18, 2023
ff5851a
reworks the tests with the new change
staheri14 Oct 18, 2023
20a1b59
returns absolute value
staheri14 Oct 18, 2023
cc712ab
adds the function name to the doc
staheri14 Oct 18, 2023
6683a04
adds a todo
staheri14 Oct 18, 2023
0d49079
fixes goimports complaints
staheri14 Oct 18, 2023
73c7664
brings all func signature into one line
staheri14 Oct 18, 2023
b3dcdcc
reorders imports
staheri14 Oct 18, 2023
91080ff
fixes another set of formatting issues
staheri14 Oct 18, 2023
b061f0c
Merge remote-tracking branch 'origin/main' into sanaz/use-of-sdk-dec-…
staheri14 Oct 19, 2023
97c1d42
avoids using math.Abs
staheri14 Oct 19, 2023
463dcc4
deletes the todo to track with a GH issue
staheri14 Oct 19, 2023
d2d16cc
calculates absolutes with a custom function
staheri14 Oct 19, 2023
6de95f5
uses sdk.Dec for all operations
staheri14 Oct 19, 2023
aa7a736
Merge branch 'sanaz/use-sdkdec-for-powerdiff' into sanaz/use-of-sdk-d…
staheri14 Oct 19, 2023
7e96c5b
removes todo
staheri14 Oct 19, 2023
c3cf767
deletes remnants of int64 calculations
staheri14 Oct 19, 2023
5fe16f0
uses equality check of Dec type
staheri14 Oct 19, 2023
f7d07c7
resolves go linter issues
staheri14 Oct 19, 2023
bddfcfa
converts uint64 to bigint
staheri14 Oct 19, 2023
d3c7ae0
adds another conversion from uint64 to bigint
staheri14 Oct 19, 2023
9951654
Merge remote-tracking branch 'origin/main' into sanaz/use-of-sdk-dec-…
staheri14 Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions x/blobstream/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,6 @@ import (
)

const (
// SignificantPowerDifferenceThreshold is the threshold of change in the
// validator set power that would trigger the creation of a new valset
// request.
SignificantPowerDifferenceThreshold = 0.05

oneDay = 24 * time.Hour
oneWeek = 7 * oneDay
// AttestationExpiryTime is the expiration time of an attestation. When this
Expand All @@ -25,6 +20,11 @@ const (
AttestationExpiryTime = 3 * oneWeek // 3 weeks
)

// SignificantPowerDifferenceThreshold is the threshold of change in the
// validator set power that would trigger the creation of a new valset
// request.
var SignificantPowerDifferenceThreshold = sdk.NewDecWithPrec(5, 2) // 0.05

// EndBlocker is called at the end of every block.
func EndBlocker(ctx sdk.Context, k keeper.Keeper) {
// we always want to create the valset at first so that if there is a new
Expand Down Expand Up @@ -117,7 +117,8 @@ func handleValsetRequest(ctx sdk.Context, k keeper.Keeper) {
panic(sdkerrors.Wrap(err, "invalid latest valset members"))
}

significantPowerDiff = intCurrMembers.PowerDiff(*intLatestMembers) > SignificantPowerDifferenceThreshold
significantPowerDiff = intCurrMembers.PowerDiff(*intLatestMembers).GT(SignificantPowerDifferenceThreshold)

}

if (latestValset == nil) || (latestUnbondingHeight == uint64(ctx.BlockHeight())) || significantPowerDiff {
Expand Down
11 changes: 7 additions & 4 deletions x/blobstream/types/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
mrand "math/rand"
"testing"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/celestiaorg/celestia-app/x/blobstream/types"
gethcommon "github.com/ethereum/go-ethereum/common"
"github.com/stretchr/testify/assert"
Expand All @@ -14,7 +16,7 @@ func TestValsetPowerDiff(t *testing.T) {
specs := map[string]struct {
start types.BridgeValidators
diff types.BridgeValidators
exp float64
exp sdk.Dec
}{
"no diff": {
start: types.BridgeValidators{
Expand All @@ -27,7 +29,7 @@ func TestValsetPowerDiff(t *testing.T) {
{Power: 2, EvmAddress: "0x8E91960d704Df3fF24ECAb78AB9df1B5D9144140"},
{Power: 3, EvmAddress: "0xF14879a175A2F1cEFC7c616f35b6d9c2b0Fd8326"},
},
exp: 0.0,
exp: sdk.NewDecWithPrec(0, 1), // 0.0
},
"one": {
start: types.BridgeValidators{
Expand All @@ -40,7 +42,7 @@ func TestValsetPowerDiff(t *testing.T) {
{Power: 858993459, EvmAddress: "0x8E91960d704Df3fF24ECAb78AB9df1B5D9144140"},
{Power: 2576980377, EvmAddress: "0xF14879a175A2F1cEFC7c616f35b6d9c2b0Fd8326"},
},
exp: 0.2,
exp: sdk.NewDecWithPrec(2, 1), // 0.2
},
"real world": {
start: types.BridgeValidators{
Expand All @@ -63,9 +65,10 @@ func TestValsetPowerDiff(t *testing.T) {
{Power: 291759231, EvmAddress: "0xF14879a175A2F1cEFC7c616f35b6d9c2b0Fd8326"},
{Power: 6785098, EvmAddress: "0x37A0603dA2ff6377E5C7f75698dabA8EE4Ba97B8"},
},
exp: 0.010000000011641532,
exp: sdk.NewDecWithPrec(10000000011641532, 18), // 0.010000000011641532
},
}

for msg, spec := range specs {
t.Run(msg, func(t *testing.T) {
startInternal, _ := spec.start.ToInternal()
Expand Down
16 changes: 11 additions & 5 deletions x/blobstream/types/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package types

import (
"bytes"
math "math"
"math"
"sort"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/ethereum/go-ethereum/common"

"cosmossdk.io/errors"
Expand Down Expand Up @@ -32,7 +34,7 @@ func (b BridgeValidators) ToInternal() (*InternalBridgeValidators, error) {
return &ret, nil
}

// Bridge Validator but with validated EVMAddress.
// InternalBridgeValidator bridges Validator but with validated EVMAddress.
type InternalBridgeValidator struct {
Power uint64
EVMAddress common.Address
Expand Down Expand Up @@ -112,11 +114,11 @@ 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{}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

// 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?
Copy link
Collaborator Author

@staheri14 staheri14 Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

@evan-forbes evan-forbes Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

// subtract c powers from powers in the map, initializing
Expand All @@ -135,7 +137,11 @@ func (ibv InternalBridgeValidators) PowerDiff(c InternalBridgeValidators) float6
delta += math.Abs(float64(v))
}

return math.Abs(delta / float64(math.MaxUint32))
decDelta := sdk.NewDec(int64(delta))
staheri14 marked this conversation as resolved.
Show resolved Hide resolved
decMaxUint32 := sdk.NewDec(math.MaxUint32)
q := decDelta.Quo(decMaxUint32)

return q.Abs()
}

// TotalPower returns the total power in the bridge validator set.
Expand Down