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: implement get_public_nodes command #5607

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@xiphon
Copy link
Contributor

commented Jun 4, 2019

Requires #5606

@xiphon xiphon force-pushed the xiphon:rpc-get_public_nodes branch from b99446e to 67bafc3 Jun 4, 2019

@xiphon xiphon force-pushed the xiphon:rpc-get_public_nodes branch from 67bafc3 to 6dbf97d Jun 4, 2019

@@ -84,7 +84,7 @@ namespace cryptonote
// advance which version they will stop working with
// Don't go over 32767 for any of these
#define CORE_RPC_VERSION_MAJOR 2
#define CORE_RPC_VERSION_MINOR 6
#define CORE_RPC_VERSION_MINOR 8

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jun 4, 2019

Contributor

Any reason for skipping 7 ? Another PR somewhere ?

This comment has been minimized.

Copy link
@hyc

hyc Jun 4, 2019

Contributor

As a matter of fact, #5549 uses 7. Not sure it's relevant, we'll rebase to whatever's the current version at the time anyway.

This comment has been minimized.

Copy link
@xiphon

xiphon Jun 4, 2019

Author Contributor

Due to the #5582 PR. I guess it will be merged before this one

struct public_node
{
std::string host;
uint64_t last_seen;

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jun 4, 2019

Contributor

You might want to zero this is restricted RPC.

This comment has been minimized.

Copy link
@xiphon

xiphon Jun 4, 2019

Author Contributor

Have some concerns about it being used for fingerprinting.
Currently get_public_nodes works only in non-restricted mode only (https://github.com/monero-project/monero/pull/5607/files#diff-8238353d591b683ea88296ba92b7ee25R111)
For example, get_peer_list acts the same as well.

Any thoughts?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jun 4, 2019

Contributor

Oh good, it's fine then.


const auto collect = [](const std::vector<peer> &peer_list, std::vector<public_node> &public_nodes)
{
for (const auto &entry : peer_list)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jun 4, 2019

Contributor

public_nodes.reserve(peer_list.size())

This comment has been minimized.

Copy link
@xiphon

xiphon Jun 4, 2019

Author Contributor

Not the case, peer_list.size() ≫ public_nodes.size()

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jun 4, 2019

Contributor

I don't see why that matters. The reserve call will remove allocations in this loop (to one before the loop) and a decent (but admittedly indeterminate) chunk of of the list is going to be copied anyway. And the std::vector is nearly immediately deallocated - once the message is serialized and transmitted.

This comment has been minimized.

Copy link
@xiphon

xiphon Jun 5, 2019

Author Contributor

I prefer doing a few re-allocations here over allocating almost completely unused big chunk of memory.

@stoffu

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

I get something like this response:

{
  "status": "OK",
  "white": [{
    "host": "238925241",
    "last_seen": 1559536540,
    "rpc_port": 18081
  },{
    "host": "1512725083",
    "last_seen": 1559119904,
    "rpc_port": 18081
  },{
    "host": "2306562215",
    "last_seen": 1559539321,
    "rpc_port": 18081
  }]
}

Shouldn't the host field display a valid IP address?

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

That's #5606 (and technically, that is a valid IP address, just seldom used :P)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.