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.

Sign up for GitHub

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

Jump to bottom

rpc server: listen to ipv6 socket if available and --experimental-rpc-endpoint CLI option #4792

Merged
merged 75 commits into from
Aug 28, 2024
Merged

rpc server: listen to ipv6 socket if available and --experimental-rpc-endpoint CLI option #4792

merged 75 commits into from
Aug 28, 2024

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Jun 13, 2024 •
edited
Loading

Close #3488, #4331

This changes/adds the following:

  1. The default setting is that substrate starts a rpc server that listens to localhost both Ipv4 and Ipv6 on the same port. Ipv6 is allowed to fail because some platforms may not support it
  2. A new RPC CLI option --experimental-rpc-endpoint which allow to configure arbitrary listen addresses including the port, if this is enabled no other interfaces are enabled.
  3. If the local addr is not found for any of the sockets the server is not started throws an error.
  4. Remove the deny_unsafe from the RPC implementations instead this is an extension to allow different polices for different interfaces/sockets such one may enable unsafe on local interface and safe on only the external interface.

So for instance in this PR it's now possible to start up three RPC endpoints as follows:

$ polkadot --experimental-rpc-endpoint "listen-addr=127.0.0.1:9944,rpc-methods=unsafe" --experimental-rpc-endpoint "listen-addr=0.0.0.0:9945,rpc-methods=safe,rate-limit=100" --experimental-rpc-endpoint "listen-addr=[::1]:9944,optional=true"

Needs to be addressed

1. Support binding to a random port if it's fails with the default stuff for backward compatible reasons
2. How to sync that the rpc CLI params and that the rpc-listen-addr align, hard to maintain...
3. Add similar warning prints for exposing unsafe methods on external interfaces..
4. Inline todos + the hacky String conversion from rpc params.

Cons with this PR

Manual strings parsing impl more error-prone than relying on clap....

//cc @jsdw @BulatSaif @PierreBesson @bkchr

@niklasad1 niklasad1 changed the title rps server: support ipv6 socket rpc server: support ipv6 socket Jun 13, 2024
@niklasad1 niklasad1 changed the title rpc server: support ipv6 socket rpc server: add ipv6 by default and --rpc-listen-addrs Jun 23, 2024
@PierreBesson
Copy link

make it possible to use different polices for each listening interface with a special notation which is very similar to an URL which is ip:addr/?rpc-methods=safe&cors=*

This is definitely very useful for multiple use cases and it is a feature that has been lacking for:

  • Validator/Collator security hardening: having 2 listen addresses with different permissions: one for regular queries (eg. sync state check) and one for admin tasks (eg. setting up keys)
  • RPC node: having 2 listen addresses, one public with rate limits and CORS and one internal which allows unsafe RPC calls.

@jsdw
Copy link
Contributor

jsdw commented Aug 8, 2024

$ polkadot --rpc-listen-addrs "127.0.0.1:9944:/?rpc-methods=unsafe, 0.0.0.0:9945:/?rpc-methods=safe&rpc-rate-limit=100"

This format for allowing options to be passed with multiple listen addrs looks good to me!

@BulatSaif
Copy link
Contributor

Very useful feature. One remark: can we avoid using whitespace in the arguments? It sometimes causes issues. Ideally, allow specifying multiple --rpc-listen-addr arguments.

@niklasad1
Copy link
Member Author

niklasad1 commented Aug 11, 2024 •
edited
Loading

Very useful feature. One remark: can we avoid using whitespace in the arguments? It sometimes causes issues. Ideally, allow specifying multiple --rpc-listen-addr arguments.

Yeah, it should work without whitespaces but I can add a test for it.

@niklasad1 niklasad1 changed the title rpc server: listen to ipv6 socket if available and --rpc-endpoint CLI option rpc server: listen to ipv6 socket if available and --experimental-rpc-endpoint CLI option Aug 23, 2024
@lexnv lexnv assigned lexnv and unassigned lexnv Aug 27, 2024
@lexnv lexnv self-requested a review August 27, 2024 09:27
lexnv
lexnv approved these changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Nice job here 🙏

@niklasad1 niklasad1 mentioned this pull request Aug 28, 2024
Open
@niklasad1 niklasad1 added this pull request to the merge queue Aug 28, 2024
Merged via the queue into master with commit 09254eb Aug 28, 2024
178 of 189 checks passed
@niklasad1 niklasad1 deleted the na-rpc-support-ipv6-sockets branch August 28, 2024 10:48
@niklasad1 niklasad1 mentioned this pull request Aug 29, 2024
Merged
4 tasks
ordian added a commit that referenced this pull request Aug 29, 2024
@ordian
Merge branch 'master' into ao-enact-candidate-weight
629a541 
* master: (39 commits)
  short-term fix for para inherent weight overestimation (#5082)
  CI: Add backporting bot (#4795)
  Fix benchmark failures when using `insecure_zero_ed` flag (#5354)
  Command bot GHA v2 - /cmd <cmd> (#5457)
  Remove pallet::getter usage from treasury (#4962)
  Bump blake2b_simd from 1.0.1 to 1.0.2 (#5404)
  Bump rustversion from 1.0.14 to 1.0.17 (#5405)
  Bridge zombienet tests: remove old command (#5434)
  polkadot-parachain: Add omni-node variant with u64 block number (#5269)
  Refactor verbose test (#5506)
  Use umbrella crate for minimal template (#5155)
  IBP Coretime Polkadot bootnodes (#5499)
  rpc server: listen to `ipv6 socket` if available and `--experimental-rpc-endpoint` CLI option (#4792)
  Update approval-voting-regression-bench (#5504)
  change try-runtime rpc domains (#5443)
  polkadot-parachain-bin: Remove contracts parachain (#5471)
  Add feature to allow Aura collator to use full PoV size (#5393)
  Adding stkd bootnodes (#5470)
  Make `PendingConfigs` storage item public (#5467)
  frame-omni-bencher maintenance (#5466)
  ...
@nazar-pc nazar-pc mentioned this pull request Aug 30, 2024
Merged
This was referenced Sep 9, 2024
Closed
Closed
@niklasad1 niklasad1 mentioned this pull request Sep 11, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Sep 11, 2024
@niklasad1 @ggwpez
rpc server: fix deny unsafe on RpcMethods::Auto (#5678)
4e7c9e7 
Close #5677

I made a nit when I moved this code:
https://github.com/paritytech/polkadot-sdk/blob/v1.14.0-rc1/substrate/client/service/src/lib.rs#L379-#L385
in #4792

Thus:
 - (ip.is_loopback(), RpcMethods::Auto) -> allow unsafe
 - (!ip.is_loopback(), RpcMethods::Auto) -> deny unsafe

---------

Co-authored-by: ggwpez <[email protected]>
niklasad1 added a commit that referenced this pull request Sep 12, 2024
@niklasad1 @ggwpez
rpc server: fix deny unsafe on RpcMethods::Auto (#5678)
814cd3d 
Close #5677

I made a nit when I moved this code:
https://github.com/paritytech/polkadot-sdk/blob/v1.14.0-rc1/substrate/client/service/src/lib.rs#L379-#L385
in #4792

Thus:
 - (ip.is_loopback(), RpcMethods::Auto) -> allow unsafe
 - (!ip.is_loopback(), RpcMethods::Auto) -> deny unsafe

---------

Co-authored-by: ggwpez <[email protected]>
@github-actions github-actions bot mentioned this pull request Oct 8, 2024
Open
@TDemeco TDemeco mentioned this pull request Oct 14, 2024
Merged
TarekkMA
TarekkMA reviewed Nov 19, 2024
View reviewed changes
substrate/client/rpc-servers/src/lib.rs
.custom_tokio_runtime(cfg.tokio_handle.clone())
.set_id_provider(RandomStringIdProvider::new(16));

if let Some(provider) = id_provider2.take() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @niklasad1, Thank you for this contribution. I've a question about this change.

Why are we using .take() here. in the previous version this was not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey, if you take a look we are now using a tokio::spawn task for each RPC endpoint and that's why we need take() and clone() because it's used in a loop

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the response,

In a test in moonbeam we found that the id_provider is being reset somehow and changed to the default one. Changing .take() to .clone() would fix the issue.

I've opened a PR: #6588 addressing this issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, weird that wasn't obvious to me but your fix looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TarekkMA TarekkMA TarekkMA left review comments

@lexnv lexnv lexnv approved these changes

@skunert skunert skunert approved these changes

Assignees
No one assigned
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Specify listen address for RPC service Bind to IPv6 addresses if available
9 participants
@niklasad1 @PierreBesson @jsdw @BulatSaif @pepoviola @skunert @paritytech-cicd-pr @TarekkMA @lexnv