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

Provision light-client in mutual remote attestation #1399

Merged
merged 8 commits into from
Aug 1, 2023

Conversation

clangenb
Copy link
Contributor

@clangenb clangenb commented Jul 31, 2023

Closes #1370. As the the light-client db is provisioned now, we don't need to wait until the worker has synced the entire parentchain (except for the very first worker). This may greatly speed up the startup of new workers.

Other notes

@clangenb clangenb force-pushed the cl/provision-light-client-master branch from 4ae26f2 to 4a2d31e Compare July 31, 2023 13:47
core/parentchain/light-client/src/io.rs Show resolved Hide resolved
for LightClientStateSeal<B, LightClientState>
{
type LightClientState = LightClientState;
Copy link
Contributor Author

@clangenb clangenb Jul 31, 2023

Choose a reason for hiding this comment

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

Associated type is more flexible, as otherwise I would have to declare an extra generic param for the block when being generic of LightClientSealing because the generic type of the state is generic over the block, which in turn means that I would have had to introduce a phantom type for the block, not funny.

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.
}

impl<Block: BlockT> RelayState<Block> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Todo: don't define this on the RelayState itself, rather introduce a new type HeaderQueue, which implements those methods, and can be unit tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -63,12 +61,12 @@ impl FullParachainHandler {

let genesis_header = params.genesis_header.clone();

let light_client_seal = EnclaveLightClientSeal::new(base_path.join(LIGHT_CLIENT_DB_PATH))?;
let light_client_seal = GLOBAL_LIGHT_CLIENT_SEAL.get()?;
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 is now initialized at an earlier stage and made available as a global component.

Comment on lines -49 to +54
ShieldingKey = 0,
StateKey = 1,
State = 2,
ShieldingKey,
StateKey,
State,
LightClient,
Copy link
Contributor Author

@clangenb clangenb Jul 31, 2023

Choose a reason for hiding this comment

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

Clippy gave me warnings here, and as From<u8> is implemented manually too, I don't think it adds a lot of value. The TLS part should be improved anyhow.

@@ -57,6 +60,7 @@ impl From<u8> for Opcode {
0 => Opcode::ShieldingKey,
1 => Opcode::StateKey,
2 => Opcode::State,
3 => Opcode::LightClient,
_ => unimplemented!("Unsupported/unknown Opcode for MU-RA exchange"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only use this code on the tls-client side during initialization, still this is a runtime panic that could occur. Not nice: #1400

Comment on lines -37 to -44
pub struct SealHandler<ShieldingKeyRepository, StateKeyRepository, StateHandler>
where
ShieldingKeyRepository: AccessKey<KeyType = Rsa3072KeyPair> + MutateKey<Rsa3072KeyPair>,
StateKeyRepository: AccessKey<KeyType = Aes> + MutateKey<Aes>,
// Constraint StateT = StfState currently necessary because SgxExternalities Encode/Decode does not work.
// See https://github.com/integritee-network/sgx-runtime/issues/46.
StateHandler: HandleState<StateT = StfState>,
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anti-pattern: add traitbounds to type definitions when it is not necessary.

@clangenb clangenb added A0-core Affects a core part B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E0-breaksnothing labels Jul 31, 2023
@clangenb
Copy link
Contributor Author

clangenb commented Aug 1, 2023

Ooops, sorry it seems that I fixed the sidechain demo, but I bricked the indirect invocation. I did not expect that. I have to look into it.

Comment on lines +121 to +126
#[derive(Debug)]
pub struct LightClientStateSealSync<B, LightClientState> {
seal: LightClientStateSeal<B, LightClientState>,
_rw_lock: RwLock<()>,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
#[derive(Debug)]
pub struct LightClientStateSealSync<B, LightClientState> {
seal: LightClientStateSeal<B, LightClientState>,
_rw_lock: RwLock<()>,
}
// safe to clone into threads.
#[derive(Clone, Debug)]
pub struct LightClientStateSealSync<B, LightClientState> {
seal: Arc<RwLock<LightClientStateSeal<B, LightClientState>>,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think a bit more before we tackle that: #1401

@clangenb clangenb marked this pull request as ready for review August 1, 2023 08:48
@clangenb
Copy link
Contributor Author

clangenb commented Aug 1, 2023

I have the strong belief that the CI errors are simply due to instability: #1403

Hence, I think that this PR is ready to merge. (The required CI tests will be introduced in the #1337 PR, so we still merge this PR)

Copy link
Contributor

@OverOrion OverOrion left a comment

Choose a reason for hiding this comment

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

LGTM, as the small things that have to be ironed out are in their own separate issue.

@clangenb clangenb merged commit f5e059f into master Aug 1, 2023
25 checks passed
@clangenb clangenb deleted the cl/provision-light-client-master branch August 14, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-core Affects a core part B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E0-breaksnothing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send light-client state with state provisioning.
2 participants