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

Use jsonrpsee as the RPC server impl #1602

Merged
merged 6 commits into from
Feb 12, 2022

Conversation

ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented Feb 1, 2022

While I still need to do some research around applying constraints outlined in https://github.com/AleoHQ/snarkOS/issues/1597, I can already provide some insight into how the jsonrpsee-powered implementation is going to look like. The decision to finally make the switch was motivated by the crate no longer warning about its instability and the required functionalities becoming too advanced for a hand-written implementation (which was always only intended to be temporary in the first place).

This PR closes https://github.com/AleoHQ/snarkOS/issues/754 and likely https://github.com/AleoHQ/snarkOS/issues/1597 as well, though that could also be applied as a follow-up.

@ljedrz ljedrz added dependencies Pull requests that update a dependency file RPC RPC-related issues labels Feb 1, 2022
src/rpc/tests/mod.rs Outdated Show resolved Hide resolved
src/rpc/tests/mod.rs Outdated Show resolved Hide resolved
src/rpc/tests/mod.rs Outdated Show resolved Hide resolved
src/rpc/tests/mod.rs Outdated Show resolved Hide resolved
@ljedrz ljedrz added the feature New feature or request label Feb 1, 2022
@niklasad1
Copy link

Hey, you shouldn't enable serde-json/arbitrary-precision when using jsonrpsee, see paritytech/jsonrpsee#480.

You will likely run into weird errors for instance when the request id is a number.

src/rpc/context.rs Outdated Show resolved Hide resolved
@ljedrz
Copy link
Collaborator Author

ljedrz commented Feb 1, 2022

Hey, you shouldn't enable serde-json/arbitrary-precision when using jsonrpsee, see paritytech/jsonrpsee#480.

You will likely run into weird errors for instance when the request id is a number.

I don't even know why we need that one, everything seems to compile just fine without it; @howardwu?

@ljedrz
Copy link
Collaborator Author

ljedrz commented Feb 1, 2022

Ok, I now know where that feature might be used:

thread 'rpc::tests::test_get_block_template' panicked at 'called `Result::unwrap()` on an `Err` value: Error("u128 is not supported", line: 0, column: 0)', src/rpc/rpc_impl.rs:165:12

@ljedrz
Copy link
Collaborator Author

ljedrz commented Feb 1, 2022

Ok, I think the PR is ready for reviews; for now:

  • the temporary JSON-RPC server is replaced with jsonrpsee
  • a basic, quite conservative limit of 10 concurrent requests is introduced, with each request just counting as 1
  • the serde-json/arbitrary-precision feature is an open point; to remove it, we could just not serve cumulative_weight or send it as a hex-encoded String (I implemented the latter for now)

@ljedrz ljedrz marked this pull request as ready for review February 1, 2022 17:20
Cargo.toml Outdated Show resolved Hide resolved
src/rpc/context.rs Outdated Show resolved Hide resolved
src/rpc/tests/mod.rs Outdated Show resolved Hide resolved
@ljedrz
Copy link
Collaborator Author

ljedrz commented Feb 2, 2022

Some jobs seem to be failing for unrelated reasons:

W: Failed to fetch http://archive.ubuntu.com/ubuntu/dists/focal/InRelease  Could not connect to archive.ubuntu.com:80 (91.189.88.152), connection timed out Could not connect to archive.ubuntu.com:80 (91.189.88.142), connection timed out

Copy link
Collaborator

@niklaslong niklaslong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, looks good other than that u128 panic in the tests.

@ljedrz
Copy link
Collaborator Author

ljedrz commented Feb 2, 2022

Left some comments, looks good other than that u128 panic in the tests.

I already proposed the hex-encoding solution in ProvableHQ@fb1ec16.

@niklaslong
Copy link
Collaborator

niklaslong commented Feb 2, 2022

I already proposed the hex-encoding solution in fb1ec16

My bad, didn't update my local branch after your recent push 👍

@dvdplm
Copy link

dvdplm commented Feb 2, 2022

Happy to see this happen, we (Parity) are fans of Aleo and would love to see you as users. Let us know if we can do anything to help. :)

Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
@howardwu
Copy link
Contributor

howardwu commented Feb 12, 2022

LGTM.

Due to the number of systems depending on the current format for cumulative_weight (as a u128 rather than a hex-string), I've included arbitrary-precision to prevent breakages. Note that existing systems already pass in the id as a string for RPC, so the numeric id shouldn't be an issue. (Our documentation also reflects a string id in its examples)

We should investigate a real solution to displaying a u128 without breakage of the id field, as the cumulative_weight is likely the most important value returned from getnodestate (it defines how heavy the canonical chain is, which is read by the explorer, miners, and mining pool operators)

@howardwu howardwu merged commit d5b4703 into AleoNet:testnet2 Feb 12, 2022
@ljedrz ljedrz deleted the testnet2_jsonrpsee branch February 12, 2022 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file feature New feature or request RPC RPC-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change the JSON-RPC crate when the successor is available
5 participants