Skip to content
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

rpc: getaddrmaninfo followups #28565

Merged
merged 3 commits into from Oct 16, 2023

Conversation

stratospher
Copy link
Contributor

@stratospher stratospher commented Oct 3, 2023

  • 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
Copy link
Contributor

DrahtBot commented Oct 3, 2023

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.

src/rpc/net.cpp Outdated Show resolved Hide resolved
@0xB10C
Copy link
Contributor

0xB10C commented Oct 3, 2023

Code Review ACK b62f7d6

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
Copy link
Contributor Author

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

@0xB10C
Copy link
Contributor

0xB10C commented Oct 3, 2023

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.

src/rpc/net.cpp Outdated Show resolved Hide resolved
- 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
@stratospher
Copy link
Contributor Author

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

Rebased and included the EnsureAnyAddrman refactor in getrawaddrman too.

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.

Code-review ACK e6e444c

@DrahtBot DrahtBot requested a review from 0xB10C October 10, 2023 10:42
Copy link
Member

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tested ACK e6e444c

Verified that the rpc command `getaddrmaninfo` is now public (not hidden anymore),

./src/bitcoin-cli -signet help
...
image
...

RPC Help shows `all_networks`

image

and RPC Help doesn't display "This RPC is for testing only." anymore before

image

after

image

test/functional/rpc_net.py Show resolved Hide resolved
@0xB10C
Copy link
Contributor

0xB10C commented Oct 16, 2023

Code Review re-ACK e6e444c

@DrahtBot DrahtBot removed the request for review from 0xB10C October 16, 2023 08:11
@maflcko maflcko added this to the 27.0 milestone Oct 16, 2023
@fanquake fanquake merged commit 22fa1f4 into bitcoin:master Oct 16, 2023
16 checks passed
@maflcko maflcko removed this from the 27.0 milestone Oct 16, 2023
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 19, 2023
- 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

Github-Pull: bitcoin#28565
Rebased-From: 3931e6a
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants