bitcoin [WIP] net: return result from addnode RPC

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:

{
    {RPCResult::Type::STR, "operation", "The operation called (onetry/add/remove)"},
    {RPCResult::Type::STR, "result", "The result of the operation (success/failed)"},
    {RPCResult::Type::STR, "address", "The address of the peer"},
}

Jul 03 '24 10:07 willcl-ark

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)
#28584 (Fuzz: extend CConnman tests 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.

Jul 03 '24 10:07 DrahtBot

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

Jul 03 '24 10:07 willcl-ark

Before these changes:

$ src/bitcoin-cli -signet addnode i-am-not-an-address onetry

After:

$ src/bitcoin-cli -signet addnode i-am-not-an-address onetry
error code: -24
error message:
Unable to open connection

Jul 03 '24 10:07 willcl-ark

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 https://github.com/bitcoin/bitcoin/issues/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! ❤️

Jul 04 '24 14:07 willcl-ark

The rationale is directly from https://github.com/bitcoin/bitcoin/issues/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."

Jul 04 '24 14:07 stickies-v

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?