Skip to content

cli: Display all proxies in -getinfo #22959

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 1 commit into from
Sep 20, 2021
Merged

cli: Display all proxies in -getinfo #22959

laanwj merged 1 commit into from
Sep 20, 2021

Conversation

@klementtan
Copy link
Contributor

@klementtan klementtan commented Sep 13, 2021 •
edited
Loading

Changes: Display all proxies in -getinfo

Motivation:

  • Currently -getinfo only return the proxy of the first network in getnetworkinfo.
  • This PR will display all unique proxies in getnetworkinfo as suggested in Re-thinking bitcoin-cli -getinfo #17314 (comment)

    List all proxies, at least if they're different from the IPv4 one

image

Testing:

You can verify this change by starting bitcoind with

./src/bitcoind -signet --proxy=127.0.0.1:9050 --i2psam=127.0.0.1:7656

Execute -getinfo

./src/bitcoin-cli -signet -getinfo

@ghost
Copy link

ghost commented Sep 13, 2021

Concept ACK

Since i2p is added in Bitcoin Core which uses a different proxy, it's better if both are shared in -getinfo

-i2psam is used for proxy and incoming both as documented in doc/i2p.md

theStack
theStack reviewed Sep 14, 2021
View reviewed changes
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

@klementtan klementtan force-pushed the getinfo-multiple-proxies branch from 60502df to 869ad51 Compare September 15, 2021 01:06
@Zero-1729
Copy link
Contributor

Zero-1729 commented Sep 15, 2021

Concept ACK

@laanwj
Copy link
Member

laanwj commented Sep 16, 2021

Concept ACK. Showing only one proxy is an artifact from the old getinfo RPC call, we can do better now.

Suggestion: maybe add the networks each proxy is used for, example format

Proxies: 127.0.0.1:7656 (i2p) 127.0.0.1:9050 (ipv4,ipv6,onion)

@ghost
Copy link

ghost commented Sep 16, 2021 •
edited by ghost
Loading

Suggestion: maybe add the networks each proxy is used for, example format

@laanwj Is this something similar to checkboxes we have in GUI which I was trying to remove in bitcoin-core/gui#421?

If yes, this is confusing for me TBH considering the changes and discussions we had for #22834

Also the comments:

127 2021-08-30T11:36:52 vasild: when using a proxy, bitcoind has no idea whether it's connecting to ipv4, ipv6 or something else completely :)

https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-30.html#l-127

@klementtan klementtan force-pushed the getinfo-multiple-proxies branch from 869ad51 to 7d6f0fa Compare September 19, 2021 15:27
@klementtan
Copy link
Contributor Author

klementtan commented Sep 19, 2021 •
edited
Loading

@laanwj Thanks for the suggestion.

869ad51 to 7d6f0fa: Updated the PR to display the networks each proxy is used for.

image

jonatack
jonatack reviewed Sep 19, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested almost-ACK. The order of proxies is sorted by proxy value; maybe maintain the original order printed by getnetworkinfo "networks".

@klementtan klementtan force-pushed the getinfo-multiple-proxies branch from 7d6f0fa to dedbc9d Compare September 20, 2021 10:34
@klementtan klementtan force-pushed the getinfo-multiple-proxies branch from dedbc9d to 7c3712f Compare September 20, 2021 10:52
@klementtan
Copy link
Contributor Author

klementtan commented Sep 20, 2021

7c3712f changes:

  • Use , to separate proxies
  • s/N/A/n/a
  • Proxies are displayed in the same order as getnetworkinfo. Added a new std::vector<std::string> ordered_proxies to store the original ordering of the proxies.
    image

@laanwj
Copy link
Member

laanwj commented Sep 20, 2021 •
edited
Loading

Thank you! Will test.

  • ✔️ Node without any proxy configured
Proxies: n/a
  • ✔️ Clearnet node which is also connected to onion/i2p:
Proxies: 127.0.0.1:9050 (onion), 127.0.0.1:7656 (i2p)
  • ✔️ Generic -proxy=…:
Proxies: 127.0.0.1:9050 (ipv4, ipv6, onion)
  • ✔️ Different -proxy=… -onion=…
Proxies: 127.0.0.1:3333 (ipv4, ipv6), 127.0.0.1:9050 (onion)

Tested ACK 7c3712f

127 2021-08-30T11:36:52 vasild: when using a proxy, bitcoind has no idea whether it's connecting to ipv4, ipv6 or something else completely :)

This was specifically about names, so connecting to a DNS seed, for example, or a node passed as argument. For P2P peers it is known what the network is because this information is gossiped along with the address itself. This is what the information here is about.

@ghost
Copy link

ghost commented Sep 20, 2021

utACK 7c3712f

@laanwj laanwj merged commit d809d8b into bitcoin:master Sep 20, 2021
@klementtan klementtan deleted the getinfo-multiple-proxies branch September 20, 2021 16:37
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 21, 2021
@laanwj
Merge bitcoin#22959: cli: Display all proxies in -getinfo
d862d3f 
7c3712f cli: Display all proxies in -getinfo (klementtan)

Pull request description:

  **Changes**: Display all proxies in `-getinfo`

  **Motivation**:

  * Currently `-getinfo` only return the proxy of the first network in `getnetworkinfo`.
  * This PR will display all unique proxies in `getnetworkinfo` as suggested in bitcoin#17314 (comment)
       >List all proxies, at least if they're different from the IPv4 one

  ![image](https://user-images.githubusercontent.com/49265907/133991832-a1f38b36-2975-4ce2-a427-e4ffab23383e.png)

  **Testing**:

  You can verify this change by starting bitcoind with
  ```shell
  ./src/bitcoind -signet --proxy=127.0.0.1:9050 --i2psam=127.0.0.1:7656
  ```

  Execute `-getinfo`
  ```shell
  ./src/bitcoin-cli -signet -getinfo
  ```

ACKs for top commit:
  laanwj:
    Tested ACK 7c3712f
  prayank23:
    utACK bitcoin@7c3712f

Tree-SHA512: 9eae97866220227f30ca4585f52799fa66fc1135047d869c4aabe598aee1a9414cb9e1c4a8d19165e52d65005f3c6d4bcc37463ace0ddb44389dfbcd4ca74095
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@theStack theStack theStack left review comments

@jonatack jonatack jonatack left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Utils/log/libs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@klementtan @Zero-1729 @laanwj @theStack @jonatack @DrahtBot