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

willcl-ark · 2024-07-03T10:21:04Z

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

DrahtBot · 2024-07-03T10:21:07Z

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.

willcl-ark · 2024-07-03T10:22:08Z

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

willcl-ark · 2024-07-03T10:26:38Z

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

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.

stickies-v

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?

stickies-v · 2024-07-04T10:54:17Z

src/rpc/net.cpp

    }

-    if (command == "add")
-    {
+    std::string error_message;


nit: is this necessary?

willcl-ark · 2024-07-04T14:11:40Z

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:

bitcoin/src/rpc/net.cpp

Line 317 in bd5d168

    
           {"v2transport", RPCArg::Type::BOOL, RPCArg::DefaultHint{"set by -v2transport"}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"},

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

jonatack

Concept ACK, I'd find this useful.

jonatack · 2024-07-04T14:50:05Z

src/rpc/net.cpp

+
+    if (command == "onetry") {
+        if (!connman.OpenNetworkConnection(CAddress(), false, {}, node_arg.c_str(), ConnectionType::MANUAL, use_v2transport)) {
+            throw JSONRPCError(RPC_CLIENT_NODE_NOT_ADDED, "Unable to open connection");


Suggest Unable to open connection to <peer address>

jonatack · 2024-07-04T14:52:43Z

src/rpc/net.cpp

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


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

stickies-v · 2024-07-04T14:59:14Z

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

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 · 2024-07-06T17:42:24Z

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 · 2024-09-28T04:33:35Z

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 · 2024-09-28T13:14:39Z

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?

DrahtBot · 2025-02-05T16:25:32Z

🐙 This pull request conflicts with the target branch and needs rebase.

willcl-ark · 2025-02-20T19:21:52Z

Closing for now until I can circle back.

c6eca6f doc: add guidance for RPC to developer notes (tdb3) Pull request description: Adds guidance statements to the RPC interface section of the developer notes with examples of when to implement `-deprecatedrpc=`. Wanted to increase awareness of preferred RPC implementation approaches for newer contributors. This implements some of what's discussed in #29912 (comment) Opinions may differ, so please don't be shy. We want to make RPC as solid/safe as possible. Examples of discussions where guidelines/context might have added value: #30212 (comment) #29845 (comment) #30381 (review) #29954 (comment) #30410 (review) #30713 #30381 #29060 (review) ACKs for top commit: l0rinc: ACK c6eca6f fjahr: ACK c6eca6f maflcko: lgtm ACK c6eca6f jonatack: ACK c6eca6f Tree-SHA512: 01a98a8dc0eb91762b225d3278cdb4a5e380ceb7486fd096b4ad9122bed859cea8584d8996d3dce51272fdb792f4a793a1bd1c5445efeb87f0a30f9b6e59a790

DrahtBot added the Needs rebase label Jul 3, 2024

willcl-ark mentioned this pull request Jul 3, 2024

addnode RPC call should return success/failure indicator #20552

Open

willcl-ark and others added 3 commits July 3, 2024 11:43

net: make OpenNetworkConnection return a bool

cb3f784

This lets us propagate errors back to `CConnman::AddConnection()` and the `addnode` RPC in the future. Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

net: make CConnMan::AddConnection return success

53e5882

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>

willcl-ark force-pushed the addnode-failure branch from 14312c3 to 4c19971 Compare July 3, 2024 10:46

DrahtBot removed the Needs rebase label Jul 3, 2024

DrahtBot mentioned this pull request Jul 3, 2024

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Open

stickies-v reviewed Jul 4, 2024

View reviewed changes

src/rpc/net.cpp

}

if (command == "add")

{

std::string error_message;

Copy link

Contributor

stickies-v Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is this necessary?

jonatack reviewed Jul 4, 2024

View reviewed changes

DrahtBot mentioned this pull request Jul 5, 2024

net: fix race condition in self-connect detection #30394

Merged

tdb3 reviewed Jul 5, 2024

View reviewed changes

tdb3 mentioned this pull request Jul 5, 2024

doc: add guidance for RPC to developer notes #30142

Merged

DrahtBot mentioned this pull request Aug 6, 2024

tracing: network connection tracepoints #25832

Merged

DrahtBot mentioned this pull request Sep 27, 2024

Split CConnman #30988

Draft

DrahtBot added the CI failed label Oct 19, 2024

DrahtBot removed the CI failed label Oct 27, 2024

DrahtBot mentioned this pull request Nov 9, 2024

Fuzz: extend CConnman tests #28584

Open

DrahtBot added the Needs rebase label Feb 5, 2025

willcl-ark closed this Feb 20, 2025

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

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

Uh oh!

Conversation

willcl-ark commented Jul 3, 2024

Uh oh!

DrahtBot commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

willcl-ark commented Jul 3, 2024

Uh oh!

willcl-ark commented Jul 3, 2024

Uh oh!

stickies-v left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented Jul 4, 2024

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

stickies-v commented Jul 4, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Jul 6, 2024

Uh oh!

vasild commented Sep 28, 2024

Uh oh!

jonatack commented Sep 28, 2024

Uh oh!

DrahtBot commented Feb 5, 2025

Uh oh!

willcl-ark commented Feb 20, 2025

Uh oh!

Uh oh!

DrahtBot commented Jul 3, 2024 •

edited

Loading

stickies-v left a comment •

edited

Loading