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

change(rpc): Add getpeerinfo RPC method #5951

Merged
merged 58 commits into from
Jan 17, 2023
Merged

change(rpc): Add getpeerinfo RPC method #5951

merged 58 commits into from
Jan 17, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jan 13, 2023

Motivation

Mining pools use the getpeerinfo RPC to make sure the mining pool is still connected to peers.

Depends-On: #5884

Closes #5723.

Solution

  • Passes the address_book to RpcServer::spawn and GetBlockTemplateRpcImpl
  • Calls recently_live_peers and returns a list of peer addresses
  • Adds AddressBookPeers trait and MockAddressBookPeers for use in tests
  • Updates recently_live_peers method to return a Vec instead of an iterator

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

arya2 and others added 30 commits December 15, 2022 00:41
skips solution check for BlockProposal requests

calls CheckBlockValidity instead of Commit block for BlockProposal requests
adds/fixes some comments, adds TODOs

general cleanup from a self-review.
@teor2345 teor2345 self-requested a review January 16, 2023 07:21
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good!

Can you please post a zcash-rpc-diff getpeerinfo with zcashd, to make sure we have the response format correct?

Do we need to add a test that calls this RPC in CI?
It doesn't need to have a cached state, it can just be one of the acceptance tests. This will also avoid any breaking changes to the RPC.

zebra-network/src/address_book.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_observer.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 changed the title change(rpc): Add getpeerinfo method change(rpc): Add getpeerinfo RPC method Jan 16, 2023
@teor2345 teor2345 added the S-waiting-on-author Status: waiting on the ticket or PR author label Jan 16, 2023
@arya2
Copy link
Contributor Author

arya2 commented Jan 16, 2023

Looks good!

Can you please post a zcash-rpc-diff getpeerinfo with zcashd, to make sure we have the response format correct?

Yep:

zebra git:(getpeerinfo-rpc) ZCASH_CLI=../zcash/src/zcash-cli ./zebra-utils/zcash-rpc-diff 3000 getpeerinfo
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 3000) and zcashd (../zcash/src/zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

WARNING: comparing RPC responses from different heights:
zebrad is at: 1951017
zcashd is at: 1951019

Request:
getpeerinfo

Querying zebrad main chain at height >=1951017...

real    0m0.002s
user    0m0.002s
sys     0m0.000s

Querying zcashd main chain at height >=1951019...

real    0m0.002s
user    0m0.002s
sys     0m0.000s


Response diff between zebrad and zcashd:

Response:

--- /tmp/tmp.gnxKauLyNF.rpc-diff/zebrad-main-1951017-getpeerinfo.json   2023-01-16 18:11:08.681000866 -0500
+++ /tmp/tmp.gnxKauLyNF.rpc-diff/zcashd-main-1951019-getpeerinfo.json   2023-01-16 18:11:08.681000866 -0500
@@ -1,80 +1,193 @@
 [
   {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
-  },
-  {
-    "addr": "x.x.x.x:8233"
+    "id": 1,
+    "addr": "x.x.x.x:8233",
+    "addrlocal": "x.x.x.x:61556",
+    "services": "0000000000000005",
+    "relaytxes": true,
+    "lastsend": 1673910652,
+    "lastrecv": 1673910652,
+    "bytessent": 598109,
+    "bytesrecv": 118785965,
+    "conntime": 1673895922,
+    "timeoffset": 0,
+    "pingtime": 0.106408,
+    "version": 170100,
+    "subver": "/MagicBean:5.3.0/",
+    "inbound": false,
+    "startingheight": 1950812,
+    "banscore": 0,
+    "synced_headers": 1951019,
+    "synced_blocks": 1951019,
+    "inflight": [
+    ],
+    "whitelisted": false
+  },
+  {
+    "id": 3,
+    "addr": "x.x.x.x:8233",
+    "services": "0000000000000001",
+    "relaytxes": true,
+    "lastsend": 1673910650,
+    "lastrecv": 1673910652,
+    "bytessent": 2853306,
+    "bytesrecv": 43265172,
+    "conntime": 1673895923,
+    "timeoffset": -1,
+    "pingtime": 0.240153,
+    "version": 170100,
+    "subver": "/MagicBean:5.3.2/",
+    "inbound": false,
+    "startingheight": 1950812,
+    "banscore": 0,
+    "synced_headers": 1951019,
+    "synced_blocks": 1951019,
+    "inflight": [
+    ],
+    "whitelisted": false
+  },
+  {
+    "id": 4,
+    "addr": "x.x.x.x:8233",
+    "addrlocal": "x.x.x.x:61596",
+    "services": "0000000000000005",
+    "relaytxes": true,
+    "lastsend": 1673910654,
+    "lastrecv": 1673910657,
+    "bytessent": 7694700,
+    "bytesrecv": 66215784,
+    "conntime": 1673895924,
+    "timeoffset": -4,
+    "pingtime": 0.107663,
+    "version": 170100,
+    "subver": "/MagicBean:5.3.2/",
+    "inbound": false,
+    "startingheight": 1950812,
+    "banscore": 0,
+    "synced_headers": 1951019,
+    "synced_blocks": 1951019,
+    "inflight": [
+    ],
+    "whitelisted": false
+  },
+  {
+    "id": 5,
+    "addr": "x.x.x.x:8233",
+    "addrlocal": "x.x.x.x:61580",
+    "services": "0000000000000005",
+    "relaytxes": true,
+    "lastsend": 1673910652,
+    "lastrecv": 1673910647,
+    "bytessent": 20002261,
+    "bytesrecv": 37563937,
+    "conntime": 1673895927,
+    "timeoffset": -2,
+    "pingtime": 0.08204500000000001,
+    "version": 170100,
+    "subver": "/MagicBean:5.3.2/",
+    "inbound": false,
+    "startingheight": 1950812,
+    "banscore": 0,
+    "synced_headers": 1951019,
+    "synced_blocks": 1951019,
+    "inflight": [
+    ],
+    "whitelisted": false
+  },
+  {
+    "id": 6,
+    "addr": "x.x.x.x:8233",
+    "addrlocal": "x.x.x.x:61628",
+    "services": "0000000000000005",
+    "relaytxes": true,
+    "lastsend": 1673910653,
+    "lastrecv": 1673910653,
+    "bytessent": 1109135,
+    "bytesrecv": 42833285,
+    "conntime": 1673895936,
+    "timeoffset": 0,
+    "pingtime": 0.036377,
+    "version": 170100,
+    "subver": "/MagicBean:5.3.0/",
+    "inbound": false,
+    "startingheight": 1950812,
+    "banscore": 0,
+    "synced_headers": 1951019,
+    "synced_blocks": 1951019,
+    "inflight": [
+    ],
+    "whitelisted": false
+  },
+  {
+    "id": 7,
+    "addr": "x.x.x.x:8233",
+    "addrlocal": "x.x.x.x:61574",
+    "services": "0000000000000005",
+    "relaytxes": true,
+    "lastsend": 1673910653,
+    "lastrecv": 1673910653,
+    "bytessent": 2019806,
+    "bytesrecv": 39574709,
+    "conntime": 1673895938,
+    "timeoffset": -1,
+    "pingtime": 0.119679,
+    "version": 170100,
+    "subver": "/MagicBean:5.3.0/",
+    "inbound": false,
+    "startingheight": 1950812,
+    "banscore": 0,
+    "synced_headers": 1951019,
+    "synced_blocks": 1951019,
+    "inflight": [
+    ],
+    "whitelisted": false
+  },
+  {
+    "id": 8,
+    "addr": "x.x.x.x:8233",
+    "addrlocal": "x.x.x.x:61632",
+    "services": "0000000000000005",
+    "relaytxes": true,
+    "lastsend": 1673910653,
+    "lastrecv": 1673910653,
+    "bytessent": 3618634,
+    "bytesrecv": 45650400,
+    "conntime": 1673895945,
+    "timeoffset": -1,
+    "pingtime": 0.181401,
+    "version": 170100,
+    "subver": "/MagicBean:5.3.2/",
+    "inbound": false,
+    "startingheight": 1950812,
+    "banscore": 0,
+    "synced_headers": 1951019,
+    "synced_blocks": 1951019,
+    "inflight": [
+    ],
+    "whitelisted": false
+  },
+  {
+    "id": 9,
+    "addr": "x.x.x.x:8233",
+    "addrlocal": "x.x.x.x:61558",
+    "services": "0000000000000005",
+    "relaytxes": true,
+    "lastsend": 1673910660,
+    "lastrecv": 1673910663,
+    "bytessent": 204732,
+    "bytesrecv": 57428842,
+    "conntime": 1673896001,
+    "timeoffset": 0,
+    "pingtime": 0.028809,
+    "version": 170100,
+    "subver": "/MagicBean:5.3.0/",
+    "inbound": false,
+    "startingheight": 1950812,
+    "banscore": 0,
+    "synced_headers": 1951019,
+    "synced_blocks": 1951019,
+    "inflight": [
+    ],
+    "whitelisted": false
   }
 ]

Do we need to add a test that calls this RPC in CI? It doesn't need to have a cached state, it can just be one of the acceptance tests. This will also avoid any breaking changes to the RPC.

Okay, I'll add an acceptance test.

@arya2 arya2 changed the base branch from main to gbt-proposal-test January 17, 2023 00:30
@arya2 arya2 requested a review from teor2345 January 17, 2023 00:51
teor2345
teor2345 previously approved these changes Jan 17, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks for the tests!

Base automatically changed from gbt-proposal-test to main January 17, 2023 04:03
@teor2345 teor2345 removed the S-waiting-on-author Status: waiting on the ticket or PR author label Jan 17, 2023
mergify bot added a commit that referenced this pull request Jan 17, 2023
@mergify mergify bot merged commit 1bb8a9c into main Jan 17, 2023
@mergify mergify bot deleted the getpeerinfo-rpc branch January 17, 2023 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Async code, needs extra work to make it work properly. A-network Area: Network protocol updates or fixes A-rpc Area: Remote Procedure Call interfaces C-enhancement Category: This is an improvement C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the getpeerinfo RPC
3 participants