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

SIMD-0129: Alt_BN128 Syscalls - Simplified Error Code #129

Merged

Conversation

0x0ece
Copy link
Contributor

@0x0ece 0x0ece commented Mar 19, 2024

Summary

Simplify the return error code for the family of Alt_BN128 syscalls:
sol_alt_bn128_group_op, sol_alt_bn128_compression and sol_poseidon.

A single error code is sufficient, in line with all other syscalls.

Motivation

Syscalls in Solana can return:

  • Success, e.g. represented in Rust as Ok(0) or Ok(SUCCESS)
  • Error to the program, e.g. represented in Rust as Ok(1)
  • Fatal error to the VM, aborting the transaction, e.g. represented in Rust
    as Err(<something>)

Most syscalls only have a single error code returned to the program, i.e. Ok(1).
The family of Alt_BN128 syscalls, viceversa, has a richer set of error codes.

This proposal aims to simplify the error value for these syscalls, in line with
all the other syscalls, and simply return Ok(1) (in addition to fatal errors,
that are left unchanged).

We stress that multiple error codes cause a maintenance burden for the validators.
Moreover, if two different implementation were to return different error codes,
an attacker could exploit the different behavior to cause consensus failure.

Copy link
Contributor

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

I think this proposal is straightforward, simple to implement, and a definite improvement for consensus/security.

proposals/0129-alt-bn128-simplified-error-code.md Outdated Show resolved Hide resolved
proposals/0129-alt-bn128-simplified-error-code.md Outdated Show resolved Hide resolved
proposals/0129-alt-bn128-simplified-error-code.md Outdated Show resolved Hide resolved
proposals/0129-alt-bn128-simplified-error-code.md Outdated Show resolved Hide resolved
0x0ece and others added 4 commits March 25, 2024 14:16
Co-authored-by: samkim-crypto <skim13@cs.stanford.edu>
Co-authored-by: samkim-crypto <skim13@cs.stanford.edu>
Co-authored-by: samkim-crypto <skim13@cs.stanford.edu>
Co-authored-by: samkim-crypto <skim13@cs.stanford.edu>
Copy link
Contributor

@ripatel-fd ripatel-fd left a comment

Choose a reason for hiding this comment

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

I support this SIMD because it is straightforward and simplifies the runtime significantly. The proposal is well specified and unambiguous.

For the record, Light Protocol was concerned about any delays to the activation of alt_bn128 syscalls on mainnet. Discussions in Discord reached a rough agreement that the alt_bn128 syscalls will be activated with the complicated error logic first. Then, at a later point in time, this proposal will go into effect via a second feature gate.

https://discord.com/channels/428295358100013066/967028962746327060/1219725414960857232

@ripatel-fd
Copy link
Contributor

ripatel-fd commented Mar 26, 2024

@jacobcreech @t-nelson This SIMD is ready to move to 'finalized draft' stage. Please feel free to add more reviewers if you think more eyes are needed. Otherwise, I suggest we merge this PR.

@jacobcreech
Copy link
Contributor

Going to do a last call for any comments on this SIMD. If there are no more issues, we'll merge this in on Friday.

@jacobcreech
Copy link
Contributor

Looks like general consensus was made. Merging. Thank you @0x0ece for championing this!

@jacobcreech jacobcreech merged commit 7eb5f99 into solana-foundation:main Mar 30, 2024
2 checks passed
@0x0ece 0x0ece deleted the alt-bn128-simplified-error-code branch April 1, 2024 15:36
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