rpc, test: `addnode` improv + add test coverage for invalid command #26366

pull brunoerg wants to merge 3 commits into bitcoin:master from brunoerg:2022-10-getpeerinfo-improv changing 2 files +15 −15
  1. brunoerg commented at 9:46 PM on October 21, 2022: contributor

    This PR:

    • Adds test coverage for an invalid command in addnode.
    • Rename test_getaddednodeinfo to test_addnode_getaddednodeinfo and its log since this function also tests addnode and it doesn't worth to split into 2 ones.
    • Makes it clear in docs that node in addnode refers to the node's address. It seemed a little weird for me "The node (see getpeerinfo for nodes)", it could mean a lot of things e.g. the node id.
    • Some small improv/clean-up: use const where possible, rename some vars, and remove the check for nullance for command since it's a non-optional field.
  2. DrahtBot commented at 1:44 PM on October 22, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK jonatack, theStack, achow101

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28331 (BIP324 integration by sipa)
    • #24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)

    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.

  3. brunoerg renamed this:
    p2p, rpc, test: `getpeerinfo` clean-ups + add test coverage for invalid command
    p2p, rpc, test: `getpeerinfo` improv + add test coverage for invalid command
    on Oct 28, 2022
  4. achow101 requested review from mzumsande on Apr 25, 2023
  5. theStack approved
  6. theStack commented at 11:48 AM on May 5, 2023: contributor

    Code-review ACK 40660d517022b30e3ab4dcf852a6c86502fa97f3

  7. DrahtBot added the label CI failed on May 29, 2023
  8. brunoerg commented at 12:32 AM on May 30, 2023: contributor

    CI failure seems unrelated to these changes.

  9. maflcko commented at 6:55 AM on May 30, 2023: member

    CI failure seems unrelated to these changes.

    Thanks, fixed in https://github.com/bitcoin/bitcoin/pull/27777

  10. DrahtBot removed the label CI failed on May 31, 2023
  11. in src/rpc/net.cpp:300 in a461276962 outdated 
     289 | @@ -290,35 +290,33 @@ static RPCHelpMan addnode()
 290 |                  },
 291 |          [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 292 |  {
 293 | -    std::string strCommand;
 294 | -    if (!request.params[1].isNull())
 295 | -        strCommand = request.params[1].get_str();
 296 | -    if (strCommand != "onetry" && strCommand != "add" && strCommand != "remove") {
 297 | +    const std::string command{request.params[1].get_str()};
    mzumsande commented at 10:17 PM on May 31, 2023:

    The title and commit messages don't fit: This PR changes concerns addnode, not getpeerinfo.

    brunoerg commented at 11:44 PM on May 31, 2023:

    Ooops, I think things changed during the way, thanks for notice it! Done!

  12. brunoerg renamed this:
    p2p, rpc, test: `getpeerinfo` improv + add test coverage for invalid command
    p2p, rpc, test: `addnode` improv + add test coverage for invalid command
    on May 31, 2023
  13. brunoerg force-pushed on May 31, 2023
  14. brunoerg requested review from theStack on Aug 1, 2023
  15. theStack approved
  16. theStack commented at 7:20 PM on August 1, 2023: contributor

    re-ACK c2349d6a04832d9c96a8746fc588bae8fc85376d

    The only change since my last ACK was adapting the first commit's subject (s/getpeerinfo/addnode/). Sorry for late re-review, I must have missed the force-push two months ago.

  17. in test/functional/rpc_net.py:222 in c2349d6a04 outdated 
     217 | @@ -218,6 +218,8 @@ def test_getaddednodeinfo(self):
 218 |          # check that node can be removed
 219 |          self.nodes[0].addnode(node=ip_port, command='remove')
 220 |          assert_equal(self.nodes[0].getaddednodeinfo(), [])
 221 | +        # check that an invalid command returns an error
 222 | +        assert_raises_rpc_error(-1, "addnode", self.nodes[0].addnode, node=ip_port, command='abc')
    jonatack commented at 7:49 PM on August 1, 2023:

    nit, this seems a bit clearer and specific

            assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port, command='abc')
    brunoerg commented at 1:31 PM on August 2, 2023:

    Done

  18. in src/rpc/net.cpp:290 in c2349d6a04 outdated 
     286 | @@ -287,7 +287,7 @@ static RPCHelpMan addnode()
 287 |                  strprintf("Addnode connections are limited to %u at a time", MAX_ADDNODE_CONNECTIONS) +
 288 |                  " and are counted separately from the -maxconnections limit.\n",
 289 |                  {
 290 | -                    {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The node (see getpeerinfo for nodes)"},
 291 | +                    {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The node's address (see getpeerinfo for nodes)"},
    jonatack commented at 7:51 PM on August 1, 2023:

    While touching this line, mentioning getpeerinfo doesn't seem particularly relevant as addnode is generally used to add a trusted peer that you already know (e.g. in order to avoid any eclipse situation).

                        {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The node's address"},

    or even better

    "The address of the peer to connect to"

    brunoerg commented at 1:31 PM on August 2, 2023:

    Done!

  19. in src/rpc/net.cpp:324 in c2349d6a04 outdated 
     329 | +        if (!connman.AddNode(node_arg)) {
 330 |              throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: Node already added");
 331 |          }
 332 |      }
 333 | -    else if(strCommand == "remove")
 334 | +    else if(command == "remove")
    jonatack commented at 7:56 PM on August 1, 2023: 
        else if (command == "remove")

    and could also fix the conditional bracket formatting (run clang-format)

    brunoerg commented at 1:31 PM on August 2, 2023:

    Done!

  20. jonatack commented at 8:00 PM on August 1, 2023: member

    ACK c2349d6a04832d9c96a8746fc588bae8fc85376d

    some non-blocking suggestions

    Edit: suggest dropping "p2p" from the PR title and the first commit title.

  21. rpc, refactor: clean-up `addnode`
    1. Use const where possible;
2. Rename variables to make them clearer;
3. There is no need to check whether `command` is null since it's a non-optional field.
    56b27b8487
  22. test: `addnode` with an invalid command should throw an error effd1efefb
  23. doc: make it clear that `node` in `addnode` refers to the node's address f52cb02f70
  24. brunoerg force-pushed on Aug 2, 2023
  25. brunoerg renamed this:
    p2p, rpc, test: `addnode` improv + add test coverage for invalid command
    rpc, test: `addnode` improv + add test coverage for invalid command
    on Aug 2, 2023
  26. brunoerg commented at 1:32 PM on August 2, 2023: contributor

    Force-pushed addressing @jonatack's review. Addressed:

  27. in test/functional/rpc_net.py:222 in f52cb02f70 
     217 | @@ -218,6 +218,8 @@ def test_getaddednodeinfo(self):
 218 |          # check that node can be removed
 219 |          self.nodes[0].addnode(node=ip_port, command='remove')
 220 |          assert_equal(self.nodes[0].getaddednodeinfo(), [])
 221 | +        # check that an invalid command returns an error
 222 | +        assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port, command='abc')
    jonatack commented at 1:43 PM on August 2, 2023:

    "remove the check for nullance for command since it's a non-optional field"

    while here, maybe test that assertion

    -        # check that an invalid command returns an error
+        # check that an invalid or missing command returns an error
         assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port, command='abc')
+        assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port)
  28. in src/rpc/net.cpp:328 in f52cb02f70 
     334 | +    else if (command == "remove")
 335 |      {
 336 | -        if (!connman.RemoveAddedNode(strNode)) {
 337 | +        if (!connman.RemoveAddedNode(node_arg)) {
 338 |              throw JSONRPCError(RPC_CLIENT_NODE_NOT_ADDED, "Error: Node could not be removed. It has not been added previously.");
 339 |          }
    jonatack commented at 1:46 PM on August 2, 2023:

    <details><summary>suggestion while touching this</summary><p>

    -    if (command == "onetry")
-    {
+    if (command == "onetry") {
         CAddress addr;
         connman.OpenNetworkConnection(addr, /*fCountFailure=*/false, /*grantOutbound=*/nullptr, node_arg.c_str(), ConnectionType::MANUAL);
         return UniValue::VNULL;
-    }
-
-    if (command == "add")
-    {
+    } elsif (command == "add") {
         if (!connman.AddNode(node_arg)) {
             throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: Node already added");
         }
-    }
-    else if (command == "remove")
-    {
+    } else if (command == "remove") {
         if (!connman.RemoveAddedNode(node_arg)) {
             throw JSONRPCError(RPC_CLIENT_NODE_NOT_ADDED, "Error: Node could not be removed. It has not been added previously.");
         }

    </p></details>

  29. jonatack commented at 1:47 PM on August 2, 2023: member

    ACK f52cb02f700b58bca921a7aa24bfeee04760262b

    Two non-blocking suggestions.

  30. DrahtBot requested review from theStack on Aug 2, 2023
  31. theStack approved
  32. theStack commented at 7:17 PM on August 2, 2023: contributor

    re-ACK f52cb02f700b58bca921a7aa24bfeee04760262b

  33. brunoerg requested review from mzumsande on Aug 2, 2023
  34. achow101 assigned achow101 on Sep 20, 2023
  35. achow101 commented at 10:03 AM on September 21, 2023: member

    ACK f52cb02f700b58bca921a7aa24bfeee04760262b

  36. achow101 merged this on Sep 21, 2023
  37. achow101 closed this on Sep 21, 2023
  38. jonesk7734 commented at 12:41 PM on September 21, 2023: none

    Ok

  39. Frank-GER referenced this in commit 0f84fa5f1d on Sep 25, 2023
  40. sidhujag referenced this in commit 66a38f8a4a on Sep 26, 2023
  41. bitcoin locked this on Sep 20, 2024
Contributors
brunoergDrahtBottheStackmaflckomzumsandejonatackachow101jonesk7734


mzumsande

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me