v2.1: Extract client code in send_transaction_service into a new structure (backport of #3423) #3480

mergify · 2024-11-05T12:07:52Z

Problem

In order to be able to use new client code in the SendTransactionService (see #3444), I need to wrap network-related code with the new structure ConnectionCacheClient.

Summary of Changes

To extract network code into a new structure, I had to move some auxiliary code into separate files.
Beside of that, changes are pretty minimal and I deliberately haven't tried to optimize existing code to keep changes minimal. It just does almost exactly the same as the old one.

This is an automatic backport of pull request #3423 done by [Mergify](https://mergify.com).

…3423) In order to be able to use new client code in the `SendTransactionService`, I need to wrap network-related code with the new structure ConnectionCacheClient. To to that I also had to move some auxiliary code into separate files. (cherry picked from commit 144925e)

mergify · 2024-11-05T12:08:26Z

If this PR represents a change to the public RPC API:

Make sure it includes a complementary update to rpc-client/ (example)
Open a follow-up PR to update the JavaScript client @solana/web3.js (example)

Thank you for keeping the RPC clients in sync with the server API @mergify[bot].

alessandrod · 2024-11-05T13:24:55Z

This is RPC only code, doesn't change anything in the validator and it's mostly moving existing code around.

I want to backport it because I did some changes to the quic server that give better throughput and lower resource load if the client generates a transaction stream that doesn't have high fragmentation. The FD quic server does the same, and is even stricter in that it only allows very minimal fragmentation.

This is a preparatory PR to ultimately make send-transaction-service optionally use the new quic-client-next crate to send transactions. quic-client-next doesn't create stream fragmentation. Again, RPC only, so zero risk for the validator.

KirillLykov · 2024-12-05T08:23:48Z

@bw-solana could you have a look?

bw-solana

The code itself looks fine to me.

The only hesitation I have is changing public items in published sts crate. The amount of changes for public items is surprisingly minimal, but non-zero.

Do we know who we might break with this?

KirillLykov · 2024-12-06T09:13:33Z

This service is quite internal thing (can be used only together with RPC), so I generally doubt anyone uses it.
@bw-solana the only public crate that depends on it is elipsis client (they depend on version < 1.19), but looking into their code I found that they don't really need to depend directly on it. So this is not a problem.

anza-team · 2024-12-06T09:54:54Z

😱 New commits were pushed while the automerge label was present.

jacobcreech · 2024-12-06T17:20:56Z

@bw-solana @alessandrod can we not just separate the crate and bump the major version? If this is making a breaking change with >2000 downloads a day, we cannot possibly alert everyone using it ahead of time.

bw-solana · 2024-12-06T21:37:45Z

@bw-solana @alessandrod can we not just separate the crate and bump the major version? If this is making a breaking change with >2000 downloads a day, we cannot possibly alert everyone using it ahead of time.

We've discussed this - haven't come to consensus yet. @willhickey captured things well here: #3724

t-nelson · 2024-12-06T21:58:56Z

i don't understand why the interfaces were changed in the same PR as breaks the types out to their own module. way too noisy even for master

KirillLykov · 2024-12-07T09:43:46Z

i don't understand why the interfaces were changed in the same PR as breaks the types out to their own module. way too noisy even for master

Changes in public API that I found:

SendTransactionService::new_with_config and new: removed reference from connection_cache: &Arc<ConnectionCache>. It is because it was cloned twice anyways.
pub(crate) struct SendTransactionServiceStatsReport -- added pub(crate), used to be pub
pub(crate) struct CurrentLeaderInfo<T> -- added pub(crate), used to be pub

What has been moved:

SendTransactionServiceStats to a separate module. The structure is unchanged, just moved from the send_transaction_service.rs to reduce the lenght of this file. I can do it in a separate commit to simplify the review.
Also moved CurrentLeaderInfo to a another module.

Would it help if I create a separate commit that would extract these structures from the original module? I can also revert for this backport these pub(crate) change.

alessandrod · 2024-12-07T09:46:34Z

i don't understand why the interfaces were changed in the same PR as breaks the types out to their own module. way too noisy even for master

Because nobody ever cared about APIs so far, and we don't have a CI hook that breaks CI if API is inadvertently broken. I welcome the interest in being adults with APIs going forward, but you can't just manifest it, things have to change. In the meantime, we're trying to unfuck RPC so that we stop doing ethereum level throughput. This PR is one of many, we're already trying to split, but we're also trying to get everything landed at a decent speed. Keep in mind that this is a huge chore for Kirill who's agreed to fix code that nobody wants to touch, and a source of aneurysms for me as I review all his changes.

alessandrod · 2024-12-07T09:49:29Z

i don't understand why the interfaces were changed in the same PR as breaks the types out to their own module. way too noisy even for master

Changes in public API that I found:
1. `SendTransactionService::new_with_config` and `new`: removed reference from `connection_cache: &Arc<ConnectionCache>`. It is because it was cloned twice anyways.

Can this be undone without breaking anything?

2. `pub(crate) struct SendTransactionServiceStatsReport` -- added pub(crate), used to be pub
3. `pub(crate) struct CurrentLeaderInfo<T>`  -- added pub(crate), used to be pub

Then I guess this can easily be reverted.

What has been moved:
1. `SendTransactionServiceStats` to a separate module. The structure is unchanged, just moved from the `send_transaction_service.rs` to reduce the lenght of this file. I can do it in a separate commit to simplify the review.

2. Also moved `CurrentLeaderInfo` to a another module.
Would it help if I create a separate commit that would extract these structures from the original module? I can also revert for this backport these pub(crate) change.

Moving seems fine, just re-export from where they used to be exported.

Let's fix the API breakage, but also let's minimize churn please. I don't want this STS fix to take another month to land. I want to move on and go back to pretending that RPC doesn't exist.

alessandrod · 2024-12-07T09:58:24Z

@bw-solana @alessandrod can we not just separate the crate and bump the major version? If this is making a breaking change with >2000 downloads a day, we cannot possibly alert everyone using it ahead of time.

https://crates.io/crates/solana-rpc/reverse_dependencies

there are probably 2 to 3 actual consumers of the API besides us

t-nelson · 2024-12-07T16:06:40Z

> i don't understand why the interfaces were changed in the same PR as breaks the types out to their own module. way too noisy even for master Because nobody ever cared about APIs so far, and we don't have a CI hook that breaks CI if API is inadvertently broken. I welcome the interest in being adults with APIs going forward, but you can't just manifest it, things have to change. In the meantime, we're trying to unfuck RPC so that we stop doing ethereum level throughput. This PR is one of many, we're already trying to split, but we're also trying to get everything landed at a decent speed. Keep in mind that this is a huge chore for Kirill who's agreed to fix code that nobody wants to touch, and a source of aneurysms for me as I review all his changes.

Oh I don’t care that they were both changed so much as that they were both changed in the same pr, making the entire set difficult to review with confidence

mergify · 2024-12-09T19:24:17Z

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

mergify · 2024-12-09T19:30:46Z

If this PR represents a change to the public RPC API:

Make sure it includes a complementary update to rpc-client/ (example)
Open a follow-up PR to update the JavaScript client @solana/web3.js (example)

Thank you for keeping the RPC clients in sync with the server API @mergify[bot].

KirillLykov · 2024-12-09T19:38:23Z

send-transaction-service/src/send_transaction_service.rs

 /// Metrics of the send-transaction-service.
 #[derive(Default)]
-struct SendTransactionServiceStats {
+pub struct SendTransactionServiceStats {


This became pub here to be used in transaction_client

KirillLykov · 2024-12-09T19:39:49Z

@t-nelson I added a commit that moves back the Stats structure and reverts some other changes (pub(crate)). The intention is to revert this commit later in this PR

KirillLykov · 2024-12-09T19:40:55Z

Red CI is unrelated, see https://discord.com/channels/428295358100013066/560503042458517505/1315755154338877511

KirillLykov · 2024-12-11T08:17:58Z

now CI breaks after update branch because error: 2 vulnerabilities found! for some crates. Again, unrelated to these changes.

mergify bot requested a review from a team as a code owner November 5, 2024 12:07

mergify bot assigned KirillLykov Nov 5, 2024

alessandrod previously approved these changes Nov 5, 2024

View reviewed changes

Merge branch 'v2.1' into mergify/bp/v2.1/pr-3423

9841611

KirillLykov added the automerge automerge Merge this Pull Request automatically once CI passes label Dec 5, 2024

This was referenced Dec 5, 2024

Add support of tpu-client-next to validator #3454

Open

v2.1: use tpu-client-next in send_transaction_service (backport of #3515) #3884

Open

bw-solana reviewed Dec 5, 2024

View reviewed changes

Merge branch 'v2.1' into mergify/bp/v2.1/pr-3423

4dca724

anza-team removed the automerge automerge Merge this Pull Request automatically once CI passes label Dec 6, 2024

KirillLykov dismissed alessandrod’s stale review via e91ca42 December 9, 2024 19:23

KirillLykov force-pushed the mergify/bp/v2.1/pr-3423 branch from 09f4795 to e91ca42 Compare December 9, 2024 19:23

KirillLykov force-pushed the mergify/bp/v2.1/pr-3423 branch from e91ca42 to 4dca724 Compare December 9, 2024 19:30

Undo STS API changes

4c9369d

KirillLykov reviewed Dec 9, 2024

View reviewed changes

Merge branch 'v2.1' into mergify/bp/v2.1/pr-3423

ec7f9ad

Merge branch 'v2.1' into mergify/bp/v2.1/pr-3423

02db28e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2.1: Extract client code in send_transaction_service into a new structure (backport of #3423) #3480

v2.1: Extract client code in send_transaction_service into a new structure (backport of #3423) #3480

mergify bot commented Nov 5, 2024

mergify bot commented Nov 5, 2024

alessandrod commented Nov 5, 2024 •

edited

Loading

KirillLykov commented Dec 5, 2024

bw-solana left a comment

KirillLykov commented Dec 6, 2024 •

edited

Loading

anza-team commented Dec 6, 2024

jacobcreech commented Dec 6, 2024

bw-solana commented Dec 6, 2024

t-nelson commented Dec 6, 2024

KirillLykov commented Dec 7, 2024

alessandrod commented Dec 7, 2024

alessandrod commented Dec 7, 2024

alessandrod commented Dec 7, 2024

t-nelson commented Dec 7, 2024 via email •

edited

Loading

mergify bot commented Dec 9, 2024

mergify bot commented Dec 9, 2024

KirillLykov Dec 9, 2024 •

edited

Loading

KirillLykov commented Dec 9, 2024 •

edited

Loading

KirillLykov commented Dec 9, 2024

KirillLykov commented Dec 11, 2024 •

edited

Loading

v2.1: Extract client code in send_transaction_service into a new structure (backport of #3423) #3480

Are you sure you want to change the base?

v2.1: Extract client code in send_transaction_service into a new structure (backport of #3423) #3480

Conversation

mergify bot commented Nov 5, 2024

Problem

Summary of Changes

mergify bot commented Nov 5, 2024

alessandrod commented Nov 5, 2024 • edited Loading

KirillLykov commented Dec 5, 2024

bw-solana left a comment

Choose a reason for hiding this comment

KirillLykov commented Dec 6, 2024 • edited Loading

anza-team commented Dec 6, 2024

jacobcreech commented Dec 6, 2024

bw-solana commented Dec 6, 2024

t-nelson commented Dec 6, 2024

KirillLykov commented Dec 7, 2024

alessandrod commented Dec 7, 2024

alessandrod commented Dec 7, 2024

alessandrod commented Dec 7, 2024

t-nelson commented Dec 7, 2024 via email • edited Loading

mergify bot commented Dec 9, 2024

mergify bot commented Dec 9, 2024

KirillLykov Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

KirillLykov commented Dec 9, 2024 • edited Loading

KirillLykov commented Dec 9, 2024

KirillLykov commented Dec 11, 2024 • edited Loading

alessandrod commented Nov 5, 2024 •

edited

Loading

KirillLykov commented Dec 6, 2024 •

edited

Loading

t-nelson commented Dec 7, 2024 via email •

edited

Loading

KirillLykov Dec 9, 2024 •

edited

Loading

KirillLykov commented Dec 9, 2024 •

edited

Loading

KirillLykov commented Dec 11, 2024 •

edited

Loading