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

Search for online accounts #1646

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

agodnic
Copy link
Contributor

@agodnic agodnic commented Feb 1, 2025

Summary

(This pull request is part of the features described in AlgoNode#4. The aforementioned features will be presented in separate, smaller pull requests to make it easier to accept/reject/discuss each one individually)

This pull request adds the online-only parameter to GET /v2/accounts, which lets the user search for accounts whose participation status is "online".

Test Plan

Tested manually in a local environment, e.g.:

$ curl -Ss 'http://127.0.0.1:8980/v2/accounts?limit=10&online-only=true&exclude=all' | jq '.accounts[] .status'
"Online"
"Online"
"Online"
"Online"
"Online"
"Online"
"Online"
"Online"
"Online"
"Online"

Performance

The performance of is-online=true lookups can be improved with the following index:

-- create the index
ledgerdb=# CREATE INDEX account_idx_online_only
        ON account ((((account_data ->> 'onl')) = '1'), addr)
        WHERE (NOT deleted) AND ((account_data ->> 'onl') IS NOT NULL) AND ((account_data ->> 'voteLst') IS NOT NULL)
;
CREATE INDEX

-- Verify the index is being used by the query optimizer.
-- This query corresponds to /v2/accounts?limit=10&online-only=true&exclude=all
ledgerdb=# explain (analyze) WITH qaccounts AS (SELECT a.addr, a.microalgos, a.rewards_total, a.created_at, a.closed_at, a.deleted, a.rewardsbase, a.keytype, a.account_data FROM account a WHERE NOT a.deleted AND (a.account_data->>'onl' IS NOT NULL) and (account_data->>'voteLst' IS NOT NULL) and ((a.account_data->>'onl') = '1') ORDER BY a.addr ASC LIMIT 10) SELECT za.addr, za.microalgos, za.rewards_total, za.created_at, za.closed_at, za.deleted, za.rewardsbase, za.keytype, za.account_data FROM qaccounts za ORDER BY za.addr ASC;
                                                                      QUERY PLAN                                                                      
------------------------------------------------------------------------------------------------------------------------------------------------------
 Limit  (cost=0.28..37.70 rows=10 width=103) (actual time=0.023..0.038 rows=10 loops=1)
   ->  Index Scan using account_idx_online_only on account a  (cost=0.28..407632.13 rows=108930 width=103) (actual time=0.023..0.036 rows=10 loops=1)
         Index Cond: (((account_data ->> 'onl'::text) = '1'::text) = true)
 Planning Time: 0.268 ms
 Execution Time: 0.049 ms
(5 rows)

@agodnic
Copy link
Contributor Author

agodnic commented Feb 1, 2025

Discussion

  1. If we want to treat suspended accounts differently from other online accounts, my understanding is that we'd need to somehow persist that information in the database (probably within the account table).
    It could be a new integer nullable field: account.account_data->suspended_until

  2. A follow-up question would be whether we want to add a parameter in GET /v2/accounts to search for suspended accounts.

@jannotti
Copy link
Contributor

jannotti commented Feb 1, 2025

A suspended account has status = 0 (offline) but has non-zero key material (SelectionID, VoterID, etc).

@agodnic agodnic marked this pull request as ready for review February 1, 2025 21:48
@agodnic agodnic marked this pull request as draft February 4, 2025 21:32
Fix an issue in the underlying SQL query for the `online-only`
parameter.

The previous SQL query was returning online accounts with null key
material. This commit filters out those accounts - only accounts
with non-null key material are returned.
@agodnic agodnic marked this pull request as ready for review February 5, 2025 00:26
@gmalouf
Copy link
Contributor

gmalouf commented Feb 5, 2025

@agodnic thank you for the PR, a couple thoughts:

  • There could be performance impacts for API services on the new parameter, perhaps we should support enabling/disabling it as we do with other params: Interpreting the Configuration. There are pre-existing disabled and enabled parameters tests to help exercise this change as well.
  • Covering the functionality with an additional test case in handlers_e2e_test.go would be ideal.

@agodnic agodnic marked this pull request as draft February 6, 2025 00:35
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