Skip to content

Add CJDNS network to -addrinfo and -netinfo #23175

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
Closed
wants to merge 2 commits into from
Closed

Add CJDNS network to -addrinfo and -netinfo #23175

wants to merge 2 commits into from

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Oct 4, 2021 •
edited by laanwj
Loading

Follow-up to #23077 and #23324.

$ ./src/bitcoin-cli -addrinfo
{
  "addresses_known": {
    "ipv4": 47782,
    "ipv6": 10307,
    "onion": 8030,
    "i2p": 41,
    "cjdns": 1,
    "total": 66161
  }
}
$ ./src/bitcoin-cli -netinfo 
Bitcoin Core client v22.99.0-deb6223d4c55 - server 70016/Satoshi:22.99.0(jon)/

        ipv4    ipv6   onion     i2p   cjdns   total   block  manual
in         0       5      12       5       1      23
out        2       2       9       5       2      20       2      10
total      2       7      21      10       3      43

$ ./src/bitcoin-cli -netinfo 1

Screenshot from 2021-10-10 12-01-58

@laanwj
Copy link
Member

laanwj commented Oct 4, 2021

Concept ACK

@dunxen
Copy link
Contributor

dunxen commented Oct 5, 2021 •
edited
Loading

Concept ACK

I'm rebasing this on #23077 and testing it out :)

@practicalswift
Copy link
Contributor

Concept ACK

Thanks for improving -addrinfo and -netinfo.

@ghost
Copy link

ghost commented Oct 5, 2021

I am still not sure about CJDNS and have no opinions on #23077

However, looking at the support from others it will most likely get merged so this PR makes sense.

Concept ACK

@jonatack jonatack mentioned this pull request Oct 20, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 21, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

laanwj added a commit that referenced this pull request Oct 21, 2021
@laanwj
Merge #23324: Print peer counts for all reachable networks in -netinfo
ee1294f 
96f469f netinfo: print peer counts for all reachable networks (Jon Atack)

Pull request description:

  instead of only for networks we have peer connections to.

  Users reported the previous behavior caused confusion, as no column was printed when a network was reachable but no peers were connected. Users expected a column to be printed with 0 peers. This commit aligns behavior with that expectation.

  In addition, the ipv4, ipv6, and onion columns were always printed whether or not they were reachable. With this change, only the reachable ones will be returned.

  Example with CJDNS reachable but no CJDNS peers (built on #23077 and #23175):

  before
  ```
          ipv4    ipv6   onion     i2p   total   block  manual
  in         0       0      12       5      17
  out        8       1       6       4      19       2       8
  total      8       1      18       9      36
  ```
  after
  ```
           ipv4    ipv6   onion     i2p   cjdns   total   block  manual
  in          0       0      12       5       0      17
  out         8       1       6       4       0      19       2       8
  total       8       1      18       9       0      36
  ```

  There is one additional space between the in/out/total row headers and the network counts.

ACKs for top commit:
  jsarenik:
    Tested ACK 96f469f
  prayank23:
    utACK 96f469f
  laanwj:
    Code review and lightly tested ACK 96f469f
  naumenkogs:
    ACK 96f469f.
  hebasto:
    ACK 96f469f, tested by comparing the output of `watch src/bitcoin-cli -netinfo` with the Peer tab of the Node window in the GUI 🐅

Tree-SHA512: 3489f40148a2bd0afc9eef1e1577d44150c1fccec8dbf2a675bc23aa9343bfcae6c4039f5b96e54730668c83f40bc932fb6808f5540e86ff7249fde8dc0fff67
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2021
@laanwj
Merge bitcoin#23324: Print peer counts for all reachable networks in …
b43d231 
…-netinfo

96f469f netinfo: print peer counts for all reachable networks (Jon Atack)

Pull request description:

  instead of only for networks we have peer connections to.

  Users reported the previous behavior caused confusion, as no column was printed when a network was reachable but no peers were connected. Users expected a column to be printed with 0 peers. This commit aligns behavior with that expectation.

  In addition, the ipv4, ipv6, and onion columns were always printed whether or not they were reachable. With this change, only the reachable ones will be returned.

  Example with CJDNS reachable but no CJDNS peers (built on bitcoin#23077 and bitcoin#23175):

  before
  ```
          ipv4    ipv6   onion     i2p   total   block  manual
  in         0       0      12       5      17
  out        8       1       6       4      19       2       8
  total      8       1      18       9      36
  ```
  after
  ```
           ipv4    ipv6   onion     i2p   cjdns   total   block  manual
  in          0       0      12       5       0      17
  out         8       1       6       4       0      19       2       8
  total       8       1      18       9       0      36
  ```

  There is one additional space between the in/out/total row headers and the network counts.

ACKs for top commit:
  jsarenik:
    Tested ACK 96f469f
  prayank23:
    utACK bitcoin@96f469f
  laanwj:
    Code review and lightly tested ACK 96f469f
  naumenkogs:
    ACK 96f469f.
  hebasto:
    ACK 96f469f, tested by comparing the output of `watch src/bitcoin-cli -netinfo` with the Peer tab of the Node window in the GUI 🐅

Tree-SHA512: 3489f40148a2bd0afc9eef1e1577d44150c1fccec8dbf2a675bc23aa9343bfcae6c4039f5b96e54730668c83f40bc932fb6808f5540e86ff7249fde8dc0fff67
@Zero-1729
Copy link
Contributor

Concept ACK

@laanwj
Copy link
Member

laanwj commented Nov 8, 2021 •
edited
Loading

Needs rebase (and can go out of draft) now that #23175 is merged.

@prusnak
Copy link
Contributor

prusnak commented Nov 8, 2021

Needs rebase (and can go out of draft) now that #23175 is merged.

laanwj meant #23077 (#23175 is this PR)

@jonatack jonatack marked this pull request as ready for review November 11, 2021 12:25
@jonatack
Copy link
Member Author

Thanks! Rebased and updated (simplified) following the merge of #23324, and bringing out of draft now that #23077 has been merged.

@jonatack jonatack force-pushed the add-cjdns-to-addrinfo-and-netinfo branch from deb6223 to 7b65287 Compare November 11, 2021 12:26
prusnak
prusnak approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

utACK

@laanwj
Copy link
Member

laanwj commented Nov 15, 2021

Code review ACK 7b65287

laanwj added a commit to bitcoin-core/gui that referenced this pull request Nov 15, 2021
@laanwj
Merge bitcoin/bitcoin#23175: Add CJDNS network to -addrinfo and -netinfo
caf8b26 
7b65287 cli: hoist networks class data members to a constant (Jon Atack)
5bd40a3 cli: add cjdns network to -addrinfo and -netinfo (Jon Atack)

Pull request description:

  Follow-up to #23077 and #23324.
  ```
  $ ./src/bitcoin-cli -addrinfo
  {
    "addresses_known": {
      "ipv4": 47782,
      "ipv6": 10307,
      "onion": 8030,
      "i2p": 41,
      "cjdns": 1,
      "total": 66161
    }
  }
  $ ./src/bitcoin-cli -netinfo
  Bitcoin Core client v22.99.0-deb6223d4c55 - server 70016/Satoshi:22.99.0(jon)/

          ipv4    ipv6   onion     i2p   cjdns   total   block  manual
  in         0       5      12       5       1      23
  out        2       2       9       5       2      20       2      10
  total      2       7      21      10       3      43
  ```
  ```
  $ ./src/bitcoin-cli -netinfo 1
  ```

  ![Screenshot from 2021-10-10 12-01-58](https://user-images.githubusercontent.com/2415484/136691258-8b3fa7aa-3edb-4428-854a-adadfef302e3.png)

ACKs for top commit:
  laanwj:
    Code review ACK 7b65287

Tree-SHA512: 9c740d394d9842d38a1c01a824271b25277baac11ed090f0430daa15b580c2bf3d114ac6b8254b19d6aaee57cbe1ca6a414996e6994e0bf4a577bed771382eca
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@prusnak
Copy link
Contributor

prusnak commented Nov 15, 2021

This PR has been merged into master via caf8b26

@sipa sipa closed this Nov 15, 2021
@sipa
Copy link
Member

sipa commented Nov 15, 2021

Closing as merged.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 16, 2021
@laanwj
Merge bitcoin#23175: Add CJDNS network to -addrinfo and -netinfo
5371b96 
7b65287 cli: hoist networks class data members to a constant (Jon Atack)
5bd40a3 cli: add cjdns network to -addrinfo and -netinfo (Jon Atack)

Pull request description:

  Follow-up to bitcoin#23077 and bitcoin#23324.
  ```
  $ ./src/bitcoin-cli -addrinfo
  {
    "addresses_known": {
      "ipv4": 47782,
      "ipv6": 10307,
      "onion": 8030,
      "i2p": 41,
      "cjdns": 1,
      "total": 66161
    }
  }
  $ ./src/bitcoin-cli -netinfo
  Bitcoin Core client v22.99.0-deb6223d4c55 - server 70016/Satoshi:22.99.0(jon)/

          ipv4    ipv6   onion     i2p   cjdns   total   block  manual
  in         0       5      12       5       1      23
  out        2       2       9       5       2      20       2      10
  total      2       7      21      10       3      43
  ```
  ```
  $ ./src/bitcoin-cli -netinfo 1
  ```

  ![Screenshot from 2021-10-10 12-01-58](https://user-images.githubusercontent.com/2415484/136691258-8b3fa7aa-3edb-4428-854a-adadfef302e3.png)

ACKs for top commit:
  laanwj:
    Code review ACK 7b65287

Tree-SHA512: 9c740d394d9842d38a1c01a824271b25277baac11ed090f0430daa15b580c2bf3d114ac6b8254b19d6aaee57cbe1ca6a414996e6994e0bf4a577bed771382eca
@jonatack jonatack deleted the add-cjdns-to-addrinfo-and-netinfo branch November 16, 2021 01:25
@prusnak prusnak mentioned this pull request Nov 19, 2021
Closed
@bitcoin bitcoin locked and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@prusnak prusnak prusnak approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Needs rebase Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jonatack @laanwj @dunxen @practicalswift @DrahtBot @Zero-1729 @prusnak @sipa