Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Apr 17, 2021

This removes the 10 occurrences of throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled"); and replaces them with EnsureConnman.

@DrahtBot
Copy link
Contributor

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

Conflicts

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.

@practicalswift
Copy link
Contributor

Concept ACK: neat cleanups!

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK fad37e6

This is a nice simplification. Tested the error message with the patch below. Used a similar patch with EnsurePeerman. Tested that, with this refactor, functionality remains the same by applying a similar patch to the occurrences of if(!node.connman).

--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -41,7 +41,7 @@ const std::vector<std::string> CONNECTION_TYPE_DOC{
 
 CConnman& EnsureConnman(const NodeContext& node)
 {
-    if (!node.connman) {
+    if (true) {
         throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
     }
     return *node.connman;

@DrahtBot DrahtBot mentioned this pull request Apr 18, 2021
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Concept ACK

There is one other instance in the ping RPC that can be tackled with EnsurePeerman:

diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 4d192ad95..fff7d3b60 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -92,12 +92,10 @@ static RPCHelpMan ping()
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
     NodeContext& node = EnsureAnyNodeContext(request.context);
-    if (!node.peerman) {
-        throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
-    }
+    PeerManager& peerman = EnsurePeerman(node);

     // Request that each node send a ping during next message processing pass
-    node.peerman->SendPings();
+    peerman.SendPings();
     return NullUniValue;
 },
     };

Also, including <any> isn't needed in the new header src/rpc/net.h.

@maflcko
Copy link
Member Author

maflcko commented Apr 19, 2021

Thanks, done

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK fafb68a

@jarolrod
Copy link
Member

re-ACK fafb68a

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK fafb68a

@maflcko maflcko merged commit 2bce932 into bitcoin:master Apr 21, 2021
@maflcko maflcko deleted the 2104-netConnman branch April 21, 2021 06:07
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 21, 2021
domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Apr 28, 2021
Use new EnsureConnman from bitcoin/bitcoin#21719
in the auxpow check code, and also use the request context chainstate
there to get rid of globals.
domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Apr 28, 2021
Use new EnsureConnman from bitcoin/bitcoin#21719
in the namecoin RPC code.
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants