rpc: getaddrmaninfo followups

stratospher commented at 5:26 am on October 3, 2023: contributor

make getaddrmaninfo RPC public since it’s not for development purposes only and regular users might find it useful. #26988 (comment)
add missing all_networks key to RPC help. #27511 (comment)
fix clang format spacing
add and use EnsureAddrman in RPC code. #27511 (comment)

DrahtBot commented at 5:26 am on October 3, 2023: 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
ACK	theStack, pablomartin4btc, 0xB10C

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label RPC/REST/ZMQ on Oct 3, 2023

in src/rpc/net.cpp:1078 in 0538579abd outdated

1074@@ -1082,7 +1075,7 @@ void RegisterNetRPCCommands(CRPCTable& t)
1075         {"hidden", &addconnection},
1076         {"hidden", &addpeeraddress},
1077         {"hidden", &sendmsgtopeer},
1078-        {"hidden", &getaddrmaninfo},
1079+        {"network", &getaddrmaninfo},

kevkevinpal commented at 5:51 am on October 3, 2023:

nit: looks like this is ordered by commands using network then commands using hidden if you could move this up to line 1075

stratospher commented at 6:00 am on October 3, 2023:

done.

stratospher force-pushed on Oct 3, 2023

DrahtBot added the label CI failed on Oct 3, 2023

DrahtBot removed the label CI failed on Oct 3, 2023

0xB10C commented at 8:27 am on October 3, 2023: contributor

Code Review ACK b62f7d678fa3f9f6a5f32f8c7f4ae23e3ba1adff

The changes look good to me. I think it makes sense to un-hide this RPC.

Reviewers might want to use --ignore-all-space on the first commit.

stratospher commented at 2:14 pm on October 3, 2023: contributor

added a new commit to include refactor suggestion in #27511 (comment).

0xB10C commented at 2:35 pm on October 3, 2023: contributor

added a new commit to include refactor suggestion in #27511 (comment).

That’s useful for #28523 too. Can be included there in a follow up.

in src/rpc/net.cpp:1032 in 00421fe23e outdated

1029@@ -1032,24 +1030,22 @@ static RPCHelpMan getaddrmaninfo()
1030         RPCExamples{HelpExampleCli("getaddrmaninfo", "") + HelpExampleRpc("getaddrmaninfo", "")},
1031         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
1032             NodeContext& node = EnsureAnyNodeContext(request.context);

maflcko commented at 2:41 pm on October 3, 2023:

Could add EnsureAnyAddrman and remove this line as well?

stratospher commented at 3:31 am on October 4, 2023:

good idea. done in e6e444c.

DrahtBot added the label Needs rebase on Oct 3, 2023

rpc: `getaddrmaninfo` followups

- make `getaddrmaninfo` RPC public since it's not for development
  purposes only and regular users might find it useful
- add missing `all_networks` key to RPC help
- use clang format spacing

3931e6abc3

doc: add release notes for #27511 bf589a50a0

refactor: add and use EnsureAnyAddrman in rpc e6e444c06c

stratospher force-pushed on Oct 4, 2023

stratospher commented at 3:34 am on October 4, 2023: contributor

That’s useful for #28523 too. Can be included there in a follow up.

Rebased and included the EnsureAnyAddrman refactor in getrawaddrman too.

DrahtBot removed the label Needs rebase on Oct 4, 2023

in test/functional/rpc_net.py:377 in e6e444c06c

370@@ -371,11 +371,6 @@ def test_getaddrmaninfo(self):
371         self.log.info("Test getaddrmaninfo")
372         node = self.nodes[1]
373 
374-        self.log.debug("Test that getaddrmaninfo is a hidden RPC")
375-        # It is hidden from general help, but its detailed help may be called directly.
376-        assert "getaddrmaninfo" not in node.help()
377-        assert "getaddrmaninfo" in node.help("getaddrmaninfo")

kevkevinpal commented at 3:55 am on October 4, 2023:

is there a reason why we’re removing this test and not changing it to test that it isn’t a hidden rpc

stratospher commented at 4:47 am on October 4, 2023:

mostly because all the other functional tests only check whether it’s a hidden rpc. i think we’d have coverage from doing something like assert "getaddrmaninfo" in node.help() already with write_all_rpc_commands.

pablomartin4btc commented at 3:16 am on October 16, 2023:

IMO I’d leave the validation to detect whether the rpc command is hidden or not and make the change to the failing test accordingly depending if the change was intentional.

I’ve checked write_all_rpc_commands and I had to run the test_runner.py with argument --coverage twice with and without this branch in order to see if a rpc command is written in the coverage file or not (node.help()).

I think leaving the test as is it’s more practical, but perhaps I’m missing something or it’s just not as important.

pablomartin4btc commented at 6:47 pm on October 27, 2023:

After an offline chat, I do agree with the author that while we have these checks being performed to verify that an rpc command is hidden, the reverse action doesn’t make much sense as it’s not as dangerous.

theStack approved