/bitcoin

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: Support addnode onetry without making the connection priviliged #12674

Open
wants to merge 1 commit into
base: master
from
+11 −6

Conversation

Reviewers

@jnewbery jnewbery

@MeshCollider MeshCollider

@promag promag

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
9 participants
@luke-jr @MarcoFalke @MeshCollider @TheBlueMatt @DrahtBot @sipa @jnewbery @promag @fanquake
@luke-jr
Member

luke-jr commented Mar 12, 2018 •
edited by DrahtBot

The behaviour of addnode onetry changed without notice. This restores the ability to at least get the original behaviour.

Needed for properly testing DoS rules.

@fanquake fanquake added the RPC/REST/ZMQ label Mar 12, 2018

@MarcoFalke

This comment has been minimized.

Sign in to view
Member

MarcoFalke commented Mar 12, 2018

rpc_net.py fails
src/rpc/net.cpp Outdated
(strCommand != "onetry" && strCommand != "add" && strCommand != "remove"))
throw std::runtime_error(
"addnode \"node\" \"add|remove|onetry\"\n"
"addnode \"node\" \"add|remove|onetry\" (privileged)\n"

This comment has been minimized.

Sign in to view
Copy link
@promag

promag Mar 12, 2018

Member

Missing spaces inside ().
src/rpc/net.cpp Outdated
return NullUniValue;
}
if (privileged) {

This comment has been minimized.

Sign in to view
Copy link
@promag

promag Mar 12, 2018

Member

Should be if (!request.params[2].isNull()) {? Furthermore, if the default is true then this will always fail.

This comment has been minimized.

Sign in to view
Copy link
@luke-jr

luke-jr Mar 12, 2018

Member

request.params[2].isNull() is checked when setting the bool above. But yes, it's inverted here.

This comment has been minimized.

Sign in to view
Copy link
@promag

promag Mar 12, 2018

Member

I mean, it doesn't matter the value, at this point command is not onetry, so privileged should not be specified.

This comment has been minimized.

Sign in to view
Copy link
@luke-jr

luke-jr Mar 12, 2018

Member

privileged=true is required for non-onetry. (Maybe some day we will support false too.)

This comment has been minimized.

Sign in to view
Copy link
@MeshCollider

MeshCollider Mar 13, 2018

Member

The error should be thrown for (!privileged) then, it's backwards?
src/rpc/net.cpp
@@ -215,14 +214,19 @@ UniValue addnode(const JSONRPCRequest& request)
throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
std::string strNode = request.params[0].get_str();
const bool privileged = request.params[2].isNull() ? true : request.params[2].get_bool();

This comment has been minimized.

Sign in to view
Copy link
@promag

promag Mar 12, 2018

Member

WDYT moving this to onetry if block below.

This comment has been minimized.

Sign in to view
Copy link
@luke-jr

luke-jr Mar 13, 2018

Member

It's needed for all cases.
src/rpc/net.cpp Outdated
return NullUniValue;
}
if (privileged) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Unprivileged connections are only supported for the \"onetry\" command for now");

This comment has been minimized.

Sign in to view
Copy link
@MeshCollider

MeshCollider Mar 13, 2018

Member

This error seems ambiguous to me, could either mean onetry only supports unprivileged, or unprivileged is only supported by onetry and not add

This comment has been minimized.

Sign in to view
Copy link
@luke-jr

luke-jr Mar 13, 2018

Member

I don't see how it could mean the former in standard English.
@MeshCollider

This comment has been minimized.

Sign in to view
Member

MeshCollider commented Mar 13, 2018

Concept ACK
@TheBlueMatt

This comment has been minimized.

Sign in to view
Contributor

TheBlueMatt commented Mar 28, 2018

This breaks our outbound-connection-counting logic in the aut-connect loop as it uses the manual connection flag to figure out how many other connections to make.
@jnewbery

jnewbery reviewed Mar 28, 2018
View changes

Why no PR description? What's the use case for this?

src/rpc/net.cpp
"\nArguments:\n"
"1. \"node\" (string, required) The node (see getpeerinfo for nodes)\n"
"2. \"command\" (string, required) 'add' to add a node to the list, 'remove' to remove a node from the list, 'onetry' to try a connection to the node once\n"
"3. privileged (boolean, optional, default=true) If true, nodes added will be protected from DoS disconnection and not required to be full nodes or support segwit as other outbound peers are (though such peers will not be synced from). Only supported for command \"onetry\" for now.\n"

This comment has been minimized.

Sign in to view
Copy link
@jnewbery

jnewbery Mar 28, 2018

Member

This setting is called 'manual_connection' internally. I don't think it makes sense to add new terminology here.

This comment has been minimized.

Sign in to view
Copy link
@luke-jr

luke-jr Mar 31, 2018

Member

manual_connection does not express the nature of the flag.
@luke-jr

This comment has been minimized.

Sign in to view
Member

luke-jr commented Mar 31, 2018

Added a PR description, and kicked Travis.

@DrahtBot DrahtBot closed this Jul 21, 2018

@DrahtBot

This comment has been minimized.

Sign in to view
Contributor

DrahtBot commented Jul 21, 2018

The last travis run for this pull request was 112 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@DrahtBot DrahtBot reopened this Jul 21, 2018

@sipa

This comment has been minimized.

Sign in to view
Member

sipa commented Jul 21, 2018

Can you address @TheBlueMatt's comment here: #12674 (comment) ?
@luke-jr

This comment has been minimized.

Sign in to view
Member

luke-jr commented Jul 21, 2018

@TheBlueMatt @sipa I don't consider that broken. A non-privileged connection should be treated the same as any other normal outgoing connection.

@DrahtBot DrahtBot referenced this pull request Aug 12, 2018

Closed

Refactoring CRPCCommand with enum category #13945

@DrahtBot DrahtBot referenced this pull request Aug 22, 2018

Merged

qa: Add TestNode::assert_debug_log #14024

This was referenced Oct 20, 2018

Closed
Open
@DrahtBot

This comment has been minimized.

Sign in to view
Contributor

DrahtBot commented Oct 20, 2018 •
edited

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14459 (More RPC help description fixes by ch4ot1c)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment