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

New RPC command disconnectnode #6271

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Alex-van-der-Peet
Contributor

Alex-van-der-Peet commented Jun 12, 2015

Original issue #2729 was implemented in pull request #6259. Based on the discussion there, this pull request adds a new RPC command disconnectnode. Tested on Ubuntu 14.04 with bitcoin-cli, works.

Show outdated Hide outdated src/rpcnet.cpp
string strNode = params[0].get_str();
CNode* pNode = FindNode(strNode.c_str());
if (pNode != NULL)

This comment has been minimized.

@laanwj

laanwj Jun 12, 2015

Member

I think it should raise an error when the node is not found. e.g.

if (pNode == NULL)
    throw JSONRPCError(RPC_NOT_FOUND, "Node not found in connected nodes");

This makes it possible to see if an operation actually happened.

@laanwj

laanwj Jun 12, 2015

Member

I think it should raise an error when the node is not found. e.g.

if (pNode == NULL)
    throw JSONRPCError(RPC_NOT_FOUND, "Node not found in connected nodes");

This makes it possible to see if an operation actually happened.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 12, 2015

Member

+1 @laanwj
I would also remove the 'strNode' round trip and directly add params[0].get_str() within FindNode()

@jonasschnelli

jonasschnelli Jun 12, 2015

Member

+1 @laanwj
I would also remove the 'strNode' round trip and directly add params[0].get_str() within FindNode()

@laanwj laanwj added the RPC/REST/ZMQ label Jun 12, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 12, 2015

Member

utACK

Member

laanwj commented Jun 12, 2015

utACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 12, 2015

Member

Tested ACK.
Would be cool if you could pull in this RPC test (two commits):
jonasschnelli@ef4647a and jonasschnelli@d20a389
from https://github.com/jonasschnelli/bitcoin/tree/DisconnectNode

Member

jonasschnelli commented Jun 12, 2015

Tested ACK.
Would be cool if you could pull in this RPC test (two commits):
jonasschnelli@ef4647a and jonasschnelli@d20a389
from https://github.com/jonasschnelli/bitcoin/tree/DisconnectNode

@Alex-van-der-Peet

This comment has been minimized.

Show comment
Hide comment
@Alex-van-der-Peet

Alex-van-der-Peet Jun 13, 2015

Contributor

@jonasschelli Just to avoid git confusion, you're asking me to add a source in git for your branch, merge the changes into mine and push it back? That's ok, I won't see my dev machine untli Monday though.

Contributor

Alex-van-der-Peet commented Jun 13, 2015

@jonasschelli Just to avoid git confusion, you're asking me to add a source in git for your branch, merge the changes into mine and push it back? That's ok, I won't see my dev machine untli Monday though.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 13, 2015

Member

@Alex-van-der-Peet yes, except you should use git cherry-pick instead of merging; git merges and github pulls interact very badly. If it's too much trouble we can add the tests in another pull.

Member

laanwj commented Jun 13, 2015

@Alex-van-der-Peet yes, except you should use git cherry-pick instead of merging; git merges and github pulls interact very badly. If it's too much trouble we can add the tests in another pull.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 13, 2015

Member

@Alex-van-der-Peet: right. You need to add my remote git remote add jonasschnelli https://github.com/jonasschnelli/bitcoin, then you can fetch via git fetch jonasschnelli, then you can cherry pick the two commits: git cherry-pick ef4647a8ae2973cd02662752add7e7ba2e926de2 and git cherry-pick d20a3896460370f0f2564cd4db8d2ee16834a836

Member

jonasschnelli commented Jun 13, 2015

@Alex-van-der-Peet: right. You need to add my remote git remote add jonasschnelli https://github.com/jonasschnelli/bitcoin, then you can fetch via git fetch jonasschnelli, then you can cherry pick the two commits: git cherry-pick ef4647a8ae2973cd02662752add7e7ba2e926de2 and git cherry-pick d20a3896460370f0f2564cd4db8d2ee16834a836

conn = httplib.HTTPConnection(urlNode2.hostname, urlNode2.port)
conn.connect()
conn.request('POST', '/', '{"method": "getbestblockhash"}', headers)
out1 = conn.getresponse().read();
assert_equal('"error":null' in out1, True)
assert_equal(conn.sock!=None, True) #connection must be closed because bitcoind should use keep-alive by default
###########################

This comment has been minimized.

@laanwj

laanwj Jun 15, 2015

Member

@jonasschnelli
I don't see the connection to add these tests in httpbasics.py (even renaming it). Let's add a seperate test script for (upcoming) RPC node disconnect/ban/etc handling?

@laanwj

laanwj Jun 15, 2015

Member

@jonasschnelli
I don't see the connection to add these tests in httpbasics.py (even renaming it). Let's add a seperate test script for (upcoming) RPC node disconnect/ban/etc handling?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 15, 2015

Member

I was trying to save some precious test time because another test will at least add another chain init and maybe mining of 100blocks on the top.
But right. A separate test could make sense if we look what else could get in there. Will do so.

@jonasschnelli

jonasschnelli Jun 15, 2015

Member

I was trying to save some precious test time because another test will at least add another chain init and maybe mining of 100blocks on the top.
But right. A separate test could make sense if we look what else could get in there. Will do so.

This comment has been minimized.

@laanwj

laanwj Jun 15, 2015

Member

Saving time is not worth it at the expense of sanity :)
Maybe the chain init can be avoided for tests like this that don't need a chain?

In any case - httpbasics will become more extensive when we switch HTTP servers, so keeping the just-HTTP tests in a script of their own makes sense.

@laanwj

laanwj Jun 15, 2015

Member

Saving time is not worth it at the expense of sanity :)
Maybe the chain init can be avoided for tests like this that don't need a chain?

In any case - httpbasics will become more extensive when we switch HTTP servers, so keeping the just-HTTP tests in a script of their own makes sense.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 16, 2015

Member

@jonasschnelli To make it easier for our new contributor I'm going to go ahead and merge this without the tests, we can add those in a separate pull (or as part of #6158, even better)

Member

laanwj commented Jun 16, 2015

@jonasschnelli To make it easier for our new contributor I'm going to go ahead and merge this without the tests, we can add those in a separate pull (or as part of #6158, even better)

laanwj added a commit that referenced this pull request Jun 16, 2015

Merge pull request #6271
60dbe73 New RPC command disconnectnode (Alex van der Peet)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 16, 2015

Member

Merged via 754aae5

Member

laanwj commented Jun 16, 2015

Merged via 754aae5

@laanwj laanwj closed this Jun 16, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 16, 2015

Member

@laanwj: Good idea. Will try to include the tests in #6158.

Member

jonasschnelli commented Jun 16, 2015

@laanwj: Good idea. Will try to include the tests in #6158.

@str4d str4d referenced this pull request Feb 14, 2017

Merged

Bitcoin 0.12 RPC PRs 1 #2100

@jnewbery jnewbery referenced this pull request Apr 6, 2017

Closed

Add updatepeer RPC #10160

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