Use tpu client next in validator with receiver #3893

KirillLykov · 2024-12-03T16:48:59Z

Problem

add flag to validator to use tpu-client-next,
add tests on rpc side to work with both ConnectionCache and tpu-client-next
all the plumbing to select client on the validator side

Summary of Changes

mergify · 2024-12-03T16:49:33Z

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 @KirillLykov.

KirillLykov · 2024-12-03T16:51:48Z

core/src/validator.rs

 };

+static GLOBAL_RUNTIME: LazyLock<TokioRuntime> = LazyLock::new(|| {


Probably, it would be better to define it validator/src/main.rs and pass to the Validator::new. But doing so would require changing new which means a lot of changes in the codebase. I propose to do so in separate PR which is not necessary to backport

static is fine, but this needs a better name imo. The validator already uses a bunch
of runtimes, GLOBAL_RUNTIME suggests that this is "the" runtime, while this is
only used by the tpu client as far as I can tell.

STS_CLIENT_RUNTIME?

alessandrod

looks good! left some minor comments

alessandrod · 2024-12-06T10:42:32Z

core/src/validator.rs

 };

+static GLOBAL_RUNTIME: LazyLock<TokioRuntime> = LazyLock::new(|| {


static is fine, but this needs a better name imo. The validator already uses a bunch
of runtimes, GLOBAL_RUNTIME suggests that this is "the" runtime, while this is
only used by the tpu client as far as I can tell.

STS_CLIENT_RUNTIME?

alessandrod · 2024-12-06T10:43:02Z

core/src/validator.rs

+});
+
+// Function to get a handle to the runtime
+fn get_runtime_handle() -> tokio::runtime::Handle {


This has one caller in new, I probably wouldn't make it a function, just inline
in the call site.

alessandrod · 2024-12-06T10:53:14Z

rpc/src/rpc.rs

-            .my_contact_info()
-            .tpu(connection_cache.protocol())
-            .unwrap();
+        let my_tpu_address = cluster_info.my_contact_info().tpu(Protocol::QUIC).unwrap();


This now hardcodes QUIC - did the old code actually work with UDP?

the old code checks DEFAULT_CONNECTION_CACHE_USE_QUIC which is true. So the old code could work with UDP if this DEFAULT_CONNECTION_CACHE_USE_QUIC modified in code to be false (which means never).

What I think is sufficient to do for tests is to add check that client protocol is always QUIC so that if someone changes the implementation of ConnectionCache to use UDP, tests will not work.

I agree that this is testing RPC and so it doesn't matter what the underlying
protocol is

alessandrod · 2024-12-06T11:16:55Z

core/src/validator.rs

@@ -989,6 +1008,9 @@ impl Validator {

        let staked_nodes = Arc::new(RwLock::new(StakedNodes::default()));

+        // ConnectionCache might be used for JsonRpc and for Forwarding. Since
+        // the later is not migrated yet to the tpu-client-next, create


typo: latter

alessandrod · 2024-12-06T11:20:06Z

core/src/validator.rs

+            key_notifies.push(client_updater);
+        } else {
+            // add connection_cache because it is still used in Forwarder.
+            key_notifies.push(connection_cache);


doesn't this need to be added even if tpu_client_next is used for STS, or the
forwarder identity won't change?

alessandrod · 2024-12-06T11:24:57Z

send-transaction-service/src/transaction_client.rs

+        let handle = self.join_and_cancel.clone();
+
+        let join_handle = {
+            let Ok(mut lock) = handle.lock() else {


nit: let Ok((token, handle)) = ...

Sorry but I don't understand what do you propose here

My bad you can't destructure there because the guard needs to be alive. But
below you should do:

let (handle, token) = lock; token.cancel(); handle.take();

if you use lock.1 and lock.0 I as a reader have no idea what's what.

I can take it:

let (handle, token) = std::mem::take(&mut *lock);

alessandrod

almost there, just a few more comments

alessandrod · 2024-12-13T13:13:45Z

rpc/src/rpc.rs

-            .my_contact_info()
-            .tpu(connection_cache.protocol())
-            .unwrap();
+        let my_tpu_address = cluster_info.my_contact_info().tpu(Protocol::QUIC).unwrap();


I agree that this is testing RPC and so it doesn't matter what the underlying
protocol is

alessandrod · 2024-12-13T13:15:24Z

rpc/src/rpc.rs

+    }
+
+    #[tokio::test(flavor = "multi_thread")]
+


extra newline? (how does this pass CI?)

alessandrod · 2024-12-13T13:26:32Z

send-transaction-service/src/transaction_client.rs

+        let handle = self.join_and_cancel.clone();
+
+        let join_handle = {
+            let Ok(mut lock) = handle.lock() else {


My bad you can't destructure there because the guard needs to be alive. But
below you should do:

let (handle, token) = lock; token.cancel(); handle.take();

if you use lock.1 and lock.0 I as a reader have no idea what's what.

alessandrod · 2024-12-13T13:28:24Z

send-transaction-service/src/transaction_client.rs

+        };
+
+        if let Some(join_handle) = join_handle {
+            let Ok(result) = join_handle.await else {


nit: you don't need let else ... and then a match below. You can do everything
in the match.

Something like:

if let Some(Ok(join_handle)) = join_handle.map(async |handle| handle.await) {

would be possible if async closures existed. But even in this case, I would like to have this log message about panic.

KirillLykov · 2024-12-16T15:06:05Z

@alessandrod added a commit with graceful exit of the tpu-client-next and also added a test for admin_rpc that checks that change of the identity and exit rpc calls work as expected.

KirillLykov requested a review from alessandrod December 3, 2024 16:50

KirillLykov commented Dec 3, 2024

View reviewed changes

KirillLykov force-pushed the klykov/use-tpu-client-next-in-validator-with-receiver-2 branch from 9a2de64 to 4670c01 Compare December 3, 2024 17:09

This was referenced Dec 3, 2024

use tpu client next in validator #3795

Closed

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

Open

KirillLykov changed the title ~~Use tpu client next in validator with receiver 2~~ Use tpu client next in validator with receiver Dec 5, 2024

alessandrod reviewed Dec 6, 2024

View reviewed changes

KirillLykov force-pushed the klykov/use-tpu-client-next-in-validator-with-receiver-2 branch from bf56164 to 5bca5cd Compare December 11, 2024 11:18

alessandrod reviewed Dec 13, 2024

View reviewed changes

KirillLykov force-pushed the klykov/use-tpu-client-next-in-validator-with-receiver-2 branch from 5bca5cd to 67a4ea3 Compare December 13, 2024 18:12

KirillLykov added 7 commits December 16, 2024 16:04

Add update_key for transaction client

5de1266

use tpu-client-next in validator

8c17ca6

Rename runtime

55b8070

Address PR comments

13efce4

address PR comments

20d5615

add exit to TransactionClient

fc53bea

parametrize test for admin set_identity with client

50fd073

KirillLykov force-pushed the klykov/use-tpu-client-next-in-validator-with-receiver-2 branch from 67a4ea3 to 50fd073 Compare December 16, 2024 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use tpu client next in validator with receiver #3893

Use tpu client next in validator with receiver #3893

KirillLykov commented Dec 3, 2024 •

edited

Loading

mergify bot commented Dec 3, 2024

KirillLykov Dec 3, 2024 •

edited

Loading

alessandrod Dec 6, 2024

alessandrod left a comment

alessandrod Dec 6, 2024

alessandrod Dec 6, 2024

alessandrod Dec 6, 2024

KirillLykov Dec 10, 2024 •

edited

Loading

alessandrod Dec 13, 2024

alessandrod Dec 6, 2024

alessandrod Dec 6, 2024

alessandrod Dec 6, 2024

KirillLykov Dec 10, 2024

alessandrod Dec 13, 2024

KirillLykov Dec 13, 2024

alessandrod left a comment

alessandrod Dec 13, 2024

alessandrod Dec 13, 2024

alessandrod Dec 13, 2024

alessandrod Dec 13, 2024

KirillLykov Dec 13, 2024 •

edited

Loading

KirillLykov commented Dec 16, 2024

		};

		static GLOBAL_RUNTIME: LazyLock<TokioRuntime> = LazyLock::new(\|\| {

Use tpu client next in validator with receiver #3893

Are you sure you want to change the base?

Use tpu client next in validator with receiver #3893

Conversation

KirillLykov commented Dec 3, 2024 • edited Loading

Problem

Summary of Changes

mergify bot commented Dec 3, 2024

KirillLykov Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirillLykov Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirillLykov Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

KirillLykov commented Dec 16, 2024

KirillLykov commented Dec 3, 2024 •

edited

Loading

KirillLykov Dec 3, 2024 •

edited

Loading

KirillLykov Dec 10, 2024 •

edited

Loading

KirillLykov Dec 13, 2024 •

edited

Loading