Stabilize BLS signatures and implement host functions #10327
Conversation
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
davxy
left a comment
davxy
left a comment
There was a problem hiding this comment.
Technically this looks fine, as you are only removing the bls-experimental feature.
My main concern is the target of this PR. I don't think it should be bundled with the PR for RFC-4/RFC-145, which covers a completely different topic. If we proceed this way, merging the PR for RFC-4 would also include the BLS stabilization on master branch. I think these changes should be introduced upstream in a dedicated PR, and only after RFC-156 is merged.
|
|
||
| /// Returns the `ecdsa & bls12-381` public key for the given key type and index in the keystore. | ||
| /// Panics if the key index is out of bounds. | ||
| fn ecdsa_bls381_public_key( |
There was a problem hiding this comment.
So now there is a dedicated hostcall to fetch a single public key and to count the public keys? As hostcalls are precious, I'd say no? Perhaps only the functions under wrapper exposed as hostcalls?
The wrapper/wrapped mechanism is introduced in the branch targeted by this PR, and I am not sure how it works.
This is mostly due to my current ignorance of how RFC-4/145 is implemented in the target PR
There was a problem hiding this comment.
So now there is a dedicated hostcall to fetch a single public key and to count the public keys?
Yes, it follows the logic introduced by RFC-145: https://github.com/s0me0ne-unkn0wn/RFCs/blob/rfc4/text/0145-remove-unnecessary-allocator-usage.md#ext_crypto_ed25519sr25519ecdsa_num_public_keys
The old behavior ("give me all the public keys") is obsolete, as the caller doesn't know the size of the buffer that it should provide to store all the keys.
To make the transition smoother, I implemented wrappers. A wrapper is basically a front-end code that only exists on the runtime side. It provides the old familiar interface to the developer, and inside it uses the new host functions as a backend to perform actual host calls.
In this case, we're exposing both new hostcalls (*_num_public_keys and _public_key) and a wrapper (_public_keys). Developers are free to use any combination of them.
However, we cannot exclude the possibility that this public key fetching interface may change during the RFC-145 discussion (I'm personally not entirely satisfied with its design).
We bundle everything requiring breaking changes in the host functions: #9876 (comment) I do understand it will require more effort to make it through (first submitting all the RFCs for voting and then merging the PRs one by one), but I don't see any other choice. I.e, given this PR, it has to be based on the RFC-145 PR as it uses mechanisms introduced by that PR. So basically, I'm chaining them. When RFC-145 PRs are merged into master, this PR's base branch will automatically switch to master, and we will merge it into master, not the other way round. |
drskalman
left a comment
drskalman
left a comment
There was a problem hiding this comment.
I'm also concern about extra host functions. The RFC at least does not mandates that every crypto type needs a sign function right?
| /// Stores the signature in the provided output buffer. | ||
| /// Returns 0 on success, -1 on error. | ||
| #[wrapped] | ||
| fn bls381_sign( |
There was a problem hiding this comment.
Why do we need bls381_sign?
| /// Stores the signature in the provided output buffer. | ||
| /// Returns 0 on success, -1 on error. | ||
| #[wrapped] | ||
| fn ecdsa_bls381_sign( |
There was a problem hiding this comment.
Same here. We never call sign from runtime/offchain worker. Additionally we can generate separete signatures and glue them together.
| /// Stores the signature in the provided output buffer. | ||
| /// Returns 0 on success, -1 on error. | ||
| #[wrapped] | ||
| fn ecdsa_bls381_sign_with_keccak256( |
There was a problem hiding this comment.
same here, bls doesn't sign with sha3 it is only ecdsa so we can simulate the action with other functions.
This PR stabilizes BLS signatures by removing
bls-experimentalfeature gate and implements RFC-156.It uses RFC-145 implementation branch as the base branch, as the implementations have to be bundled according to #9876.