feat(ethexe): Producer provides only promises hashes, instead of its full body #5132
feat(ethexe): Producer provides only promises hashes, instead of its full body #5132ecol-master wants to merge 13 commits intomasterfrom
Conversation
Changed Files
|
Summary of ChangesHello @ecol-master, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the handling and propagation of transaction promises within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of promises by the producer to send compact, signed promise hashes instead of the full promise body, which is a good optimization for network bandwidth. The changes are spread across multiple crates, introducing PromisesNetworkBundle and CompactSignedPromise and updating the database, consensus, and network layers to support this new flow.
However, the implementation appears to be incomplete and contains several critical issues that must be addressed:
- The verification logic for promise bundles in
ethexe/network/src/validator/topic.rsis commented out, which poses a significant security risk. - The associated tests for this verification are also disabled.
- The RPC API implementation in
ethexe/rpc/src/apis/injected.rscontains multipletodo!placeholders for crucial logic and error handling.
These issues suggest the feature is not ready for merging. I've left specific comments on these points. Additionally, I've suggested a minor performance improvement in ethexe/compute/src/compute.rs.
77cdb86 to
a1e405c
Compare
grishasobol
left a comment
There was a problem hiding this comment.
LGTM , but please wait for @ark0f approve
|
|
||
| /// The hashes of [`Promise`]. | ||
| #[derive(Debug, Clone, Encode, Decode, PartialEq, Eq)] | ||
| pub struct CompactPromiseHashes { |
There was a problem hiding this comment.
| pub struct CompactPromiseHashes { | |
| pub struct CompactPromise { |
| /// | ||
| /// [`CompactPromiseHashes`] is a lightweight version of [`SignedPromise`], that is | ||
| /// needed to reduce the amount of data transferred in network between validators. | ||
| pub type CompactSignedPromise = SignedMessage<CompactPromiseHashes>; |
There was a problem hiding this comment.
For naming compatibility I think better to use SignedCompactPromise (using Signed as prefix)
| pub fn provide_compact_promise(&self, compact_promise: CompactSignedPromise) { | ||
| self.injected_api.receive_compact_promise(compact_promise); | ||
| } | ||
|
|
||
| pub fn provide_compact_promises(&self, compact_promises: Vec<CompactSignedPromise>) { | ||
| compact_promises | ||
| .into_iter() | ||
| .for_each(|compact_promise| self.provide_compact_promise(compact_promise)); |
There was a problem hiding this comment.
I'm not insisting, but I think receive_compact_promises is better naming. You even have the same for injected_api.
Or maybe something like handle_compact_promises or process_compact_promises
| #[method(name = "getTransactionPromise")] | ||
| async fn get_transaction_promise( | ||
| &self, | ||
| tx_hash: HashOf<InjectedTransaction>, | ||
| ) -> RpcResult<Option<SignedPromise>>; |
There was a problem hiding this comment.
Not important. I think for RPC call it's better to return error in None cases
| promises.clone().into_iter().for_each(|promise| { | ||
| db.set_promise(promise); | ||
| }); |
There was a problem hiding this comment.
Could we move this to RPC? as I can see it's completely useless except if it's RPC. So, better to avoid useless cross services guaranties (in this case compute guaranties for RPC that promises are set in database)
| .collect::<Result<_, _>>()?; | ||
| let (signer, public_key) = (&self.ctx.core.signer, self.ctx.core.pub_key); | ||
| let signed_promises = | ||
| sign_announce_promises(signer, public_key, computed_data.promises)?; |
There was a problem hiding this comment.
not important, for me is much more clear without sign_announce_promises - the logic is too specific and trivial to move it to separate function
| // TODO: rename to `Validators` | ||
| Commitments(SignedValidatorMessage), | ||
| Promise(SignedPromise), | ||
| Promise(CompactSignedPromise), |
No description provided.