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

tpu-client-next: return receiver in scheduler::run #4454

Merged

Conversation

KirillLykov
Copy link

Problem & Solution

Return receiver from scheduler run so that it can be reused if required. This is needed to use in validator because user can make an rpc call to update underlying authority, and due to the way this mechanism is implemented, we have to reutilize the same receiver.

These changes are part of the reverted PR https://github.com/anza-xyz/agave/pull/3515/files#diff-17507818c3be9c444eb55f6c0b2d82ac0662746abf0dff6b200d43bf9ca125c2

They are split into a separate PR for the sake of backporting and to have a clearer history.

@KirillLykov KirillLykov force-pushed the klykov/tpu-client-next-return-receiver branch from 97ed5c2 to ad3fb31 Compare January 14, 2025 09:45
@KirillLykov KirillLykov changed the title Return receiver in scheduler::run tpu-client-next: return receiver in scheduler::run Jan 14, 2025
pub fn new(keypair: &Keypair) -> Self {
let (certificate, key) = new_dummy_x509_certificate(keypair);
Self { certificate, key }
pub fn new(keypair: Option<&Keypair>) -> Self {
Copy link
Author

Choose a reason for hiding this comment

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

As said in the PR description, not a new change. Adding back reverted PR.

Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. Left a few minor suggestions.

It seems like there are multiple changes here that could be broken out into separate, smaller PRs. E.g. the name change, keypair management, and returning receiver

Self { certificate, key }
} else {
let (certificate, key) = new_dummy_x509_certificate(&Keypair::new());
Self { certificate, key }

Choose a reason for hiding this comment

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

could condense this using unwrap_or_else:

let keypair = keypair.unwrap_or_else(|| &Keypair::new());
let (certificate, key) = new_dummy_x509_certificate(keypair);
Self { certificate, key }

Copy link
Author

Choose a reason for hiding this comment

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

unwrap_or_else(|| &Keypair::new()) won’t work because the new Keypair instance would not live long enough for the reference to remain valid. The only thing I can do is

let keypair = if let Some(keypair) = keypair {
            keypair // Use the provided keypair
        } else {
            &Keypair::new() // Create a new keypair if None
        };
        let (certificate, key) = new_dummy_x509_certificate(keypair);

This way avoiding two calls of new_dummy_x509_certificate

tpu-client-next/src/connection_workers_scheduler.rs Outdated Show resolved Hide resolved
@KirillLykov KirillLykov force-pushed the klykov/tpu-client-next-return-receiver branch from ad3fb31 to acd49ec Compare January 15, 2025 14:33
@KirillLykov KirillLykov added the v2.1 Backport to v2.1 branch label Jan 15, 2025
@KirillLykov KirillLykov requested a review from bw-solana January 15, 2025 15:02
Copy link

mergify bot commented Jan 15, 2025

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

@KirillLykov KirillLykov force-pushed the klykov/tpu-client-next-return-receiver branch from acd49ec to 1db2845 Compare January 15, 2025 21:05
@KirillLykov
Copy link
Author

@bw-solana I've renamed identity/validator_identity to be always stake_identity (also in tests). So now this naming is consistent.

@KirillLykov KirillLykov force-pushed the klykov/tpu-client-next-return-receiver branch from 1db2845 to 2865da8 Compare January 16, 2025 08:23
@KirillLykov KirillLykov merged commit 85b6118 into anza-xyz:master Jan 17, 2025
30 checks passed
mergify bot pushed a commit that referenced this pull request Jan 17, 2025
Return receiver from scheduler::run so that it can be reused if required. This is needed to use tpu-client-next in validator because user can make an rpc call to update underlying authority, and due to the way this mechanism is implemented, we have to re-utilize the same receiver.

(cherry picked from commit 85b6118)
KirillLykov added a commit that referenced this pull request Jan 24, 2025
…4454) (#4521)

tpu-client-next: return receiver in scheduler::run (#4454)

Return receiver from scheduler::run so that it can be reused if required. This is needed to use tpu-client-next in validator because user can make an rpc call to update underlying authority, and due to the way this mechanism is implemented, we have to re-utilize the same receiver.

(cherry picked from commit 85b6118)

Co-authored-by: kirill lykov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.1 Backport to v2.1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants