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

Reapply changes from reverted PR 3423: move structures to new modules #4427

Merged
merged 3 commits into from
Feb 3, 2025

Conversation

KirillLykov
Copy link

@KirillLykov KirillLykov commented Jan 13, 2025

Problem

This PR moves structures SendTransactionServiceStatsand TransactionClient (introduced recently) to their own modules: these changes are split into two commits for simplicity of the review. There are no changes in the logic, just moves code to new modules.

Summary of Changes

@KirillLykov KirillLykov force-pushed the klykov/reapply-3423-2 branch from ad01f1c to c7ffd36 Compare January 13, 2025 13:35
@KirillLykov KirillLykov force-pushed the klykov/reapply-3423-2 branch from c7ffd36 to 780be19 Compare January 28, 2025 20:58
@KirillLykov KirillLykov requested review from bw-solana and removed request for bw-solana January 28, 2025 20:59
@KirillLykov KirillLykov force-pushed the klykov/reapply-3423-2 branch 2 times, most recently from 6d16879 to b0948e1 Compare January 29, 2025 15:52
@@ -1,5 +1,11 @@
#[deprecated(
since = "2.1.12",
Copy link
Author

Choose a reason for hiding this comment

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

Not sure about the version here in case of backporting

@t-nelson
Copy link

are the changes that are intended to be backported all already in? ideally we do not take a bunch of refactor/reorg/cosmetics through backport. just logic changes

@KirillLykov
Copy link
Author

KirillLykov commented Jan 30, 2025

are the changes that are intended to be backported all already in? ideally we do not take a bunch of refactor/reorg/cosmetics through backport. just logic changes

No, in order for the validator to be able to use tpu-client-next, I need to add the following:

  1. add new client code to the SendTransactionService (currently it has a client implemented using only ConnectionCache)
  2. Plumbing on the rpc crate side to be able to select one of the clients
  3. Plumbing on the validatator.rs to select the client and pass it to RPC, RPC will pass to SendTransactionService.

So although these changes do not change much of the logic, they are big code-wise.

Moving data structures to the new place might be postponed and done after (3), bu they are quite risk-free changes.

alessandrod
alessandrod previously approved these changes Feb 3, 2025
@KirillLykov
Copy link
Author

KirillLykov commented Feb 3, 2025

I think it is reasonable now to retreat on backporting to 2.1 (because there are a lot of changes), so move these structures around

@KirillLykov KirillLykov force-pushed the klykov/reapply-3423-2 branch from 7101ea0 to 8c4eda7 Compare February 3, 2025 12:23
alessandrod
alessandrod previously approved these changes Feb 3, 2025
@KirillLykov KirillLykov force-pushed the klykov/reapply-3423-2 branch from 8c4eda7 to f1fb95b Compare February 3, 2025 12:50
@KirillLykov KirillLykov added the automerge automerge Merge this Pull Request automatically once CI passes label Feb 3, 2025
Copy link

@alexpyattaev alexpyattaev left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit bc9218c into anza-xyz:master Feb 3, 2025
48 checks passed
@t-nelson
Copy link

t-nelson commented Feb 4, 2025

nothing after this is intended for backport, right? we aren't backporting reorg/refactor changes

@KirillLykov
Copy link
Author

nothing after this is intended for backport, right? we aren't backporting reorg/refactor changes

correct, no backporting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants