epoching: wrapper type `Epoch` and simplify code using this type #54

SebastianElvis · 2022-07-08T12:11:17Z

This PR

introduces a wrapped type for Epoch with a number of interfaces,
simplifies the existing code by using these interfaces,
removes many previous interfaces in the epoching keeper
replaces sdk.Uint with uint64 for the epoch number

In addition, this PR also adds a new function GetValidatorSetSorted that returns a slice of validator addresses, ordered by their voting power from big to small.

I also marked @gitferry as a reviewer since 1) this PR made some modifications concerning epoch operations in the checkpointing module, and 2) this PR introduces GetValidatorSetSorted, which will be used by the checkpointing module.

…-beginblock

aakoshh · 2022-07-08T15:16:42Z

proto/babylon/epoching/v1/epoching.proto

+message Epoch {
+ uint64 epoch_number = 1;
+ uint64 epoch_interval = 2;
+}


I'm not sure we should publish this type in the API, looks like epoch_interval could change at some point. Maybe epoch_length, so it reflects that it's the size of this epoch, and not necessarily a global cadence.

Would it be challenging for us to remember the difference between epoch_interval and epoch_length? How about we rename epoch_interval to current_epoch_interval, or add some comments saying that epoch_interval here means the current epoch interval and not necessarily the value in the future?

After a second thought, it seems that changing epoch interval on-the-fly affects many things more than this API. For example, when a validator synchronises the entire chain from scratch, it needs to traverse the entire chain to know its current epoch status, including the epoch number, first/last block of this epoch, etc..

So if we want variable epoch interval, then this message needs to include at least:

epoch number

current epoch interval

first or last block height

To keep things less complex, we may also enforce that changing the epoch interval can only happen at an epoch boundary and can happen infrequently under certain scenarios.

Well, even if the epoch interval can be changed in the Params during an epoch, it would not take effect until the next epoch.

Would it be challenging for us to remember the difference between epoch_interval and epoch_length?

If we think something needs remembering to be interpreted correctly, isn't that a code smell?
You are right that to support variable intervals it should contain the block height(s)!

What I'm suggesting is that we pick a representation that is unambiguous and doesn't need to change, and also easy to interpret:

epoch number

first block height

epoch length

(last block height = first block height + epoch length - 1) I guess that can be just documented

That seems like no matter what happens will capture the truth, whereas the "number + interval" requires the client to repeat the modulo calculation, and then we can't change it without breaking them.

The goal is to keep this logic in one place, calculate a values when the epoch starts, so that it can be disseminated as is to downstream systems.

What I'm suggesting is that we pick a representation that is unambiguous and doesn't need to change, and also easy to interpret:

epoch number

first block height

epoch length

(last block height = first block height + epoch length - 1) I guess that can be just documented

Totally agree with this. The current implementation has two differences compared to this version: 1) epoch length is called CurrentEpochInterval, and 2) LastBlockHeight is calculated via a function rather than being a field. Does this look fine to you?

Yes, thanks, it looks to be just a naming difference 👍 I also put last block height in parentheses because I didn't think it necessarily has to be a field.

aakoshh · 2022-07-08T15:18:12Z

x/checkpointing/abci.go

 // note that this epochNum is obtained before the BeginBlocker of the epoching module is executed
 // meaning that the epochNum has not been incremented upon a new epoch
- epochNum := k.GetEpochNumber(ctx)
+ epochNum := k.GetEpoch(ctx).EpochNumber


Suggested change

epochNum := k.GetEpoch(ctx).EpochNumber

epochNum := epoch.EpochNumber

aakoshh · 2022-07-08T15:19:27Z

x/checkpointing/keeper/keeper.go

+func (k Keeper) GetEpochBoundary(ctx sdk.Context) uint64 {
+ return k.epochingKeeper.GetEpoch(ctx).LastBlockHeight()
 }


Honestly we could just get rid of the GetEpochBoundary altogether, to get rid of all those comments everywhere. The call site should use the Epoch interface instead.

aakoshh · 2022-07-08T15:22:10Z

x/epoching/keeper/epoch_val_set.go

@@ -26,10 +31,39 @@ func (k Keeper) GetValidatorSet(ctx sdk.Context, epochNumber sdk.Uint) map[strin
 return valSet
 }

+// GetValidatorSetSorted returns the set of validators of a given epoch, where the validators are ordered by their


Doesn't say by what.

It also would be better to say it's descending order in the doc.

aakoshh · 2022-07-08T15:23:36Z

x/epoching/keeper/epoch_val_set.go

+ ss = append(ss, kv{k, v})
+ }
+ sort.Slice(ss, func(i, j int) bool {
+ return ss[i].Value > ss[j].Value


Suggested change

return ss[i].Value > ss[j].Value

return ss[i].Value > ss[j].Value || ss[i].Value == ss[j].Value && ss[i].Key > ss[j].Key

Just to make sure we specified the total order.

Unless you want to order by keys which are unique.

Would it be better to implement Golang's sort interface? See here

aakoshh

Great! A type for EpochNumber would be nice but the tests look nicer without the sdk.Int(0) for sure.

gitferry

Nice work. Thank you for introducing the Epoch interface and doing all the replacements. Only left some minor comments.

gitferry · 2022-07-09T01:55:40Z

x/epoching/keeper/epoch_slashed_val_set.go

 store := k.slashedVotingPowerStore(ctx)

 // key: epochNumber
- epochNumberBytes, err := epochNumber.Marshal()
+ epochNumberBytes, err := sdk.NewUint(epochNumber).Marshal()


Would it be better to use sdk.Uint64ToBigEndian(epochNumber)?

It seems to have little difference. In fact I also don't understand the docstring comment of Uint64ToBigEndian, which says that this function aims at sorting uint64 values -- can't we sort uint64 values directly? 😓

I guess the purpose of Uint64ToBigEndian is to allow the converted bytes to be also sortable. For Uint64ToBigEndian(1) -> X, Uint64ToBigEndian(2) -> Y, and X < Y still stands. It's helpful in a case where we don't need to unmarshal the raw bytes to compare two numbers.

Another reason I recommend using Uint64ToBigEndian and BigEndianToUint64 is that it is used across other modules, so it would be nice for us to keep consistent such that if there would be some changes in the future it's easy to replace them all. See here.

gitferry · 2022-07-09T01:59:13Z

x/epoching/keeper/epoch_val_set.go

@@ -26,10 +31,39 @@ func (k Keeper) GetValidatorSet(ctx sdk.Context, epochNumber sdk.Uint) map[strin
 return valSet
 }

+// GetValidatorSetSorted returns the set of validators of a given epoch, where the validators are ordered by their


It also would be better to say it's descending order in the doc.

gitferry · 2022-07-09T02:12:28Z

x/epoching/keeper/epoch_val_set.go

+ ss = append(ss, kv{k, v})
+ }
+ sort.Slice(ss, func(i, j int) bool {
+ return ss[i].Value > ss[j].Value


Would it be better to implement Golang's sort interface? See here

gitferry · 2022-07-09T02:43:24Z

x/epoching/keeper/epoch_val_set.go

 "github.com/babylonchain/babylon/x/epoching/types"
 "github.com/cosmos/cosmos-sdk/store/prefix"
 sdk "github.com/cosmos/cosmos-sdk/types"
 sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 )

 // GetValidatorSet returns the set of validators of a given epoch
-func (k Keeper) GetValidatorSet(ctx sdk.Context, epochNumber sdk.Uint) map[string]int64 {
+func (k Keeper) GetValidatorSet(ctx sdk.Context, epochNumber uint64) map[string]int64 {


To get a sorted array of validators, the Checkpointing module needs to call GetValidatorSet and then GetValidatorSetSorted. And GetValidatorSetSorted also calls GetValidatorSet. I feel like it's a bit redundant.

I would create a struct like ValidatorInfo which contains address and voting power. For example,

type ValidatorInfo struct { addr sdk.ValAddress power int64 }

Such that we can combine the two interfaces to only one GetValidatorsByEpoch(ctx sdk.Context, epochNumber uint64) []*ValidatorInfo

Can't the checkpointing module just call GetValidatorSetSorted without GetValidatorSet? Basically GetValidatorSetSorted requires the same inputs as GetValidatorSet and provides different outputs instead.

But the ValidatorInfo here looks like a good idea to me. The biggest advantage of this to me is that we don't need to convert sdk.ValAddress to string anymore. Will do!

If the checkpointing module only calls GetValidatorSetSorted, what's the point of keeping GetValidatorSet? Voting power is also needed for BLS sig aggregation.

Yeah GetValidatorSet was originally used in the epoching module. I did revisit the usage of GetValidatorSet again and found that it can be replaced by something else. Since chpt module uses the voting power as well, I will then merge them together and follow the ValidatorInfo approach. Thanks for the comments!

SebastianElvis · 2022-07-11T04:51:29Z

Thanks for the great comments Akosh and Gai! I have fixed all the comments, and feel free to review again.

Regarding type Epoch under variable epoch interval in the future, I have added a new field FirstBlockHeight to Epoch, and added a strawman function to calculate FirstBlockHeight. Once we plan to support variable epoch interval, we can modify this function to do so.

gitferry

Thank you for the changes. Looking good to me 👏. Just left some minor comments about naming.

gitferry · 2022-07-11T07:39:06Z

go.sum

@@ -450,6 +450,7 @@ github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5m
 github.com/googleapis/gax-go/v2 v2.1.0/go.mod h1:Q3nei7sK6ybPYH7twZdmQpAd1MKb7pfu6SK+H1/DsU0=
 github.com/googleapis/gax-go/v2 v2.1.1/go.mod h1:hddJymUZASv3XPyGkUpKj8pPO47Rmb0eJc8R6ouapiM=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
+github.com/gordonklaus/ineffassign v0.0.0-20200309095847-7953dde2c7bf/go.mod h1:cuNKsD1zp2v6XfE/orVX2QE1LC+i254ceGcVeDT3pTU=


Maybe you could run go mod tidy to get rid of this?

gitferry · 2022-07-11T07:57:25Z

x/epoching/keeper/epoch_val_set.go

 }
+ sort.Sort(types.Validators(vals))


Why not declare a types.Validators in the first place? And we can implement an Add() method to append validators.

Another minor thing is that maybe we can rename Validators to ValidatorSet? Just because it's not easy to distinguish Validator vs. Validators with bare eyes.

aakoshh · 2022-07-11T09:20:59Z

x/epoching/keeper/epoch_slashed_val_set.go

+ // we don't panic here since it's possible that the most powerful validator outside the validator set enrols to the validator after this validator is slashed.
+ return err


This error will not stop block processing, right?

No it won't stop block processing. It is used in BeforeValidatorSlashed hook. Upon this error the hook will consider this validator to have no voting power and return directly, as per our previous discussion.

aakoshh

Looks dandy!

SebastianElvis added 30 commits June 24, 2022 15:05

init

951dcf3

abci interface

af5ff8d

module stuff

e12dd3f

slash without jail

d6333c1

minor

6cd242a

minor

d997725

apply mature unbonding fix

82eb108

avoid assertions

d6d5996

a lot

a91c059

fix order of beginblockers and endblockers

6bc1fb3

Merge branch 'main' into epoching-staking-code-migration

729da0b

don't touch evidence/slashing at the moment

3d5f5a1

fix and make required staking functions a part of epoching

a6aa599

minor

34caaa5

add dependency to evidence and slashing

97c36cd

Merge branch 'epoching-staking-code-migration' into epoching-endblock…

8818a67

…-beginblock

minor

1b8de2f

minor

3273562

minor

bf09eb8

minor

39b7d61

Merge branch 'epoching-staking-code-migration' into epoching-endblock…

a06bbc1

…-beginblock

msgserver

602ffee

Merge branch 'main' into epoching-endblock-beginblock

a766236

order

8a6a88c

epoching hooks to staking

94e91e6

BeforeValidatorSlashed hook in epoching

fe44d45

BeforeValidatorSlashed hook

be19c4f

minor

12cfbf6

Merge branch 'main' into epoching-endblock-beginblock

8b014f3

fix

6e7872a

SebastianElvis changed the title ~~epoching: wrapper type Epoch~~ epoching: wrapper type Epoch and simplify code using this type Jul 8, 2022

fix

509c09e

Base automatically changed from epoching-slash-panicing to main July 8, 2022 13:30

merge

abcdcaa

SebastianElvis marked this pull request as ready for review July 8, 2022 14:00

SebastianElvis requested review from gitferry and aakoshh July 8, 2022 14:00

aakoshh reviewed Jul 8, 2022

View reviewed changes

gitferry reviewed Jul 9, 2022

View reviewed changes

SebastianElvis added 3 commits July 11, 2022 12:10

fix

522d7f4

replace sdk.Uint with uint64

c8c60b0

fix EpochInterface

4ae0a62

SebastianElvis requested review from aakoshh and gitferry July 11, 2022 04:51

SebastianElvis added 3 commits July 11, 2022 14:52

Merge branch 'main' into epoching-epoch-struct

0565c96

getter of epoch type to keep functions consistent

2cea378

fuzzepoch

82dd3ae

gitferry approved these changes Jul 11, 2022

View reviewed changes

aakoshh reviewed Jul 11, 2022

View reviewed changes

aakoshh approved these changes Jul 11, 2022

View reviewed changes

SebastianElvis merged commit 5896e7c into main Jul 11, 2022

SebastianElvis deleted the epoching-epoch-struct branch July 11, 2022 09:43

KonradStaniec pushed a commit that referenced this pull request Oct 25, 2023

btcstaking: API for querying voting power at current height (#54)

19d0fb7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

epoching: wrapper type `Epoch` and simplify code using this type #54

epoching: wrapper type `Epoch` and simplify code using this type #54

SebastianElvis commented Jul 8, 2022 •

edited

Loading

aakoshh Jul 8, 2022

SebastianElvis Jul 11, 2022

SebastianElvis Jul 11, 2022

aakoshh Jul 11, 2022

SebastianElvis Jul 11, 2022

aakoshh Jul 11, 2022

aakoshh Jul 8, 2022

aakoshh Jul 8, 2022

aakoshh Jul 8, 2022

gitferry Jul 9, 2022 •

edited

Loading

aakoshh Jul 8, 2022

gitferry Jul 9, 2022

aakoshh left a comment

gitferry left a comment

gitferry Jul 9, 2022

SebastianElvis Jul 11, 2022 •

edited

Loading

gitferry Jul 11, 2022 •

edited

Loading

gitferry Jul 9, 2022 •

edited

Loading

gitferry Jul 9, 2022

gitferry Jul 9, 2022 •

edited

Loading

SebastianElvis Jul 11, 2022

SebastianElvis Jul 11, 2022

gitferry Jul 11, 2022

SebastianElvis Jul 11, 2022

SebastianElvis commented Jul 11, 2022

gitferry left a comment

gitferry Jul 11, 2022

gitferry Jul 11, 2022

aakoshh Jul 11, 2022

SebastianElvis Jul 11, 2022

aakoshh left a comment

	epochNum := k.GetEpoch(ctx).EpochNumber
	epochNum := epoch.EpochNumber

	return ss[i].Value > ss[j].Value
	return ss[i].Value > ss[j].Value \|\| ss[i].Value == ss[j].Value && ss[i].Key > ss[j].Key

		// we don't panic here since it's possible that the most powerful validator outside the validator set enrols to the validator after this validator is slashed.
		return err

epoching: wrapper type Epoch and simplify code using this type #54

epoching: wrapper type Epoch and simplify code using this type #54

Conversation

SebastianElvis commented Jul 8, 2022 • 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

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

Choose a reason for hiding this comment

gitferry Jul 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh left a comment

Choose a reason for hiding this comment

gitferry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

gitferry Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

gitferry Jul 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry Jul 9, 2022 • 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

Choose a reason for hiding this comment

SebastianElvis commented Jul 11, 2022

gitferry left a comment

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

Choose a reason for hiding this comment

aakoshh left a comment

Choose a reason for hiding this comment

epoching: wrapper type `Epoch` and simplify code using this type #54

epoching: wrapper type `Epoch` and simplify code using this type #54

SebastianElvis commented Jul 8, 2022 •

edited

Loading

gitferry Jul 9, 2022 •

edited

Loading

SebastianElvis Jul 11, 2022 •

edited

Loading

gitferry Jul 11, 2022 •

edited

Loading

gitferry Jul 9, 2022 •

edited

Loading

gitferry Jul 9, 2022 •

edited

Loading