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
rpc: Fix addnode remove command error #19696
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 9795d93
Feel free to ignore the comments below unless you have to retouch.
test/functional/rpc_net.py
Outdated
@@ -133,6 +133,13 @@ def _test_getaddednodeinfo(self): | |||
assert_equal(added_nodes[0]['addednode'], ip_port) | |||
# check that a non-existent node returns an error | |||
assert_raises_rpc_error(-24, "Node has not been added", self.nodes[0].getaddednodeinfo, '1.1.1.1') | |||
# check that node can not be added again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/
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 |
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. |
If the change stays, it needs release notes, because it is a breaking RPC change |
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 |
This also adds test coverage for the remove command which was uncovered before.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 a51d0ad2de
IMO changes to the string description shouldn't be considered a breaking change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
I agree here. Code review ACK a51d0ad |
In a previous version the error code (integer) was also changed. Changing the error code has usually been considered a breaking change |
Summary: This also adds test coverage for the remove command which was uncovered before. PR description: > 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 as well as changes the error message to something that makes sense. This is a backport of [[bitcoin/bitcoin#19696 | core#19696]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10072
The
addnode
RPC with theremove
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.