rpc: Fix addnode remove command error#19696
Conversation
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Tested ACK 9795d93ab445c0e
Feel free to ignore the comments below unless you have to retouch.
test/functional/rpc_net.py
Outdated
There was a problem hiding this comment.
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. |
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
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. |
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
8 participants
The
addnodeRPC with theremovecommand 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.