Solana JSON-RPC api #960

CriesofCarrots · 2018-08-13T19:34:45Z

This implements a preliminary JSON-RPC api infra, toward #702
It is currently built as a separate service (à la drone), but could be integrated into FullNode deployment instead.

Right now, the api just emulates the current wallet functionality plus several thin client functions. (0.9.0 JSON RPC Node Service step #1)

It uses a very janky method of pulling a client keypair to generate a transaction, to be rewritten as soon as integration with web-wallet progresses (in-wallet signatures).

(Incidentally, this module exposes a lot of work that needs to be done on the thin client module, including reworking of transfers to accommodate web-wallet signatures, and request timeouts. Will follow up with new issues.)

CriesofCarrots · 2018-08-13T20:45:49Z

Parity JSON-RPC has an active issue to update its hyper crate dependency (paritytech/jsonrpc#298), which should alleviate the cargo audit vulnerability.

garious · 2018-08-13T21:06:39Z

I was thinking this would be implemented within Fullnode and may render the RPU obsolete rather than building on top of it. I'd call it JsonRpcService.

garious

You might find it helpful to run cargo cov on this. I think you'd see a lack of code coverage on cases that'd be terribly boring to write tests for, and that to up that coverage, you might find it easier to remove those cases than to write those tests!

garious · 2018-08-15T14:01:58Z

src/fullnode.rs

@@ -54,6 +55,7 @@ impl Fullnode {
        keypair: Keypair,
        network_entry_for_validator: Option<SocketAddr>,
        sigverify_disabled: bool,
+        json_rpc_enabled: bool,


Like the RPU, I'd prefer this always be enabled. If there's some port conflict, can we resolve that elsewhere?

Done! It wasn't really a port-conflict issue, but a thread blocking issue.

garious · 2018-08-15T14:04:38Z

src/rpc.rs

+    pub request_processor: Option<JsonRpcRequestProcessor>,
+}
+impl Metadata for Meta {}
+impl Default for Meta {


Does some library require we implement Default? If not, it'd be better to get rid of that Option<> and pass the request processor into a Meta constructor. Then you'd be able to get rid of all those unwrap() calls on request_processor.

The version of jsonrpc on crates.io requires Default for Metadata, but that has been fixed in more recent commits. I have changed the dependency to pull from github for now.

garious · 2018-08-15T14:08:21Z

src/rpc.rs

+    fn confirm_transaction(&self, meta: Self::Metadata, id: String) -> Result<bool> {
+        let signature_vec = bs58::decode(id)
+            .into_vec()
+            .expect("base58-encoded public key");


We can't use expect() or unwrap() here because that'd cause this thread to shut down if someone sent us bad input. Looks like it should be Err(Error::invalid_request()). Also, the string is wrong - it's not a public key.

garious · 2018-08-15T14:10:18Z

src/rpc.rs

+    pub trait RpcSol {
+        type Metadata;
+
+        #[rpc(meta, name = "solana_confirmTransaction")]


Pity to have that solana_ prefix everywhere. What happens if we got rid of that?

Nothing bad. I will do that. We might want to use prefixes at some point to differentiate method types, but there's no reason to clutter it up with an meaningless one now.

garious · 2018-08-15T14:14:58Z

src/rpc.rs

+            .expect("base58-encoded public key");
+
+        if signature_vec.len() != mem::size_of::<Signature>() {
+            Err(Error::invalid_request())


Use early returns to handle edge cases, so that the main case don't need to be nested under else. The value-add here is more obvious when there are multiple edge cases, but we should do it here too to keep the codebase consistent.

garious · 2018-08-15T14:19:31Z

src/rpc.rs

+        } else {
+            let signature = Signature::new(&signature_vec);
+            let req = JsonRpcRequest::GetSignature { signature };
+            let resp = meta.request_processor.unwrap().process_request(req);


Calling process_request here forces unnecessary branches below to decode the response. Since we already know this is a "GetSignature" request, I'd expect this code to call a get_signature method that returns a Result<SignatureStatus>. If we're going to do a match on the result, it should only be to convert an internal type to its JSON RPC equivalent. The Some(_) => Err case is the red flag here.

Oh, nice. That's a much better method. Will build it out, thanks!

CriesofCarrots · 2018-08-15T22:56:16Z

@mvines , is there a way to temporarily ignore the cargo audit error for smallvec until the jsonrpc crate is updated?

...I think the large-network test timeout is unrelated to this PR; looks like the tip of master has the same issue.

mvines · 2018-08-15T23:18:20Z

Try this:

--- a/ci/test-stable.sh
+++ b/ci/test-stable.sh
@@ -23,4 +23,4 @@ echo --- ci/localnet-sanity.sh
   USE_INSTALL=1 ci/localnet-sanity.sh
 )
 
-_ ci/audit.sh
+_ ci/audit.sh || true

with an issue filed to revert of course

mvines

Looks great!

mvines · 2018-08-16T02:32:53Z

src/fullnode.rs

@@ -190,7 +191,7 @@ impl Fullnode {
        let tick_duration = None;
        // TODO: To light up PoH, uncomment the following line:
        //let tick_duration = Some(Duration::from_millis(1000));
-
+        // let node_info = node.data.clone();


mvines · 2018-08-16T02:33:46Z

src/rpc.rs

@@ -0,0 +1,278 @@
+//! The `rpc` module implements the Solana rpc interface.


nit: "... Solana RPC interface."

CriesofCarrots · 2018-08-18T18:29:44Z

Woot! #996 allows this to pass the large-network test, so finally landing this.

solana-labs#1116) * Added validator stake account list storage, deprecated old tests * Added join and leave stake pool instructions, error messages refactoring * Stake pool tests refactoring, tests for join and leave pool added * Added validator stake account creation instruction, join/leave pool instructions renamed, version field added * Formatting fixes * Added update list/pool instructions (no tests yet), updated deposit instruction logic, claim instruction removed, refactoring * Updated deposit logic and tests, updated withdraw logic and added tests, refactoring * Stake pool CLI updated to work with new deposit/withdraw instructions, claim usage removed * Added validator stake account management and balance update commands to the stake pool CLI, updated dependency versions, updated devnet program address * Merge conflicts fixed * Removed deprecated tests * Fixes for review comments * Additional program id checks across the code * Formatting errors fixed * Changed minimum stake balance in CLI, removed deprecated tests, removed check for stake history id * Added TODO for stake account warmup status check * Cargo.lock conflict fix * Formatting fixed * Update Cargo lock file for CI * Pin themis version of subtle Co-authored-by: Yuriy Savchenko <yuriy.savchenko@gmail.com>

CriesofCarrots requested review from garious and mvines August 13, 2018 19:34

CriesofCarrots added the work in progress This isn't quite right yet label Aug 14, 2018

CriesofCarrots force-pushed the rpc-api branch from 81f1850 to 7d4b21b Compare August 15, 2018 00:38

CriesofCarrots removed the work in progress This isn't quite right yet label Aug 15, 2018

CriesofCarrots force-pushed the rpc-api branch from 7d4b21b to 6ae25c4 Compare August 15, 2018 00:46

garious reviewed Aug 15, 2018

View reviewed changes

CriesofCarrots force-pushed the rpc-api branch 4 times, most recently from bdf7d6a to 0f68c84 Compare August 15, 2018 19:06

garious approved these changes Aug 15, 2018

View reviewed changes

CriesofCarrots force-pushed the rpc-api branch from 6d6fea7 to f632b45 Compare August 15, 2018 22:04

mvines approved these changes Aug 16, 2018

View reviewed changes

CriesofCarrots force-pushed the rpc-api branch 9 times, most recently from 0396807 to 4daa494 Compare August 17, 2018 21:47

CriesofCarrots added 3 commits August 18, 2018 12:20

Implement json-rpc functionality

6bdec79

Rpc tests

e9d7aaf

Rpc get last id endpoint

3f7e67c

CriesofCarrots added 10 commits August 18, 2018 12:20

Update jsonrpc dependency syntax

91452c6

Implement Rpc in Fullnode

0a79115

Disable Rpc module for other tests to prevent port conflicts

ab655b2

Use jsonrpc git repo, allowing removal of Default bound for Metadata

36dc016

Handle potential panics

eb89b1a

Remove unnecessary solana_ prefixes

925b34f

Fix early return for invalid parameter

09c308e

Simplify Rpc request processing

86b1c58

Revert ill-advised jsonrpc marker, and handle jsonrpc server close

cef1962

Temporarily disable cargo audit CI failure

d755f01

CriesofCarrots force-pushed the rpc-api branch from 4daa494 to d755f01 Compare August 18, 2018 18:20

CriesofCarrots merged commit 1bf15ae into solana-labs:master Aug 18, 2018

mvines mentioned this pull request Aug 18, 2018

testnet-sanity panic: solana-jsonrpchttp.worker3 panic: Address already in use #1005

Closed

jeffwashington added a commit to jeffwashington/solana that referenced this pull request Apr 22, 2024

modify geyser account iter at snapshot load (solana-labs#960)

862c79e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solana JSON-RPC api #960

Solana JSON-RPC api #960

CriesofCarrots commented Aug 13, 2018

CriesofCarrots commented Aug 13, 2018

garious commented Aug 13, 2018

garious left a comment

garious Aug 15, 2018

CriesofCarrots Aug 15, 2018

garious Aug 15, 2018

CriesofCarrots Aug 15, 2018

garious Aug 15, 2018

garious Aug 15, 2018

CriesofCarrots Aug 15, 2018

garious Aug 15, 2018

garious Aug 15, 2018

CriesofCarrots Aug 15, 2018

CriesofCarrots commented Aug 15, 2018 •

edited

Loading

mvines commented Aug 15, 2018

mvines left a comment

mvines Aug 16, 2018

mvines Aug 16, 2018

CriesofCarrots commented Aug 18, 2018

		@@ -0,0 +1,278 @@
		//! The `rpc` module implements the Solana rpc interface.

Solana JSON-RPC api #960

Solana JSON-RPC api #960

Conversation

CriesofCarrots commented Aug 13, 2018

CriesofCarrots commented Aug 13, 2018

garious commented Aug 13, 2018

garious 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

Choose a reason for hiding this comment

CriesofCarrots commented Aug 15, 2018 • edited Loading

mvines commented Aug 15, 2018

mvines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CriesofCarrots commented Aug 18, 2018

CriesofCarrots commented Aug 15, 2018 •

edited

Loading