-
Notifications
You must be signed in to change notification settings - Fork 37.8k
refactor: Add and use EnsureConnman in rpc code #21719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fa72296
to
fad37e6
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
Concept ACK: neat cleanups! |
There was a problem hiding this 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;
There was a problem hiding this 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
.
fad37e6
to
fafb68a
Compare
Thanks, done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fafb68a
re-ACK fafb68a |
There was a problem hiding this 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
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.
Use new EnsureConnman from bitcoin/bitcoin#21719 in the namecoin RPC code.
This removes the 10 occurrences of
throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
and replaces them withEnsureConnman
.