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

feat: calculate the stats via stats queue #76

Merged
merged 2 commits into from
May 3, 2024

Conversation

jrwbabylonlab
Copy link
Collaborator

  • Pending merge of this PR: feat: add stats queue staking-queue-client#14
  • This PR eliminates the logical database sharding for finality provider stats. Instead, we've redirected the stats calculation to its own dedicated queue. This abstraction separates it from the core state transition logic. The advantage of this approach is the implementation of a retryable delayed queue. If the stats calculation fails due to a database lock (e.g., trying to update the same record in a collection), it will be reprocessed several minutes later.
  • There is also a pending task to retry failed transactions, which will further enhance reliability. However, this can be addressed later: add retry for db transactions #63

@@ -19,6 +19,7 @@ db.unbonding_queue.createIndex({'unbonding_tx_hash_hex': 1}, {unique: true});
db.timelock_queue.createIndex({'expire_height': 1}, {unique: false});
db.delegations.createIndex({'staker_pk_hex': 1, 'staking_tx.start_height': -1}, {unique: false});
db.staker_stats.createIndex({'active_tvl': -1, '_id': 1}, {unique: false});
db.finality_providers_stats.createIndex({'active_tvl': -1, '_id': 1}, {unique: false});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@liam-icheng-lai or @filippos47 i will need your help on updating the DB indexes to include all of them from line 18 to 22. I think we already have the index for 18-20. but not for 21 and 22.

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

Looks good in general, but let's first reach consensus on the comment about the finality providers result.

internal/db/stats.go Outdated Show resolved Hide resolved
}

var finalityProviderDetailsPublic []FpDetailsPublic
for _, fp := range resultMap.Data {
Copy link
Member

Choose a reason for hiding this comment

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

Won't this skip finality providers that are in the global params, but do not have any delegations? For example, consider that the system is live, and our finality providers registry contains fp1, fp2, fp3, while fp1, fp2, and fp4 have received delegations. Will the output contain fp1, fp2, fp3, and fp4 or just fp1, fp2, fp4? I think we should have the first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it will be p1, p2 and p4.
The UI will be showing a paginated and sorted by active TVL FP list, right? This would make fp3 not a natural fit into the paginated response as its tvl will be 0. But what we can do is by checking if we have more result from db, if no more items, we can append the 0 tvl FP from the global params.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vitsalis i had a quick look into the implementation if we want to make it return fp1,2,3,4 all together and work with pagination. It's a little bit more work to do than simply append the global params FP into the end of the return list.
I would prefer track this conversation and the implementation in a seperate ticket so it at least unblock the load test etc.
For context, if we wanna return fp1,2,3,4. we need to do below:

  1. Check if we are at the end of the pagination. i.e no more items to query in db.
  2. make a new query with provided FP pk to db. if PKs not exist in the db, we append the FP into the returned list with tvl of 0.

Copy link
Member

Choose a reason for hiding this comment

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

It is a requirement to return fp1,2,3,4. Otherwise, when we launch the system, as soon as a single finality provider gets a delegation, all others won't appear. We have the pre-registration with the deposit for this reason -- you register so that you appear on the website and attract delegations. My recommendation is to do this in the current PR, as it is a core requirement of the functionality. If you think it is blocking, then we can do this in another PR, but it's a pre-requisite for testnet launch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

noted. yeah, will make sure this is fixed before testnet launch (should be fixed by next week). would prefer to close off all existing PRs first and have the staking cap get out the way first.
#88

@jrwbabylonlab jrwbabylonlab force-pushed the stats-calculation-through-stats-queue branch from 2065790 to ce43ce8 Compare May 3, 2024 01:55
@jrwbabylonlab jrwbabylonlab merged commit 75e964e into main May 3, 2024
2 checks passed
@jrwbabylonlab jrwbabylonlab deleted the stats-calculation-through-stats-queue branch May 3, 2024 02:08
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.

2 participants