feat(ethexe): Producer provides only promises hashes, instead of its full body#5377
feat(ethexe): Producer provides only promises hashes, instead of its full body#5377ecol-master wants to merge 11 commits intomasterfrom
Conversation
ecol-master
added
A1-inprogress
ecol-master
force-pushed
the
kd/reopen/producer-distribute-promise-hashes
branch
from
bbd6434 to
85cfa8a
Compare
Summary of ChangesHello, 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 optimizes the promise handling mechanism within the ethexe node by introducing a compact representation for promises. By transferring only hashes instead of full promise bodies, the network load is significantly reduced. Additionally, the RPC API for injected transactions has been refactored to support this new flow, ensuring efficient promise subscription and relaying. These changes also include updates to the node configuration to support different promise emission modes. 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. 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 refactors the promise handling logic in ethexe by introducing a compact promise system to optimize network data transfer. It adds SignedCompactPromise, a PromiseSubscriptionManager for RPC subscriptions, and a BoundPromiseSink to link promises with their originating announces. The changes span the compute, consensus, network, and service layers, including the addition of a PromiseEmissionMode configuration. Feedback highlights a need for immediate error propagation when promise restoration fails to prevent client timeouts and suggests standardizing on sha3 hashing to align with repository conventions.
ecol-master
added
A0-pleasereview
…//github.com/gear-tech/gear into kd/reopen/producer-distribute-promise-hashes
|
@claude review |
|
Claude finished @ecol-master's task in 4m 17s —— View job Code ReviewThis PR reduces network bandwidth by having the producer gossip only hashes of promises ( severity: high — Compact promise with wrong reply_hash silently leaves subscriber in permanent limbo File: In The only way // promise_manager.rs line 117
match compact.restore(promise) {
Ok(signed_promise) => { … }
Err(err) => {
warn!(…);
self.waiting_for_compute.insert(tx_hash, compact); // ← never useful here
}
}The subscriber's oneshot sender should be removed from the map and dropped here (which closes the receiver), so the spawner task wakes up and exits cleanly rather than timing out: Err(err) => {
warn!(…);
self.cancel_registration(tx_hash); // signal timeout to subscriber now
}severity: high — Compact promise signature is never cryptographically verified before reaching File:
The severity: medium — File: When a compact promise arrived first (stored in Err(_err) => {
trace!(…); // ← subscriber silently hangs
}Same fix as issue 1 — drop the sender so the subscriber task can exit cleanly. severity: medium — File:
severity: medium — File:
severity: medium — Panic rather than graceful shutdown when File: if rpc.is_none() && network.is_none() {
panic!("Promise without network or rpc");
}A misconfigured node hits an unrecoverable panic in the main event loop. severity: low — File: When severity: low — File:
SummaryThe protocol change itself is solid — the |
…//github.com/gear-tech/gear into kd/reopen/producer-distribute-promise-hashes
| false, | ||
| "Connect node received the promise for signing, this should never happen" | ||
| ); | ||
| // Nothing to do. |
| impl Default for ProtocolTimelines { | ||
| fn default() -> Self { | ||
| Self { | ||
| genesis_ts: 0, | ||
| era: NonZeroU64::new(10_000).unwrap(), | ||
| election: 200, | ||
| slot: NonZeroU64::new(2).unwrap(), | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove this, in tests ProtocolTimelines::mock() can be used
| warn!( | ||
| ?compact, %tx_hash, error=?err, "failed to create signed promise from parts, producer send invalid signature: compact_promise={compact:?}" | ||
| ); | ||
| self.waiting_for_compute.insert(tx_hash, compact); |
There was a problem hiding this comment.
Looks like any validator can create infinite number of signed random hashes and overflow rpc heap memory
| pub fn try_register_subscriber( | ||
| &self, | ||
| tx_hash: HashOf<InjectedTransaction>, | ||
| ) -> Result<PendingSubscriber, RegisterSubscriberError> { | ||
| match self.subscribers.entry(tx_hash) { | ||
| Entry::Occupied(_) => Err(RegisterSubscriberError::AlreadyRegistered(tx_hash)), | ||
| Entry::Vacant(entry) => { | ||
| let (sender, receiver) = oneshot::channel(); | ||
| entry.insert(sender); | ||
| Ok(PendingSubscriber::new(&self.db, tx_hash, receiver)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not a bug now, but can lead to the bugs in future - tx can be already computed and with promise inside, so rpc would never reply to user, so better to check that here.
| if let Some((_, compact_promise)) = self.waiting_for_compute.remove(&promise.tx_hash) { | ||
| match compact_promise.restore(promise) { | ||
| Ok(signed_promise) => { | ||
| self.db.set_compact_promise(&compact_promise); | ||
| self.dispatch_promise(signed_promise); |
There was a problem hiding this comment.
Should be discussed with @breathx - whether it's ok to overwrite promise signature by any other validator? Looks like we are not checking that it's validator also
| pub enum PromiseEmissionMode { | ||
| /// Node should always emit promises during announces execution. | ||
| /// Always set [`PromisePolicy::Enabled`]. | ||
| AlwaysEmit, |
There was a problem hiding this comment.
better to append at least one test in compute service for this mode
2 participants
@gear-tech/dev