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

Add recent uptimes to validator metadata in the client api #800

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jberci
Copy link
Contributor

@jberci jberci commented Nov 18, 2024

No description provided.

Copy link
Member

@ptrus ptrus left a comment

Choose a reason for hiding this comment

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

👍 Just a couple of minor comments.

window_length:
type: integer
format: uint64
description: The length of the window this object describes, in blocks.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention that this is currently always 14400 (~last 24 hours).

partial_length:
type: integer
format: uint64
description: The length of the partial windows within window_length, in blocks.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention that this will currently always be 1200 (~2 hours).

@@ -316,6 +318,10 @@ func (m *processor) queueDbUpdates(batch *storage.QueryBatch, data allData) erro
}
}

if m.mode == analyzer.SlowSyncMode {
batch.Queue(validatorUptimesViewRefreshQuery)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to run this after every processed block. Since the "partial windows" are 1200 blocks in size, refreshing every ~12 blocks (1% of the window size) or ~60 blocks (5% of the window size) should be reasonable. Any change is unlikely to be noticeable before that.

It might be clearer to run this periodically, such as every minute or so, with the interval being configurable.

@@ -44,6 +44,12 @@ const (
maxTotalCount = 1000
)

var (
// These two should be kept the same as in the views.validator_uptimes materialized view.
Copy link
Member

Choose a reason for hiding this comment

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

These two can be made constants instead of variables.

Copy link
Collaborator

@Andrew7234 Andrew7234 left a comment

Choose a reason for hiding this comment

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

Thank you! Nice job with the sql-fu to calculate the partial windows, left some comments as well. Lmk if you have any questions!

Comment on lines +19 to +20
UNNEST(signer_entity_ids) AS signer_entity_id,
(ROW_NUMBER() OVER (ORDER BY height DESC) - 1) / 1200 AS slot_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you double check that the unnest does not cause unexpected results with row_number here? I'm wondering if each (signer x height) will have its own row which would then cause row_number to vastly surpass 14400 and thus overflow the slot_id.

SELECT * FROM views.validator_uptimes
WHERE ($1::text IS NULL OR signer_entity_id = $1::text)
ORDER BY signer_entity_id`
/*ValidatorUptimes = `
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove

@@ -433,6 +433,58 @@ const (
LIMIT $4::bigint
OFFSET $5::bigint`

ValidatorUptimes = `
SELECT * FROM views.validator_uptimes
WHERE ($1::text IS NULL OR signer_entity_id = $1::text)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the input param $1 is the staking address, can add a join with chain.entities.address to filter by address here. Alternatively, it might be cleaner to just include the entity address in the materialized view to avoid the join.

Comment on lines +37 to +44
signers.signer_entity_id,
ARRAY_AGG(COALESCE(slot_counts.signed_blocks_count, 0) ORDER BY slot_counts.slot_id) AS slot_signed,
COALESCE(SUM(signed_blocks_count), 0) AS overall_signed
FROM
-- Ensure we have all windows for each signer, even if they didn't sign in a particular window.
(SELECT DISTINCT signer_entity_id FROM slot_counts) AS signers
CROSS JOIN all_slots
LEFT JOIN slot_counts ON signers.signer_entity_id = slot_counts.signer_entity_id AND all_slots.slot_id = slot_counts.slot_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

Neat sql-fu! Seems like the e2e tests might not be suited to testing this properly; mind validating it locally by running a local nexus instance and printing the table / querying the api? Thanks!

Comment on lines +49 to +50
uptimeWindowBlocks = uint64(14400)
uptimeSlotBlocks = uint64(1200)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm wdyt of hardcoding these into the postgres query instead of here? It'd be nice to colocate them together so that if it changes, we only need to make changes in one place.

Comment on lines +2101 to +2104
partial_length:
type: integer
format: uint64
description: The length of the partial windows within window_length, in blocks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
partial_length:
type: integer
format: uint64
description: The length of the partial windows within window_length, in blocks.
segment_length:
type: integer
format: uint64
description: The length of the window segment, in blocks. We subdivide the window into segments of equal length and aggregate the uptime of each segment into `segment_uptimes`. Currently defaults to 1200 blocks, which is approximately 2 hours.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not very enthusiastic about segment, I considered using chunk or subwindow too. Feel free to change

window_length:
type: integer
format: uint64
description: The length of the window this object describes, in blocks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: The length of the window this object describes, in blocks.
description: The length of the historical window for which this object provides uptime information, in blocks. Currently defaults to 14400 blocks, or approximately 24 hours.

Comment on lines +2109 to +2111
partial_uptimes:
type: array
description: An array showing the signed block counts for each partial slot within window_length.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
partial_uptimes:
type: array
description: An array showing the signed block counts for each partial slot within window_length.
segment_uptimes:
type: array
description: An array showing the signed block counts for each sub-segment within window_length. The segments are in reverse-chronological order; ie the first element represents the most recent segment of blocks.

@@ -316,6 +318,10 @@ func (m *processor) queueDbUpdates(batch *storage.QueryBatch, data allData) erro
}
}

if m.mode == analyzer.SlowSyncMode {
batch.Queue(validatorUptimesViewRefreshQuery)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's refresh this in a new ItemBasedAnalyzer, see accounts_list.go for an example. Alternatively, feel free to refactor that analyzer to refresh both views; I think we'll soon have more views that need refreshing, and it'd be good to have them all in one spot so we can tweak how often they get recalculated.

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