Introduce test_network feature

[kpandl]

Motivation

For testing purposes, it would be practical to test the snarkVM consensus version (and proof targets) heights through a --features test_network compilation flag in snarkOS. This PR introduces such a flag.

It also updates the snarkVM ref to the version where ProvableHQ/snarkVM#2693 is merged.

Test Plan

Tested with an added CLI output of consensus height thresholds for testing purposes, and verified it works.

[kaimast]

This looks fine to me.

There is one thing to consider, though. Should the test feature enable test for all crates in the workspace? ledger-service, storage-service, router, and sync all have such a feature.
If not, we should leave a comment somewhere that explains what features/behavior it enables and why. Alternatively, we could name the feature something like test-consenus-heights instead.

"Big Picture" Comments

• I dug through these feature flags a while ago when trying to debug [Bug] A test in the cli package fails when testing the entire workspace #3546. It's a little confusing how the test and test_targets features are used in some, but not all, unit tests (usually by having the feature set in a dev-dependency).

• We should document all feature flags for the top-level crate/workspace. The Rust documentation points to this README as a good example of how to do it.

[vicsn]

We should document all feature flags for the top-level crate/workspace. The Rust documentation points to this README as a good example of how to do it.

Really good idea. Not sure if its overkill to use a tool to visualize the feature trees. This documentation can also document the discrepancy you noted regarding test and test_targets. CC Perhaps @joske wants to take a stab at this because he also has context on the addition of test_targets.

[niklaslong]

Not sure if its overkill to use a tool to visualize the feature trees.

You should be able to get quite far with cargo tree: https://doc.rust-lang.org/cargo/commands/cargo-tree.html, specifically the -e option used for searching through features, perhaps combined with -i for a reverse search.

[vicsn]

There is one thing to consider, though. Should the test feature enable test for all crates in the workspace? ledger-service, storage-service, router, and sync all have such a feature.

@kpandl want to give this a try, and see if we can still run a local devnet and land a transaction? No rush on this.

[kpandl]

Thanks, I've added the test feature to router, sync, and through feature propagation of bft to ledger-service and storage-service. I've also updated the router crate to propagate the test feature to the submodule.

Ran a devnet and verified a transaction still lands when using the test feature (didn't test anything else).

[kaimast]

Looks good!

[kaimast]

Seems like adding the snarkVM test feature broke something.

Does the test feature get enabled in unit tests? If it isn't, these changes shouldn't affect the tests, but it seems like they do.

[vicsn]

Seems like adding the snarkVM test feature broke something.
Does the test feature get enabled in unit tests? If it isn't, these changes shouldn't affect the tests, but it seems like they do.

Indeed, it seems like for some subcrates, features = [ "test" ] is enabled in a dev-dependencies subsection. Note for example that router had an empty test feature which is now suddenly enabled with a lot more junk in there: https://github.com/ProvableHQ/snarkOS/pull/3572/files#diff-f5e82e097aa5005ad9c92fe29abb9de43cddfbf974a5dcafcdad98e4cbb7b715R20

I see a couple of directions to achieve the original goal: ensure we can run a snarkOS network where novel features are enabled on a low block height.

1. We introduce a new test_consensus_versions feature in snarkVM and snarkOS, which will use the test consensus version heights.
2. We introduce test_targets and test_consensus_versions in snarkVM, but use a single feature in snarkOS called test_network which will enable both in snarkVM.
3. We consolidate test_targets and test_consensus_versions into a single feature called test, but this requires untangling the morass of git grep -C2 test | grep Cargo.toml.

I vote for starting with option 2., and we can create an issue to clean up 3.

[kpandl]

Thanks, addressed option 2 now. Tested it along with ProvableHQ/snarkVM#2693 on a local devnet.
Note that CI will fail because it needs this snarkVM: ProvableHQ/snarkVM#2693

I'd suggest I temporarily change the snarkVM ref here so that we can let CI run and see if it passes.

[vicsn]

Why do we only have to activate these two subdirectories (cli and node)?

Given that we didn't activate snarkos-node/test_targets before, was that effectively some dead configuration code?

[vicsn]

@kpandl this question still stands - is snarkos-node/test_network actually used? Should it be called from cli/Cargo.toml or not?

[vicsn]

Lets wait with merging until snarkVM test_migration_heights is fixed