[WIP] net: return result from addnode RPC

willcl-ark commented at 10:21 am on July 3, 2024: member

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}

DrahtBot commented at 10:21 am on July 3, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	jonatack, tdb3, vasild

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:

#30988 (Split CConnman by vasild)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#25832 (tracing: network connection tracepoints by 0xB10C)

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.

willcl-ark commented at 10:22 am on July 3, 2024: member

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

DrahtBot added the label Needs rebase on Jul 3, 2024

willcl-ark commented at 10:26 am on July 3, 2024: member

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

net: make OpenNetworkConnection return a bool

This lets us propagate errors back to `CConnman::AddConnection()` and
the `addnode` RPC in the future.

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

cb3f784e54

net: make CConnMan::AddConnection return success

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>

53e5882a86

rpc: return result from `addnode` rpc

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.

4c199714f9

willcl-ark force-pushed on Jul 3, 2024

DrahtBot removed the label Needs rebase on Jul 3, 2024

in src/rpc/net.cpp:352 in 4c199714f9

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;

stickies-v commented at 10:54 am on July 4, 2024:

nit: is this necessary?

stickies-v commented at 10:55 am on July 4, 2024: contributor

There’s a lot of stuff happening in one commit:

behaviour change: allow removing a v2 connection when !node_v2transport
behaviour change: explicitly throwing a a JSONRPCError when opening the connection for a onetry fails, instead of returning NULL (which makes it indistinguishable from a success operation)
behaviour change: always returning a results object with input values and result==success
clang-tidy fixes

I 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:

can you talk more about the rationale here?
this seems like a useful improvement, not being able to distinguish between a failure and success case is problematic
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 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:

if we’re going to return something, the peer id seems like actual useful (new) information?
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?

willcl-ark commented at 2:11 pm on July 4, 2024: member

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 JSONRPCError when opening the connection for a onetry fails, 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 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.

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! ❤️

in src/rpc/net.cpp:356 in 4c199714f9

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

jonatack commented at 2:50 pm on July 4, 2024:

Suggest Unable to open connection to <peer address>

in src/rpc/net.cpp:323 in 4c199714f9

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

jonatack commented at 2:52 pm on July 4, 2024:

Agree with @stickies-v that returning the peer id would be more useful than the result.

jonatack commented at 2:53 pm on July 4, 2024: member

Concept ACK, I’d find this useful.

stickies-v commented at 2:59 pm on July 4, 2024: contributor

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

tdb3 commented at 2:44 pm on July 5, 2024: contributor

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.

luke-jr commented at 5:42 pm on July 6, 2024: member

The return value seems entirely redundant. Maybe it should just be the new peer id (Object-encapsulated for future extension), and make getpeerinfo accept a peer id param to just investigate that one?

vasild commented at 4:33 am on September 28, 2024: contributor

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?

jonatack commented at 1:14 pm on September 28, 2024: member

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?

[WIP] net: return result from addnode RPC #30381

Code Coverage

Reviews

Conflicts