RPC: allow binding of restricted port in addition to core port #2836

Timo614 · 2017-11-17T15:48:14Z

This branch adds two new configuration options --rpc-restricted-bind-port and --testnet-rpc-restricted-bind-port which allow for a second port to be opened to handle RPC requests.

Let me know if this direction is unwise. I spent some time looking over the code and this was the approach that seemed most sensible to me but I'm still learning C++ so completely understand if I've done something asinine. Currently the login is the same across both but if there's value that could be split into a separate set of configuration arguments as well.

I was able to test this by using the /getinfo call across both and confirmed that /get_peer_list only works for the unrestricted.

It's currently possible for someone to both set the core RPC port as restricted via --restricted-rpc and enable the restricted RPC port -- if it's worth it I can add something quickly to prevent that though.

ref: #835

Timo614 · 2017-11-17T15:58:43Z

src/daemon/rpc.h

    {
-      throw std::runtime_error("Failed to initialize core rpc server.");
+      throw std::runtime_error("Failed to initialize " + m_description + " rpc server.");


I wasn't able to use the << syntax here as it returned an error during compilation related to invalid operands to binary expression ('const char *' and 'const std::string'...)

Trying to figure out why (and will replace this once I do figure that out) but just making note of it if anyone knows offhand (still learning C++ so this isn't immediately obvious to me).

<< is when the object is something which has this operator, such as a stringtream. operator+ is the right thing to use here.

moneromooo-monero

Looks good in general, just a couple minor questions.

moneromooo-monero · 2017-11-17T16:18:22Z

src/daemon/daemon.cpp

+    const auto restricted_rpc_port = command_line::get_arg(vm, testnet ? cryptonote::core_rpc_server::arg_testnet_rpc_restricted_bind_port : cryptonote::core_rpc_server::arg_rpc_restricted_bind_port);
+    rpcs.emplace_back(new t_rpc{vm, core, p2p, restricted, testnet, main_rpc_port, "core"});
+
+    if(restricted_rpc_port.size())


That should probably be somhting like "if (command_line::is_arg_defaulted(vm, ...))" and have the get_arg inside the if.

moneromooo-monero · 2017-11-17T16:19:42Z

src/daemon/daemon.cpp


-    std::unique_ptr<daemonize::t_command_server> rpc_commands;
+    for(const auto& rpc: mp_internals->rpcs)
+      rpc->run();


Logically, it seems odd to call run on a const object. I'd make it non const, it's less surprising I think ?

Sounds good, yeah agreed I'll switch those instances of it to remove that.

moneromooo-monero · 2017-11-17T16:25:32Z

src/daemon/rpc.h

+    , const bool restricted
+    , const bool testnet
+    , const std::string port
+    , const std::string description


Maybe forgot a & ? (unless you know what you're doing with copy elision, I'm not very familiar with those)

Timo614 · 2017-11-17T17:27:12Z

I've pushed up some changes via https://github.com/Timo614/monero/commit/a45022eda85a048707910e14185fb1b5328d2a11 which I squashed and force pushed up here to one commit.

Added the & to the method signatures for port and description to pass by reference
Switched the iteration of the rpcs to remove the const declaration
Used is_arg_defaulted for confirming whether the restricted port was requested

Timo614 · 2017-11-17T17:51:10Z

Going to push up one more change to just rename a variable I introduced in https://github.com/Timo614/monero/commit/a45022eda85a048707910e14185fb1b5328d2a11 from restricted_rpc_arg to restricted_rpc_port_arg as there's another --restricted-rpc arg and I want to reduce confusion that naming may cause.

Edit: should be set now.

moneromooo-monero

Nice trick that emplace_back, I should start using it too...

hyc · 2017-11-17T18:52:22Z

IMO we've got a useless proliferation of duplicate options, for testnet vs mainnet. It's not as if we allow a single daemon to operate on both simultaneously (do we?)

There should be just one --testnet option, which completely changes the daemon's operating context, and thus all of the other parameters, in one fell swoop.

Timo614 · 2017-11-17T19:06:28Z

Edit: Removed, originally attempted to include the changes to merge the testnet parameters here but it's not worth including in this pull request / outside of scope and the comments I had added just muddy the conversation so nuking them.

moneromooo-monero · 2017-11-17T20:23:43Z

I'd rather the testnet arguments merge be in a totally separate patch (PR or not at your convenience, I don't really care, just a separate patch as it's logically separate).

moneromooo-monero · 2017-11-17T20:24:38Z

BTW, I think fluffypony had a preference for separating the arguments, so I'll go ping him to see this.

Timo614 · 2017-11-17T20:45:18Z

I pushed up https://github.com/Timo614/monero/commit/7c1a19a77f9fb73db75466e4cd758b158fcf7437

Which reverts https://github.com/Timo614/monero/commit/6201ccd55871d0558990c2c5a11566660774ba08

~~I'll hold off on squashing it for now.~~

If we decide to combine the testnet and regular arguments I'll nix the testnet one I added here and save any additional changes to combine the testnet and regular versions to outside of this branch in its own patch (the behavior of the --rpc-bind-port and --rpc-restricted-bind-port will diverge in that case for testnet but only until the additional PR is merged so not terrible).

Alternatively I can just squash now and we can revisit this in a separate issue later (it'll add the new --testnet-rpc-restricted-bind-port command here but it can be deprecated in the same fashion as the --testnet-rpc-bind-port when we file those changes).

Timo614 · 2017-11-17T21:30:14Z

I ended up squashing this -- it's back to the code it was at previously where there's the two RPC arguments (one for testnet and one for the normal use case).

Can swing back and remove the testnet one for this particular use case if we decide to combine them or can just do that in a subsequent patch and deprecate that command in a similar way (or just delete it then if we haven't released a new version yet / wouldn't break backwards compatibility).

fluffypony

Reviewed

23b6f68 RPC: allow binding of restricted port in addition to core port (Tim L)

Timo614 commented Nov 17, 2017

View reviewed changes

Timo614 changed the title ~~RPC: allow binding of restricted API in addition to core API~~ RPC: allow binding of restricted port in addition to core port Nov 17, 2017

moneromooo-monero reviewed Nov 17, 2017

View reviewed changes

moneromooo-monero approved these changes Nov 17, 2017

View reviewed changes

RPC: allow binding of restricted port in addition to core port

23b6f68

moneromooo-monero approved these changes Nov 29, 2017

View reviewed changes

fluffypony approved these changes Dec 2, 2017

View reviewed changes

fluffypony merged commit 23b6f68 into monero-project:master Dec 2, 2017

fluffypony added a commit that referenced this pull request Dec 2, 2017

Merge pull request #2836

3052dab

23b6f68 RPC: allow binding of restricted port in addition to core port (Tim L)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC: allow binding of restricted port in addition to core port #2836

RPC: allow binding of restricted port in addition to core port #2836

Timo614 commented Nov 17, 2017 •

edited

Timo614 Nov 17, 2017

moneromooo-monero Nov 17, 2017

moneromooo-monero left a comment

moneromooo-monero Nov 17, 2017

moneromooo-monero Nov 17, 2017

Timo614 Nov 17, 2017

moneromooo-monero Nov 17, 2017

Timo614 commented Nov 17, 2017

Timo614 commented Nov 17, 2017 •

edited

moneromooo-monero left a comment

hyc commented Nov 17, 2017

Timo614 commented Nov 17, 2017 •

edited

moneromooo-monero commented Nov 17, 2017

moneromooo-monero commented Nov 17, 2017

Timo614 commented Nov 17, 2017 •

edited

Timo614 commented Nov 17, 2017 •

edited

fluffypony left a comment

RPC: allow binding of restricted port in addition to core port #2836

RPC: allow binding of restricted port in addition to core port #2836

Conversation

Timo614 commented Nov 17, 2017 • edited

Timo614 Nov 17, 2017

Choose a reason for hiding this comment

moneromooo-monero Nov 17, 2017

Choose a reason for hiding this comment

moneromooo-monero left a comment

Choose a reason for hiding this comment

moneromooo-monero Nov 17, 2017

Choose a reason for hiding this comment

moneromooo-monero Nov 17, 2017

Choose a reason for hiding this comment

Timo614 Nov 17, 2017

Choose a reason for hiding this comment

moneromooo-monero Nov 17, 2017

Choose a reason for hiding this comment

Timo614 commented Nov 17, 2017

Timo614 commented Nov 17, 2017 • edited

moneromooo-monero left a comment

Choose a reason for hiding this comment

hyc commented Nov 17, 2017

Timo614 commented Nov 17, 2017 • edited

moneromooo-monero commented Nov 17, 2017

moneromooo-monero commented Nov 17, 2017

Timo614 commented Nov 17, 2017 • edited

Timo614 commented Nov 17, 2017 • edited

fluffypony left a comment

Choose a reason for hiding this comment

Timo614 commented Nov 17, 2017 •

edited

Timo614 commented Nov 17, 2017 •

edited

Timo614 commented Nov 17, 2017 •

edited

Timo614 commented Nov 17, 2017 •

edited

Timo614 commented Nov 17, 2017 •

edited