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

CHIA-902 make Program.run() and Program.run_with_cost() default to enabling all the most recent features #18370

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Jul 29, 2024

Purpose:

This is a follow-up on #18287

This came out of a conversation with @trepca . Testing puzzles that use features from recent soft-forks (and the hard fork) is made more difficult because the default run() functions don't enable those features. This patch fixes that by enabling everything.

It also runs in mempool-mode by default.

Additionally, a new run() variant is added to control exactly which features (flags) are enabled.

Current Behavior:

Program.run() does not support new opcodes, and ignores unknown ones.

New Behavior:

Program.run() does supports new opcodes, and fails on unknown ones.

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jul 29, 2024
@arvidn arvidn marked this pull request as ready for review July 30, 2024 08:58
@arvidn arvidn requested a review from a team as a code owner July 30, 2024 08:58
@arvidn arvidn requested a review from richardkiss July 30, 2024 08:58
Copy link
Contributor

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

See comments

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 31, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

1 similar comment
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 31, 2024
richardkiss
richardkiss previously approved these changes Aug 1, 2024
@arvidn arvidn changed the title make Program.run() and Program.run_with_cost() default to enabling all the most recent features CHIA-902 make Program.run() and Program.run_with_cost() default to enabling all the most recent features Aug 7, 2024
@arvidn arvidn requested a review from emlowe August 7, 2024 20:29
@emlowe
Copy link
Contributor

emlowe commented Aug 7, 2024

There doesn't seem much difference now between run and run_with_flags

@richardkiss
Copy link
Contributor

There doesn't seem much difference now between run and run_with_flags

Yeah, as I mentioned above maybe we consider deprecating it.

@Starttoaster
Copy link
Contributor

Starttoaster commented Aug 7, 2024

Waiting on merge for all conversations to reach a resolution.

@richardkiss richardkiss self-requested a review August 8, 2024 04:39
@arvidn
Copy link
Contributor Author

arvidn commented Aug 8, 2024

the difference is that run() only returns the result, not the cost. I think we should keep both for now, to not break any 3rd party using this class.

@Starttoaster Starttoaster merged commit 9273a61 into main Aug 8, 2024
374 checks passed
@Starttoaster Starttoaster deleted the program-run branch August 8, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants