rpc, test: `addnode` improv + add test coverage for invalid command by brunoerg · Pull Request #26366 · bitcoin/bitcoin

brunoerg · 2022-10-21T21:46:47Z

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.

DrahtBot · 2022-10-22T13:44:02Z

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

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.

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.

theStack

Code-review ACK 40660d517022b30e3ab4dcf852a6c86502fa97f3

brunoerg · 2023-05-30T00:32:40Z

CI failure seems unrelated to these changes.

maflcko · 2023-05-30T06:55:52Z

CI failure seems unrelated to these changes.

Thanks, fixed in #27777

src/rpc/net.cpp

theStack

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.

jonatack

ACK c2349d6a04832d9c96a8746fc588bae8fc85376d

some non-blocking suggestions

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

test/functional/rpc_net.py

src/rpc/net.cpp

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.

brunoerg · 2023-08-02T13:32:31Z

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

rpc, test: addnode improv + add test coverage for invalid command #26366 (comment)
rpc, test: addnode improv + add test coverage for invalid command #26366 (comment)
rpc, test: addnode improv + add test coverage for invalid command #26366 (comment)

jonatack

ACK f52cb02

Two non-blocking suggestions.

jonatack · 2023-08-02T13:43:06Z

test/functional/rpc_net.py

        self.nodes[0].addnode(node=ip_port, command='remove')
        assert_equal(self.nodes[0].getaddednodeinfo(), [])
+        # check that an invalid command returns an error
+        assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port, command='abc')


"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)

jonatack · 2023-08-02T13:46:09Z

src/rpc/net.cpp

-        if (!connman.RemoveAddedNode(strNode)) {
+        if (!connman.RemoveAddedNode(node_arg)) {
            throw JSONRPCError(RPC_CLIENT_NODE_NOT_ADDED, "Error: Node could not be removed. It has not been added previously.");
        }


suggestion while touching this

- 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."); }

theStack

re-ACK f52cb02

achow101 · 2023-09-21T10:03:48Z

ACK f52cb02

jonesk7734

Ok

…for invalid command f52cb02 doc: make it clear that `node` in `addnode` refers to the node's address (brunoerg) effd1ef test: `addnode` with an invalid command should throw an error (brunoerg) 56b27b8 rpc, refactor: clean-up `addnode` (brunoerg) Pull request description: 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. ACKs for top commit: achow101: ACK f52cb02 jonatack: ACK f52cb02 theStack: re-ACK f52cb02 Tree-SHA512: e4a69e58b784e233463945b4d55a401957f9fe4562c129f59216a44f44fb3221d3449ac578fb35e665ca654c6ade2e741b72c3df78040f7527229c77b6c5b82e

This was referenced Oct 23, 2022

BIP324: Enable v2 P2P encrypted transport #24545

Closed

p2p, rpc: Manual block-relay-only connections with addnode #24170

Closed

brunoerg changed the title ~~p2p, rpc, test: getpeerinfo clean-ups + add test coverage for invalid command~~ p2p, rpc, test: getpeerinfo improv + add test coverage for invalid command Oct 28, 2022

DrahtBot mentioned this pull request Dec 17, 2022

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

achow101 requested a review from mzumsande April 25, 2023 15:20

theStack approved these changes May 5, 2023

View reviewed changes

DrahtBot added the CI failed label May 29, 2023

DrahtBot removed the CI failed label May 31, 2023

mzumsande reviewed May 31, 2023

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

brunoerg changed the title ~~p2p, rpc, test: getpeerinfo improv + add test coverage for invalid command~~ p2p, rpc, test: addnode improv + add test coverage for invalid command May 31, 2023

brunoerg force-pushed the 2022-10-getpeerinfo-improv branch from 40660d5 to c2349d6 Compare May 31, 2023 23:43

DrahtBot mentioned this pull request Jul 28, 2023

net: improves addnode / m_added_nodes logic #28155

Merged

brunoerg requested a review from theStack August 1, 2023 14:12

theStack approved these changes Aug 1, 2023

View reviewed changes

jonatack reviewed Aug 1, 2023

View reviewed changes

test/functional/rpc_net.py Outdated Show resolved Hide resolved

src/rpc/net.cpp Outdated Show resolved Hide resolved

src/rpc/net.cpp Outdated Show resolved Hide resolved

brunoerg added 3 commits August 2, 2023 10:28

rpc, refactor: clean-up addnode

56b27b8

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.

test: addnode with an invalid command should throw an error

effd1ef

doc: make it clear that node in addnode refers to the node's address

f52cb02

brunoerg force-pushed the 2022-10-getpeerinfo-improv branch from c2349d6 to f52cb02 Compare August 2, 2023 13:31

brunoerg changed the title ~~p2p, rpc, test: addnode improv + add test coverage for invalid command~~ rpc, test: addnode improv + add test coverage for invalid command Aug 2, 2023

jonatack reviewed Aug 2, 2023

View reviewed changes

DrahtBot requested a review from theStack August 2, 2023 13:47

theStack approved these changes Aug 2, 2023

View reviewed changes

brunoerg requested a review from mzumsande August 2, 2023 19:21

DrahtBot mentioned this pull request Aug 24, 2023

BIP324 integration #28331

Merged

DrahtBot mentioned this pull request Sep 11, 2023

Add BIP324-specific labels to peer details bitcoin-core/gui#754

Merged

achow101 self-assigned this Sep 20, 2023

achow101 merged commit 5027d41 into bitcoin:master Sep 21, 2023

jonesk7734 reviewed Sep 21, 2023

View reviewed changes

bitcoin locked and limited conversation to collaborators Sep 20, 2024

Conversation

brunoerg commented Oct 21, 2022

Uh oh!

DrahtBot commented Oct 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented May 30, 2023

Uh oh!

maflcko commented May 30, 2023

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Aug 2, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Sep 21, 2023

Uh oh!

jonesk7734 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Oct 22, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading