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

Refactor/ipa multipoint interfaces #153

Conversation

thomas-quadratic
Copy link
Contributor

This PR initialise a refactor of the JNI/FFI_interface to standardize the interfaces between Java and Rust.

Notably, we want to leverage only crypto-primitives related to vector commitments from rust-verkle.
Any verkle logic is for besu-verkle-tries.
Moreover, methods are composable since they follow a uniform format.

Here are the new features:

  • Inputs and outputs between java and rust are rlp-encoded with all values being little-endian bytes.
  • types module includes 3 types: CommitmentBytes, CommitmentBytesCompressed, ScalarBytes. These are the only primitives that are used by the ffi-interface.
  • convert modules implements conversions to and from rust-verkle types.
  • encoding/decoding modules implements rlp Decodable and Encodable for our types.
  • traits module includes Committable and Updatable for vector commitments functionalities.

* @return commitment.to_bytes() - uncompressed serialization
* Commit to a vector of values.
*
* @param byte_size byte size of serialised scalars, at most 32.

Check notice

Code scanning / CodeQL

Spurious Javadoc @param tags Note

@param tag "byte_size" does not match any actual parameter of method "commit()".
* @return commitment.to_bytes() - compressed serialization
* Commit to a vector of values.
*
* @param byte_size byte size of serialised scalars, at most 32.

Check notice

Code scanning / CodeQL

Spurious Javadoc @param tags Note

@param tag "byte_size" does not match any actual parameter of method "commitAsCompressed()".
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
Signed-off-by: Thomas Zamojski <thomas.zamojski@quadratic-labs.com>
@thomas-quadratic thomas-quadratic force-pushed the refactor/ipa-multipoint-interfaces branch from e01245f to 5103fec Compare March 5, 2024 22:09
@kevaundray
Copy link
Contributor

This PR takes code from ffi_interface meaning that this repo explicitly needs to know about ark-ed-on-bls12-381-bandersnatch and banderwagon -- I don't think this is the right abstraction to take, as it leaks details about the cryptography downstream. ffi_interface never exposes details about the underlying curve, banderwagon because this is just not important for downstream crates for example.

If there are changes that are needed for the cryptography interface, then ideally this is done in ffi_interface and not in besu_native, so besu_native only ever imports ffi_interface.

RE RLP -- I'll have to defer to @matkt or someone who knows more -- from looking at the rest of the packages, this doesn't seem like the right place to add RLP related code, perhaps besu-verkle-trie is where it should be?

Copy link
Contributor

@matkt matkt left a comment

Choose a reason for hiding this comment

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

If the Besu native APIs expect a specific RLP format, I think the Java objects should be in the Besu native repo (java object), and these objects should have a serialize() method that will construct the RLP. This is so that there is no need to look at the native code to understand how the RLP should be constructed.

Bytes result = Bytes.of(LibIpaMultipoint.pedersenHash(total));
assertThat(result).isEqualTo(Bytes32.fromHexString("0xff6e8f1877fd27f91772a4cec41d99d2f835d7320e929b8d509c5fa7ce095c51"));
}
// @Test
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to keep that ?

@matkt
Copy link
Contributor

matkt commented Mar 7, 2024

This PR takes code from ffi_interface meaning that this repo explicitly needs to know about ark-ed-on-bls12-381-bandersnatch and banderwagon -- I don't think this is the right abstraction to take, as it leaks details about the cryptography downstream. ffi_interface never exposes details about the underlying curve, banderwagon because this is just not important for downstream crates for example.

If there are changes that are needed for the cryptography interface, then ideally this is done in ffi_interface and not in besu_native, so besu_native only ever imports ffi_interface.

RE RLP -- I'll have to defer to @matkt or someone who knows more -- from looking at the rest of the packages, this doesn't seem like the right place to add RLP related code, perhaps besu-verkle-trie is where it should be?

Indeed, having RLP here is a bit strange. However, I think it's necessary to consider the performance aspect to see if it allows for faster processing rather than having to do multiple JNI<>Java round-trips. Once we have the benchmarks, we can determine if it's worth exploring the RLP further or if we need to approach it differently.

@thomas-quadratic
Copy link
Contributor Author

Deprecated.

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.

3 participants