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

cln-rpc: Oxidizing c-lightning #5010

Merged
merged 10 commits into from
Feb 11, 2022
Merged

cln-rpc: Oxidizing c-lightning #5010

merged 10 commits into from
Feb 11, 2022

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Jan 18, 2022

cln-rpc is the first step towards adding rust as a language to c-lightning. It consists of the following steps:

  • Add msggen to generate a variety of messages from the JSON-RPC schemas, and maybe soon from the wire CSV files.
  • Generate structs for the rust side that match requests and responses for the JSON-RPC. These just serve as containers for the data sent to and returned by the JSON-RPC. They are as strongly typed as the schemas would allow at the moment, but further constraints and types could be introduced later.
  • Conditionally compile rust targets when we detect RUST=1 (whether cargo is in the path or not)
  • Implement the codec for encoding and decoding of the JSON-RPC to and from the Rust structs

msggen is currently based off of the JSON-RPC schemas, but the schemas are lacking some of the higher level concepts such as methods (binding request and response together) or services (binding multiple methods into a grouping). We could eventually create a new root document and make the JSON-RPC schemas derived from that. This would have the advantage of reducing the very verbose JSON-schemas, and being a superset of the information we need for various formats (grpc field identities for example which must not change despite schema evolutions). However I decided against bikeshedding that format for now, and will revisit it in the future.

We are still missing quite a few request schemas, which is why not all methods are mapped at the moment. It's gruntwork that I'll add in a separate PR.

@cdecker cdecker added this to the v0.11 milestone Jan 18, 2022
@cdecker cdecker force-pushed the cln-rpc branch 3 times, most recently from 4c972f6 to c939b18 Compare January 19, 2022 10:45
cln-rpc/src/primitives.rs Outdated Show resolved Hide resolved
@cdecker cdecker marked this pull request as ready for review January 27, 2022 17:20
@cdecker
Copy link
Member Author

cdecker commented Jan 27, 2022

Ok, this should be ready to start reviewing, the discussion started with @elsirion is definitely one point I'd like to dig in. The Amount type needs to also get All and Any variants, which have no nice mapping to an msatoshi amount.

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM, but I have a small comment in some autogen code.

In addition, I'm curious about the main motivation that you choose to use tokio as dependencies, I mean why not just a synchronous library?

@cdecker
Copy link
Member Author

cdecker commented Jan 28, 2022

Thanks for the review @vincenzopalazzo. I chose tonic primarily to minimize the memory footprint, since threads each get their own stack preallocated, and may have considerable overhead. Since we are likely much more IO bound than CPU bound, it seems to me like a good fit. Also it's what we're using in greenlight which is where I'm backporting this from 😉

@cdecker cdecker force-pushed the cln-rpc branch 3 times, most recently from 7e45678 to 37fd5c7 Compare January 30, 2022 15:39
@cdecker
Copy link
Member Author

cdecker commented Jan 30, 2022

Following the discussion with @elsirion I went and simplified the Amount type and introduced two new types AmountOrAll and AmountOrAny (I didn't go for a generic type since I couldn't see how it'd be useful for other types at all).

I don't think we need to differentiate off-chain and on-chain amounts for now, but they'd be easy to introduce later on.

The range-diff can be found here: https://bot.bitcoinstats.com/ElementsProject/lightning/pull/5010/range_diff/7e456784ed2fac1871340e60e211f331b461eae6..37fd5c7b9a11fc6e5f0758520ee8feed34b3d8b4

 1:  fec9f68cd =  1:  f72fe4615 pyln: Delete psql DBs after testing
 2:  0f25011c3 =  2:  66a40cb8d --- START OF cln-rpc
 3:  cca9b8082 =  3:  636b47996 msggen: Parse JSON-RPC schemas and build the in-memory model
 4:  ba9ffb0a5 =  4:  46a8c3730 msggen: Generate the cln-rpc Rust structs
 5:  466395d3f =  5:  affe8f652 json-rpc: Add request stubs for a couple of calls
 6:  eb7a5d702 =  6:  e25eec08c rust: Add rust detection to configure and a target to add binaries
 7:  afb9d3bf9 =  7:  c06234da0 cln-rpc: Scaffolding for the cln-rpc crate
 -:  --------- >  8:  3bfb36608 fixup! cln-rpc: Scaffolding for the cln-rpc crate
 8:  6b2acc85a =  9:  8ebb0a9b9 pytest: Test the rust bindings from cln-rpc
 9:  246ddd946 = 10:  f88e4cf10 gci: Add rust configuration to Github actions
10:  7e456784e = 11:  42a4a6ddf gci: Limit the RUST=1 config to test rust-related functionality
 -:  --------- > 12:  c7b805038 fixup! cln-rpc: Scaffolding for the cln-rpc crate
 -:  --------- > 13:  c27ba39c6 fixup! 3bfb36608c5159a7ad5f1afeb5c1e0a1ec955fff
 -:  --------- > 14:  7c752aceb fixup! cln-rpc: Scaffolding for the cln-rpc crate
 -:  --------- > 15:  37fd5c7b9 cln-rpc: Add type for AmountOrAll and AmountOrAny

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 4c12c50

Maybe add the Changelog-Add: .... can be a good information to have inside the changelog?

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack 3003a01

configure Outdated Show resolved Hide resolved
We build an in-memory model of what the API should look like, which
will later be used to generate a variety of bindings. In this PR we
will use the model to build structs corresponding to the requests and
responses for the various methods.

The JSON-RPC schemas serve as ground-truth, however they are missing a
bit of context: methods, and the request-response matching (as well as
a higher level grouping we'll call a Service). I'm tempted to create a
new document that describes this behavior and we could even generate
the rather repetitive JSON schemas from that document. Furthermore
it'd allow us to add some required metadata such as grpc field
numbering once we generate those bindings.

Changelog-Added: JSON-RPC: A new `msggen` library allows easy generation of language bindings for the JSON-RPC from the JSON schemas
We're generating these structs so we can parse them directly into
native objects.
These are required so we can generate the requests, not just the
responses. We'll add more as we test compatibility with our generation
code.
We detect whether we have the rust tooling available (mainly `cargo`)
and enable or disable the rust libraries, plugins and examples when it
is enabled. Since the rest of the Makefiles assumes that executables
have an associated header and C source file, we also needed to add a
target that we can add non-C binaries to.
Changelog-Added: cln-rpc: A new Rust library called `cln-rpc` can be used to interact with the JSON-RPC
@cdecker
Copy link
Member Author

cdecker commented Feb 9, 2022

Re-applying Rusty's ACK after fixing the redundant check in configure (range-diff and adding Changelog Entries.

ACK f48f898

@cdecker cdecker merged commit 6d256fd into master Feb 11, 2022
@rustyrussell rustyrussell deleted the cln-rpc branch August 15, 2022 00:42
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.

4 participants