feat(ethexe): Create PromisesBundle for sending promises in network#5099
Conversation
|
Review changes with Changed Files
|
ecol-master
changed the base branch from
master
to
kuzmindev/feat/rpc-subscribe-all-promises
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 significantly enhances the handling and distribution of transaction promises within the Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a PromisesBundle structure and a helper function split_promises_into_bundles to divide a list of promises into smaller chunks that fit within the maximum gossip message size. This is a good step towards handling large sets of promises for network distribution.
My review has identified a critical bug in the splitting logic that could lead to data loss, as well as a couple of medium-severity issues related to code clarity and robustness. Please see the detailed comments for suggestions on how to address these points.
| let promise_size = promise.encoded_size(); | ||
| if size_counter + promise_size > MAX_GOSSIP_MESSAGE_SIZE { | ||
| bundles.push(PromisesBundle { | ||
| promises: current_bundle_promises.drain(..).collect(), | ||
| announce_hash, | ||
| seqnum, | ||
| is_last: false, | ||
| }); | ||
| seqnum += 1; | ||
| size_counter = PromisesBundle::encoding_offset(); | ||
| } else { | ||
| current_bundle_promises.push(promise); | ||
| size_counter += promise_size; | ||
| } |
There was a problem hiding this comment.
There is a critical bug in the promise splitting logic. When a promise causes the current bundle to exceed MAX_GOSSIP_MESSAGE_SIZE, a new bundle is created from the existing promises, but the current promise that caused the overflow is skipped and not added to any bundle. This will lead to data loss.
The logic should be adjusted to create a new bundle when it's full, and then add the current promise to the new current bundle.
let promise_size = promise.encoded_size();
if !current_bundle_promises.is_empty() && size_counter + promise_size > MAX_GOSSIP_MESSAGE_SIZE {
bundles.push(PromisesBundle {
promises: std::mem::take(&mut current_bundle_promises),
announce_hash,
seqnum,
is_last: false,
});
seqnum += 1;
size_counter = PromisesBundle::encoding_offset();
}
current_bundle_promises.push(promise);
size_counter += promise_size;| /// Currently set to 1 MB. | ||
| pub const MAX_GOSSIP_MESSAGE_SIZE: usize = 1024 * 1024; | ||
|
|
||
| /// The bundle of promises which will be distributed in network in a single gossip message. #[cfg_attr(feature = "std", derive(serde::Deserialize, serde::Serialize))] |
There was a problem hiding this comment.
There's a typo in the documentation for PromisesBundle. The #[cfg_attr(...)] attribute appears to have been accidentally included in the comment.
| /// The bundle of promises which will be distributed in network in a single gossip message. #[cfg_attr(feature = "std", derive(serde::Deserialize, serde::Serialize))] | |
| /// The bundle of promises which will be distributed in network in a single gossip message. |
| /// SCALE encoded size offset of the bundle without the promises. | ||
| /// This is needed, because of the size of the encoded data is not constant and depends of the data. | ||
| /// | ||
| /// `100` bytes is approximately three times exceeding the actual size of the bundle without promises. | ||
| pub fn encoding_offset() -> usize { | ||
| 100 | ||
| } |
There was a problem hiding this comment.
The encoding_offset function uses a hardcoded magic number 100. This is brittle and not precise. It would be more robust to calculate this value dynamically by encoding a default PromisesBundle instance with an empty promises vector. This will give you the exact size of the bundle's metadata.
/// Returns the SCALE encoded size of the bundle's metadata (all fields except `promises`).
pub fn encoding_offset() -> usize {
// An empty `PromisesBundle` will give us the size of all fixed-size fields
// plus the overhead for an empty `Vec`.
Self::default().encoded_size()
}1 participant
No description provided.