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

refactor(staking): 🌹 hoist uptime tracking into ValidatorUptimeTracker extension trait #4099

Conversation

cratelyn
Copy link
Contributor

this is a small collection of noop refactors, cherry-picked out of #4070.

most importantly, this hoists track_uptime into a standalone extension trait. additionally, some improvements to telemetry helped make debugging tests in #4070 easier.

first, we put this in a standalone function. some type aliases help us
do this so that the signature here is not too unruly.

some comments are added to highlight the intent to perform these lookups
in parallel.
we do so below this loop. let's do that here as well.
> This is used to model causal relationships such as when a single
> future spawns several related background tasks, et cetera.

\- https://docs.rs/tracing/latest/tracing/span/struct.Span.html#method.follows_from

NB: this commit is best viewed using `-w` to ignore whitespace changes.
commit information identifies validators by a truncated sha256 hash of
their consensus key.

we perform this operation in a couple disparate places. this
consolidates things so that we have a single `validator_address`
function to do this.
like the validator information lookup before, we outline another phase
of tracking uptime into a standalone function.
log votes received for the last block. used while debugging test cases.

re: perf, i was mildly curious and dug up:
tokio-rs/tracing#722 (comment)

..which indicates that tracing won't calculate attributes unless a
subscriber is interested in an event. i'm happy to hear that this is the
case!
@cratelyn cratelyn added A-staking Area: Design and implementation of staking and delegation A-mock-consensus Area: Relates to the mock consensus engine labels Mar 25, 2024
@cratelyn cratelyn added this to the Sprint 3 milestone Mar 25, 2024
@cratelyn cratelyn self-assigned this Mar 25, 2024
@cratelyn cratelyn marked this pull request as ready for review March 25, 2024 21:31
@cratelyn cratelyn changed the title refactor(staking): 🌹 minor changes to uptime tracker refactor(staking): 🌹 hoist uptime tracking into ValidatorUptimeTracker extension trait Mar 25, 2024
@cratelyn
Copy link
Contributor Author

merging eagerly to clear the way for #4070, if that's alright!

@cratelyn cratelyn merged commit c8094a8 into main Mar 25, 2024
7 checks passed
@cratelyn cratelyn deleted the kate/staking-refactors-cherry-picked-from-validator-uptime-signatures branch March 25, 2024 21:34
cratelyn added a commit that referenced this pull request Mar 26, 2024
**NB:** this is based on #4099.

this adds test coverage, complementary to the work in #4061, which
asserts that we properly track the _affirmative_ case of validators
signing blocks.

fixes #4040. see #3995.
Copy link
Contributor Author

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

i've opened #4102 to address these comments, thanks for taking a look @erwanor!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mock-consensus Area: Relates to the mock consensus engine A-staking Area: Design and implementation of staking and delegation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants