gRPC support #3159

cjcobb23 · 2019-11-15T22:42:24Z

This PR is to add gRPC support to rippled. In this initial iteration, 4 API's are implemented: fee, account_info, tx and submit. tx only supports Payment transactions at this time, but will be expanded on in a future iteration.

Design document can be found here: https://github.com/cjcobb23/grpcRippledDesign/blob/master/design/design.md
Requirements document can be found here: https://github.com/cjcobb23/grpcRippledDesign/blob/master/requirements/requirements.md
Full RPC documentation will be provided in a later PR.

Please note, there are issues with Travis CI currently. We are working on them and will get them resolved before merging. This will most likely result in significant changes to the cmake files. However, we would like to get eyes on the code in the meantime. For now, to build the code: clone the repo, checkout the branch, and run:
git submodule update --init --recursive.

Reach out to me if you have any issues.

MarkusTeufelberger

Didn't look at the C++ code, only mainly at the proto files.

I hope I don't come across as too negative, I'm actually happy that rippled will get a gRPC API. I'm just a bit concerned about how it'll play out and some details. In general I like the concept.

MarkusTeufelberger · 2019-11-16T07:57:19Z

.gitmodules

@@ -0,0 +1,3 @@
+[submodule "grpc"]


Why a submodule, not a subtree as every other vendored dependency in this repository?

good point - we'll be fixing that shortly (to make grpc consistent with our other third party deps).

MarkusTeufelberger · 2019-11-16T07:57:52Z

Builds/CMake/RippledCore.cmake

@@ -134,6 +134,8 @@ target_include_directories (xrpl_core
  PUBLIC
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src/ripple>
+    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/grpc>
+    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/proto>


Why a top level subfolder, not a folder in /src?

you are right, I think the proto files should be moved to /src/ripple/proto/rpc/v1. Also, grpc will be removed as a submodule most likely

moved to src/ripple/proto/rpc/v1

MarkusTeufelberger · 2019-11-16T08:00:04Z

Builds/CMake/RippledCore.cmake

 exclude_if_included (rippled)
 # define a macro for tests that might need to
 # be exluded or run differently in CI environment
 if (is_ci)
  target_compile_definitions(rippled PRIVATE RIPPLED_RUNNING_IN_CI)
 endif ()
+
+if(NOT WIN32)
+set_source_files_properties(src/ripple/unity/app_main1.cpp PROPERTIES COMPILE_FLAGS -Wno-suggest-override)


Would be nice to know why -Wno-suggest-override is enabled everywhere here. Maybe in a comment?

This is because when grpc was included as a submodule, building grpc generated a lot of warnings (all suggest-override warnings). However, since grpc will most likely be removed as a submodule (and included in a different way), suppressing these warnings will not be necessary

MarkusTeufelberger · 2019-11-16T08:12:24Z

proto/rpc/v1/account_info.proto

+// A request to get info about an account.
+message GetAccountInfoRequest {
+    // The address to get info about.
+    string address = 1;


This is called "account" in https://xrpl.org/account_info.html and described as "most commonly", not "always" being the address.

Will change to "account"

MarkusTeufelberger · 2019-11-16T08:13:57Z

proto/rpc/v1/account_info.proto

+    bool strict = 2;
+
+    oneof ledger_index {
+        string ledger_index_shortcut_string = 3;


This can only be one of "validated", "closed" or "current", might be worth a separate type instead of a full string.

Good idea. I could just use an enum

It's not obvious that a "shortcut_string" is "validated" or "closed". I know that this is going to be changed to an enum, but I have two pieces of feedback:

We should document every fields in these proto buf files, including strings encode data

I would not add a type suffix to field names. I.e. no _string here.

changed to an enum

MarkusTeufelberger · 2019-11-16T15:53:21Z

proto/rpc/v1/transaction.proto

+}
+
+message Payment {
+    // The amount of currency to pay, in either fiat or XRP.


Suggested change

// The amount of currency to pay, in either fiat or XRP.

// The amount of currency to pay, in either issued currency or XRP.

MarkusTeufelberger · 2019-11-16T15:54:06Z

proto/rpc/v1/transaction.proto

+    string issuer = 3;
+}
+
+message Payment {


Tons more fields on this transaction type, it's not the easiest one for a start. ;-)

I believe (and CJ can correct me) that this is at least the basics we would need to start building XRP transactions here. We just need to make sure what we have is extensible and doesn't cause breaking changes in the future.

I'm just going to add the missing fields. SendMax and Paths will be moved from the Transaction type to here. I see that DestinationTag, InvoiceID and DeliverMin are missing. Any others?

added fields, lmk if i missed any (non-deprecated) fields

Are these fields getting added?

The fields to be added were added. If there are fields missing, please point them out. Note though, we are only supporting Payment transactions at this time, so some fields necessary for other transaction types were not added.

Fields missing; TransactionType, AccountTxnID. But everything else looks good. Transaction, TX and meta. You're returning all the necessary information via messages.

TransactionType is omitted, since the type is defined via protobuf; each transaction type has it's own unique protobuf message type.
You are right that AccountTxnID is missing from the Transaction message. I added it to the AccountRoot ledger object message, but missed it in the Transaction message. I'll add that now; thanks for pointing it out.

Oh cool. So I'm looking to add Escrow next. Seems I would follow the message format of the Payment protobuf? I can help out as my use case is wrapping up and Escrow is next.

@dangell7 I added AccountTxnID (naming is a bit different though).

As to adding Escrow: I assume you mean adding protobuf messages for EscrowCreate, EscrowFinish and EscrowCancel. You are free to do so and create a PR, and yes, follow the same format as the Payment message, but you are going to have to also implement that functionality on the C++ side as well, in addition to adding the protobuf messages. Also, you would need to also add support (in protobuf and C++) for all ledger objects that those Escrow transaction types affect, in order to return complete meta for those Escrow transactions.

MarkusTeufelberger · 2019-11-16T15:56:38Z

proto/rpc/v1/tx.proto

+    bytes hash = 1;
+
+    // if true, return data in binary format
+    // TODO: does this make sense for protobuf?


...maaaybe? A client could always just encode the result themselves, but it might be nice to know what the server is actually working with.

@keefertaylor what do you think about this? Is there any benefit to returning this data in binary format? protobuf is already a binary format over the wire and is heavily optimized, so I don't think it makes sense to provide a binary option for efficiency reasons. But maybe there are other reasons a client would want the data in binary form.

As far as I can tell I don't think there is.

It would be easiest to leave this off for now, and if we find a use case we really must have later we can always add it back in (conversely, it's harder to remove stuff).

A simple use case would be that this is the bulk of the data one needs to verify hashes. If data only arrives in some deserialized format, one first needs tom implement a serializer to verify incoming data.

I kept this and removed the comment

MarkusTeufelberger · 2019-11-16T15:58:43Z

proto/rpc/v1/tx.proto

+        bytes tx_bytes = 2;
+    };
+    // Sequence number of ledger that contains this transaction
+    uint64 ledger_index = 3;


Usually you used uint32 for this one elsewhere so far.

Good catch. I am wondering whether it makes sense to define a separate type for this, so that way one does not have to remember that ledger sequence numbers are uint32

changed to uint32

MarkusTeufelberger · 2019-11-16T16:01:59Z

proto/rpc/v1/xrp_ledger.proto

+
+
+// RPCs available to interact with the XRP Ledger.
+service XRPLedgerAPIService {


No error handling so far, I guess that'll come later?

Could you elaborate on what you'd expect to see here?

Obviously, gRPC supports plain error codes and descriptions. There's the advanced error model, which isn't really standard (yet): https://cloud.google.com/apis/design/errors#error_model.

If we really needed to encode error details, we could always roll them into the response metadata?

Or are you asking for their to be a generic ResponseMessage, that looks like:

message Response { oneof { google.protobuf.Any response = 1 google.rpc.Status error = 2; } } message RippledError { // ... }

Error handling is built into grpc in a way. I'm using these status codes, along with string messages: https://github.com/grpc/grpc/blob/master/doc/statuscodes.md
If you look at the c++ handler code, you can see how I'm handling errors. For instance, if one passes a non-existent account to the GetAccountInfo RPC, I return status code NOT_FOUND, with message "account not found". gRPC will return this status code and message instead of the defined response type.

@MarkusTeufelberger lmk what you think here or just resolve this

This is a comment, it can't be "resolved" on GitHub. Also I'm not in the Ripple team, so I also don't have some moderation tools available that project owners might have.

If the response to errors looks like in JSONRPC I guess it's fine.

keefertaylor

Only really looked at the .proto files.

keefertaylor · 2019-11-18T22:48:30Z

proto/rpc/v1/account_info.proto

+import "rpc/v1/ledger_objects.proto";
+
+// A request to get info about an account.
+message GetAccountInfoRequest {


Just FWIW, Google's naming conventions also recommend the Get prefix (https://cloud.google.com/apis/design/naming_convention#method_names). Uber's prototool also seems to implicitly support this as well: https://github.com/uber/prototool/tree/dev/style#servicesrpcs.

Just because other folks are using get doesn't mean we have to. I do think that (AFAIK) these all are pretty widely used style guides which makes it somewhat idiomatic to folks who work with gRPC.

keefertaylor · 2019-11-18T22:49:41Z

proto/rpc/v1/amount.proto

+
+package rpc.v1;
+
+message CurrencyAmount {


This is a bad abstraction which is my fault. Supportive of moving it to be a single field called IssuedCurrencyAmount.

keefertaylor · 2019-11-18T22:51:48Z

proto/rpc/v1/amount.proto

+// A representation of an amount of XRP.
+message XRPAmount {
+    // A numeric string representing the number of drops of XRP.
+    uint64 drops = 1;


I think the important thing is that this is a typed number (rather than an untyped uint64 or untyped string). That way developers have to consciously spell out the word drops rather than guessing at the unit. I suppose we could remove this message and have each field be postfixed with drops?

Regardless, XRPDrops sounds reasonable to me. WDYT?

keefertaylor · 2019-11-18T22:54:36Z

proto/rpc/v1/amount.proto

+    string value = 2;
+
+    // Unique account address of the entity issuing the currency.
+    string issuer = 3;


I think this matches up with the pattern we are using with XRPAmount, and does make it safer for developers to understand what's going on. Happy to hear thoughts from others.

keefertaylor · 2019-11-18T22:55:06Z

proto/rpc/v1/fee.proto

+
+}
+
+//Response to a GetFee RPC


nit: add a space after //

added space

keefertaylor · 2019-11-18T23:09:10Z

proto/rpc/v1/transaction.proto

+
+    uint32 last_ledger_sequence = 8;
+
+    repeated Path paths = 9;


+1, I think this lives in payments.

keefertaylor · 2019-11-18T23:10:23Z

proto/rpc/v1/transaction.proto

+    string issuer = 3;
+}
+
+message Payment {


I believe (and CJ can correct me) that this is at least the basics we would need to start building XRP transactions here. We just need to make sure what we have is extensible and doesn't cause breaking changes in the future.

keefertaylor · 2019-11-18T23:11:54Z

proto/rpc/v1/tx.proto

+message TxResponse {
+
+    // The actual transaction
+    oneof sttx {


Not sure what sttx means. Would recommend dropping the abbreviation.

It's a naming convention I copied from rippled (think it means Serialized Type Tx), though I agree that it is very cryptic. What do you think is a good name for the one of? I will probably expand tx and tx_bytes to transaction and transaction_bytes btw.

I could call it serialized_transaction

changed to serialized_transaction

keefertaylor · 2019-11-18T23:12:14Z

proto/rpc/v1/tx.proto

+    bool validated = 5;
+
+    // metadata about the transaction
+    oneof stmeta {


What do you think is a good name for this? I could call it serialized_meta.

changed to serialized_meta

keefertaylor · 2019-11-18T23:17:15Z

proto/rpc/v1/xrp_ledger.proto

+
+
+// RPCs available to interact with the XRP Ledger.
+service XRPLedgerAPIService {


Could you elaborate on what you'd expect to see here?

Obviously, gRPC supports plain error codes and descriptions. There's the advanced error model, which isn't really standard (yet): https://cloud.google.com/apis/design/errors#error_model.

If we really needed to encode error details, we could always roll them into the response metadata?

Or are you asking for their to be a generic ResponseMessage, that looks like:

message Response { oneof { google.protobuf.Any response = 1 google.rpc.Status error = 2; } } message RippledError { // ... }

seelabs · 2019-11-19T05:53:00Z

Great job on this @cjcobb23! You got up to speed very quickly and submitted an ambitions addition to rippled. I'll have detailed feedback later, but I wanted to give some initial feedback high-level feedback:

As others have pointed out, fields like send_max and paths are payment specific, and not part of all transactions. There are also common fields like SourceTag that are important to support. The documentation on the common transaction fields is here: https://xrpl.org/transaction-common-fields.html. If we're going to initially support the payment transaction, I'd add all the fields that a payment transaction can support. The documentation for that is here: https://xrpl.org/payment.html. I don't think it makes sense to release this API without supporting all the options available for payments, but we decide not to, we at least need to support DestinationTag.
I have to think about the "string" based interface proposed here. As written, many binary values are encoded strings (addresses, amounts, keys, for example). Since protocol buffers are a binary format I would have assumed these would be binary encoded and making these binary would simplify the handlers in rippled. I do understand this would require clients to implement encoders and decoders for base58 and hex at least. Is that the trade-off or is there another reason for the string-based interface?
This patch made compile time shoot up 50% on my system (5m 40s vs 3m 38s on a debug build). We'll have to address that.
We don't usually merge partial functionality into rippled. I'd have to think about merging an API that just supported the payment call only. I'd vote to wait until a much larger set of API calls were supported before this was merged into rippled.
I'd recommend organizing the commits before you submit a pull-request. Before submitting, I usually either squash it down to one commit, or prepare a set of commits that can be logically reviewed as a unit. As it is, the 61 commits on this PR don't really help reviewers.

cjcobb23 · 2019-11-19T22:00:38Z

@seelabs About the strings interface: there is no difference between strings and bytes over the wire; both are encoded the same way and use the same amount of space. When using strings, protobuf enforces the invariant that each character is UTF-8. Since there is no difference in encoding, I decided that strings were a better type for data that is meant to be human readable. The string type signals that the data is meant to be readable, and the UTF-8 invariant enforces that the data actually is readable. Btw, with the C++ api, bytes and strings are both represented as std::string. In the API for other languages however, strings are represented differently and it is (supposedly) more convenient to work with strings than bytes when the data is meant to be readable (see here: https://groups.google.com/forum/#!topic/protobuf/au6eQBkRT5s ) @keefertaylor what do you think about this?

I'll move send_max and paths to the Payment type, and also add support for all the options available for Payments. It shouldn't be too difficult.

I agree that merging partial functionality is less than ideal; however, providing full support for the Tx command would require expanding on the proto files quite a bit, as well as writing a lot more handler code. @keefertaylor is going to use this to develop Xpring SDK's, and doesn't need the full Tx functionality (correct me if I'm wrong here keefer), so in the interest of time, I just implemented what is necessary for Payments.

About the commits, I do see the benefits of organizing the commits better, though I'm not sure that squashing something this big into one commit makes sense. I could squash this into several commits (say 10 commits as opposed to 61), though doing that now would require me to force-push to this branch. Should I reorganize the commits and force-push, or just leave as is, and remember to better organize the commits for future PRs?

I appreciate the praise. @pwang200 and @p2peer deserve some credit as well, as they helped write the unit tests and work out build issues.

Edit: I actually am going to take back what I said about squashing into several commits as opposed to 1. I think it will be difficult to create a handful of sensible commits out of these 61 commits, and its much simpler and less error prone just to squash everything into one commit. Plus, I have some trailing white space, which I could get rid of during the squashing.

seelabs · 2019-11-19T23:52:55Z

@cjcobb23 @keefertaylor

Re: Binary Interface

When I say "string" based I mean this API encodes binary data as ASCII strings, instead of binary data directly. I.e with a hex encoding the 8-bit number 255 is not encoded as one byte, but by the two ASCII characters 'f' and 'f'. Protocol buffers support binary data, why are we encoding it?

This is an API to interface between the rippled server and other programming languages. The client may take encoded data from a human and decode it to binary before talking to rippled, and it may encode binary data from rippled before presenting it to a human, but why should the two programming languages talk to each other with encoded data?

I do understand this requires encoders and decoders in the client (base58 and hex at least), but it also simplifies the handlers in rippled. I'd much rather have a binary interface to rippled and then build encoded APIs on top of that than the other way around. (As an aside, I understand the rippled json interface requires these codecs, but I'm hoping we can eventually move that outside of rippled and use this new binary interface instead).

Unless I'm missing a reason why the interface is using strings, I'd prefer a binary interface.

Re: Implementing a subset of functionality

I understand that a fuller API will require more time. I'm OK supporting the small subset of functionality needed by xspring, but with a couple conditions:

This has to be the start of a new API for rippled that supports everything we do with the JSON API now. It's hard to justify the cost of integrating gRPC into rippled to support the small set of functionality in this PR.
The API has to be marked as unstable/experimental, and the API must be able to change until we mark it as stable. I expect this will ship in 1.5 as "unstable/experimental" and then 1.6 will have a fuller API with possible breakage. I want to remove the unstable API after a very short transition (maybe one version). xspring will have to update their back end when we break the API. If we're committing to a backward compatible API now or supporting an "experimental" API for several releases, then we want to do much, much more upfront design of the API. If a backward compatible API is needed, we should think about having xspring use the JSON API until the new one is ready, and I doubt it would be ready for 1.5.

keefertaylor · 2019-11-20T02:31:11Z

Re, Strings vs Bytes:
Certainly agree that we're doubling the API response size for these fields across the wire. In general though, the data that we're talking about is small (20 bytes vs 40 bytes of ASCII for an account).

In general, I would optimize a public API for useability rather than raw efficiency. For instance, when you return the bytes for an account, are you going to return the raw (unprefixed, unchecksummed) bytes? Or will you add the prefix and checksum and return that as bytes? If the former, now the client has to understand Base58Check encoding and the specific inputs ripple uses, if the latter, then the client still probably has to go read that documentation anyway, then run the base58check encoding. Then, you have to have a Base58Check encoding library in your language and to wire it up. In general, I'd favor centralizing this business logic wherever possible (it's easier for rippled to implement this once than for every developer to implement it).

Put succinctly, is saving 20 bytes over the wire a valuable tradeoff for every developer who touches this API needing to go read the documentation to go understand what they have to do to get to an address string, which is what they wanted anyway.

As additional data: We're already returning these fields as ASCII in the JSON API - are folks actively complaining about the data or RPC size?

seelabs · 2019-11-20T05:40:18Z

@keefertaylor I'm not worried about the size of the data on the wire at all. I'm also not arguing that a "string" based API that encodes and decodes from our various formats isn't needed - it absolutely is. Here's where I'm coming from: I've thought about adding a low-level API to rippled, and moving the high-level APIs to a separate server outside of rippled. I was trying to steer this gRPC API into the low-level API that I want.

However, I agree: without a high-level gRPC API in place, it's not realistic to force clients to do base58 encoding and decoding themselves. Address at least should be base58 encoded and not raw binary.

I'm not sure I agree about hex encoded data though. Why are we encoding hex data instead of sending raw bytes?

I also want to give an example why I want to eventually move the encoding out of rippled. Take a look at the code to decode secrets seeds: First it checks if it's been encoded as a ripple lib seed, because that used a non-standard encoding. Next, it checks if it's encoded as a base58 account, node public key, node private key, or account secret, in case someone accidentally sends those instead of a secret. Next it checks if it's encoded as hex. Next it checks if it's encoded as a base 58 seed. Moving this complexity out of rippled is a win. I understand it needs to live somewhere, but I'd like to make rippled itself as stable and secure as possible.

keefertaylor · 2019-11-20T05:44:21Z

proto/rpc/v1/amount.proto

+
+// A representation of an amount of XRP.
+message XRPAmount {
+    // A numeric string representing the number of drops of XRP.


Comment out of date

keefertaylor · 2019-11-20T05:53:04Z

Cool. I think we've reached agreement on string making sense for addresses.

As far as other data, I think I generally agree with you too! I was narrowed in on addresses. I audited the code just now and it appears to (to me) that the only places strings appear in proto files are:

As addresses / accountIDs across a slew of messages - it sounds like we think this is valid
As a currency identifier
As an engine_result and engine_result_message (there's an open discussion to move the former to an enum
ledger_entry_type in AffectedNode
transaction_result in Meta (also has an open discussion about moving to an enum)
As a value in an IssuedCurrency
As a ledger_index_shortcut_string in GetAccountInfoRequest (also has an open discussion about moving to an enum)

I think all of those (modulo the enum changes are valid use cases for strings). Let me know if there is a place I am missing.

Apologies if you were looking at some of the early prototype files I was using in Xpring. I was building an MVP super quickly and I think CJ has really done a great job at refactoring away my hastily made and incorrect abstractions.

Edit: tagging @seelabs to make sure this comment gets visibility.

seelabs · 2019-11-20T13:29:08Z

@keefertaylor I was looking at the bytes signing_public_key_hex field: https://github.com/cjcobb23/rippled/blob/grpcSupport/proto/rpc/v1/transaction.proto#L24-L25. Is there a reason that's hex encoded?

keefertaylor · 2019-11-20T16:22:58Z

@seelabs That field is actually of type bytes, though it's certainly incorrectly named. I flagged the naming comment in the review.

keefertaylor · 2019-11-20T16:23:16Z

proto/rpc/v1/transaction.proto

+    }
+
+    // The public key of the account which signed the transaction in hexadecimal.
+    bytes signing_public_key_hex = 5;


cjcobb23 · 2019-11-20T18:21:05Z

@seelabs signing_public_key_hex is just named wrong. It is of the bytes type, and in the c++ code, I send the raw bytes and don't encode to hex: https://github.com/cjcobb23/rippled/blob/grpcSupport/src/ripple/rpc/impl/RPCHelpers.cpp#L1252
I did not intend to encode any data as a hex string in this API, since converting to and from hex is rather simple; if I am sending or receiving hex strings, it is a mistake on my part and should be fixed.

seelabs

I did a pass mostly focusing on the GRPCServer files. I'll do some more review later, but I think I have enough in this batch that I'd like post them.

There are a couple of formatting nits we need to fix (I didn't flag these individually):

There are several lines that end with extra whitespace, we need to remove the whitespace
The public: and private: keywords aren't indented like the rest of rippled's code
We put a space after commas in function arguments

Instead of auditing the code and fixing those, I'd just set up clang-format and use the .clang-format file in the repo to reformat the new code.

There are also a couple of style nits I'd like to fix:

There are places where the code uses snake_case for variables and functions. We use pascalCase.
I wouldn't add comments that don't add value, they get in the way of code. It's not a big deal, and there are not that many in this patchset, but I usually flag comments I don't think add value so too many of them don't build up. Examples:

    // gRPC server
    std::unique_ptr<grpc::Server> server_;

    // referernce to ripple::Application
    Application& app_;
    
    // hash
    bytes hash = 4;

seelabs · 2019-11-21T18:11:23Z

proto/rpc/v1/account_info.proto

+    oneof ledger_index {
+        string ledger_index_shortcut_string = 3;
+        uint32 ledger_index_seq = 4;
+        bytes ledger_hash = 5;


The field "ledger index" contains things that are not ledger indexes (the shortcut string isn't an index, and this hash isn't an index). I'd rename parent group.

Can we rename the parent group to just "ledger"?

@cjcobb23 What do you think of ledger_id?

@seelabs I changed this, lmk what you think

seelabs · 2019-11-21T18:14:01Z

proto/rpc/v1/account_info.proto

+    bool strict = 2;
+
+    oneof ledger_index {
+        string ledger_index_shortcut_string = 3;


It's not obvious that a "shortcut_string" is "validated" or "closed". I know that this is going to be changed to an enum, but I have two pieces of feedback:

We should document every fields in these proto buf files, including strings encode data

I would not add a type suffix to field names. I.e. no _string here.

seelabs · 2019-11-21T18:16:26Z

src/ripple/app/main/GRPCServer.h

+    {
+        auto ptr = std::make_shared<CallData<Request,Response>>(
+                service_,*cq_,app_,bl,handler, condition, load_type);
+        return std::static_pointer_cast<Processor>(ptr);


There's no need for this cast, we can just return the result of make_shared.

Will remove this whole function

seelabs · 2019-11-21T18:17:48Z

src/ripple/app/main/GRPCServer.h

+    {
+        auto ptr = makeCallData(bl,handler,condition,load_type);
+        requests_.push_front(ptr);
+        ptr->set_iter(requests_.begin());


We shouldn't be storing the iterator in the object.

seelabs · 2019-11-21T18:24:43Z

src/ripple/basics/strHex.h

+
+//converts data to a string of bytes
+template <class T>
+std::string toBytes(T const & data)


I would remove this function. Even if we needed this for protobufs (I don't think we do), this function has too wide of a scope. I'd like to use the void set_foo(const void* value, size_t size); overloads instead of the string overloads..

We can definitely just use those overloads; that will be more efficient anyways since we don't have to create a std::string temporary. Will remove this toBytes() function

removed this function and used the other overloads

seelabs · 2019-11-21T19:25:12Z

src/ripple/app/main/GRPCServer.h

+            BindListener<Request,Response> bl,
+            Handler<Request,Response> handler,
+            RPC::Condition condition,
+            Resource::Charge load_type)


If we're getting rid of storing the iterator with the object, I'd remove this function.

Will remove this function as well

@seelabs I actually don't think this function should be removed. While we don't need to set the iterator, we still need to add each shared_ptr<CallData> object to the requests_ collection. Without this function, I would have to write requests_.push_front() (or whatever insertion method is used) for each RPC, as opposed to just calling this function

We have to call makeAndPush for each RPC now. The difference is we replace code like

makeAndPush<rpc::v1::GetFeeRequest, rpc::v1::GetFeeResponse>( &rpc::v1::XRPLedgerAPIService::AsyncService::RequestGetFee, doFeeGrpc, RPC::NEEDS_CURRENT_LEDGER, Resource::feeReferenceRPC );

with

{ using cd = CallData<rpc::v1::GetFeeRequest, rpc::v1::GetFeeResponse>; requests_.push_front(std::make_shared<cd>( service_, *cq_, app_, &rpc::v1::XRPLedgerAPIService::AsyncService::RequestGetFee, doFeeGrpc, RPC::NEEDS_CURRENT_LEDGER, Resource::feeReferenceRPC)); }

You do save repeating the service_, *cq_, app_, parameters with a helper function, but I'm not sure it's worth defining a helper function just for that. I don't really object if you want to keep a helper function like this, there's not great harm, but I also don't see much benefit either.

seelabs · 2019-11-21T19:26:04Z

src/ripple/app/main/GRPCServer.h

+            BindListener<Request,Response> bl,
+            Handler<Request,Response> handler,
+            RPC::Condition condition,
+            Resource::Charge load_type)


I'd remove this function and just call make_shared directly where needed.

src/ripple/app/main/GRPCServer.h

seelabs · 2019-11-21T19:39:29Z

src/ripple/app/main/GRPCServer.h

+        // Possible states of the RPC
+        enum CallStatus { PROCESSING, FINISH };
+        // The current serving state.
+        CallStatus status_;


I think a bool finished_ or somesuch is clearer. I understand this started with more states, and we pared it down to two, but now that it's two states I'd just use a bool. Tho I guess I don't object too strongly if other reviews are OK with enum.

I think a bool would be clearer as well, will change

changed to bool

seelabs · 2019-11-21T19:40:30Z

src/ripple/app/main/GRPCServer.h

+
+        // iterator pointing to this object in the requests list
+        // for lifetime management
+        boost::optional<std::list<std::shared_ptr<Processor>>::iterator> iter_;


I'd remove iter_ as a member.

cjcobb23 · 2019-11-22T22:09:41Z

proto/rpc/v1/tx.proto

+    // metadata about the transaction
+    oneof stmeta {
+        Meta meta = 6;
+        bytes meta_bytes = 7;


@seelabs would you also omit the type suffix here? I could change it to meta_binary , but that's still adding the type as a suffix, even if its not the exact name of the protobuf type.

@cjcobb23 Maybe raw_meta? I'm OK with _binary too. Names are hard, maybe someone else has a good suggestion.

changed to meta_binary and transaction_binary

mellery451 · 2019-11-25T19:32:35Z

src/ripple/app/main/GRPCServer.cpp

+              bind_listener_(bind_listener),
+              handler_(handler),
+              required_condition_(required_condition),
+              load_type_(load_type)


pass by value types can be moved

changed to move

mellery451 · 2019-11-25T21:41:30Z

src/ripple/app/main/GRPCServer.cpp

+        Section section = app_.config().section("port_grpc");
+
+        //get the default values of ip and port
+        std::size_t colon_pos = server_address_.find(':');


I don't think we should have default grpc server addr/port. If it wasn't specified in the config, it's either an error or you just don't start grpc (probably the latter).

@mellery451 I think we should not start grpc if the addr/port is not in the config. However, the unit tests need the grpc server to start, so if someone runs the unit tests without addr/port for grpc in the config, the unit tests will fail.

Edit: I guess we could just detect in the unit test whether the grpc server should be running (probably just test if addr/port for grpc is present in the config), and if its not, just do nothing.

we have a default config that is used for unit tests: https://github.com/ripple/rippled/blob/db6eb7840d6edd6d7344bcf1906fff71afe5555f/src/test/jtx/impl/envconfig.cpp#L36-L64
I would suggest adding a grpc_server helper function in that same file that sets-up the grpc config needed and then add that to the Env initialization for any tests that need it (e.g. Env env {*this, envconfig(grpc_server)}; )

Added the helper and initialization code

mellery451 · 2019-11-25T21:55:50Z

src/ripple/app/main/GRPCServer.cpp

+            port_str = port_pair.first;
+        }
+
+        server_address_ = ip_str + ":" + port_str;


this is not ipv6 friendly. I would suggest using beast::IP::Endpoint to create a string from previously parsed address and port values (see setup_ServerHandler above.

mellery451 · 2019-11-25T22:10:19Z

src/ripple/app/main/GRPCServer.h

+namespace {
+
+//helper function. strips port from endpoint string
+std::string getEndpoint(std::string const& peer)


this method is not ipv6 friendly. I'm not sure what format these peer strings have, but it seems like mainly they are endpoint strings with an optional scheme (dns://) ? See if parseURL will work for you (I think it will only parse if a scheme is present..not sure) or maybe just strip the scheme yourself by searching for // and then use Endpoint::from_string to do the rest.

In my testing, the peer strings look like ipv4:127.0.0.1:49808. parseUrl is not working, and Endpoint::from_string does not work either. If I strip ipv4 off of the front, I can then pass the string to Endpoint::from_string, which is actually what I'm doing already (if you look at where this function is used). I'm not sure the best way to do this; the documentation for the peer() method says a string is returned in uri format.

I think what I have here will work, though correct me if I'm wrong. Apparently, the string is always of the form protocol:ip:port. See this PR for reference (you sent this to me actually): grpc/grpc#14667

seelabs · 2019-11-27T17:40:42Z

The non-unity build doesn't compile.

codecov-io · 2019-11-28T02:22:37Z

Codecov Report

Merging #3159 into develop will decrease coverage by 0.08%.
The diff coverage is 65.34%.

@@            Coverage Diff             @@
##           develop   #3159      +/-   ##
==========================================
- Coverage    70.78%   70.7%   -0.09%     
==========================================
  Files          689     691       +2     
  Lines        54296   55089     +793     
==========================================
+ Hits         38432   38949     +517     
- Misses       15864   16140     +276

Impacted Files	Coverage Δ
src/ripple/rpc/handlers/LedgerHandler.h	`68.75% <ø> (ø)`	⬆️
src/ripple/rpc/impl/RPCHelpers.h	`100% <ø> (ø)`	⬆️
src/ripple/rpc/handlers/DownloadShard.cpp	`0% <0%> (ø)`	⬆️
src/ripple/rpc/handlers/Reservations.cpp	`0% <0%> (ø)`	⬆️
src/ripple/rpc/handlers/Random.cpp	`0% <0%> (ø)`	⬆️
src/ripple/rpc/handlers/LogRotate.cpp	`0% <0%> (ø)`	⬆️
src/ripple/rpc/handlers/BlackList.cpp	`0% <0%> (ø)`	⬆️
src/ripple/rpc/handlers/AccountTxOld.cpp	`0% <0%> (ø)`	⬆️
src/ripple/rpc/handlers/LedgerCleanerHandler.cpp	`0% <0%> (ø)`	⬆️
src/ripple/rpc/handlers/LogLevel.cpp	`0% <0%> (ø)`	⬆️
... and 70 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c624fc9...7d867b8. Read the comment docs.

cjcobb23 · 2019-12-03T21:30:54Z

src/test/jtx/envconfig.h

@@ -27,8 +27,7 @@ namespace test {

 extern std::atomic<bool> envUseIPv4;

-inline


I setup my editor to use the .clang-format file in the repo, but inadvertently reformatted code that I didn't touch. I'm going to revert those formatting changes to this file (and the others).

I'll revert these changes when I squash the commits

cjcobb23 · 2019-12-04T23:35:19Z

@seelabs @keefertaylor @MarkusTeufelberger I pushed all requested changes. Take a look and let me know if I missed anything or if I should change anything. Otherwise, resolve your comments.

seelabs · 2019-12-06T16:53:12Z

src/ripple/app/main/GRPCServer.cpp

+                return sPtr.get() == ptr;
+            });
+        BOOST_ASSERT(it != requests.end());
+        *it = requests.back();


Don't assign, swap instead. Assigning needs to change the atomic reference counts. Swapping should be able to just move pointers around.

changed to swap

seelabs · 2019-12-06T16:54:26Z

src/ripple/app/main/GRPCServer.cpp

+GRPCServerImpl::start()
+{
+    // if config does not specify a grpc server address, don't start
+    if (serverAddress_ == "")


I'd call empty() instead of comparing to ""

changed to calling empty()

seelabs · 2019-12-06T16:55:29Z

src/ripple/app/main/GRPCServer.h

+public:
+    virtual ~Processor()
+    {
+    }


I'd default this instead. virtual ~Processor() = default

seelabs · 2019-12-06T16:58:13Z

src/ripple/app/main/GRPCServer.h

+    ~GRPCServerImpl()
+    {
+    }


I don't think we need to define this at all. I'd leave it out.

seelabs · 2019-12-06T17:00:15Z

src/ripple/rpc/Context.h

+template <class T>
+struct ContextGeneric
+{
+    beast::Journal j;
+    T params;
+    Application& app;
+    Resource::Charge& loadType;
+    NetworkOPs& netOps;
+    LedgerMaster& ledgerMaster;
+    Resource::Consumer& consumer;
+    Role role;
+    std::shared_ptr<JobQueue::Coro> coro;
+    InfoSub::pointer infoSub;
+};
+


This duplicates most of the Context struct. Maybe have a base class with the common fields? Also ContextGeneric isn't a great name. Maybe GRPCContext or somesuch.

Created a base class and changed the names of the derived classes.

seelabs · 2019-12-06T17:05:53Z

src/ripple/rpc/impl/DeliveredAmount.cpp

+    rpc::v1::CurrencyAmount& proto,
+    RPC::ContextGeneric<T>& context,
+    std::shared_ptr<Transaction> transaction,
+    TxMeta const& transactionMeta)


We should look at trying to get rid of this code duplication if we can. Maybe we can do it in another PR - but we should at least make a TODO to look at this (you don't need to do this now).

Added a TODO

seelabs · 2019-12-06T17:10:41Z

src/ripple/rpc/impl/DeliveredAmount.cpp

+    rpc::v1::CurrencyAmount&,
+    ContextGeneric<rpc::v1::GetTxRequest>&,
+    std::shared_ptr<Transaction>,
+    TxMeta const&);


We're going to have to insert one of these for every request type that is added. I don't love those kind of manual changes (tho we will get a linker error to remind us to do it, so it's not killer).

It looks like we only need the app and the ledger master. What do you think about passing those as parameters directly? That way this function doesn't need to be a template. If that's a problem, we should look at moving this implementation to the header.

If Context and ContextGeneric were refactor into a common base class, we could also look at passing the base class in. That might help with the code duplication as well.

Passed in base class to remove template

seelabs · 2019-12-06T17:12:44Z

src/test/rpc/GRPCTestClientBase.h

+
+struct GRPCTestClientBase
+{
+    GRPCTestClientBase(std::string const& port)


Be careful with one parameter non-explicit constructors - they define an automatic conversion. Some classes want that, but most of the time we should declare these constructors explicit. I'd declare this one explicit.

changed to explicit

seelabs · 2019-12-06T17:13:15Z

src/test/rpc/Submit_test.cpp

+        rpc::v1::SubmitTransactionRequest request;
+        rpc::v1::SubmitTransactionResponse reply;
+
+        SubmitClient(std::string const& port) : GRPCTestClientBase(port)


seelabs · 2019-12-06T17:13:45Z

src/test/rpc/Tx_test.cpp

+        rpc::v1::GetTxRequest request;
+        rpc::v1::GetTxResponse reply;
+
+        GrpcTxClient(std::string const& port) : GRPCTestClientBase(port)


cjcobb23 · 2019-12-06T20:07:27Z

@seelabs pushed the additional requested changes

seelabs

👍 Assuming tests pass.

We still need to make it clear that this is experimental/prototype code that may be removed in the future or changed in backward incompatible ways. Let's make sure that's documented somewhere front and center or we're going to make life hard for ourselves.

The two biggest things we need to work on to are:

A spec for what this gRPC API should be. We have limited chances to introduce new APIs. Let's make the most of our chance here. Elliot and Rome can help.
We need to reduce the code duplication of two nearly identical APIs.

keefertaylor

Thank you for carrying this across the finish line.

MarkusTeufelberger reviewed Nov 16, 2019

View reviewed changes

keefertaylor reviewed Nov 18, 2019

View reviewed changes

keefertaylor reviewed Nov 20, 2019

View reviewed changes

seelabs self-requested a review November 20, 2019 15:10

seelabs assigned seelabs and mDuo13 Nov 20, 2019

seelabs requested a review from mDuo13 November 20, 2019 15:11

seelabs assigned mellery451 Nov 20, 2019

seelabs requested a review from mellery451 November 20, 2019 15:15

keefertaylor reviewed Nov 20, 2019

View reviewed changes

seelabs reviewed Nov 21, 2019

View reviewed changes

cjcobb23 commented Nov 22, 2019

View reviewed changes

mellery451 reviewed Nov 25, 2019

View reviewed changes

cjcobb23 commented Dec 3, 2019

View reviewed changes

seelabs reviewed Dec 6, 2019

View reviewed changes

seelabs approved these changes Dec 6, 2019

View reviewed changes

keefertaylor approved these changes Dec 6, 2019

View reviewed changes

	// The amount of currency to pay, in either fiat or XRP.
	// The amount of currency to pay, in either issued currency or XRP.



		// RPCs available to interact with the XRP Ledger.
		service XRPLedgerAPIService {

		@@ -27,8 +27,7 @@ namespace test {

		extern std::atomic<bool> envUseIPv4;

		inline

gRPC support #3159

gRPC support #3159

Conversation

cjcobb23 commented Nov 15, 2019

MarkusTeufelberger 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

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

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

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

keefertaylor 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

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

seelabs commented Nov 19, 2019

cjcobb23 commented Nov 19, 2019 • edited Loading

seelabs commented Nov 19, 2019

Re: Binary Interface

Re: Implementing a subset of functionality

keefertaylor commented Nov 20, 2019

seelabs commented Nov 20, 2019

Choose a reason for hiding this comment

keefertaylor commented Nov 20, 2019 • edited Loading

seelabs commented Nov 20, 2019

keefertaylor commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcobb23 commented Nov 20, 2019

seelabs left a comment

Choose a reason for hiding this comment

cjcobb23 commented Nov 19, 2019 •

edited

Loading

keefertaylor commented Nov 20, 2019 •

edited

Loading