-
Notifications
You must be signed in to change notification settings - Fork 83
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
[AIP-20] Generic Operations of Algebraic Structures #86
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine, my major concern is how do we validate this for correctness and completeness. The section on ### Implementation of BLS12-381 structures
is very verbose with statements like /// NOTE: currently information-only and no operations are implemented for this structure.
Can we make this more succinct?
@davidiw if your concern is on the dependencies, arkworks cryptography libraries are widely adopted (stats), and to our knowledge is our best option in terms of correct & complete implementation. Wonder if additional auditing is still needed. |
Should be fixed now. |
aips/aip-20.md
Outdated
Below is the full specification in pseudo-Move. | ||
|
||
```rust | ||
module aptos_std::algebra { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we give this a more specific name than 'algebra'? It sounds weird for some one with math background, because there are many different algebras, and they would be named according to what they do. It could be crypt_algebra
, or whatever is adequate for this particular instance here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
|
||
```rust | ||
module aptos_std::algebra_bls12381 { | ||
/// The finite field $F_q$ used in BLS12-381 curves. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
F_q
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's intended latex. I also rewrote the bls12381 section so those equations can render.
aips/aip-20.md
Outdated
|
||
The construction of BLS12-381 curves involve many groups/fields, some frequently interacted by applications (e.g., `Fq12`, `Fr`, `G1Affine`, `G2Affine`, `Gt`) while others rarely used. Marker types for using these structures with `aptos_std::algebra` APIs should be defined and exposed to developers, along with their widely-used serialization formats and hash-to-group suites, (ideally in its own module named `aptos_std::algebra_bls12381`). | ||
|
||
Below are the full specification in pseudo-Move. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Below is the...'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rewrote the bls12381 section
aips/aip-20.md
Outdated
|
||
## Rationale | ||
|
||
An alternative non-generic approach is to expose instantiated schemes directly in stdlib. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: in aptos_stdlib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extra pass! Fixed.
aips/aip-20.md
Outdated
An alternative non-generic approach is to expose instantiated schemes directly in stdlib. | ||
For example, we can define a Groth16 proof verification function | ||
`0x1::groth16_b::verify_proof(vk, proof, public_inputs): bool` | ||
for every bilinear map `b`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial suggestion: replace ';' by a dot. Move "for ECDSA" as a "For ECDSA" new paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial suggestion 2: Replace b
by <curve>
and say "for every pairing-friendly elliptic curve <curve>
"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe <group>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
For example, we can define a Groth16 proof verification function | ||
`0x1::groth16_b::verify_proof(vk, proof, public_inputs): bool` | ||
for every bilinear map `b`; | ||
for ECDSA signatures which requires a hash function and a group, we can define |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: , which require**,** (remove 's')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
for every bilinear map `b`; | ||
for ECDSA signatures which requires a hash function and a group, we can define | ||
`0x1::ecdsa_h_g::verify_signature(pk, msg, sig):bool` | ||
for each pair of proper hash function `h` and group `g`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same suggestion as above: <hash>
and <group>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
`0x1::ecdsa_h_g::verify_signature(pk, msg, sig):bool` | ||
for each pair of proper hash function `h` and group `g`. | ||
|
||
Compared with the proposed approach, the alternative approach saves the work of constructing the schemes for Move developers. However, the size of stdlib can multiply too fast in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aptos_stdlib
"Furthermore, the non-generic approach is not scalable from a development standpoint: a new native is needed for every combination of cryptosystem and its underlying algebraic structure (e.g., elliptic curve)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
|
||
In general, every structure implements basic operations like (de)serialization, equality check, random sampling. | ||
|
||
A group may also implement the following operations. (Additive notions are used.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial: "For example, a group may [...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
- `multi_scalar_mul()` for efficient group multi-scalar multiplication. | ||
- `hash_to()` for hash-to-group. | ||
|
||
A field may also implement the following operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial: "As another example, a field may [...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
- `sqr()` for efficient field element squaring. | ||
- `from_u64()` for quick conversion from u64 to field element. | ||
|
||
For 3 groups that form a bilinear map, `pairing()` and `multi_pairing()` may be implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial: "Similarly, for 3 groups G1
, G2
, GT
that admit a bilinear map [...], pairing<G1, G2, GT>()
and [...]."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
For 3 groups that form a bilinear map, `pairing()` and `multi_pairing()` may be implemented. | ||
|
||
For a subset/superset relationship between 2 structures, `upcast()` and `downcast()` may be implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give examples here (e.g., the BLS12-381 use cases)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
#### Shared Scalar Fields | ||
|
||
Some groups share the same group order, | ||
and an ergonomic design from this is to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
Some groups share the same group order, | ||
and an ergonomic design from this is to | ||
allow multiple groups to share the same scalar field | ||
(mainly for the purpose scalar multiplication), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: for the purpose of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
(mainly for the purpose scalar multiplication), | ||
if they have the same order. | ||
|
||
E.g. the following should be supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial: "In other words, the following [...]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
#### Handling Incorrect Type Parameter(s) | ||
|
||
There is currently no easy way to ensure type safety for the generic operations. | ||
E.g., `pairing<A,B,C>(a,b,c)` can compile even there is no pairing between `A,B,C` existing/implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: even if groups A
, B
and C
do not admit a pairing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
There is currently no easy way to ensure type safety for the generic operations. | ||
E.g., `pairing<A,B,C>(a,b,c)` can compile even there is no pairing between `A,B,C` existing/implemented. | ||
|
||
Therefore the backend should handle the type checks at runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Therefore**,** the backend must handle [...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
|
||
Therefore the backend should handle the type checks at runtime. | ||
|
||
If a group operations that takes 2+ type parameters is invoked with incompatible type parameters, it should abort. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
operation (singular)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial: remove paragraph, and start sentence with "For example, [...]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it must abort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
Therefore the backend should handle the type checks at runtime. | ||
|
||
If a group operations that takes 2+ type parameters is invoked with incompatible type parameters, it should abort. | ||
E.g. `scalar_mul<GroupA, ScalarForBx>()` should abort with a “not implemented” error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would write this more clearly as:
For example, scalar_mul<GroupA, ScalarB>()
, where GroupA
and ScalarB
have different orders, will abort with a “not implemented” error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
If a group operations that takes 2+ type parameters is invoked with incompatible type parameters, it should abort. | ||
E.g. `scalar_mul<GroupA, ScalarForBx>()` should abort with a “not implemented” error. | ||
|
||
Invoking operation functions with user-defined types should also abort with a “not implemented” error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add: "For example, zero<std::option::Option<u64>>()
will abort."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
|
||
### Implementation of BLS12-381 structures | ||
|
||
To support all potential BLS12-381 operations using the `aptos_std::crypto_algebra` API, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial suggestion to replace these two paragraphs by:
To support a wide-enough variety of BLS12-381 operations using the aptos_std::crypto_algebra
API,
we implement several marker types for the relevant groups of order
We also implement marker types for popular serialization formats and hash-to-group suites.
Below, we describe all possible marker types we could implement for BLS12-381 and mark the ones that we actually implement as "implemented".
These, we believe, should be sufficient to support most BLS12-381 applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
|
||
## Risks and Drawbacks | ||
|
||
For move application developers, constructing cryptographic schemes manually with these building blocks can be error-prone, or even result in vulnerable applications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial suggestion: Replace this with the following paragraph:
Developing cryptographic schemes, whether in Move or in any other language, is very difficult due to the inherent mathematic complexity of such schemes, as well as the difficulty of using cryptographic libraries securely.
As a result, we caution Move application developers that implementing cryptographic schemes using the crypto_algebra.move
and/or the bls12381_algebra.move
modules will be error prone and could result in vulnerable applications.
That being said, the crypto_algebra.move
and the bls12381_algebra.move
Move modules have been designed with safety in mind.
First, we offer a minimal, hard-to-misuse abstraction for algebraic structures like groups and fields.
Second, our Move modules are type safe (e.g., inversion in a group G
returns an Option<G>
).
Third, our BLS12-381 implementation always performs prime-order subgroup checks when deserializing group elements, to avoid serious implementation bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
|
||
## Future Potential | ||
|
||
The module can be extended to support more structures and operations, allowing more complicated cryptographic applications to be built. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial: The crypto_algebra.move
Move module can be extended to support more structures (e.g., new elliptic curves) and operations (e.g., batch inversion of field elements). This will:
- Allow porting existing generic cryptosystems built on top of this module to new, potentially-faster-or-smaller curves.
- Allow building more complicated cryptographic applications that leverage new operations or new algebraic structures (e.g., rings, hidden-order groups, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
aips/aip-20.md
Outdated
|
||
The module can be extended to support more structures and operations, allowing more complicated cryptographic applications to be built. | ||
|
||
Once Move interface is available, it can be use to rewrite the move side specifications to ensure type safety at compile time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial: Once the Move language is upgraded with support for some kind of interfaces, [...] rewrite the Move [...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the piece missing here is that this involves a form of dispatch between the Move language and adapter layer. This is rather exceptional and unusual behavior. Could we please add a brief description of that?
Maybe we can add that in the rationale... we have chosen ...
well done |
No description provided.