rpc: Fix addnode remove command error #19696

pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:rpc_net_tests changing 2 files +8 −1

fjahr commented at 10:11 PM on August 10, 2020: member

The addnode RPC with the remove command parameter is used to remove a node from the "added nodes". It did not have test coverage and in case of failure to remove the node it responded with the confusing message "Error: Node has not been added.".

This PR adds test coverage and introduces a new error code as well as changes the error message to something that makes sense.
fanquake added the label RPC/REST/ZMQ on Aug 10, 2020

in test/functional/rpc_net.py:136 in 9795d93ab4 outdated

 132 | @@ -133,6 +133,13 @@ def _test_getaddednodeinfo(self):
 133 |          assert_equal(added_nodes[0]['addednode'], ip_port)
 134 |          # check that a non-existent node returns an error
 135 |          assert_raises_rpc_error(-24, "Node has not been added", self.nodes[0].getaddednodeinfo, '1.1.1.1')
 136 | +        # check that node can not be added again

jonatack commented at 8:37 AM on August 11, 2020:

Perhaps place this test (or all the new ones) two lines higher as it is order-dependent on the addnode call.

s/can not/cannot/

jonatack approved
jonatack commented at 8:43 AM on August 11, 2020: member

Tested ACK 9795d93ab445c0e

Feel free to ignore the comments below unless you have to retouch.
MarcoFalke commented at 8:54 AM on August 11, 2020: member

I think the previous error was fine. Can't remove node, because node has not been added. No strong opinion, though.

Concept ACK on adding test coverage here: https://marcofalke.github.io/btc_cov/total.coverage/src/rpc/net.cpp.gcov.html#274
jonatack commented at 8:59 AM on August 11, 2020: member

I agree with @fjahr that this error message is confusing. Good idea on adding the missing coverage; thank you for reminding me of your btc_cov link.
MarcoFalke commented at 9:15 AM on August 11, 2020: member

If the change stays, it needs release notes, because it is a breaking RPC change
promag commented at 10:31 AM on August 11, 2020: member

Agree with @MarcoFalke, the change in the error isn't worth a breaking change. I'd rather document the error.

On a side note, instead of raising RPC errors, I'd return { "message": "node not found" } or something like that.

rpc: Improve addnode remove command error message

This also adds test coverage for the remove command which was uncovered before.

a51d0ad2de

fjahr force-pushed on Aug 11, 2020
fjahr commented at 12:09 PM on August 11, 2020: member

I was not sure but I assume changing the error code is breaking but changing the error message is not considered breaking? I have changed the error code back and kept an improved error message that should explain the reasoning behind the error code better. Also addressed @jonatack 's nits.
theStack approved
theStack commented at 8:11 PM on August 11, 2020: member

I think the confusing part in the error message was that "not added" could be possibly interpreted here both as an action/verb (i.e. "didn't add node in the course of this RPC call") as well as referring to a state (i.e. "is currently not in the list of added nodes"). It is clearer now that the latter is meant, and increasing test coverage is always a good idea.

Tested ACK https://github.com/bitcoin/bitcoin/commit/a51d0ad2de
luke-jr commented at 10:01 PM on August 11, 2020: member

IMO changes to the string description shouldn't be considered a breaking change.
luke-jr approved
luke-jr commented at 10:01 PM on August 11, 2020: member

utACK
laanwj commented at 4:59 PM on August 12, 2020: member

IMO changes to the string description shouldn't be considered a breaking change.

I agree here.

Code review ACK a51d0ad2de89b9757d158df95ddeba2bfcb23935
laanwj merged this on Aug 12, 2020
laanwj closed this on Aug 12, 2020
MarcoFalke commented at 7:59 PM on August 12, 2020: member

In a previous version the error code (integer) was also changed. Changing the error code has usually been considered a breaking change
Fabcien referenced this in commit 6bbc85dcd3 on Sep 8, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels