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

[Fix] Make --node optional to ensure it has an influence even when using --dev; renames --storage_path to --storage #3189

Merged
merged 3 commits into from Apr 3, 2024

Conversation

vicsn
Copy link
Collaborator

@vicsn vicsn commented Mar 25, 2024

Motivation

Previously, when using --dev, the node ports would automatically be incremented alongside the the dev index. This makes it hard to indicate --peers on other machines, as you'll need to know the dev index / port number before it works. This design is similar to the --bft flag.

This PR also renames --storage_path to --storage in the CLI start command.

Test Plan

This only changes a node's behaviour when both --node and --dev are used.

Tested manually locally and on aws.

Related PRs

Needed because we're testing: #3163

Copy link

@christianwwwwwwww christianwwwwwwww left a comment

Choose a reason for hiding this comment

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

LGTM 🤓

@vicsn
Copy link
Collaborator Author

vicsn commented Mar 26, 2024

Made an issue for the flaky test, which was already flaky before this PR: #3190

@vicsn vicsn marked this pull request as ready for review March 26, 2024 09:19
@howardwu
Copy link
Contributor

howardwu commented Mar 26, 2024

  • Does this change the production snarkos start command for any node operators?
  • Does the devnet.sh script or .devnet/start.sh script need updating?
  • Do the run-client.sh or run-prover.sh scripts need updating?
  • Have the above items been tested by hand and confirmed to work as before?

@vicsn
Copy link
Collaborator Author

vicsn commented Mar 27, 2024

Note behavior only changes if you use both --node and --dev.

Does this change the production snarkos start command for any node operators?

Both devnet and canary do not use --dev, so not impacted.

Does the devnet.sh script or .devnet/start.sh script need updating?

They do not use --node, so not impacted.

Do the run-client.sh or run-prover.sh scripts need updating?

They do not use --dev or --node, so not impacted.

Have the above items been tested by hand and confirmed to work as before?

I ran devnet.sh and run-client.sh and they still work. I did not run .devnet/start.sh but it has the same --dev as devnet.sh.

@raychu86
Copy link
Contributor

raychu86 commented Apr 3, 2024

Tested on .devnet/start.sh and it seems to work.

@howardwu howardwu changed the title Make --node optional to ensure it has an influence even when using --dev Make --node optional to ensure it has an influence even when using --dev; renames --storage_path to storage Apr 3, 2024
@howardwu
Copy link
Contributor

howardwu commented Apr 3, 2024

^ This commit renames --storage_path to --storage in the CLI start command.

@howardwu howardwu changed the title Make --node optional to ensure it has an influence even when using --dev; renames --storage_path to storage Make --node optional to ensure it has an influence even when using --dev; renames --storage_path to --storage Apr 3, 2024
@howardwu howardwu merged commit a5dbf77 into mainnet-staging Apr 3, 2024
0 of 20 checks passed
@howardwu howardwu deleted the optional_node_ip branch April 3, 2024 21:48
@howardwu howardwu changed the title Make --node optional to ensure it has an influence even when using --dev; renames --storage_path to --storage [Fix] Make --node optional to ensure it has an influence even when using --dev; renames --storage_path to --storage Apr 12, 2024
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.

None yet

4 participants