rpc server: add `ipv6 by default` and `--rpc-listen-addrs`

[niklasad1]

Builds on #4730

Close https://github.com/paritytech/polkadot-sdk/issues/3488, https://github.com/paritytech/polkadot-sdk/issues/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.
2. A new RPC CLI option --rpc-listen-addrs 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.

In addition I have added a little crazy idea/solution to 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=*

Currently on the subset of settings that are:

• cors
• rate-limiting cfg
• rpc-method policy

So for instance it could work that one may do:

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

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.~

//cc @jsdw @BulatSaif @PierreBesson @bkchr

What do you think about that this approach? Any suggestions? If all are happy with this approach I'll tidy up and it make it reviewable/mergable...

[PierreBesson]

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]

$ 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]

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]

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.

[pepoviola]

look great! just a nit comment about the naming. Since we are using the string to also set other options I think we can use something like --rpc-listen-uri. wdyt?

[BulatSaif]

Since we are using the string to also set other options I think we can use something like --rpc-listen-uri. wdyt?

If it is called a URI, it will impose some limitations on what can be included in the string, as it needs to follow RFC 3986.

[niklasad1]

look great! just a nit comment about the naming. Since we are using the string to also set other options I think we can use something like --rpc-listen-uri. wdyt?

Yes, Bulat is correct

The reason is that the format isn't strictly an URL/URI and currently the format ip:port/<query str> which isn't a valid URL because it's a relative URL without a base such as wss://127.0.0.1:9944. So I actually had to write some custom parsing for the query params but nothing complicated.

Thus, I think --rpc-listen-addrs is more accurate but open for other suggestions :)

[pepoviola]

look great! just a nit comment about the naming. Since we are using the string to also set other options I think we can use something like --rpc-listen-uri. wdyt?

Yes, Bulat is correct

The reason is that the format isn't strictly an URL/URI and currently the format ip:port/<query str> which isn't a valid URL because it's a relative URL without a base such as wss://127.0.0.1:9944. So I actually had to write some custom parsing for the query params but nothing complicated.

Thus, I think --rpc-listen-addrs is more accurate but open for other suggestions :)

Then sounds good 👍. Thx!!

[skunert]

I like the direction. Overall, we have one million CLI args and it can be overwhelming, so it seems like a good idea to summarize them.

Should be stick to one format? If we are using this new string format for everything, why keep all the command line arguments around? IMO we should then deprecate and remove at some point.

I think currently the help message is relatively short. It says

      --rpc-listen-addrs <RPC_LISTEN_ADDRS>
          Specify the JSON-RPC server listen address.

          The format of the listen addr is `"<ip:port>/?<query params>"`

          For example: if you want to listen on `127.0.0.1:9944` and enable the `rpc-methods=unsafe query parameter, you can use: `--rpc_listen_addrs
          127.0.0.1:9933/?rpc-methods=unsafe&cors=*"`.

But how do I discover all the query parameters that are accepted? By looking at the other CLI flags and then translating? We should probably list alll acceptable parameters.

About the format, I am not sure what I think about it. One alternative would be to just have kv pairs like:

polkadot --rpc-listen-addr "addr=127.0.0.1:9944,rpc-methods=unsafe" --rpc-listen-addr "addr=0.0.0.0:9945,rate-limit=100"

Not sure if that is better, but at least to me it seems more familiar, but not a strong opinion.

[niklasad1]

Should be stick to one format? If we are using this new string format for everything, why keep all the command line arguments around? IMO we should then deprecate and remove at some point.

I agree but I haven't moved all rpc related params to this new format but that shouldn't require much code.

But how do I discover all the query parameters that are accepted? By looking at the other CLI flags and then translating? We should probably list alll acceptable parameters.

Yes good point, it's a bit annoying to describe everything manually but yeah it must be documented properly.

About the format, I am not sure what I think about it. One alternative would be to just have kv pairs like:

polkadot --rpc-listen-addr "addr=127.0.0.1:9944,rpc-methods=unsafe" --rpc-listen-addr "addr=0.0.0.0:9945,rate-limit=100"

Not sure if that is better, but at least to me it seems more familiar, but not a strong opinion.

I don't care that much either but I figured that everyone is familiar URL syntax but your suggestion is easier to read without the query params syntax so I think I prefer it but lemme think about it.

[niklasad1]

So, I have addressed some your main feedback

1. Changed --rpc-listen-addrs to --rpc-endpoint and moved to key-value pairs instead of the url-like syntax. See PR description for an example
2. Moved all rpc configurations to the RpcEndpoint struct, so it should be easy to deprecate the old CLI flags now
3. Documented all key-value pairs that may be configured.

As I see it the main downside with this PR is the manual parsing and the manual documentation but I reckon having this new scheme is still much better and that we will let folks specify the listen addr (this could be a footgun if misused) and to enable any number of rpc endpoints.

Would be cool if you can have another look and I could add the deprecation stuff in this PR if you want.

[niklasad1]

Even though it runs with --rpc-port 39971, so maybe a bug was introduced here.

It's probably the parsing of the log: Running JSON-RPC server: addr=0.0.0.0:9944,[::]:39621

Previously it was: $ Running JSON-RPC server: addr=0.0.0.0:9944,allowed_origins=[*]

So perhaps just it's just parsing the port that failed but I don't know.... Or do you know that zombienet actually connected to either 0.0.0.0:9944 or [::]:39621? Then it's a bug but I don't think so.

So for instance in substrate/subxt we split it based addr=<sockaddr, which still works in the integration test etc

[pepoviola]

Even though it runs with --rpc-port 39971, so maybe a bug was introduced here.

It's probably the parsing of the log: Running JSON-RPC server: addr=0.0.0.0:9944,[::]:39621

Previously it was: $ Running JSON-RPC server: addr=0.0.0.0:9944,allowed_origins=[*]

So perhaps just it's just parsing the port that failed but I don't know.... Or do you know that zombienet actually connected to either 0.0.0.0:9944 or [::]:39621? Then it's a bug but I don't think so.

So for instance in substrate/subxt we split it based addr=<sockaddr, which still works in the integration test etc

Hi @niklasad1 / @skunert, let me try to reproduce since zombienet assign the ports without parse the logs (e.g with --rpc-port flag).

Thx!

[skunert]

Even though it runs with --rpc-port 39971, so maybe a bug was introduced here.

It's probably the parsing of the log: Running JSON-RPC server: addr=0.0.0.0:9944,[::]:39621

Previously it was: $ Running JSON-RPC server: addr=0.0.0.0:9944,allowed_origins=[*]

So perhaps just it's just parsing the port that failed but I don't know.... Or do you know that zombienet actually connected to either 0.0.0.0:9944 or [::]:39621? Then it's a bug but I don't think so.

So for instance in substrate/subxt we split it based addr=<sockaddr, which still works in the integration test etc

I mean the problem is that I am specifying --rpc-port 39971, but the node says it listens to Running JSON-RPC server: addr=0.0.0.0:9944,[::]:39621, so different ports.

[niklasad1]

I mean the problem is that I am specifying --rpc-port 39971, but the node says it listens to Running JSON-RPC server: addr=0.0.0.0:9944,[::]:39621, so different ports.

Aight, got it. Yepp, indeed it's a bug.

EDIT: Fixed now

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs. Job name: test-linux-stable 3/3 Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7106865

[niklasad1]

Another option is call this option --rpc-endpoint-unstable until we collect some feedback on this approach and can break things/rename/revert stuff etc without pissing off folks :)

Any opinons?

[BulatSaif]

Another option is call this option --rpc-endpoint-unstable until we collect some feedback on this approach and can break things/rename/revert stuff etc without pissing off folks :)

As I've seen in various applications, the common practice is to place --rpc-endpoint in the Experimental flags section in --help. If you want to clearly mark the flag, it's better to use the term experimental rather than unstable.

[skunert]

Another option is call this option --rpc-endpoint-unstable until we collect some feedback on this approach and can break things/rename/revert stuff etc without pissing off folks :)

As I've seen in various applications, the common practice is to place --rpc-endpoint in the Experimental flags section in --help. If you want to clearly mark the flag, it's better to use the term experimental rather than unstable.

Yes, following the pattern of our other experimental flags, it would be --experimental-rpc-endpoints.

[Polkadot-Forum]

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/substrate-rpc-server-cli-changes-updates/11122/1