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: reject multiple setup paths #351

Merged
merged 5 commits into from
Aug 24, 2024
Merged

Conversation

daejunpark
Copy link
Collaborator

@daejunpark daejunpark commented Aug 23, 2024

currently, if multiple setup paths exist, one of them is randomly selected and used for testing. since the existence of multiple setup paths are likely due to an incorrect setup(), this behavior makes debugging more challenging. to address this, now it will fail immediately when multiple setup paths are detected.

to be merged after #350

@daejunpark daejunpark mentioned this pull request Aug 23, 2024
17 tasks
Comment on lines +194 to 198
// vm.assume(mode0 < 9);
// NOTE: explicitly branch over mode0, as an infeasible path with mode0 >= 9 may not be eliminated due to an extremely inefficient solver environment (e.g, github workflow)
mode0 = split_mode_up_to_9(mode0);

_check_call0(mode0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

wait what? I don't understand what's going on here

Copy link
Collaborator Author

@daejunpark daejunpark Aug 24, 2024

Choose a reason for hiding this comment

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

this is not related to this pr. it's a temporary fix to a flaky test that i found.

details of the flaky test: when mode0 == 9, _check_call0() fails due to HalmosException (which is intentional). however, the goal of this test is to consider only success paths, so the assumption mode0 < 9 is provided at the beginning. however, the HalmosException path may still appear after path exploration, if the branching solver couldn't solve its infeasible path condition. this won't happen under normal circumstances, as the path condition mode0 < 9 && mode0 == 9 is obviously unsat. but in the github runner, the solver may timeout, leading to a test failure, due to the HalmosException path, even if it is infeasible. a proper fix is more involved, and will be implemented later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it, thank you

Copy link
Collaborator

@karmacoma-eth karmacoma-eth left a comment

Choose a reason for hiding this comment

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

I agree with the general direction, however:

  • we still have quite a few symbolic sources like balances, right? we might want to tighten these as much as possible
  • do we want this to be a hard break or more of a nudge? i.e. maybe allow a bypass with a CLI flag?

@daejunpark
Copy link
Collaborator Author

  • we still have quite a few symbolic sources like balances, right? we might want to tighten these as much as possible

balance is addressed in #352

  • do we want this to be a hard break or more of a nudge? i.e. maybe allow a bypass with a CLI flag?

we could add a bypass flag, or make this optional (e.g., --strict), later if this turns out to cause test failures too often.

@daejunpark daejunpark merged commit 23f2140 into main Aug 24, 2024
57 checks passed
@daejunpark daejunpark deleted the feat/fail-multiple-setups branch August 24, 2024 00:28
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.

2 participants