Quic update identity #33865

ryleung-solana · 2023-10-25T15:39:17Z

Problem

Summary of Changes

Implements key hotswapping for the quic server and connection cache

Fixes #

codecov · 2023-11-06T16:51:45Z

Codecov Report

Merging #33865 (ddd1efb) into master (b97b3dd) will decrease coverage by 0.1%.
Report is 38 commits behind head on master.
The diff coverage is 65.1%.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #33865     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         819      819             
  Lines      220013   220085     +72     
=========================================
  Hits       180353   180353             
- Misses      39660    39732     +72

connection-cache/src/connection_cache.rs

behzadnouri · 2023-11-08T13:46:11Z

connection-cache/src/connection_cache.rs

+        let mut map = self.map.write().unwrap();
+        map.clear();
+        let mut connection_manager = self.connection_manager.write().unwrap();
+        connection_manager.update_key(key);


Doesn't this cause a dead-lock somewhere?
i.e. don't we have anywhere else in the code that locks self.connection_manager and then later locks self.map?

As far as I can see, the order is always self.map -> self.connection_manager so this should not cause a deadlock.

Can we change the order of locking and release the lock so we don't hold the lock on both?
something like:

connection_manager.write().unwrap().update_key(key); self.map.write().unwrap().clear();

This might discard some ok connections created between the 1st and the 2nd lock but I think that is better than the possibility of a dead-lock in the future.

I just added a comment stating the required locking order. I think that is the better solution, since it ensures stricter correctness, and it's quite common to simply establish a locking order to prevent deadlocks, rather than not lock multiple locks at all.

There is no way to enforce the comment.
Someone might change/add code later unaware of that comment there.
What is the problem with my suggestion above? #33865 (comment)

This might discard some ok connections created between the 1st and the 2nd lock

There's also no way to enforce that only 1 of the locks can be held at a time (and indeed, in create_connection_internal, we need to hold both). It's not uncommon to just specify a locking order that future programmers must follow when dealing with multiple locks. Not a massive deal, but since we have 2 locks anyway, I would prefer to do the most strictly correct thing and clear the cache then update the key while holding the cache lock.

quic-client/src/lib.rs

sdk/src/quic.rs

behzadnouri · 2023-11-08T14:33:07Z

quic-client/src/lib.rs

+        ipaddr: Option<IpAddr>,
    ) -> Result<(), RcgenError> {
-        let (cert, priv_key) = new_self_signed_tls_certificate(keypair, ipaddr)?;
+        let (cert, priv_key) = if let Some(ip) = ipaddr {
+            let res = new_self_signed_tls_certificate(keypair, ip)?;
+            self.ip = ip;
+            res
+        } else {
+            new_self_signed_tls_certificate(keypair, self.ip)?
+        };


EndpointKeyUpdater in streamer is storing the gossip_host ip address.
Can't we do a similar thing for connection-cache so that this method doesn't have to do the Option<IpAddr> hack?

I found it more convenient to just store the IP in QuicConfig since that's where we the socket anyway, and bubbling it up just to store it somewhere else seems like a little too much added complexity.

streamer/src/quic.rs

behzadnouri · 2023-11-08T14:37:12Z

connection-cache/src/connection_cache.rs

@@ -47,7 +48,7 @@ pub struct ConnectionCache<
 > {
    name: &'static str,
    map: Arc<RwLock<IndexMap<SocketAddr, /*ConnectionPool:*/ R>>>,
-    connection_manager: Arc<S>,
+    connection_manager: Arc<RwLock<S>>,


The RwLock here could cause deadlocks or lock-contention.
Is there an alternative way avoiding this?

Agreed that this is not ideal. Since the map is always locked when connection_manager is locked (but not vice versa), it should be possible to just use 1 lock to protect both. However, while I have tried to remove this lock, I have not been able to make Rust happy (non-buiilding code at https://github.com/ryleung-solana/solana/tree/quic-update-identity-test if you have suggestions).

behzadnouri · 2023-11-08T14:57:32Z

quic-client/src/lib.rs

+            res
+        } else {
+            new_self_signed_tls_certificate(keypair, self.ip)?
+        };
        self.client_certificate = Arc::new(QuicClientCertificate {


when does this call into set_default_client_config?
https://docs.rs/quinn/0.10.2/quinn/struct.Endpoint.html#method.set_default_client_config

this doesn't take effect until set_default_client_config.

See QuicLazyInitializedEndpoint::create_endpoint

Right, but that is only called once:
https://github.com/solana-labs/solana/blob/a96be5d2f/quic-client/src/nonblocking/quic_client.rs#L142-L143
and the cached in:

endpoint: OnceCell<Arc<Endpoint>>,

https://github.com/solana-labs/solana/blob/a96be5d2f/quic-client/src/nonblocking/quic_client.rs#L70

@ryleung-solana you marked this resolved, but I do not know what have you changed to resolve this.

In QuicConfig::create_endpoint we create a new QuicLazyInitializedEndpoint struct. This, combined with clearing the connection cache means that the first time we get a new connection from the connection cache, we will call set_default_client_config on the Endpoint

connection-cache/src/connection_cache.rs

behzadnouri · 2023-11-14T20:39:13Z

connection-cache/src/connection_cache.rs

+        let mut map = self.map.write().unwrap();
+        map.clear();
+        let mut connection_manager = self.connection_manager.write().unwrap();
+        connection_manager.update_key(key);


There is no way to enforce the comment.
Someone might change/add code later unaware of that comment there.
What is the problem with my suggestion above? #33865 (comment)

local-cluster/src/local_cluster.rs

quic-client/src/lib.rs

validator/src/admin_rpc_service.rs

quic-client/src/lib.rs

behzadnouri

lgtm, but can you also please ask @lijunwangs to take a look and confirm there is no perf concern?
maybe also run the bench-tps with this change just to be safe.

core/src/validator.rs

This reverts commit 7ebb8da.

ryleung-solana · 2023-12-01T21:01:40Z

lgtm, but can you also please ask @lijunwangs to take a look and confirm there is no perf concern? maybe also run the bench-tps with this change just to be safe.

Ran bench-tps, perf characteristics look pretty much the same. First is with the change, second without.

…nterface

validator/src/main.rs

lijunwangs · 2023-12-07T19:18:28Z

quic-client/src/lib.rs

+        Ok(())
+    }
+
+    pub fn update_keypair(&self, keypair: &Keypair) -> Result<(), RcgenError> {


This update works only when a new connection pool is created. How do we have handle existing connection cached?

See connection-cache/src/connection_cache.rs line 141

I understand that, but I think it won't fix the existing connection's endpoints. We would similar things you did to the server side: like

let (config, _) = configure_server(key, self.gossip_host)?; self.endpoint.set_server_config(Some(config)); Ok(())

ryleung-solana · 2023-12-07T21:29:22Z

Connection refs are only held by callers to get_connection as long as they are used to do a send task, afterwards they are dropped. The endpoint is configured the first time a connection pool is used. Therefore by clearing the map, the first time get_connection is called afterwards, a new connection pool is created, and the first time a connection is obtained from the connection pool the endpoint will be given the new configuration.

…

On Fri, Dec 8, 2023, 5:14 AM Lijun Wang ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In quic-client/src/lib.rs <#33865 (comment)>: > @@ -143,7 +156,23 @@ impl QuicConfig { ipaddr: IpAddr, ) -> Result<(), RcgenError> { let (cert, priv_key) = new_self_signed_tls_certificate(keypair, ipaddr)?; - self.client_certificate = Arc::new(QuicClientCertificate { + self.addr = ipaddr; + + let mut cert_guard = self.client_certificate.write().unwrap(); + + *cert_guard = Arc::new(QuicClientCertificate { + certificate: cert, + key: priv_key, + }); + Ok(()) + } + + pub fn update_keypair(&self, keypair: &Keypair) -> Result<(), RcgenError> { I understand that, but I think it won't fix the existing connection's endpoints. We would similar things you did to the server side: like let (config, _) = configure_server(key, self.gossip_host)?; self.endpoint.set_server_config(Some(config)); Ok(()) — Reply to this email directly, view it on GitHub <#33865 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AV5GU66RUQEJLZTPE57GQO3YIIWS3AVCNFSM6AAAAAA6PTGDTOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZRGE2DANBRGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

lijunwangs · 2023-12-07T21:45:31Z

Connection refs are only held by callers to get_connection as long as they are used to do a send task, afterwards they are dropped. The endpoint is configured the first time a connection pool is used. Therefore by clearing the map, the first time get_connection is called afterwards, a new connection pool is created, and the first time a connection is obtained from the connection pool the endpoint will be given the new configuration.
…

Right, I missed the part you cleared the cache.

lijunwangs

Looks good to me

Update the Quic transport layer keypair and identity when the Validator's identity keypair is updated

Update the Quic transport layer keypair and identity when the Validator's identity keypair is updated Quic update identity (solana-labs#33865) Update the Quic transport layer keypair and identity when the Validator's identity keypair is updated

ryleung-solana requested a review from behzadnouri October 25, 2023 18:03

ryleung-solana changed the title ~~Quic update identity new~~ Quic update identity Oct 26, 2023

ryleung-solana force-pushed the quic-update-identity-new branch from 4f50ac5 to a77d78d Compare November 6, 2023 14:39

behzadnouri reviewed Nov 8, 2023

View reviewed changes

ryleung-solana requested a review from behzadnouri November 10, 2023 20:51

ryleung-solana force-pushed the quic-update-identity-new branch from 0e39688 to 703cab5 Compare November 14, 2023 15:03

behzadnouri reviewed Nov 14, 2023

View reviewed changes

ryleung-solana requested a review from behzadnouri November 16, 2023 09:37

ryleung-solana force-pushed the quic-update-identity-new branch from d31b3fa to 7d5b1cf Compare November 16, 2023 16:03

behzadnouri reviewed Nov 17, 2023

View reviewed changes

quic-client/src/lib.rs Outdated Show resolved Hide resolved

ryleung-solana requested a review from behzadnouri November 21, 2023 13:13

behzadnouri reviewed Nov 29, 2023

View reviewed changes

behzadnouri requested a review from lijunwangs November 29, 2023 00:05

lijunwangs reviewed Nov 29, 2023

View reviewed changes

core/src/validator.rs Outdated Show resolved Hide resolved

ryleung-solana added 14 commits December 2, 2023 02:43

Initial work

0471a4e

Fix problems from bad merge

f583dc1

Wire some stuff up

573b53d

Wire more stuff up

b2e6b70

Wire more stuff up

2e179ab

Clear connection cache when updating the self-signed key

4a9d1f8

Testing changes to confirm the key is updated

8588396

Using info for the debug logs

6bcf8a3

Fix clippy and formatting issues

5eb1855

Fix test compilation

a5fe8b9

Fix more tests

ec3dd44

Attempt to fix sbf compilation error

d49902d

Try to fix CI

9da6c41

Revert "Using info for the debug logs"

fa08f91

This reverts commit 7ebb8da.

ryleung-solana added 12 commits December 2, 2023 02:43

Fix more tests

3d716b0

Log errors updating the Quic client key

5b56526

Bubble up errors from updating keypair

ca0d922

Fix dumb merge error

48c9cf3

Use struct rather than tuple

95104f3

Add comment

d9fe30e

Remove misc unnecessary changes

2afb6de

Re-fix formatting

c7b7197

Misc cleanup

d253d3a

Add missing implementation

32312bc

Use separate method to update just the keypair

0093a69

Remove a lock from ConnectionCache

bf8a2ca

ryleung-solana force-pushed the quic-update-identity-new branch from af978b4 to bf8a2ca Compare December 1, 2023 18:44

ryleung-solana requested a review from lijunwangs December 1, 2023 21:04

Move notify pointers for NotifyKeyUpdate to simplify Validator::new i…

f2a0d74

…nterface

lijunwangs reviewed Dec 6, 2023

View reviewed changes

validator/src/main.rs Outdated Show resolved Hide resolved

Remove unnecessary clones

ddd1efb

ryleung-solana requested a review from lijunwangs December 7, 2023 17:21

lijunwangs reviewed Dec 7, 2023

View reviewed changes

lijunwangs approved these changes Dec 7, 2023

View reviewed changes

ryleung-solana merged commit 132c910 into solana-labs:master Dec 8, 2023
44 checks passed

AshwinSekar mentioned this pull request Feb 8, 2024

set-identity causes undesirable behavior when used while validator is waiting for supermajority. #35152

Closed

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Apr 2, 2024

Quic update identity (solana-labs#33865)

3d024b3

Update the Quic transport layer keypair and identity when the Validator's identity keypair is updated

lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Apr 2, 2024

Quic update identity (solana-labs#33865)

f9b6db0

Update the Quic transport layer keypair and identity when the Validator's identity keypair is updated

lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Apr 2, 2024

Quic update identity (solana-labs#33865)

11ed885

Update the Quic transport layer keypair and identity when the Validator's identity keypair is updated

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quic update identity #33865

Quic update identity #33865

ryleung-solana commented Oct 25, 2023

codecov bot commented Nov 6, 2023 •

edited

Loading

behzadnouri Nov 8, 2023

ryleung-solana Nov 8, 2023

behzadnouri Nov 12, 2023

ryleung-solana Nov 13, 2023

behzadnouri Nov 14, 2023

ryleung-solana Nov 15, 2023 •

edited

Loading

behzadnouri Nov 8, 2023

ryleung-solana Nov 10, 2023

behzadnouri Nov 8, 2023

ryleung-solana Nov 8, 2023

behzadnouri Nov 8, 2023

ryleung-solana Nov 8, 2023

behzadnouri Nov 9, 2023

behzadnouri Nov 12, 2023

ryleung-solana Nov 13, 2023

behzadnouri Nov 14, 2023

behzadnouri left a comment

ryleung-solana commented Dec 1, 2023

lijunwangs Dec 7, 2023

ryleung-solana Dec 7, 2023

lijunwangs Dec 7, 2023

ryleung-solana commented Dec 7, 2023 via email

lijunwangs commented Dec 7, 2023

lijunwangs left a comment

Quic update identity #33865

Quic update identity #33865

Conversation

ryleung-solana commented Oct 25, 2023

Problem

Summary of Changes

codecov bot commented Nov 6, 2023 • edited Loading

Codecov Report

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

ryleung-solana Nov 15, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzadnouri left a comment

Choose a reason for hiding this comment

ryleung-solana commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryleung-solana commented Dec 7, 2023 via email

lijunwangs commented Dec 7, 2023

lijunwangs left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 6, 2023 •

edited

Loading

ryleung-solana Nov 15, 2023 •

edited

Loading