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

core: implement network ID detection check on startup #301

Merged
merged 12 commits into from Jul 24, 2019

Conversation

@hrharder
Copy link
Contributor

hrharder commented Jul 23, 2019

Functional but a work in progress, a few outstanding points (see notes section).

Overview

Closes #237

  • Add a Metadata collection to MeshDB to store the networkID we are starting up with
  • Add a check during startup that checks if a networkID has been set, compares to the one loaded from the environment, and exits if they do not match.
  • The detection check (function initNetworkId) also stores the network ID during the initial startup (no previous DB)

Steps for testing:

  1. Start mesh on network N (let it run, collect orders, etc)
P2P_LISTEN_PORT=<port> ETHEREUM_RPC_URL=<url> ETHEREUM_NETWORK_ID=3 ./mesh
  1. Kill the process, and restart with a new network ID:
P2P_LISTEN_PORT=<port> ETHEREUM_RPC_URL=<url> ETHEREUM_NETWORK_ID=4 ./mesh

You should see:

{"level":"error","msg":"Mesh previously started on different Ethereum network; switch networks or remove DB","time":"2019-07-23T16:38:09-07:00"}
{"error_string":"expected networkID to be '4', got '3","level":"fatal","msg":"could not initialize app","time":"2019-07-23T16:38:09-07:00"}

Notes

Still a WIP due to me having questions about the following:

  • Is there a preferred key for the new collection other than []byte("metadata")? (see here)
  • Are there any changes to method names, comments, etc you would like to see?
  • Am I handling the mismatched ID case as you would expect? (see here)
hrharder added 3 commits Jul 23, 2019
update with latest changes from development
Copy link
Member

albrow left a comment

@hrharder it looks like we're making the problem harder than it needs to be. I made some suggestions to help simplify the approach.

core/core.go Outdated Show resolved Hide resolved
core/core.go Outdated Show resolved Hide resolved
meshdb/meshdb.go Outdated Show resolved Hide resolved
meshdb/meshdb.go Outdated Show resolved Hide resolved
core/core.go Outdated Show resolved Hide resolved
@albrow

This comment has been minimized.

Copy link
Member

albrow commented Jul 24, 2019

For testing, we can just call initNetworkID directly and make sure that it behaves as expected. For example, calling initNetworkID twice with two different network IDs should result in an error.

@albrow albrow self-assigned this Jul 24, 2019
hrharder added 2 commits Jul 24, 2019
@hrharder

This comment has been minimized.

Copy link
Contributor Author

hrharder commented Jul 24, 2019

@albrow I believe I addressed all your comments, please advise if I missed something. Will plan on pushing a test soon.

Copy link
Member

albrow left a comment

Just requested a few more changes. Looks pretty good!

core/core.go Outdated Show resolved Hide resolved
core/core.go Outdated Show resolved Hide resolved
core/core.go Outdated Show resolved Hide resolved
core/core_test.go Outdated Show resolved Hide resolved
@hrharder

This comment has been minimized.

Copy link
Contributor Author

hrharder commented Jul 24, 2019

The commit I just pushed should address all the comments. My only remaining questions are:

  • Should SaveMetadata accept the networkID or a meshdb.Metadata object?
  • Right now, GetMetadata returns any error coming from FindByID, is that OK? or should I explicitly return a ErrNotFound?
core/core.go Show resolved Hide resolved
meshdb/meshdb.go Outdated Show resolved Hide resolved
@hrharder

This comment has been minimized.

Copy link
Contributor Author

hrharder commented Jul 24, 2019

@fabioberger do you have any preference on the signature of the SaveMetadata method? IE should it accept a Metadata value, or just the network ID int? I'm unsure which would be more extensible in the future with more possible metadata values.

hrharder added 3 commits Jul 24, 2019
core/core.go Outdated Show resolved Hide resolved
meshdb/meshdb.go Outdated Show resolved Hide resolved
hrharder added 2 commits Jul 24, 2019
@hrharder hrharder changed the title [WIP] core: implement network ID detection check on startup core: implement network ID detection check on startup Jul 24, 2019
@albrow albrow self-requested a review Jul 24, 2019
@albrow
albrow approved these changes Jul 24, 2019
Copy link
Member

albrow left a comment

Looks great. Thanks @hrharder!

@albrow albrow merged commit ee09eea into 0xProject:development Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.