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

Add ZK opcodes definition #619

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Add ZK opcodes definition #619

wants to merge 16 commits into from

Conversation

AurelienFT
Copy link
Contributor

#615

VM PR : FuelLabs/fuel-vm#870

Before requesting review

  • I have reviewed the code myself

After merging, notify other teams

[Add or remove entries as needed]

@AurelienFT AurelienFT self-assigned this Nov 19, 2024
@xgreenx xgreenx requested review from Dentosal and a team November 27, 2024 23:01
src/fuel-vm/instruction-set.md Outdated Show resolved Hide resolved
src/fuel-vm/instruction-set.md Outdated Show resolved Hide resolved
@AurelienFT
Copy link
Contributor Author

@rymnc thank you for pointing incoherences :)

@AurelienFT AurelienFT requested a review from rymnc November 28, 2024 09:24
rymnc
rymnc previously approved these changes Nov 28, 2024
Copy link
Member

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM, small nit

src/fuel-vm/instruction-set.md Outdated Show resolved Hide resolved
Co-authored-by: Aaryamann Challani <[email protected]>
rymnc
rymnc previously approved these changes Nov 28, 2024
Copy link
Member

@Dentosal Dentosal left a comment

Choose a reason for hiding this comment

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

Panic conditions must be described as well. The operations panic at least in cases where mode-selection arguments are not from the list of available modes. Also outputs to memory require that memory to be writeable. Likely there are also cases where the argument values etc. can also be incorrect, in which case the operation should fail. Consider allowing the use of $err in those cases.

src/fuel-vm/instruction-set.md Outdated Show resolved Hide resolved
src/fuel-vm/instruction-set.md Outdated Show resolved Hide resolved
@AurelienFT
Copy link
Contributor Author

@Dentosal I have added panic cases. However, I don't make use of $err because I don't really understand in which cases it's better to use that than just make the call fail.
Following the docs here and a comment on the VM, it seems to be used only on ALU and for me on these opcodes there is no use case for that.

@AurelienFT AurelienFT requested a review from Dentosal November 28, 2024 13:52
@Dentosal
Copy link
Member

Dentosal commented Nov 29, 2024

Following the docs here and a comment on the VM, it seems to be used only on ALU and for me on these opcodes there is no use case for that.

Not only for ALU, see $err usage here: https://github.com/FuelLabs/fuel-specs/blob/master/src/fuel-vm/instruction-set.md#eck1-secp256k1-signature-recovery

The use case here would be doing operations on user-provided, possibly invalid EC points without panicing (e.g. InvalidEllipticCurvePoint). If you think this is not a relalistic use case, I'm fine with only panicking modes being available.

@AurelienFT
Copy link
Contributor Author

I don't think it's expected and I don't see any logic of modifying the input to retry directly designed in code. Also if the point isn't corrected created in the VM implementation then the addition can't be tried and the code will be more difficult and I think users whould except a panic and this will help them debugging with an explicit reason.

@AurelienFT AurelienFT requested a review from rymnc December 2, 2024 10:13
rymnc
rymnc previously approved these changes Dec 2, 2024
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.

4 participants