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

RPC sidecar changes #4491

Merged

Conversation

jacek-casper
Copy link
Contributor

@jacek-casper jacek-casper commented Jan 11, 2024

This PR includes:

  • addition of a TCP listener in the node that will be handling requests from the RPC sidecar
  • NCTL changes necessary to pull, build and run the sidecar alongside the nodes
  • a specification of the binary protocol
  • some changes to casper-types to expose types needed for the sidecar to work
  • removal of the existing RPC server

The purpose of this PR is to get a high-level review, there are still minor changes being done and unit tests being added.

PR for the addition of the sidecar: casper-network/casper-sidecar#231

rafal-ch and others added 30 commits December 11, 2023 18:49
Copy link
Collaborator

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

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

Approved subject to reactor test having timeouts applied, and with the various unresolved issues being discussed/ticketed soon.

These are:

  • speculative exec needs to be able to be disabled (and should be by default)
  • errors returned should give the client of the sidecar meaningful info
  • the two different Transform structs should get versioned
  • how we handle/set max_request_size_bytes and max_response_size_bytes should be reviewed
  • whether the REST server should still serve the sidecar's JSON-RPC schema or not

@EdHastingsCasperAssociation
Copy link
Collaborator

whether the REST server should still serve the sidecar's JSON-RPC schema or not

It should not. That functionality should be moved to the sidecar. Good catch.

@EdHastingsCasperAssociation
Copy link
Collaborator

EdHastingsCasperAssociation commented Feb 27, 2024

speculative exec needs to be able to be disabled (and should be by default)
the two different Transform structs should get versioned

Agreed.

errors returned should give the client of the sidecar meaningful info

Wouldn't that be a requirement on the sidecar itself rather than this PR? Can you elaborate on your concern?

how we handle/set max_request_size_bytes and max_response_size_bytes should be reviewed

Can you elaborate?

@Fraser999
Copy link
Collaborator

errors returned should give the client of the sidecar meaningful info

Wouldn't that be a requirement on the sidecar itself rather than this PR? Can you elaborate on your concern?

I think the sidecar won't be able to give the level of detail clients currently get via error messages if we don't provide that info to the sidecar via the response from the node.

how we handle/set max_request_size_bytes and max_response_size_bytes should be reviewed

Can you elaborate?

There's a discussion here (you'll probably need to expand the hidden items to see it) or maybe this link won't need expanding.

@jacek-casper
Copy link
Contributor Author

bors r+

casperlabs-bors-ng bot added a commit that referenced this pull request Feb 29, 2024
4491: RPC sidecar changes r=jacek-casper a=jacek-casper

This PR includes:
- addition of a TCP listener in the node that will be handling requests from the RPC sidecar
- NCTL changes necessary to pull, build and run the sidecar alongside the nodes
- a specification of the binary protocol
- some changes to casper-types to expose types needed for the sidecar to work
- removal of the existing RPC server

The purpose of this PR is to get a high-level review, there are still minor changes being done and unit tests being added.

PR for the addition of the sidecar: casper-network/casper-sidecar#231

Co-authored-by: Rafał Chabowski <rafal@casperlabs.io>
Co-authored-by: Jacek Malec <145967538+jacek-casper@users.noreply.github.com>
Copy link
Contributor

Canceled.

@jacek-casper
Copy link
Contributor Author

bors r+

casperlabs-bors-ng bot added a commit that referenced this pull request Feb 29, 2024
4491: RPC sidecar changes r=jacek-casper a=jacek-casper

This PR includes:
- addition of a TCP listener in the node that will be handling requests from the RPC sidecar
- NCTL changes necessary to pull, build and run the sidecar alongside the nodes
- a specification of the binary protocol
- some changes to casper-types to expose types needed for the sidecar to work
- removal of the existing RPC server

The purpose of this PR is to get a high-level review, there are still minor changes being done and unit tests being added.

PR for the addition of the sidecar: casper-network/casper-sidecar#231

Co-authored-by: Rafał Chabowski <rafal@casperlabs.io>
Co-authored-by: Jacek Malec <145967538+jacek-casper@users.noreply.github.com>
Copy link
Contributor

Build failed:

@jacek-casper
Copy link
Contributor Author

bors r+

Copy link
Contributor

Build succeeded:

@casperlabs-bors-ng casperlabs-bors-ng bot merged commit 2f3267a into casper-network:feat-2.0 Feb 29, 2024
4 checks passed
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.

None yet

5 participants