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

feat: Add --config and --network flags to several stacks-inspect subcommands #5551

Merged

Conversation

jbencin
Copy link
Contributor

@jbencin jbencin commented Dec 9, 2024

Description

Allow for users to pass either:

  • --config <file>
  • --network <helium|mainnet|mocknet|xenon>

to the following stacks-inspect subcommands:

  • try-mine
  • replay-block
  • replay-naka-block
  • replay-mock-mining

This allows these commands to be used with non-mainnet chainstate. These CLI options must come before the subcommand:

stacks-inspect --config my_config.toml try-mine path/to/chainstate 10 10000

Applicable issues

  • fixes #

Additional info (benefits, drawbacks, caveats)

Warning

It was discovered while testing this PR that stacks-inspect try-mine is currently broken and does not work on post-Nakamoto chainstates. For now, I have added an explicit panic when using this command on Nakamoto a chainstate, and I intend to add Nakamoto support in a separate PR

This PR moves config.rs from stacks-node to stackslib in order for the stacks-inspect command to use the same config file as a Stacks node.

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@jbencin jbencin force-pushed the feat/stacks-inspect-try-mine-with-config branch from 7a48a7a to 318f3cf Compare December 10, 2024 22:07
@jbencin jbencin changed the title feat: Allow passing --config <file> to stacks-inspect try_mine (and other commands) feat: Add --config and --network flags to several stacks-inspect subcommands Dec 11, 2024
@jbencin jbencin marked this pull request as ready for review December 11, 2024 19:43
@jbencin jbencin requested review from a team as code owners December 11, 2024 19:43
@jbencin jbencin force-pushed the feat/stacks-inspect-try-mine-with-config branch from db3b490 to 7c4e997 Compare December 11, 2024 20:41
@jbencin jbencin changed the base branch from master to develop December 11, 2024 20:53
@jbencin jbencin force-pushed the feat/stacks-inspect-try-mine-with-config branch from 7c4e997 to 5fe4948 Compare December 12, 2024 18:10
obycode
obycode previously approved these changes Dec 13, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM! This has already proven to be very useful. Thanks 🙌

@wileyj
Copy link
Contributor

wileyj commented Dec 13, 2024

The one thing i'd like to know about here is the choice to move the example configs.

was there a specific reason the existing location wouldn't work, or this was an attempt to standardize it more?
if standardizing - maybe we should move the cofigs to the docs folder etc instead.

if we do keep these changes (or decide to use ./docs etc), we'll also want to check downstream if anything is still linking to the current example config location.

but, as @obycode said - this PR has already been incredibly useful, nice work!

@jbencin jbencin force-pushed the feat/stacks-inspect-try-mine-with-config branch from b274a80 to 1767ee0 Compare December 13, 2024 20:36
obycode
obycode previously approved these changes Dec 13, 2024
@jbencin
Copy link
Contributor Author

jbencin commented Dec 13, 2024

The one thing i'd like to know about here is the choice to move the example configs.

There were unit tests in confg.rs that parsed these config files, and looked them up like this:

let conf_dir: PathBuf = Path::new(env!("CARGO_MANIFEST_DIR")).join("conf");

So this was the simplest way to get the tests passing. If these config files are just unit test fixtures, it makes sense to keep them close to the code. If they are also meant to be examples to users setting up a node, then perhaps they should be elsewhere, like a top-level examples/ directory, or we can symlink to them from stacks-node

@jbencin
Copy link
Contributor Author

jbencin commented Dec 13, 2024

@wileyj I added a symlink from the old example config location to the new one to address your concern. I think this is the best solution because:

  • It doesn't break existing links
  • Test fixtures should live close to the code
  • stackslib should not have dependencies on stacks-node

But I know symlinks can cause issues in some environments, so let me know if this is a problem

@wileyj
Copy link
Contributor

wileyj commented Dec 13, 2024

@wileyj I added a symlink from the old example config location to the new one to address your concern. I think this is the best solution because:

  • It doesn't break existing links
  • Test fixtures should live close to the code
  • stackslib should not have dependencies on stacks-node

But I know symlinks can cause issues in some environments, so let me know if this is a problem

i like the symlink idea (but i also like a more standardized location for the example configs, since they are meant for users, not necessarily for tests).
if we symlinked from the current dir to the previous location (and open an issue to standardize the location), would that break your tests?

@jbencin
Copy link
Contributor Author

jbencin commented Dec 13, 2024

if we symlinked from the current dir to the previous location (and open an issue to standardize the location), would that break your tests?

I assume the tests would be fine if stackslib/conf were a symlink. I don't know for sure, but it's easy to test. I don't think the stackslib code should depend on paths outside of stackslib/, but I don't see a problem with solving this at the filesystem layer

Copy link
Contributor

@wileyj wileyj left a comment

Choose a reason for hiding this comment

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

linked to #5575

lgtm!

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM, but please address all remaining comments before merging. Thanks for making this change!

@jbencin jbencin added this pull request to the merge queue Dec 16, 2024
Merged via the queue into stacks-network:develop with commit 3da3394 Dec 16, 2024
156 of 168 checks passed
@jbencin jbencin deleted the feat/stacks-inspect-try-mine-with-config branch December 16, 2024 20:39
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