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

Fail on bad scalar conversions #703

Merged
merged 5 commits into from
Jul 19, 2024
Merged

Fail on bad scalar conversions #703

merged 5 commits into from
Jul 19, 2024

Conversation

DavidM-D
Copy link
Contributor

Previously we'd allow bad scalars to be converted too and from bytes. This should be impossible since all conversions are done internally, but it's good to check.

Closes #702

@DavidM-D
Copy link
Contributor Author

Is it OK that this panics, or should I get it to return a Result?

@DavidM-D DavidM-D requested a review from volovyks July 19, 2024 11:35
@volovyks
Copy link
Collaborator

We were using the unsafe code to process data provided by the user. As a result, there were no guarantees about the integrity of the data usually provided for an unsafe block of code.

Now we are changing that to a panic() but it is still user-provided data that can crash the program. I would add error handling here, but I see your point, it requires a lot of refactoring.
cc @ChaoticTempest take a look, please.

@volovyks volovyks requested a review from ChaoticTempest July 19, 2024 11:52
@DavidM-D DavidM-D force-pushed the dmd/check-scalars branch 2 times, most recently from d76363a to 58c3790 Compare July 19, 2024 12:43
@DavidM-D
Copy link
Contributor Author

I'll make it an option, it's actually showing some interesting parts of our codebase. Like what happens if, when deriving epsilon, the output of the hash function doesn't fall into the field?

DavidM-D added 4 commits July 19, 2024 20:03
Previously we'd allow bad scalars to be converted too and from bytes.
This should be impossible since all conversions are done internally, but
it's good to check.
@DavidM-D DavidM-D force-pushed the dmd/check-scalars branch from b9ef520 to b7c6ef9 Compare July 19, 2024 18:04
Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

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

It's fine for now to panic until we get error handling into the contract. At least with this while deriving epsilon, it panics at the entrypoint in the contract before making it to the MPC network. It's kinda buried where this guard is happening but we will have to surface it in a later error handing PR

@ChaoticTempest ChaoticTempest merged commit baa5c29 into develop Jul 19, 2024
4 checks passed
@ChaoticTempest ChaoticTempest deleted the dmd/check-scalars branch July 19, 2024 19:34
Copy link

Terraform Feature Environment Destroy (dev-703)

Terraform Initialization ⚙️success

Terraform Destroy success

Show Destroy Plan


No changes. No objects need to be destroyed.

Either you have not created any objects yet or the existing objects were
already deleted outside of Terraform.

Destroy complete! Resources: 0 destroyed.

Pusher: @ChaoticTempest, Action: pull_request, Working Directory: ``, Workflow: Terraform Feature Env (Destroy)

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.

KS–PCS–F–7 Scalars Initiated with Unsafe Function
3 participants