New RPC command disconnectnode #6271

1. 
   Alex-van-der-Peet commented at 3:23 am on June 12, 2015: contributor

   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.
2. New RPC command disconnectnode 7fa826d464
3. in src/rpcnet.cpp: in 7fa826d464 outdated

   228+        );
   229+
   230+    string strNode = params[0].get_str();
   231+
   232+    CNode* pNode = FindNode(strNode.c_str());
   233+    if (pNode != NULL)
   

   ---

   ---

   

   laanwj commented at 7:35 am on June 12, 2015:

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

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

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

   ---

   

   jonasschnelli commented at 7:45 am on June 12, 2015:

   +1 @laanwj I would also remove the ‘strNode’ round trip and directly add params[0].get_str() within FindNode()
4. laanwj added the label RPC on Jun 12, 2015
5. Raise error when node not found + cleanup 8add12feee
6. 
   laanwj commented at 11:55 am on June 12, 2015: member

   utACK
7. 
   jonasschnelli commented at 8:17 pm on June 12, 2015: contributor

   Tested ACK. Would be cool if you could pull in this RPC test (two commits): https://github.com/jonasschnelli/bitcoin/commit/ef4647a8ae2973cd02662752add7e7ba2e926de2 and https://github.com/jonasschnelli/bitcoin/commit/d20a3896460370f0f2564cd4db8d2ee16834a836 from https://github.com/jonasschnelli/bitcoin/tree/DisconnectNode
8. 
   Alex-van-der-Peet commented at 2:04 am on June 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.
9. 
   laanwj commented at 5:01 am on June 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.
10. 
    jonasschnelli commented at 7:47 am on June 13, 2015: contributor

    @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
11. [QA] add rpc disconnectnode rpc test

    also includes some whitespace fixes

    0570c497c2
12. [QA] rename httpbasics.py to netbasics.py b2127d6de3
13. in qa/rpc-tests/netbasics.py: in b2127d6de3

    112         out1 = conn.getresponse().read();
    113         assert_equal('"error":null' in out1, True)
    114         assert_equal(conn.sock!=None, True) #connection must be closed because bitcoind should use keep-alive by default
    115-        
    116+
    117+        ###########################
    

    ---

    ---

    

    laanwj commented at 7:37 am on June 15, 2015:

    @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?

    ---

    

    jonasschnelli commented at 7:53 am on June 15, 2015:

    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.

    ---

    

    laanwj commented at 8:25 am on June 15, 2015:

    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.

14. 
    laanwj commented at 12:17 pm on June 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)
15. laanwj referenced this in commit 754aae5148 on Jun 16, 2015
16. 
    laanwj commented at 12:27 pm on June 16, 2015: member

    Merged via 754aae51488c810f3a3cf6651c88849a209ca545
17. laanwj closed this on Jun 16, 2015

    ---

18. 
    jonasschnelli commented at 2:58 pm on June 16, 2015: contributor

    @laanwj: Good idea. Will try to include the tests in #6158.
19. zkbot referenced this in commit 9af55822fb on Feb 15, 2017
20. zkbot referenced this in commit a7cf698873 on Mar 4, 2017
21. MarcoFalke locked this on Sep 8, 2021

Contributors
Alex-van-der-Peet laanwj jonasschnelli

Labels
RPC/REST/ZMQ