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

chore(inactivity): Rename inactive flag to sluggish, add queries, events, and metrics #714

Merged
merged 18 commits into from
Jul 17, 2024

Conversation

gitferry
Copy link
Contributor

@gitferry gitferry commented Jul 16, 2024

The inactive flag of a finality provider is confused with the finality providers that are not in the active set. So, this PR is to rename the flag to sluggish, which is a common concept in the BFT world to describe validators who should vote but the votes are delayed for some reason thus committing sluggish faults.

This PR also made the following additions:

  • queries for signing info
  • events emitted for sluggish finaltiy providers being detected or reverted
  • metrics for counting the number of sluggish finality providers
  • docs about the new feature

// provider is no longer considered sluggish
message EventSluggishFinalityProviderReverted {
// public_key is the BTC public key of the finality provider
string public_key = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we also add the bbn address of the fp to the events

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I woudn't mind but what would be the use case?

@@ -225,3 +225,51 @@ func (k Keeper) ListEvidences(ctx context.Context, req *types.QueryListEvidences
}
return resp, nil
}

// SigningInfo returns signing-info of a specific finality provider.
func (k Keeper) SigningInfo(ctx context.Context, req *types.QuerySigningInfoRequest) (*types.QuerySigningInfoResponse, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for this and return a different structure than the one stored in the db?
related #537

It could be done in another PR also ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice find. Should've added tests for queries there. Done in 8be91d4

Copy link
Contributor

@RafilxTenfen RafilxTenfen left a comment

Choose a reason for hiding this comment

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

Sluggish seems funny to me kkk but I like the change 🎉
Some suggestions but no impediments

Copy link
Collaborator

@KonradStaniec KonradStaniec left a comment

Choose a reason for hiding this comment

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

great work! Sluggish sounds a bit weird, but I agree inactive may be super confusing so 👍

@gitferry
Copy link
Contributor Author

gitferry commented Jul 17, 2024

great work! Sluggish sounds a bit weird, but I agree inactive may be super confusing so 👍

Yeah, tbh it's werid to me, too, but it's just temporal and will be changed to jailed eventually

@gitferry gitferry merged commit 20e6740 into dev Jul 17, 2024
5 checks passed
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.

3 participants