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

Validate grpc port info in GRPCServer ctor #4560

Closed
wants to merge 95 commits into from

Conversation

ckeshava
Copy link
Collaborator

@ckeshava ckeshava commented Jun 7, 2023

High Level Overview of Change

Fix #4015, #4557: GRPCServer and ServerHandler are created during the construction of Application instance. Do not validate GRPC IP/Port/Protocol information in ServerHandler. Instead, pass the responsibility of grpc port information validation to GRPCServer constructor.

This ensures that configuration file can specify the port_grpc section.

Context of Change

Based on suggestions from @ximinez , I implemented this code change. @cjcobb23 suggests an alternative solution to the same issue here

I welcome any other suggestions too. I felt this is a clean way to handle this problem without breaking backwards compatibility.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Test Plan

I prefer to add additional unit tests such that we do not observe any unexpected behavior from configuration settings. But I'm not sure if such cases are already being covered by other unit test suites

…ormation in ServerHandler. Pass the responsibility of grpc port information validation to GRPCServer constructor. Both GRPCServer and ServerHandler are created during the construction of Application instance.

// grpc ports are parsed by GRPCServer class. Do not validate
// grpc port information in this file.
if (name == "port_grpc")
Copy link
Contributor

@cjcobb23 cjcobb23 Jun 7, 2023

Choose a reason for hiding this comment

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

"port_grpc" should probably be defined as a constant somewhere

@ckeshava
Copy link
Collaborator Author

ckeshava commented Jun 7, 2023 via email

@scottschurr
Copy link
Collaborator

If I were to define it as a constant, which file is appropriate?

At the moment that file is probably src/ripple/core/ConfigSections.h. There are a bunch of macros starting with SECTION_ in that file to give names for config sections.

@ckeshava
Copy link
Collaborator Author

ckeshava commented Jun 7, 2023 via email

@ximinez
Copy link
Collaborator

ximinez commented Jun 8, 2023

Okay, thanks for the help @scott Schurr @.***> ! Several subsections of the [server] section in the configuration file like "port_peer", "port_ws" are being used as strings inside the codebase. I think we should refactor such occurrences as global constants in this file. Regards, Keshava

"port_peer" and "port_ws" are only found in test code and the example config file. That's because the sections listed under [server] can actually be named anything that isn't already reserved. "port_peer" and "port_ws" are only examples / suggestions. I don't think that necessitates global constants, though maybe constants under test would be valuable.

@ckeshava
Copy link
Collaborator Author

ckeshava commented Jun 9, 2023 via email

@ximinez
Copy link
Collaborator

ximinez commented Jun 9, 2023

Okay, thanks. I didn't realise that. Do you have any suggestions for the appropriate file to keep these constants? I thought test/jtx.h or test/jtx/Env.h might be appropriate since they include many useful functions.

I'd suggest test/jtx/envconfig.h, since the "canonical" usage is in envconfig.cpp, and it's included in a bunch of places.

I've made the suggested changes to "port_grpc" constant. Do I still need to change "port_grpc" to "grpc" in the documentation?

Since you're not changing the name in the code, my vote is to not change the name in the documentation.

@ckeshava
Copy link
Collaborator Author

ckeshava commented Jun 9, 2023 via email

@ckeshava
Copy link
Collaborator Author

Ok, I've made the suggested changes. Let me know if this is okay.

@legleux
Copy link
Collaborator

legleux commented Jun 30, 2023

This change prevents rippled from stopping with

2023-Jun-30 21:37:00.106206422 UTC Application:ERR Missing 'protocol' in [port_grpc]

But it doesn't log anything like the others do:

2023-Jun-30 21:37:58.017835087 UTC Server:NFO Opened 'port_rpc_admin_local' (ip=127.0.0.1:5005, admin nets:127.0.0.1/32, http)
2023-Jun-30 21:37:58.017934922 UTC Server:NFO Opened 'port_peer' (ip=0.0.0.0:51235, peer)
2023-Jun-30 21:37:58.017977815 UTC Server:NFO Opened 'port_ws_admin_local' (ip=127.0.0.1:6006, admin nets:127.0.0.1/32, ws) 

so I can't really tell if its enabled or not when the [server] port_grpc is not provided but [port_grpc] is. Which, with this change behaves the same as before, i.e. it runs if you do not provide [server] port_grpc but do provide the [port_grpc] key.

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Less of a necessary change, and more of a question, but this makes sure I see your response before signing off.

src/ripple/core/ConfigSections.h Outdated Show resolved Hide resolved
@@ -427,9 +428,9 @@ GRPCServerImpl::GRPCServerImpl(Application& app)
: app_(app), journal_(app_.journal("gRPC Server"))
{
// if present, get endpoint from config
if (app_.config().exists("port_grpc"))
if (app_.config().exists(PORT_GRPC))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@legleux Apologies for the late reply.

I can include an else section and I can log the details at this juncture. Would that help in addressing your previous concern?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it really depends if this section behaves the same as the other ones.
I don't remember now how I came across my previous concern but it was not a very important one.

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Looks like this branch needs to be merged from develop and some conflicts resolved, but otherwise is good to go.

@intelliot
Copy link
Collaborator

@legleux - will you be able to have a look at this? See ckeshava's reply above.

@ckeshava
Copy link
Collaborator Author

ckeshava commented Sep 7, 2023

I applied the patch file generated by github actions, but it doesn't modify any files :/
The patch file is not empty, I can see the diff suggestions!

@ximinez
Copy link
Collaborator

ximinez commented Sep 14, 2023

I applied the patch file generated by github actions, but it doesn't modify any files :/ The patch file is not empty, I can see the diff suggestions!

Do you have anything set up to run clang-format automatically on your local repo? It may be overriding the changes from the patch file. If you have a pre-commit hook, you can skip it with git commit --no-verify.

@ckeshava
Copy link
Collaborator Author

thanks Ed, I might have added a pre-commit hook a few months ago.
I'm not getting to the commit step at all. git apply <patch-file> doesn't apply the contents of the patch file :(

@ckeshava ckeshava changed the title Fix #4015, #4557: Do not validate GRPC IP/Port/Protocol information i… Validate grpc port info in GRPCServer ctor Sep 22, 2023
@ckeshava
Copy link
Collaborator Author

@intelliot This PR is ready for merging into develop. The first few commits haven't been signed because I was not aware of the procedure for signing them.

Can I rebase/squash all the commits into one single signed commit with an appropriate commit message?

@intelliot
Copy link
Collaborator

Can I rebase/squash all the commits into one single signed commit with an appropriate commit message?

Yes.

HowardHinnant and others added 24 commits September 22, 2023 14:16
(File was already removed from the source)
Add new transaction submission API field, "sync", which
determines behavior of the server while submitting transactions:
1) sync (default): Process transactions in a batch immediately,
   and return only once the transaction has been processed.
2) async: Put transaction into the batch for the next processing
   interval and return immediately.
3) wait: Put transaction into the batch for the next processing
   interval and return only after it is processed.
* Verify accepted ledger becomes validated, and retry
   with a new consensus transaction set if not.
 * Always store proposals.
 * Track proposals by ledger sequence. This helps slow peers catch
   up with the rest of the network.
 * Acquire transaction sets for proposals with future ledger sequences.
   This also helps slow peers catch up.
 * Optimize timer delay for establish phase to wait based on how
   long validators have been sending proposals. This also helps slow
   peers to catch up.
 * Fix impasse achieving close time consensus.
 * Don't wait between open and establish phases.
…LF#4620)

For the `account_tx` and `noripple_check` methods, perform input
validation for optional parameters such as "binary", "forward",
"strict", "transactions". Previously, when these parameters had invalid
values (e.g. not a bool), no error would be returned. Now, it returns an
`invalidParams` error.

* This updates the behavior to match Clio
  (https://github.com/XRPLF/clio).
* Since this is potentially a breaking change, it only applies to
  requests specifying api_version: 2.
* Fix XRPLF#4543.
A bridge connects two blockchains: a locking chain and an issuing
chain (also called a mainchain and a sidechain). Both are independent
ledgers, with their own validators and potentially their own custom
transactions. Importantly, there is a way to move assets from the
locking chain to the issuing chain and a way to return those assets from
the issuing chain back to the locking chain: the bridge. This key
operation is called a cross-chain transfer. A cross-chain transfer is
not a single transaction. It happens on two chains, requires multiple
transactions, and involves an additional server type called a "witness".

A bridge does not exchange assets between two ledgers. Instead, it locks
assets on one ledger (the "locking chain") and represents those assets
with wrapped assets on another chain (the "issuing chain"). A good model
to keep in mind is a box with an infinite supply of wrapped assets.
Putting an asset from the locking chain into the box will release a
wrapped asset onto the issuing chain. Putting a wrapped asset from the
issuing chain back into the box will release one of the existing locking
chain assets back onto the locking chain. There is no other way to get
assets into or out of the box. Note that there is no way for the box to
"run out of" wrapped assets - it has an infinite supply.

Co-authored-by: Gregory Popovitch <[email protected]>
* Revert "Remove CurrentThreadName.h from RippledCore.cmake (XRPLF#4697)"

This reverts commit 3b5fcd5.

* Revert "Introduce replacement for getting and setting thread name: (XRPLF#4312)"

This reverts commit 36cb5f9.
* For example, without this change, to run the TxQ tests, must specify
  `--unittest=TxQ1,TxQ2` on the command line. With this change, can use
  `--unittest=TxQ`, and both will be run.
* An exact match will prevent any further partial matching.
* This could have some side effects for different tests with a common
  name beginning. For example, NFToken, NFTokenBurn, NFTokenDir. This
  might be useful. If not, the shorter-named test(s) can be renamed. For
  example, NFToken to NFTokens.
* Split the NFToken, NFTokenBurn, and Offer test classes. Potentially speeds
  up parallel tests by a factor of 5.
Remove the `verify` and `message` function declarations. The explicit
instantiation requests could not be completed because there were no
implementations for those two member functions. It is helpful that the
Microsoft (MSVC) compiler on Windows appears to be strict when it comes
to template instantiation.

This resolves the warning:

  XChainAttestations.h(450): warning C4661: 'bool
  ripple::XChainAttestationsBase<ripple::XChainClaimAttestation>::verify(void)
  const': no suitable definition provided for explicit template
  instantiation request
This was likely put back when XRPLF#4292 was rebased.
Make the instructions a bit easier to follow. Users on different
platforms can look for their platform name to find relevant information.
Removed the unused variable `none` from `Writer.cpp` which was causing
build errors on clang version 14.
`LedgerHistory::fixIndex` returns `false` if a repair was performed.

Fix XRPLF#4572
Copy the new code to `src/secp256k1` without changes:
`src/secp256k1` is identical to bitcoin-core/secp256k1@acf5c55 (v0.3.2).

We could consider changing to a Git submodule, though that would require
changes to the build instructions because we are not using submodules
anywhere else.
* Reorganize some changelog entries
* Add note about portable binaries
* Dev blog: https://xrpl.org/blog
Add Boost::json to the list of linked Boost libraries.

This seems to be required for macOS.
gateway_balances
* When `account` does not exist in the ledger, return `actNotFound`
  * (Previously, a normal response was returned)
  * Fix XRPLF#4290
* When required field(s) are missing, return `invalidParams`
  * (Previously, `invalidHotWallet` was incorrectly returned)
  * Fix XRPLF#4548

channel_authorize
* When the specified `key_type` is invalid, return `badKeyType`
  * (Previously, `invalidParams` was returned)
  * Fix XRPLF#4289

Since these are breaking changes, they apply only to API version 2.

Supersedes XRPLF#4577
Co-authored-by: Chenna Keshava B S <[email protected]>
@intelliot intelliot changed the base branch from develop to release September 22, 2023 21:44
@intelliot intelliot changed the base branch from release to develop September 22, 2023 21:44
@ckeshava
Copy link
Collaborator Author

@intelliot I think I made a mistake with the push, I'm working on fixing the commits.

@ckeshava
Copy link
Collaborator Author

Apologies about the mess in this branch, I have cleaned up my work in this PR: #4728
Closing this PR

@ckeshava ckeshava closed this Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename the port_grpc section to avoid confusion