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

daemon: automatic public nodes discovering and bootstrap daemon switching #5799

Open
wants to merge 1 commit into
base: master
from

Conversation

@xiphon
Copy link
Contributor

commented Aug 9, 2019

Implemented --bootstrap-daemon-address auto mode.

Allows to sync and use the wallet while the daemon is syncing with the network.

  • While the daemon is syncing
    it will automatically select random public node (node that is running with --public-node mode enabled) to serve incoming requests (i.e. will use it as a remote node). If currently selected public node fails, the daemon will switch to another randomly chosen public node.
  • Once the daemon is fully synced
    it will use the local blockchain to serve incoming requests, i.e. operating in normal mode.
@erciccione

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Thanks for this @xiphon, i think would be a very nice feature. Could you clarify how this would work and what flags/commands are added? Would be good to have a short explanation in plain text on this PR, for better tracking and to make easier to update user and developer guides.

@xiphon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@erciccione

Updated the PR description

@erciccione

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Nice, thank you :)

@xiphon xiphon force-pushed the xiphon:auto-bootstrap branch from 0671a94 to 0e12963 Aug 9, 2019

@selsta

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Should ./monerod --bootstrap-daemon-address auto --no-sync be possible using this PR?

@xiphon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Should ./monerod --bootstrap-daemon-address auto --no-sync be possible using this PR?

Yep. Allows a user to sync and use the wallet without downloading and storing the local blockchain.
Even tested it as well, works as intended.

@fuwa0529

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

Yep. Allows a user to sync and use the wallet without downloading and storing the local blockchain.

That can't be right. Can you prevent these two flags from being used simultaneously?

@selsta

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

That can't be right. Can you prevent these two flags from being used simultaneously?

Why? --bootstrap-daemon-address auto --no-sync is comparable to automatically connecting to a remote node.

@fuwa0529

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

A daemon does not join a p2p network just to serve a single user, by fetching data from the network while contributing nothing back, right? These two flags, when used together, allow a user to do just that, which should be prevented.

@xiphon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Can you prevent these two flags from being used simultaneously?

Is not related to this PR

src/rpc/bootstrap_daemon.h Outdated Show resolved Hide resolved
src/rpc/bootstrap_daemon.h Outdated Show resolved Hide resolved
src/rpc/bootstrap_daemon.h Outdated Show resolved Hide resolved
{
public:
bootstrap_daemon(nodetool::node_server<cryptonote::t_cryptonote_protocol_handler<cryptonote::core>> *p2p)
: m_last_result(false)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Aug 12, 2019

Contributor

I think we've been trying to list all members (even if they have default constructors). It does make things more consistent in case the type changes.

This comment has been minimized.

Copy link
@xiphon

xiphon Aug 12, 2019

Author Contributor

I might do that, though listing members with default constructors seems questionable to me. Could you elaborate a bit more on that?

src/rpc/bootstrap_daemon.h Outdated Show resolved Hide resolved
{
}

bootstrap_daemon(const std::string &address, const boost::optional<epee::net_utils::http::login> &credentials)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Aug 12, 2019

Contributor

Take credentials by copy then move.

This comment has been minimized.

Copy link
@xiphon

xiphon Aug 12, 2019

Author Contributor

What issue do you want to address?

src/rpc/core_rpc_server.cpp Outdated Show resolved Hide resolved
src/rpc/core_rpc_server.cpp Show resolved Hide resolved
src/rpc/bootstrap_daemon.h Outdated Show resolved Hide resolved
src/rpc/bootstrap_daemon.h Outdated Show resolved Hide resolved

@xiphon xiphon force-pushed the xiphon:auto-bootstrap branch from 0e12963 to a98117d Aug 13, 2019

@xiphon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Updated. Refactored the code a lot

  • Moved all the peer list stuff to p2p node_server and peerlist_manager classes.
  • Implemented get_public_nodes and get_random_public_node in p2p node_server class,
void node_server::get_public_nodes(PeerType type, std::vector<peerlist_entry>& public_nodes);
boost::optional<std::string> node_server::get_random_public_node();
  • O(1) instead of O(N) to fetch public nodes from the peer list. No more full list scans. Using hashed non unique index.
void peerlist_manager::get_public_nodes(PeerType type, std::vector<peerlist_entry>& public_nodes);
  • Separate bootstrap_daemon.cpp
  • Fixed noexcept declarations
  • Relying on http client connection state (is_connected(), disconnect())
  • Fetching current bootstrap daemon address from underlying http client (get_host(), get_port())
  • Reusing invoke_http_json_rpc
  • boost::optional return values
  • Dropped nodetool::node_server<...> *m_p2p; from bootstrap_daemon class
@fuwa0529

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Can you prevent these two flags from being used simultaneously?

Is not related to this PR

On a second thought, this free-rider problem probably isn't too bad in this case, the market will likely work it out. Ignore my concern.

@xiphon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

On a second thought, this free-rider problem probably isn't too bad in this case, the market will likely work it out. Ignore my concern.

That is valid concern overall and it might be addressed later by altering '--no-sync' logic. For example, '--no-sync' node might periodically connect to p2p and disconnect right after refreshing the peer list and fetching current blockchain state (target height, etc.).

@xiphon xiphon force-pushed the xiphon:auto-bootstrap branch from a98117d to d998732 Aug 21, 2019

@xiphon xiphon force-pushed the xiphon:auto-bootstrap branch from d998732 to ff70383 Aug 21, 2019

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