-
Notifications
You must be signed in to change notification settings - Fork 121
feat(ethexe): Producer provides only promises hashes, instead of its full body #5132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
41a85ff
a1e405c
cc740d5
d4dd026
b7fab2d
9d788d3
a844855
d8b64b8
ac2519e
dfaa7d1
8c7829c
3afe82b
e5c49ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -22,7 +22,7 @@ use core::hash::Hash; | |||||
| use gear_core::rpc::ReplyInfo; | ||||||
| use gprimitives::{ActorId, H256, MessageId}; | ||||||
| use parity_scale_codec::{Decode, Encode}; | ||||||
| use sha3::{Digest, Keccak256}; | ||||||
| use sha3::{Digest as _, Keccak256}; | ||||||
| use sp_core::Bytes; | ||||||
|
|
||||||
| /// Recent block hashes window size used to check transaction mortality. | ||||||
|
|
@@ -133,26 +133,72 @@ pub struct Promise { | |||||
| /// It will be shared among other validators as a proof of promise. | ||||||
| pub type SignedPromise = SignedMessage<Promise>; | ||||||
|
|
||||||
| impl ToDigest for Promise { | ||||||
| fn update_hasher(&self, hasher: &mut sha3::Keccak256) { | ||||||
| let Self { tx_hash, reply } = self; | ||||||
| /// A wrapper on top of [`CompactPromiseHashes`]. | ||||||
| /// | ||||||
| /// [`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>; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For naming compatibility I think better to use SignedCompactPromise (using Signed as prefix) |
||||||
|
|
||||||
| hasher.update(tx_hash.inner()); | ||||||
| impl Promise { | ||||||
| /// Calculates the `blake2b` hash from promise's reply. | ||||||
| pub fn reply_hash(&self) -> HashOf<ReplyInfo> { | ||||||
| let ReplyInfo { | ||||||
| payload, | ||||||
| code, | ||||||
| value, | ||||||
| } = reply; | ||||||
| } = &self.reply; | ||||||
|
|
||||||
| let bytes = [ | ||||||
| payload.as_ref(), | ||||||
| code.to_bytes().as_ref(), | ||||||
| value.to_be_bytes().as_ref(), | ||||||
| ] | ||||||
| .concat(); | ||||||
| unsafe { HashOf::new(gear_core::utils::hash(&bytes).into()) } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| hasher.update(payload); | ||||||
| hasher.update(code.to_bytes()); | ||||||
| hasher.update(value.to_be_bytes()); | ||||||
| impl ToDigest for Promise { | ||||||
| fn update_hasher(&self, hasher: &mut sha3::Keccak256) { | ||||||
| // The hash of `Promise` equals to hash of `CompactPromiseHashes`. | ||||||
| CompactPromiseHashes::from(self).update_hasher(hasher); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// The hashes of [`Promise`]. | ||||||
| #[derive(Debug, Clone, Encode, Decode, PartialEq, Eq)] | ||||||
| pub struct CompactPromiseHashes { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| pub tx_hash: HashOf<InjectedTransaction>, | ||||||
| pub reply_hash: HashOf<ReplyInfo>, | ||||||
| } | ||||||
|
|
||||||
| impl From<&Promise> for CompactPromiseHashes { | ||||||
| fn from(promise: &Promise) -> Self { | ||||||
| Self { | ||||||
| tx_hash: promise.tx_hash, | ||||||
| reply_hash: promise.reply_hash(), | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| #[cfg(test)] | ||||||
| impl ToDigest for CompactPromiseHashes { | ||||||
| fn update_hasher(&self, hasher: &mut sha3::Keccak256) { | ||||||
| let Self { | ||||||
| tx_hash, | ||||||
| reply_hash, | ||||||
| } = self; | ||||||
|
|
||||||
| hasher.update(tx_hash.inner()); | ||||||
| hasher.update(reply_hash.inner()); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| #[cfg(all(test, feature = "mock"))] | ||||||
| mod tests { | ||||||
| use gsigner::PrivateKey; | ||||||
|
|
||||||
| use super::*; | ||||||
| use crate::mock::Mock; | ||||||
|
|
||||||
| #[test] | ||||||
| fn signed_message_and_injected_transactions() { | ||||||
|
|
@@ -191,4 +237,37 @@ mod tests { | |||||
| signed_tx.address() | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn promise_hashes_digest_equal_to_promise_digest() { | ||||||
| let promise = { | ||||||
| let mut promise = Promise::mock(()); | ||||||
| promise.reply.value = 123; | ||||||
| promise.reply.payload = vec![1u8, 2u8, 42u8, 66u8]; | ||||||
| promise | ||||||
| }; | ||||||
| let promise_digest = promise.to_digest(); | ||||||
| let promise_hashes = CompactPromiseHashes::from(&promise); | ||||||
|
|
||||||
| let promise_hashes_digest = promise_hashes.to_digest(); | ||||||
| assert_eq!(promise_digest, promise_hashes_digest); | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn compact_signature_valid_for_promise() { | ||||||
| let pk = PrivateKey::random(); | ||||||
|
|
||||||
| let promise = Promise::mock(()); | ||||||
| let promise_hashes = CompactPromiseHashes::from(&promise); | ||||||
| let compact_signed_promise = CompactSignedPromise::create(pk, promise_hashes).unwrap(); | ||||||
|
|
||||||
| let (signature, address) = ( | ||||||
| *compact_signed_promise.signature(), | ||||||
| compact_signed_promise.address(), | ||||||
| ); | ||||||
|
|
||||||
| let signed_promise = SignedMessage::try_from_parts(promise.clone(), signature, address) | ||||||
| .expect("SignedMessage<Promise> was correctly constructed from CompactSignedPromise"); | ||||||
| assert_eq!(signed_promise.into_data(), promise); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ use ethexe_common::{ | |
| Announce, ComputedAnnounce, HashOf, SimpleBlockData, | ||
| db::{ | ||
| AnnounceStorageRO, AnnounceStorageRW, BlockMetaStorageRO, CodesStorageRW, | ||
| LatestDataStorageRO, LatestDataStorageRW, OnChainStorageRO, | ||
| InjectedStorageRW, LatestDataStorageRO, LatestDataStorageRW, OnChainStorageRO, | ||
| }, | ||
| events::BlockEvent, | ||
| }; | ||
|
|
@@ -168,6 +168,10 @@ impl<P: ProcessorExt> ComputeSubService<P> { | |
| }) | ||
| .ok_or(ComputeError::LatestDataNotFound)?; | ||
|
|
||
| promises.clone().into_iter().for_each(|promise| { | ||
ecol-master marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| db.set_promise(promise); | ||
| }); | ||
ecol-master marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+171
to
+173
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
|
|
||
| Ok(ComputedAnnounce { | ||
| announce_hash, | ||
| promises, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,9 +22,10 @@ use super::{ | |
| use crate::{ | ||
| ConsensusEvent, | ||
| announces::{self, DBAnnouncesExt}, | ||
| utils::sign_announce_promises, | ||
| validator::DefaultProcessing, | ||
| }; | ||
| use anyhow::{Context as _, Result, anyhow}; | ||
| use anyhow::{Result, anyhow}; | ||
| use derive_more::{Debug, Display}; | ||
| use ethexe_common::{ | ||
| Announce, ComputedAnnounce, HashOf, SimpleBlockData, ValidatorsVec, db::BlockMetaStorageRO, | ||
|
|
@@ -82,15 +83,9 @@ impl StateHandler for Producer { | |
| if *expected == computed_data.announce_hash => | ||
| { | ||
| if !computed_data.promises.is_empty() { | ||
| let signed_promises = computed_data | ||
| .promises | ||
| .into_iter() | ||
| .map(|promise| { | ||
| self.ctx | ||
| .sign_message(promise) | ||
| .context("producer: failed to sign promise") | ||
| }) | ||
| .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)?; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| self.ctx.output(ConsensusEvent::Promises(signed_promises)); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.