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

Remove ThinClient from LocalCluster #977

Conversation

gregcusack
Copy link

@gregcusack gregcusack commented Apr 22, 2024

A Followup PR to: #258.
This is the 10th PR on the way to remove ThinClient completely.

Problem

ThinClient is deprecated. Replacing with TpuClient

Summary of Changes

Replace ThinClient with TpuClient in LocalCluster
Specifically, we use a Quic TpuClient, not a UDP TpuClient

Notes

This works but I can't say for sure why it works.
I had initially replaced all of ThinClient within LocalCluster with the nonblocking version of TpuClient. I had made the switch such that transfer_with_client() calls TpuClient.try_send_transaction() which calls send_wire_transaction_to_addr(). send_wire_transaction_to_addr() uses a non-blocking connection from the connection_cache to send the transaction.

However, using the changes above, I consistently ran into an issue with any test that called add_validator() more than once. Under the hood, add_validator() calls transfer_with_client(). transfer_with_client() transfers some amount of lamports to a destination account (aka the account associated with the validator we are trying to add). However in the LocalCluster tests, transfer_with_client() would consistently fail when adding the second validator with add_validator(). It would always fail with: transport custom error: "ConnectError(EndpointStopping).

I noticed that ThinClient used a blocking connection when sending transactions with retry_transfer() (which was used to send a transaction within transfer_with_client(). So, I switched over to using the blocking TpuClient and that seems to work well. I had to make a few changes to expose the leader schedule, connection cache, and fanout slots from the nonblocking/tpu_client up to the blocking tpu_client.

I am still working on investigating. But thinks its probably a good idea to get some eyes on this since it does work as expected now.

@gregcusack gregcusack force-pushed the remove-thin-client-from-local-cluster-v5 branch from f2911e4 to 9b74bf6 Compare April 22, 2024 22:38
@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.1%. Comparing base (ad12ea3) to head (b86668f).
Report is 19 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #977   +/-   ##
=======================================
  Coverage    82.1%    82.1%           
=======================================
  Files         893      893           
  Lines      236936   236931    -5     
=======================================
+ Hits       194705   194720   +15     
+ Misses      42231    42211   -20     

@gregcusack gregcusack changed the title Remove thin client from local cluster v5 Remove thin client from local cluster Apr 23, 2024
@gregcusack gregcusack changed the title Remove thin client from local cluster Remove ThinClient from LocalCluster Apr 23, 2024
@gregcusack gregcusack force-pushed the remove-thin-client-from-local-cluster-v5 branch 2 times, most recently from ed2cccf to c34a494 Compare April 24, 2024 20:40
@gregcusack gregcusack marked this pull request as ready for review April 25, 2024 05:25
@gregcusack gregcusack force-pushed the remove-thin-client-from-local-cluster-v5 branch 4 times, most recently from 18b9bb8 to dc53a97 Compare May 8, 2024 23:56
@gregcusack gregcusack force-pushed the remove-thin-client-from-local-cluster-v5 branch 2 times, most recently from f69dd31 to c514d4a Compare May 9, 2024 23:30
@gregcusack gregcusack force-pushed the remove-thin-client-from-local-cluster-v5 branch from c514d4a to b86668f Compare May 10, 2024 15:29
@gregcusack gregcusack closed this May 10, 2024
@gregcusack
Copy link
Author

closed in favor of: #1300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants