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

Refactor wallet log and default conf #220

Merged
merged 2 commits into from
Jul 15, 2020
Merged

Refactor wallet log and default conf #220

merged 2 commits into from
Jul 15, 2020

Conversation

ncying
Copy link
Member

@ncying ncying commented Jul 14, 2020

PR Details

Description

Refactor wallet log and default conf

Related Issue

#215 #208

How Has This Been Tested

All unit test passed

Types of changes

  • Refactor

Checklist

  • My code follows the code style of this project.
  • All new and existing tests passed.

@ncying ncying requested review from faddat, Icermli and utolp July 14, 2020 06:42
@ncying ncying mentioned this pull request Jul 14, 2020
9 tasks
@codecov-commenter
Copy link

Codecov Report

Merging #220 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #220   +/-   ##
=======================================
  Coverage   63.64%   63.64%           
=======================================
  Files         199      199           
  Lines        5744     5744           
  Branches      297      297           
=======================================
  Hits         3656     3656           
  Misses       2088     2088           
Impacted Files Coverage Δ
src/main/scala/vsys/wallet/Wallet.scala 92.59% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77e9f87...c915c72. Read the comment docs.

Copy link
Contributor

@faddat faddat left a comment

Choose a reason for hiding this comment

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

Config Files

On VSYS, it is hard to monitor sync progress from the log, and not possible to set up a supernode and monitor sync without API access.

I think that the best default setting for us would be API enabled, on 127.0.0.1

This will avoid making the user do additional configuration work. Since they need the API to even know if the node is shining correctly. If they want to securely access the web UI of a vsys node remotely, they can use an introspection tool like ngrok instead of binding to 0.0.0.0 (which is fine in some circumstances, but generally should be avoided).

Another alternative would be adding clear, explicit updates on sync progress to the log, every 1000 or 10,000 blocks, and shipping with the API disabled, since updates in the log on sync progress would allow the user to know if the node is syncing correctly without having the API enabled.

Seed in Log

This looks like it is fully resolved.

@ncying
Copy link
Member Author

ncying commented Jul 14, 2020

Config Files

On VSYS, it is hard to monitor sync progress from the log, and not possible to set up a supernode and monitor sync without API access.

I think that the best default setting for us would be API enabled, on 127.0.0.1

This will avoid making the user do additional configuration work. Since they need the API to even know if the node is shining correctly. If they want to securely access the web UI of a vsys node remotely, they can use an introspection tool like ngrok instead of binding to 0.0.0.0 (which is fine in some circumstances, but generally should be avoided).

Another alternative would be adding clear, explicit updates on sync progress to the log, every 1000 or 10,000 blocks, and shipping with the API disabled, since updates in the log on sync progress would allow the user to know if the node is syncing correctly without having the API enabled.

Seed in Log

This looks like it is fully resolved.

It is important to personalize the conf file before running the node. (wallet, API, some other related settings)

I think it is fine that users need to set the API key(need to change) and enable the API if they need to access the node data.

@faddat
Copy link
Contributor

faddat commented Jul 14, 2020

When running a full node (our most common scenario), users should not need to change anything in the config file, and it should be secure.

It should "just work".

In this case, especially because of #194 we have two options:

One
Add a sync monitor to the log like:

Block 10,000 synced
Block 20,000 synced
Block 30,000 synced

So that users can monitor the progress of sync without using the API or web UI. That would make shipping with the API disabled by default totally acceptable, because the user would be able to verify that the node is synced. Iinstead of actually doing configuration, many users will get frustrated and stop using our product. so, it is more desirable to ship a product that works perfectly in a known good configuration.

Two

Ship with the API enabled and on localhost by default. This is still very safe. having the API on by default is only a problem when we are binding it to 0.0.0.0

@faddat
Copy link
Contributor

faddat commented Jul 14, 2020

btcd - zeroconf
geth - zeroconf
ava-labs/gecko - zeroconf
Gaia - need to add seeds and genesis file and it's a pain. Binaries should ship with embedded seeds and those seeds should be suppressed if the user wishes to set up seeds. Genesis file should be downloaded via an embedded ipfs content address.
Peercoin - zeroconf for just running a node

Node count is a quantifiable way of measuring the health of our network. Making it very, very easy to set up a full node, almost foolproof, will improve the health of the v systems network.

On the other side of that, changing the default configuration to one that does not allow the user to monitor the sync process of their new node, is not very helpful to users , because many users likely will not follow the documentation and configuration process but instead just run the node.

@ncying ncying merged commit c0f5199 into master Jul 15, 2020
@faddat faddat mentioned this pull request Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants