feat: add alt_bn128 syscalls #246
Conversation
There was a problem hiding this comment.
I didn't comment on all of the places where I saw this possible improvement, but I think using sized arrays will get rid of the need for runtime checks on the &[u8] in cases where it is defined as a constant. We should let the user decide when/if/how they want to handle this instead of forcing a runtime check. Overall, great work @LStan 🫡
| /// Note: This function checks if the input has the correct length, | ||
| /// returning an error without incurring the cost of the syscall. | ||
| pub fn checked_alt_bn128_g1_compress( | ||
| input: &[u8], |
There was a problem hiding this comment.
Instead of taking in a &[u8], consider using a &[[u8;ALT_BN128_G1_SIZE]]. This will avoid the need for the length check at runtime for properly defined constants.
There was a problem hiding this comment.
What will be the input type of alt_bn128_compression?
There was a problem hiding this comment.
Also, will it be convenient from the user's perspective if they have just &[u8] input?
There was a problem hiding this comment.
Also, why &[[u8;ALT_BN128_G1_SIZE]] if there is only one point in the input? Maybe &[u8;ALT_BN128_G1_SIZE]?
There was a problem hiding this comment.
My bad, just do the one, you are right. For multiple we want the array, for single, we want just the one.
There was a problem hiding this comment.
So, I can change input type to &[u8;SIZE] for all compressions, then checked_ variants will be unnecessary. But I'm still unsure about the convenience of usage for callers.
| /// It will return an error if the length is invalid, incurring the cost of the syscall. | ||
| #[inline(always)] | ||
| pub fn alt_bn128_addition( | ||
| input: &[u8], |
There was a problem hiding this comment.
Consider using a sized array here as well.
There was a problem hiding this comment.
Currently, syscalls accept inputs that are equal or less than required and extend them with zeros.
There was a problem hiding this comment.
Heh, ironically, on LE, this might actually save us some CUs. Maybe you're right.
There was a problem hiding this comment.
Unfortunatelly no, it will fail.
Endianness::LE => {
if input.len() != ALT_BN128_ADDITION_INPUT_LEN {
return Err(AltBn128Error::InvalidInputData);
}
}| if input.len() % ALT_BN128_PAIRING_ELEMENT_LEN != 0 { | ||
| return Err(ProgramError::InvalidArgument); | ||
| } | ||
| alt_bn128_group_op::<32>(input, ALT_BN128_PAIRING).map(|data| data[31]) |
| /// Note: This function checks if the input has the correct length, | ||
| /// returning an error without incurring the cost of the syscall. | ||
| #[inline(always)] | ||
| pub fn checked_alt_bn128_pairing(input: &[u8]) -> Result<u8, ProgramError> { |
There was a problem hiding this comment.
Not sure we need checked if we just use an array of correctly sized u8 arrays?
There was a problem hiding this comment.
If input type for paring will be &[[u8;SIZE]] and for mult and add - &[u8;SIZE], what will be the input type for `alt_bn128_group_op?
| } | ||
|
|
||
| #[inline] | ||
| fn alt_bn128_group_op<const OUTPUT_DATA_LEN: usize>( |
There was a problem hiding this comment.
This const generic is super nice and will save plenty of CUs vs the solana-program implementation!
| pub use compression::*; | ||
| pub use group_op::*; | ||
|
|
||
| pub const ALT_BN128_FIELD_SIZE: usize = 32; |
There was a problem hiding this comment.
| pub const ALT_BN128_FIELD_SIZE: usize = 32; | |
| /// Size of the EC point field, in bytes. | |
| pub const ALT_BN128_FIELD_SIZE: usize = 32; |
| pub use group_op::*; | ||
|
|
||
| pub const ALT_BN128_FIELD_SIZE: usize = 32; | ||
| pub const ALT_BN128_G1_SIZE: usize = ALT_BN128_FIELD_SIZE * 2; // x, y each 32 byte |
There was a problem hiding this comment.
| pub const ALT_BN128_G1_SIZE: usize = ALT_BN128_FIELD_SIZE * 2; // x, y each 32 byte | |
| /// A group element in G1 consists of two field elements `(x, y)`. | |
| pub const ALT_BN128_G1_SIZE: usize = ALT_BN128_FIELD_SIZE * 2; |
|
|
||
| pub const ALT_BN128_FIELD_SIZE: usize = 32; | ||
| pub const ALT_BN128_G1_SIZE: usize = ALT_BN128_FIELD_SIZE * 2; // x, y each 32 byte | ||
| pub const ALT_BN128_G2_SIZE: usize = ALT_BN128_FIELD_SIZE * 4; // x=(x0,x1), y=(y0,y1) each 32 byte |
There was a problem hiding this comment.
| pub const ALT_BN128_G2_SIZE: usize = ALT_BN128_FIELD_SIZE * 4; // x=(x0,x1), y=(y0,y1) each 32 byte | |
| /// Elements in G2 is represented by 2 field-extension elements `(x, y)`. | |
| pub const ALT_BN128_G2_SIZE: usize = ALT_BN128_FIELD_SIZE * 4; |
There was a problem hiding this comment.
Overall looks great! I have a couple of points for discussion:
- The "non-checked" variations could use an array with the expected size, as @deanmlittle suggested. This way you can get compile-time error if you are passing an invalid input. I expect that most of the time you will have an array as an input.
- We could prefix the ones accepting a slice with
slice_instead ofchecked_. This is similar to theinvokevariants. Since these ones take a slice, they perform the length check.
Another alternative is to not have checked_ (slice_) variants and let the syscall fail if the input has the incorrect length. In the majority of the cases, an error in the syscall is not recoverable, and we should be optimizing for the "success" case. Programs have the option to do the validation if needed. In this case we would keep the argument as a slice.
A separa point is about the organization. What do you think if we follow a similar structure as the SDK? That means separating the implementation into:
addition.rscompression.rsmultiplication.rspairing.rs
No description provided.