Add a client for BankForks #10728

garious · 2020-06-20T21:32:20Z

Problems

Can't really unit-test client-side Rust apps
Little hope of leveraging RpcClient code in the browser

Summary of Changes

Add BanksClient, implemented with tarpc
Open a new validator port for the BanksClient bincode-via-TCP transport
Port solana-tokens to BanksClient

codecov · 2020-06-20T23:02:47Z

Codecov Report

Merging #10728 into master will increase coverage by 0.0%.
The diff coverage is 88.0%.

@@           Coverage Diff            @@
##           master   #10728    +/-   ##
========================================
  Coverage    81.9%    82.0%            
========================================
  Files         318      323     +5     
  Lines       74872    75267   +395     
========================================
+ Hits        61371    61732   +361     
- Misses      13501    13535    +34

garious · 2020-06-22T23:37:07Z

@jstarry, can you evaluate solana_sdk::bank_forks_client::ThinClient for usage via WebAssembly?

jstarry · 2020-06-23T14:12:13Z

@garious looks pretty good. It's cool that tarpc let's you bring your own client transport. We would need a wasm / browser friendly transport using fetch or websockets.

The only wasm blocker that I found was tarpc's use of tokio::time which uses std::time::Instant internally. That could easily replaced with something browser friendly. There may be other parts that aren't wasm friendly but nothing big stood out.

jstarry · 2020-06-23T14:12:54Z

We could use / create something like https://github.com/tomaka/wasm-timer

jstarry · 2020-07-12T09:04:08Z

Hey @garious I tried out your branch and was able to get it working in the browser using a ws transport 🎉

As I pointed out before, the main big hurdle is the lack of support of SystemTime and Instant APIs in the browser. For those, we need to use the browser api equivalent. To summarize:

tarpc uses SystemTime::now() in context::current() which is used for creating requests. I patched tarpc to be able to supply a custom SystemTime which I can create in wasm without using SystemTime::now() (which panics). Pretty simple and I imagine we could upstream this (or a similar) change to tarpc
tarpc uses tokio::time for timing out Futures. The Tokio API takes a deadline duration and adds to Instant::now() which panics in wasm. I opted to just elide timeouts entirely for my proof of concept but of course we don't want that. I'm not sure the best way to upstream a fix for this since there's not much movement on wasm support in tokio in general but they do seem open to it.

There were a few other issues:

ed25519-dalek uses clear_on_drop which is wasm incompatible. They use the wasm-friendly zeroize in the dev branch so hopefully that gets released soonish. But for now, it's easy enough to patch it for wasm.
I think you brought this up already but BanksClient has annoying lifetime constraints. I had to Rc<RefCell<..>> wrap it in order to make the rust compiler happy
Adding the WS transport wasn't too bad but the wasm / async WS crate I used for the client is pretty new and needs improvements: https://docs.rs/ws_stream_wasm
(minor) Had to reduce enabled features for some dependencies in the SDK and disable some wasm-incompatible modules like genesis_config, hard_forks, and shred_version.

carllin · 2020-07-23T20:53:12Z

@ryoqun, Right now queries to get specific account state on a cluster confirmed root < current root are unsupported, as they will just default to account state on current root right? I think this may be a bigger overarching issue with trying to query state from before the root.

CriesofCarrots · 2020-07-23T21:17:32Z

@ryoqun, Right now queries to get specific account state on a cluster confirmed root < current root are unsupported, as they will just default to account state on current root right? I think this may be a bigger overarching issue with trying to query state from before the root.

@carllin yes, see: #11161

garious · 2020-07-23T23:50:21Z

I'll rebase on #11188. More code review comments?

jstarry

Looking good! I'm definitely in favour of tarpc

banks-client/src/lib.rs

transaction-status/src/banks_trait.rs

tokens/src/commands.rs

jstarry · 2020-07-24T09:28:31Z

banks-server/src/banks_server.rs

+        let signature = transaction.signatures.get(0).cloned().unwrap_or_default();
+        let info =
+            TransactionInfo::new(signature, serialize(&transaction).unwrap(), last_valid_slot);
+        self.transaction_sender.send(info).unwrap();


Should we send the transaction through the preflight checker first?

@jstarry @mvines, so I nearly added this feature today, but am now thinking there's some good reasons not to. Unlike jsonrpc, I'd like for BanksServer to be lightweight enough that every validator could run it, and not be too concerned about a DoS. The only errors that simulate_transaction can detect are InsufficientFundsForTransactionFee and SigVerifyFailed. Both of those can be done client-side. The other thing simulating can do is find program errors without paying for the transaction fee - why should the RPC node eat that cost, when otherwise the user would pay the transaction fee, which pays for signature verification and running the program?

Public RPC nodes are not voting. The DOS vector and the cost they eat doesn't have impact on security. admins can sandbox them without performance impact on consensus.

@aeyakovenko, not talking about DOSing the cluster, just the RPC node. It's a UX issue, not a security issue.

jstarry · 2020-07-24T09:30:27Z

banks-server/src/banks_server.rs

+        self,
+        _: Context,
+        transaction: Transaction,
+        commitment: CommitmentLevel,


The web3 SDK uses confirmation counts to confirm transactions now. I think we should have the BanksClient follow that example. The confirmations in TransactionStatus represent the number of cluster confirmations.

I'll think on this. Personally, I'd prefer to subscribe to a stream of events.

jstarry · 2020-07-24T09:33:04Z

banks-server/src/banks_server.rs

+            chan.respond_with(server.serve()).execute()
+        })
+        // Max 10 channels.
+        .buffer_unordered(10)


Does this mean that only 10 concurrent requests are allowed? Seems like it should be quite a bit higher

Yeah, we'll want to tune everything in this function. I think that can be a separate PR though.

jstarry · 2020-07-24T09:49:07Z

banks-client/src/lib.rs

+    async fn send_transaction(&mut self, transaction: Transaction) -> io::Result<()>;
+    async fn get_recent_blockhash(&mut self) -> io::Result<Hash>;
+    async fn get_fees(&mut self) -> io::Result<(FeeCalculator, Hash, Slot)>;
+    async fn process_transaction_with_commitment(


If we replace commitment with confirmations in send_and_confirm_transaction, then I propose we default process_transaction to 0 or 1 confirmation(s).

Then we could have:

send_transaction

execute_transaction / process_transaction which waits for 0 or 1 confirmation(s)

settle_transaction which waits for MAX confirmations

[optional] confirm_transaction for user-specified confirmations

cc @aeyakovenko

banks-client/src/lib.rs

aeyakovenko · 2020-07-24T13:50:16Z

Execute

Transaction has been replicated by turbine across the cluster, or settled.

Settlement=X
The block or a child has reached at least X% safety or has been finalized
Finalized=X
Rooted by at least X%

Do we need to query specifically for local finality?

Tag @carllin

CriesofCarrots

I am in favor as well

banks-client/src/lib.rs

carllin · 2020-07-24T20:39:13Z

@jstarry @aeyakovenko makes sense to me. Seems like on the scale of finality, with local finality one one end and MAX on the other, not sure practically what most clients would feel safe with. >33% seems like a good enough number to make rollback impossible.

For the execution receipts, feel free to add to the issue here: #11116

garious · 2020-07-24T21:21:13Z

I don't like these APIs where the client attempts to pick one and only one event. We should be exposing the data, not interpreting it. In this case, it's an event stream. Instead of returning a value, we should be passing in a client-side object that gets notified each time a relevant event occurs.

@jstarry From what I can tell, tarpc may not work well for this case. In their pubsub example, the caller needs to provide an address for the server to connect to. That wouldn't allow a client to sit behind a firewall. Also, it lets the server pick the upstream stream transport, which is weird. It's possible tarpc needs some rework to allow for upstream communication via the same transport.

garious · 2020-07-24T23:12:17Z

@jstarry, @aeyakovenko, @CriesofCarrots, perhaps there should be a CommitmentLevel::Confirmations(u8)?

garious · 2020-07-24T23:13:56Z

sdk/src/commitment_config.rs

@@ -46,11 +46,28 @@ impl CommitmentConfig {

 #[derive(Serialize, Deserialize, Clone, Copy, Debug, PartialEq, Eq, Hash)]
 #[serde(rename_all = "camelCase")]
+/// An attribute of a slot. It describes how finalized a block is at some point in time. For example, a slot


@CriesofCarrots @jstarry, how's this look? I don't know if we've ever stated "commitment level means ...". I found it quite difficult to describe - maybe a red flag that I got it wrong.

jstarry · 2020-07-25T07:57:06Z

@jstarry From what I can tell, tarpc may not work well for this case. In their pubsub example, the caller needs to provide an address for the server to connect to. That wouldn't allow a client to sit behind a firewall. Also, it lets the server pick the upstream stream transport, which is weird. It's possible tarpc needs some rework to allow for upstream communication via the same transport.

@garious I just took a look as well and I agree. tarpc would need changes to support a proper pubsub interface with Stream responses. Right now, the framework considers a request to be finished when it a response is sent or the request is canceled. Once there are no more pending requests, the connection is closed.

jstarry · 2020-07-25T08:55:59Z

@garious As for streams of events for something like finality, I also think it would be useful. I think the primary use case would end up being a progress indicator. The progress could indicate number of confirmations or X% of stake that has confirmed. This is functionality that we don't have in the current RPC API so I think it can come later

I don't like these APIs where the client attempts to pick one and only one event

If clients want to get notified for both execution and settlement for a given transaction, they can send two requests.

perhaps there should be a CommitmentLevel::Confirmations(u8)

I'm second guessing exposing confirmations in a confirmTransaction API. It's probably best to have a few sensible defaults in the API (either execute / settle or stick with commitment) to keep things simple for clients. If devs need something custom, they'll tell us and we can learn more about what guarantees they need and go from there

sdk/src/commitment_config.rs

garious · 2020-07-29T22:59:52Z

@jstarry, fyi, new https://github.com/google/tarpc/blob/master/tarpc/examples/pubsub.rs that shares the client connection

Also: * Use BanksClient in solana-tokens

garious · 2020-08-07T00:30:01Z

What do you all think about merging this one as-is? On its current trajectory, it's an improvement over RpcClient for some Rust CLI tooling and obsoletes BankClient. But also, with 1.3 out the door, we have plenty of time to make it even better before it goes into production.

mvines · 2020-08-07T00:35:11Z

sgtm! Honestly I've not reviewed but it's gotta be an improvement. How could it not be :)

Cherry-picked from solana-labs#10728, but without the changes to solana-tokens

Cherry-picked from #10728, but without the changes to solana-tokens

garious force-pushed the bank_forks_client branch from 47483f4 to 1029c69 Compare June 20, 2020 21:44

garious force-pushed the bank_forks_client branch 4 times, most recently from 31e5e36 to d526216 Compare June 22, 2020 23:22

garious force-pushed the bank_forks_client branch from c14ccf4 to 45a4d37 Compare June 23, 2020 02:17

garious force-pushed the bank_forks_client branch 3 times, most recently from 997d575 to 9e6224c Compare June 26, 2020 23:44

garious force-pushed the bank_forks_client branch 9 times, most recently from 1d9ae59 to 7ba3d79 Compare July 7, 2020 04:31

garious force-pushed the bank_forks_client branch 7 times, most recently from 8588820 to eb9236a Compare July 11, 2020 18:06

garious force-pushed the bank_forks_client branch from 7bdde17 to df11ba7 Compare July 24, 2020 02:18

garious closed this Jul 24, 2020

garious reopened this Jul 24, 2020

jstarry reviewed Jul 24, 2020

View reviewed changes

CriesofCarrots reviewed Jul 24, 2020

View reviewed changes

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

garious commented Jul 24, 2020

View reviewed changes

jstarry reviewed Jul 25, 2020

View reviewed changes

sdk/src/commitment_config.rs Outdated Show resolved Hide resolved

sdk/src/commitment_config.rs Show resolved Hide resolved

garious force-pushed the bank_forks_client branch from a629db0 to 9c0c76a Compare July 28, 2020 21:25

garious force-pushed the bank_forks_client branch 3 times, most recently from 3a6fb8f to 201e3e4 Compare August 5, 2020 20:17

Add a client for BankForks

4184460

Also: * Use BanksClient in solana-tokens

garious force-pushed the bank_forks_client branch from 201e3e4 to 4184460 Compare August 5, 2020 22:31

garious merged commit bad4868 into solana-labs:master Aug 7, 2020

garious added a commit to garious/solana that referenced this pull request Aug 20, 2020

Add BanksClient

7d2c27b

Cherry-picked from solana-labs#10728, but without the changes to solana-tokens

garious mentioned this pull request Aug 20, 2020

Add BanksClient to v1.3 #11721

Merged

garious added a commit that referenced this pull request Aug 20, 2020

Add BanksClient (#11721)

e938925

Cherry-picked from #10728, but without the changes to solana-tokens

kevinheavey mentioned this pull request Mar 27, 2023

Make BanksClient timeout after 10 seconds instead of 60 #30904

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a client for BankForks #10728

Add a client for BankForks #10728

garious commented Jun 20, 2020 •

edited

codecov bot commented Jun 20, 2020 •

edited

garious commented Jun 22, 2020

jstarry commented Jun 23, 2020

jstarry commented Jun 23, 2020

jstarry commented Jul 12, 2020

carllin commented Jul 23, 2020 •

edited

CriesofCarrots commented Jul 23, 2020

garious commented Jul 23, 2020

jstarry left a comment

jstarry Jul 24, 2020

garious Jul 30, 2020

aeyakovenko Jul 30, 2020

garious Jul 31, 2020

jstarry Jul 24, 2020

garious Jul 24, 2020

jstarry Jul 24, 2020

garious Jul 24, 2020

jstarry Jul 24, 2020

aeyakovenko commented Jul 24, 2020

CriesofCarrots left a comment

carllin commented Jul 24, 2020 •

edited

garious commented Jul 24, 2020

garious commented Jul 24, 2020

garious Jul 24, 2020

jstarry commented Jul 25, 2020

jstarry commented Jul 25, 2020 •

edited

garious commented Jul 29, 2020

garious commented Aug 7, 2020

mvines commented Aug 7, 2020

Add a client for BankForks #10728

Add a client for BankForks #10728

Conversation

garious commented Jun 20, 2020 • edited

Problems

Summary of Changes

codecov bot commented Jun 20, 2020 • edited

Codecov Report

garious commented Jun 22, 2020

jstarry commented Jun 23, 2020

jstarry commented Jun 23, 2020

jstarry commented Jul 12, 2020

carllin commented Jul 23, 2020 • edited

CriesofCarrots commented Jul 23, 2020

garious commented Jul 23, 2020

jstarry 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

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

aeyakovenko commented Jul 24, 2020

CriesofCarrots left a comment

Choose a reason for hiding this comment

carllin commented Jul 24, 2020 • edited

garious commented Jul 24, 2020

garious commented Jul 24, 2020

Choose a reason for hiding this comment

jstarry commented Jul 25, 2020

jstarry commented Jul 25, 2020 • edited

garious commented Jul 29, 2020

garious commented Aug 7, 2020

mvines commented Aug 7, 2020

garious commented Jun 20, 2020 •

edited

codecov bot commented Jun 20, 2020 •

edited

carllin commented Jul 23, 2020 •

edited

carllin commented Jul 24, 2020 •

edited

jstarry commented Jul 25, 2020 •

edited