Fixes: #20552
Picks up a branch from by @amitiuttarwar which adds a success response to addnode RPC.
It adds a new return object to addnode RPC indicating that the call was successful:
0{
1 {RPCResult::Type::STR, "operation", "The operation called (onetry/add/remove)"},
2 {RPCResult::Type::STR, "result", "The result of the operation (success/failed)"},
3 {RPCResult::Type::STR, "address", "The address of the peer"},
4}
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
I did experiment with not throwing JSONRPCErrors at all from this call, and always returning a result and optionally an error if it occurred, but this doesn’t align with our general practice of throwing JSONRPCErrors on failures, so I abandoned this approach.
This does mean that the result field here is pretty much redundant; we either throw, or return a result (implicitly indidcating success). Perhaps it could therefore be removed? Unsure if folks would prefer the re-assurance of seeing "result": "success" (we can never return "failure" here currently!)
Before these changes:
0$ src/bitcoin-cli -signet addnode i-am-not-an-address onetry
After:
0$ src/bitcoin-cli -signet addnode i-am-not-an-address onetry
1error code: -24
2error message:
3Unable to open connection
This lets us propagate errors back to `CConnman::AddConnection()` and
the `addnode` RPC in the future.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Return the status of `AddConnection()` to the `addconnection()` RPC.
Also rename RPCErrorCode 34 to align with the behaviour it signifies.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
When we call the `addnode` RPC, previous behaviour was to return an RPC
error in some situations, but otherwise return null.
This commit changes that behaviour to instead return an `RPCResult` from
calls to `addnode`. This will include the address the result relates to,
and additionally either a `result` or an `error` field.
The presence of these fields can be used to determine success, or error
respectively.
360+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: Adding v2transport connections requires -v2transport init flag to be set.");
361 }
362
363- if (command == "add")
364- {
365+ std::string error_message;
There’s a lot of stuff happening in one commit:
!node_v2transportJSONRPCError when opening the connection for a onetry fails, instead of returning NULL (which makes it indistinguishable from a success operation)result==successI would suggest breaking things up a bit and describing the rationale for each.
Some initial thoughts/questions about each of the above points, addressed individually:
operation and address fields, and the result field is always true. I think (ideally consistently) returning newly created objects as a response can be a useful pattern, but doing it as a one-off I’m not really enthusiastic about.More general thoughts:
node and command, and then call them address and operation in the result, when they are the exact same thing?
There’s a lot of stuff happening in one commit:
Right, I wanted to open this up specifically while WIP as I ended up less-sure about the idea of the change in general (nor the implementation approach). The final commit does indeed need breaking up, but I wanted to get feedback on the mechanics of the whole change before investing more time into it, so thank you very much for your detailed review!
- behaviour change: allow removing a v2 connection when
!node_v2transport
This was actually done to align with the helptext: https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/rpc/net.cpp#L317 but can be easily reverted if undesired (although this way makes more sense to me as removing a node doesn’t depend on -v2transport).
- behaviour change: explicitly throwing a a
JSONRPCErrorwhen opening the connection for aonetryfails, instead of returning NULL (which makes it indistinguishable from a success operation)
Correct, this is basically the main aim of the PR; to clarify whether we are returning successfully or not. As mentioned in 2nd post above, initially I removed some (or all) of the JSONRPCErrors in this RPC, as if we are returning a result and/or error field, then we might as well catch and return errors gracefully to the caller. However that doesn’t align with our general RPC ethos where we throw specific JSONRPCErrors, so the behaviour is introduced here to align with that model.
- behaviour change: always returning a results object with input values and
result==success
I agree this is redundant as mentioned in comment
- if we’re going to return something, the peer id seems like actual useful (new) information?
Will be happy to implement this instead, so we have a useful return value.
- clang-tidy fixes
ok
- can you talk more about the rationale here?
The rationale is directly from #20552
- this seems like unnecessary API breakage for a one-off change (not just the return type, also the changed error messages) with no clear benefit to the user: they already know the
operationandaddressfields, and theresultfield is alwaystrue. I think (ideally consistently) returning newly created objects as a response can be a useful pattern, but doing it as a one-off I’m not really enthusiastic about.
Do you have any suggestions on another way to determine success or failure without a breaking API change? I suppose we could guarantee thrown errors on every fail-case, and retain NULL as indicating success? That could work, but feels less intuitive to me personally.
* it seems not very elegant to have the input fields named `node` and `command`, and then call them `address` and `operation` in the result, when they are the exact same thing?
Agreed. I will sort those out.
Thanks again! ❤️
364- {
365+ std::string error_message;
366+
367+ if (command == "onetry") {
368+ if (!connman.OpenNetworkConnection(CAddress(), false, {}, node_arg.c_str(), ConnectionType::MANUAL, use_v2transport)) {
369+ throw JSONRPCError(RPC_CLIENT_NODE_NOT_ADDED, "Unable to open connection");
Unable to open connection to <peer address>
315@@ -316,7 +316,14 @@ static RPCHelpMan addnode()
316 {"command", RPCArg::Type::STR, RPCArg::Optional::NO, "'add' to add a node to the list, 'remove' to remove a node from the list, 'onetry' to try a connection to the node once"},
317 {"v2transport", RPCArg::Type::BOOL, RPCArg::DefaultHint{"set by -v2transport"}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"},
318 },
319- RPCResult{RPCResult::Type::NONE, "", ""},
320+ RPCResult{
321+ RPCResult::Type::OBJ, "", "",
322+ {
323+ {RPCResult::Type::STR, "operation", "The operation called (onetry/add/remove)"},
324+ {RPCResult::Type::STR, "result", "The result of the operation (success/failed)"},
The rationale is directly from #20552
Sorry, I should have phrased this more clearly, the initial thoughts I mentioned were numbered to be addressing the same numbered elements from the paragraph above. So specifically the rationale for “behaviour change: allow removing a v2 connection when !node_v2transport”, which I now understand is just to get it in line with the documentation.
Do you have any suggestions on another way to determine success or failure without a breaking API change?
I’m okay with breaking API changes when they’re meaningful, such as to address the issue outlined in #20552. My point was about changing the API for no good reason, such as the currently proposed new result, or the changes in error messages such as "Error: Node could not be removed. It has not been added previously." -> "Node could not be removed. It has not been added previously."
Concept ACK
There is definitely value in ensuring errors are provided to the user (e.g. no longer failing silently for unsuccessful onetry).
I could see there being value in returning the peer id, but think that might be better off for a follow up PR. This one could focus on fixing the problem of masking the error (and prevent discussion on a return object vs null from impacting what is otherwise a relatively straightforward bug fix).
If this PR remains scoped to returning the returning an object then deprecatedrpc should be implemented (giving users the ability to use the old return behavior temporarily). Something like -deprecatedrpc=addnode.
getpeerinfo accept a peer id param to just investigate that one?
Concept ACK
When addnode exits silently I have always found it annoying to have to go to the debug log to find out what happened.
Isn’t one of the addnode variants asynchronous?
make getpeerinfo accept a peer id param to just investigate that one
Sounds useful.
Didn’t re-read the discussion, but I’m not sure that no connection should throw, in favor of returning an “error” field (or “errors” object). @willcl-ark do you plan to update here soon?