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: test targets #3378

Merged
merged 2 commits into from
Nov 1, 2024
Merged

feat: test targets #3378

merged 2 commits into from
Nov 1, 2024

Conversation

joske
Copy link
Contributor

@joske joske commented Aug 9, 2024

Motivation

Add feature flag to be able to lower coinbase and proof targets for testing purposes. This feature flag will lower the proof and coinbase targets to 8 and 32 respectively.

The feature flag needed to be cascaded to all dependencies of snarkVM to be sure everything uses same constants.

To use, compile with --features test_targets
e.g.

cargo install --locked --features test_targets --path .

Test Plan

tested by using tx-cannon and adding println to verify correct targets.

CI link

Related PRs

Must first merge AleoNet/snarkVM#2529 and the snarkVM revision must be updated in snarkOS in 2 places before merging this.

vicsn
vicsn previously approved these changes Aug 12, 2024
Copy link
Contributor

@vicsn vicsn left a comment

Choose a reason for hiding this comment

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

LGTM if you update the Cargo.toml to the rev of AleoNet/snarkVM#2529 (which is just for illustrative purposes, as the maintainer will have to update it anyway)

@raychu86
Copy link
Contributor

Does this apply to compiled snarkOS binaries via cargo install --path .? What does that process look like? Additional documentation in the PR description/in the code would be helpful to know the implications.

@joske
Copy link
Contributor Author

joske commented Aug 12, 2024

@raychu86 I've updated the description. Let me know if still not clear.

@raychu86
Copy link
Contributor

raychu86 commented Aug 12, 2024

So if cargo install --locked --features test_targets --path . is used, the snarkOS binary will always have the test_targets version of the coinbase targets? We should have some failsafes where the node fails to boot-up in production mode if that is the case.

We need some sort of enforcement that the test_targets is being used only for devnets via --dev

#path = "../snarkVM"
git = "https://github.com/AleoNet/snarkVM.git"
rev = "be171ce" # If this is updated, the rev in `node/rest/Cargo.toml` must be updated as well.
git = "https://github.com/joske/snarkVM.git"
Copy link
Contributor

@howardwu howardwu Oct 29, 2024

Choose a reason for hiding this comment

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

Warning: snarkOS will no longer be using AleoNet's implementation of snarkVM after this PR.

Proceed with caution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@joske I'm guessing this was changed for testing purposes and we can change it back to AleoNet's snarkVM ?

Copy link
Contributor

Choose a reason for hiding this comment

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

And merge AleoNet/staging + run cargo fmt 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was because the snarkVM PR needs to be merged first. It was always intended that it be changed to an Aleonet snarkVM repo before merging this.

@vicsn vicsn mentioned this pull request Oct 29, 2024
@vicsn
Copy link
Contributor

vicsn commented Oct 29, 2024

Superseded by #3424 to resolve merge conflict

@alzger alzger merged commit 6598df5 into AleoNet:staging Nov 1, 2024
23 of 24 checks passed
@vicsn
Copy link
Contributor

vicsn commented Nov 5, 2024

NOTE: this was not actually merged, must be a bug in Github since the branchname is the same as the superseding PR.

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.

6 participants