Architecture: Rename ClientApi -> SystemApi + Move CryptoUtils into VM Layer #1804
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
talekhinezh
commented
May 8, 2024
talekhinezh
commented
- Rename ClientApi -> SystemApi
- Move CryptoUtils from System Layer into VM Layer
|
Docker tags |
Benchmark for 00d71f3Click to view benchmark
|
mstrug-rdx
approved these changes
May 9, 2024
There was a problem hiding this comment.
When removing crypto bls related functions, you also removed trace_resource macro which was used to calculate costing coefficients. I've added comments to that macros to move them to lower layer functions in case we need to run costing evaluation again.
Also I saw in couple places that KernelNodeApi + ClientApi was changed to SystemApi, this is probably intentional but just wanted to point that out.
| } | ||
| } | ||
|
|
||
| #[trace_resources(log=message.len(), log=public_keys.len())] |
There was a problem hiding this comment.
trace_resource macro should be moved to fast_aggregate_verify_bls12381_v1() function.
| } | ||
|
|
||
| // Trace average message length and number of public_keys | ||
| #[trace_resources(log={pub_keys_and_msgs.iter().flat_map(|(_, msg)| msg).count()/pub_keys_and_msgs.len()},log=pub_keys_and_msgs.len())] |
There was a problem hiding this comment.
trace_resource macro should be moved to aggregate_verify_bls12381_v1() function.
| Y: KernelApi<System<V>>, | ||
| V: SystemCallbackObject, | ||
| { | ||
| #[trace_resources(log=message.len())] |
There was a problem hiding this comment.
trace_resource macro should be moved to verify_bls12381_v1() function.
| } | ||
| } | ||
|
|
||
| #[trace_resources(log=signatures.len())] |
There was a problem hiding this comment.
trace_resource macro should be moved to Bls12381G2Signature::aggregate() function.
| } | ||
| } | ||
|
|
||
| #[trace_resources(log=data.len())] |
There was a problem hiding this comment.
trace_resource macro should be moved to keccak256_hash() function.
iamyulong
reviewed
May 9, 2024
| Ok(result) | ||
| if pub_keys_and_msgs.is_empty() { | ||
| return Err(InvokeError::SelfError(WasmRuntimeError::InputDataEmpty)); | ||
| } |
There was a problem hiding this comment.
No it should be the same logic it just changes the logic into the "Return early" pattern. Unless you mean for something else?
There was a problem hiding this comment.
I don't see where we do this non-empty check before.
If it's not new, the order of the non-empty check and costing also matters? Transaction fails with different costs.
There was a problem hiding this comment.
The check was in the api.bls12381_v1_aggregate_verify() call, the logic of which has now moved into this function.
| Ok(result) | ||
| if public_keys.is_empty() { | ||
| return Err(InvokeError::SelfError(WasmRuntimeError::InputDataEmpty)); | ||
| } |
| let agg_sig = self.api.bls12381_g2_signature_aggregate(&signatures)?; | ||
| if signatures.is_empty() { | ||
| return Err(InvokeError::SelfError(WasmRuntimeError::InputDataEmpty)); | ||
| } |
iamyulong
approved these changes
May 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.