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

v1.18: Simd 129: alt_bn128 syscalls - simplified error code (backport of #294) #872

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Apr 17, 2024

Problem

The alt_bn128 family of syscalls have a wide range of return errors (some of which also invalid/that can never happen). We'd like to simplify to limit the risk of consensus issue among different clients (e.g. Agave vs Firedancer).

Summary of Changes

  • Main change: return Ok(1) instead of Ok(e.into()), i.e. one single error code for the syscalls
  • UnexpectedError shouldn't be 0, as 0 would be confused with SUCCESS (included for completeness, even though the previous change makes this irrelevant)
  • Minor fixes on return errors caused by copy&paste (included for completeness, even though the previous change makes this irrelevant)
  • Feature gate the change in SyscallAltBn128

Feature Gate Issue: #320

SIMD-0129: solana-foundation/solana-improvement-documents#129


This is an automatic backport of pull request #294 done by Mergify.

@samkim-crypto
Copy link

Waiting until #899 is merged.

* alt_bn128: simplify errors in sycalls (alt_bn128, compress, poseidon)

* add TODO for feature gate. remove validate from compress

* add feature gate

* fix one more error case

* all changes under feature gate

* revert removing from()

* return unexpected errors in lib

* add comment to remove error types, once the feature gate is activated

* remove unnecessary/impossible error

* fix mispelled comments

(cherry picked from commit 64260fc)
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 81.5%. Comparing base (50e4e50) to head (16d8634).
Report is 1 commits behind head on v1.18.

Additional details and impacted files
@@            Coverage Diff            @@
##            v1.18     #872     +/-   ##
=========================================
- Coverage    81.6%    81.5%   -0.1%     
=========================================
  Files         827      827             
  Lines      225122   225125      +3     
=========================================
- Hits       183712   183683     -29     
- Misses      41410    41442     +32     

@samkim-crypto samkim-crypto merged commit efc616a into v1.18 Apr 24, 2024
36 checks passed
@samkim-crypto samkim-crypto deleted the mergify/bp/v1.18/pr-294 branch April 24, 2024 12:10
anwayde pushed a commit to firedancer-io/agave that referenced this pull request Jul 23, 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