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.

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

MAT-170 - Wiggle for heimdall tx #87

Closed
wants to merge 2 commits into from
Closed

Conversation

mankenavenkatesh
Copy link
Contributor

@mankenavenkatesh mankenavenkatesh commented Nov 8, 2019

Wiggle for heimdall tx.
Approach- https://hackmd.io/055mjdcEQUa0uzAqMPLdNg

@linear
Copy link

linear bot commented Nov 8, 2019

@@ -85,6 +87,32 @@ func isProposer(cliCtx cliContext.CLIContext) bool {
return false
}

// IsStateSyncer returns if current user is state syncer or not
func IsStateSyncer(cliCtx cliContext.CLIContext) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func IsStateSyncer(cliCtx cliContext.CLIContext) bool {
func IsStateSyncer(cliCtx cliContext.CLIContext) (isStateSyncer bool) {

// IsStateSyncer returns if current user is state syncer or not
func IsStateSyncer(cliCtx cliContext.CLIContext) bool {
var stateSyncerList []types.Validator
isStateSyncer := false
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this, because we are anyway implicitly returning false and true

Comment on lines +109 to +110
isStateSyncer = true
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Directly return true here?


// fetch state syncer list
res, err := cliCtx.QueryWithData(fmt.Sprintf("custom/%s/%s/%s", clerkTypes.QuerierRoute, clerkTypes.QueryStateSyncer), nil)

Copy link
Contributor

Choose a reason for hiding this comment

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

remove blank line

for _, record := range data.EventRecords {
keeper.SetEventRecord(ctx, *record)
}
}

keeper.SetStateSyncEventCount(ctx, uint64(eventCount))
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be the case where keeper.SetEventRecord(ctx, *record) will return an error but eventCount will not reflect that. Let's handle that error and/or ensure EventCount is always equal to actual events stored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Event Count is always equal to actual events stored.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yes, it has to be, but in this case in one case it might not be. The case where SetStateSyncEventCount throws an error.


// convert state sync event to bytes
eventCountInBytes := []byte(strconv.FormatUint(eventCount, 10))

Copy link
Contributor

Choose a reason for hiding this comment

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

remove line

expectedValIDs []types.ValidatorID
}

dataItems := []TestDataItem{
Copy link
Contributor

Choose a reason for hiding this comment

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

Table tests!!! 🎉

for i := 0; i < syncerCount; {
valIndex := eventCount % uint64(validatorCount)
validator, _ := stakingKeeper.GetValidatorFromValID(ctx, types.ValidatorID(valIndex+1))
if stakingKeeper.IsCurrentValidatorByAddress(ctx, validator.Signer.Bytes()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we get validators from current validator set instead of getting all validators and filter by validator status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. we can do that.
We should sort the current validator set by valID and do a binary search of validator Index.
Sorting is extra time complexity.

Considering CurrentValSet count is less, sorting should be fine i think.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mankenavenkatesh Validators in CurrentValSet will be already sorted as signer as far as I know. Can you check?

var stateSyncerList []types.Validator
isStateSyncer := false

result, err := FetchFromAPI(cliCtx, GetHeimdallServerEndpoint(fmt.Sprintf(StateSyncerURL)))
Copy link
Contributor

Choose a reason for hiding this comment

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

What if all three validators are offline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be the case if we go with backup model also.
Ideally, Instead of 3 validators We should increase the number to 1/3rd of currentvalidatorset count. It results in more number of transactions per sync.

Let me know if there is any better idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah 1/3rd of current validator set would be better here and in bor as backup.

@vaibhavchellani
Copy link
Contributor

@mankenavenkatesh Please resolve comments

@jdkanani jdkanani deleted the wiggle-state-sync branch March 16, 2020 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants