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

Offline Node Support #3383

Merged
merged 9 commits into from
Oct 9, 2020
Merged

Conversation

Daniel-VDM
Copy link
Contributor

This feature is needed for a correct deployment of a rosetta 'offline' node. That said, it is a perfectly reasonable feature to have on its own - rosetta notwithstanding. I imagine that it would great for debugging / devs.

One can run a node in offline mode by adding the --run.offline option. This option has been tested to be backwards compatible with older config files. It defaults to an 'online' setting if the option/config is not provided.

An offline node simply means that it does not sync & is not subscribed to any p2p topic or has any p2p peers.
I've had to keep some service instantiations to ensure other services don't crash from a nil pointer.

* Add IsOffline to node config

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
@Daniel-VDM Daniel-VDM added the enhancement New feature or request label Oct 7, 2020
@Daniel-VDM Daniel-VDM self-assigned this Oct 7, 2020
@JackyWYX
Copy link
Contributor

JackyWYX commented Oct 9, 2020

I would suggest add the following logic:

  1. If running in offline mode, force p2p to listen to address 127.0.0.1 (p2p/host.go:68) (It might be better to add a field IP in p2pConfig cmd/harmony/config.go:45).
  2. If run in offline mode, instruct node.SyncingPeerProvider to use local network.

This can further prevent node from using network. Else the node is still using libp2p for peer discovery. Do you think this is reasonable?

Copy link
Contributor

@LeoHChen LeoHChen left a comment

Choose a reason for hiding this comment

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

reasonable request by @JackyWYX , please fix them. thanks.

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Use default local listening ip for p2p hors if node is in offline

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Fix TestAddPeer & TestConnectionToInvalidPeer p2p test

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
@Daniel-VDM
Copy link
Contributor Author

Daniel-VDM commented Oct 9, 2020

I would suggest add the following logic:

  1. If running in offline mode, force p2p to listen to address 127.0.0.1 (p2p/host.go:68) (It might be better to add a field IP in p2pConfig cmd/harmony/config.go:45).
  2. If run in offline mode, instruct node.SyncingPeerProvider to use local network.

This can further prevent node from using network. Else the node is still using libp2p for peer discovery. Do you think this is reasonable?

Sure seems fair enough, added this logic and fixed related tests along the way.

internal/configs/node/network.go Outdated Show resolved Hide resolved
node/node.go Outdated Show resolved Hide resolved
cmd/harmony/main.go Outdated Show resolved Hide resolved
@JackyWYX
Copy link
Contributor

JackyWYX commented Oct 9, 2020 via email

@Daniel-VDM
Copy link
Contributor Author

I think this is a public variable right? Comments are required for public stuff😂

On Fri, Oct 9, 2020 at 2:09 AM Daniel Van Der Maden < @.***> wrote: @Daniel-VDM commented on this pull request. ------------------------------ In internal/configs/node/network.go <#3383 (comment)>: > @@ -39,6 +39,10 @@ const ( ) const ( + // DefaultLocalListenIP .. Ill remove it then — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3383 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJSNLCEZ3T6HV2C55PMA2VTSJ3HLNANCNFSM4SHISFHQ .

Yea... updated the comment right after

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Copy link
Contributor

@JackyWYX JackyWYX left a comment

Choose a reason for hiding this comment

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

Nice!

@Daniel-VDM Daniel-VDM merged commit b291f18 into harmony-one:main Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants