Skip to content
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

[light-client] remove pending parentchain tx-inclusion check #1455

Merged
merged 8 commits into from
Sep 29, 2023
11 changes: 4 additions & 7 deletions core/parentchain/block-importer/src/block_importer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,10 @@ impl<
for (signed_block, raw_events) in
blocks_to_import.into_iter().zip(events_to_import.into_iter())
{
// Check if there are any extrinsics in the to-be-imported block that we sent and cached in the light-client before.
// If so, remove them now from the cache.
if let Err(e) = self.validator_accessor.execute_mut_on_validator(|v| {
v.check_xt_inclusion(&signed_block.block)?;

v.submit_block(&signed_block)
}) {
if let Err(e) = self
.validator_accessor
.execute_mut_on_validator(|v| v.submit_block(&signed_block))
{
error!("[{:?}] Header submission to light client failed: {:?}", id, e);
return Err(e.into())
}
Expand Down
1 change: 0 additions & 1 deletion core/parentchain/light-client/src/io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,6 @@ pub mod sgx_tests {
.unwrap();

assert_eq!(validator.genesis_hash().unwrap(), parachain_params.genesis_header.hash());
assert_eq!(validator.num_xt_to_be_included().unwrap(), 0);
assert_eq!(validator.latest_finalized_header().unwrap(), parachain_params.genesis_header);
assert_eq!(
validator.penultimate_finalized_block_header().unwrap(),
Expand Down
4 changes: 0 additions & 4 deletions core/parentchain/light-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ where
{
fn submit_block(&mut self, signed_block: &SignedBlock<Block>) -> Result<(), Error>;

fn check_xt_inclusion(&mut self, block: &Block) -> Result<(), Error>;

fn get_state(&self) -> &LightValidationState<Block>;
}

Expand All @@ -85,8 +83,6 @@ pub trait ExtrinsicSender {
}

pub trait LightClientState<Block: ParentchainBlockTrait> {
fn num_xt_to_be_included(&self) -> Result<usize, Error>;

fn genesis_hash(&self) -> Result<HashFor<Block>, Error>;

fn latest_finalized_header(&self) -> Result<Block::Header, Error>;
Expand Down
65 changes: 5 additions & 60 deletions core/parentchain/light-client/src/light_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,9 @@ use core::iter::Iterator;
use itp_ocall_api::EnclaveOnChainOCallApi;
use itp_storage::{Error as StorageError, StorageProof, StorageProofChecker};
use itp_types::parentchain::{IdentifyParentchain, ParentchainId};
use log::*;
use sp_runtime::{
generic::SignedBlock,
traits::{Block as ParentchainBlockTrait, Hash as HashTrait, Header as HeaderTrait},
traits::{Block as ParentchainBlockTrait, Header as HeaderTrait},
Justifications, OpaqueExtrinsic,
};
use std::{boxed::Box, fmt, sync::Arc, vec::Vec};
Expand Down Expand Up @@ -115,7 +114,10 @@ impl<Block: ParentchainBlockTrait, OcallApi: EnclaveOnChainOCallApi>
}
}

// A valid grandpa proof proves finalization of all previous unjustified blocks.
// Todo: Justifying the headers here is actually wrong, but it prevents an ever-growing
// `unjustified_headers` queue because in the parachain case we won't have justifications,
// and in solo chain setups we only get a justification upon an Grandpa authority change.
// Hence, we justify the headers here until we properly solve this in #1404.
relay.justify_headers();
relay.push_header_hash(header.hash());

Expand All @@ -128,17 +130,6 @@ impl<Block: ParentchainBlockTrait, OcallApi: EnclaveOnChainOCallApi>

Ok(())
}

fn submit_xt_to_be_included(&mut self, extrinsic: OpaqueExtrinsic) {
let relay = self.light_validation_state.get_relay_mut();
relay.verify_tx_inclusion.push(extrinsic);

debug!(
"[{:?}] {} extrinsics in cache, waiting for inclusion verification",
self.parentchain_id,
relay.verify_tx_inclusion.len()
);
}
}

impl<Block, OCallApi> Validator<Block> for LightValidation<Block, OCallApi>
Expand All @@ -160,44 +151,6 @@ where
self.submit_finalized_headers(header.clone(), vec![], justifications)
}

fn check_xt_inclusion(&mut self, block: &Block) -> Result<(), Error> {
let relay = self.light_validation_state.get_relay_mut();

if relay.verify_tx_inclusion.is_empty() {
return Ok(())
}

let mut found_xts = vec![];
block.extrinsics().iter().for_each(|xt| {
if let Some(index) = relay.verify_tx_inclusion.iter().position(|xt_opaque| {
<HashingFor<Block>>::hash_of(xt) == <HashingFor<Block>>::hash_of(xt_opaque)
}) {
found_xts.push(index);
}
});
Comment on lines -171 to -177
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the piece of code that didn't work and that caused a delay in block import.


// sort highest index first
found_xts.sort_by(|a, b| b.cmp(a));

let rm: Vec<OpaqueExtrinsic> =
found_xts.into_iter().map(|i| relay.verify_tx_inclusion.remove(i)).collect();

if !rm.is_empty() {
info!(
"[{:?}] Verified inclusion proof of {} extrinsics.",
self.parentchain_id,
rm.len()
);
}
debug!(
"[{:?}] {} extrinsics remaining in cache, waiting for inclusion verification",
self.parentchain_id,
relay.verify_tx_inclusion.len()
);

Ok(())
}

fn get_state(&self) -> &LightValidationState<Block> {
&self.light_validation_state
}
Expand All @@ -210,10 +163,6 @@ where
OCallApi: EnclaveOnChainOCallApi,
{
fn send_extrinsics(&mut self, extrinsics: Vec<OpaqueExtrinsic>) -> Result<(), Error> {
for xt in extrinsics.iter() {
self.submit_xt_to_be_included(xt.clone());
}

self.ocall_api
.send_to_parentchain(extrinsics, &self.parentchain_id)
.map_err(|e| {
Expand All @@ -230,10 +179,6 @@ where
Block: ParentchainBlockTrait,
OCallApi: EnclaveOnChainOCallApi,
{
fn num_xt_to_be_included(&self) -> Result<usize, Error> {
self.light_validation_state.num_xt_to_be_included()
}

fn genesis_hash(&self) -> Result<HashFor<Block>, Error> {
self.light_validation_state.genesis_hash()
}
Expand Down
5 changes: 0 additions & 5 deletions core/parentchain/light-client/src/light_validation_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,6 @@ impl<Block> LightClientState<Block> for LightValidationState<Block>
where
Block: ParentchainBlockTrait,
{
fn num_xt_to_be_included(&self) -> Result<usize, Error> {
let relay = self.get_relay();
Ok(relay.verify_tx_inclusion.len())
}

fn genesis_hash(&self) -> Result<HashFor<Block>, Error> {
Ok(self.get_relay().genesis_hash)
}
Expand Down
8 changes: 0 additions & 8 deletions core/parentchain/light-client/src/mocks/validator_mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,6 @@ impl Validator<Block> for ValidatorMock {
Ok(())
}

fn check_xt_inclusion(&mut self, _block: &Block) -> Result<()> {
Ok(())
}

fn get_state(&self) -> &LightValidationState<Block> {
&self.light_validation_state
}
Expand All @@ -65,10 +61,6 @@ impl ExtrinsicSender for ValidatorMock {
}

impl LightClientState<Block> for ValidatorMock {
fn num_xt_to_be_included(&self) -> Result<usize> {
todo!()
}

fn genesis_hash(&self) -> Result<HashFor<Block>> {
todo!()
}
Expand Down
11 changes: 3 additions & 8 deletions core/parentchain/light-client/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@

use codec::{Decode, Encode};
use sp_consensus_grandpa::{AuthorityList, SetId};
use sp_runtime::{
traits::{Block as BlockT, Header as HeaderT},
OpaqueExtrinsic,
};
use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
use std::{collections::VecDeque, fmt, vec::Vec};

/// Defines the amount of parentchain headers to keep.
Expand All @@ -35,7 +32,6 @@ pub struct RelayState<Block: BlockT> {
pub current_validator_set_id: SetId,
header_hashes: VecDeque<Block::Hash>,
pub unjustified_headers: Vec<Block::Hash>, // Finalized headers without grandpa proof
pub verify_tx_inclusion: Vec<OpaqueExtrinsic>, // Transactions sent by the relay
pub scheduled_change: Option<ScheduledChangeAtBlock<Block::Header>>, // Scheduled Authorities change as indicated in the header's digest.
}

Expand Down Expand Up @@ -79,7 +75,6 @@ impl<Block: BlockT> RelayState<Block> {
current_validator_set: validator_set,
current_validator_set_id: 0,
unjustified_headers: Vec::new(),
verify_tx_inclusion: Vec::new(),
scheduled_change: None,
}
}
Expand All @@ -95,11 +90,11 @@ impl<Block: BlockT> fmt::Debug for RelayState<Block> {
write!(
f,
"RelayInfo {{ last_finalized_block_header_number: {:?}, current_validator_set: {:?}, \
current_validator_set_id: {} amount of transaction in tx_inclusion_queue: {} }}",
current_validator_set_id: {}, number of unjustified headers: {} }}",
self.last_finalized_block_header.number(),
self.current_validator_set,
self.current_validator_set_id,
self.verify_tx_inclusion.len()
self.unjustified_headers.len()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should always be 0, but I still want to log this, just in case.

)
}
}
Loading