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

bump teaclave-sdk to v1.1.6-testing #1074 #1076

Merged

Conversation

OverOrion
Copy link
Contributor

@OverOrion OverOrion commented Oct 24, 2022

This PR aims to:

  • update to a newer Rust SGX SDK (1.1.6-testing branch)
  • update the toolchain (2022-10-22)

Fixes #1000
Unblocks Update polkadot-v0.9.29 #1053


There are some clippy lint fixes I was unsure about:

  1. The clippy::large_enum_variant marked enum TrustedOperationOrHash<Hash> as large (enum StreamState as well) and suggested using Boxed values instead. I did so as the variant OperationEncoded(Vec<u8>) is heap allocated as well. Is it okay or should this lint be surpressed?
  2. The clippy::or_fun_call lint in fn pruned function's
self.finality_watchers.entry(block_hash).or_insert(vec![]).push(tx.clone());

line could be written as

self.finality_watchers.entry(block_hash).or_default().push(tx.clone());

However the enclave-runtime can not find this method:

error[E0599]: no method named `or_default` found for enum `linked_hash_map::Entry` in the current scope
method not found in `linked_hash_map::Entry<'_, H256, Vec<H>>`

I suspect this might be due to SGX STD env, but I haven't confirmed it (yet), which is why I opted for skipping it.

  1. clippy::result_large_err
    This seems to come from one of our dependencies, which is why I added it to the allowlist.

@OverOrion OverOrion force-pushed the szp/teaclave-sdk-1.1.6-testing branch from fd58756 to c6758fe Compare October 24, 2022 14:11
@OverOrion OverOrion self-assigned this Oct 25, 2022
@OverOrion OverOrion force-pushed the szp/teaclave-sdk-1.1.6-testing branch 6 times, most recently from e814147 to f049d52 Compare October 26, 2022 22:05
@OverOrion OverOrion marked this pull request as ready for review October 28, 2022 20:42
@OverOrion OverOrion marked this pull request as draft October 28, 2022 20:42
@OverOrion OverOrion force-pushed the szp/teaclave-sdk-1.1.6-testing branch from 09a21b4 to 09db4ef Compare October 28, 2022 21:47
@OverOrion OverOrion force-pushed the szp/teaclave-sdk-1.1.6-testing branch 3 times, most recently from 2f0ffce to 9c34a8e Compare November 8, 2022 12:50
@OverOrion OverOrion added A0-core Affects a core part B1-releasenotes E3-hardmerge PR introduces a lot changes in a lot of files. Requires some effort to merge or rebase to. C1-low 📌 Does not elevate a release containing this beyond "low priority" labels Nov 8, 2022
@OverOrion OverOrion marked this pull request as ready for review November 8, 2022 14:42
@OverOrion OverOrion requested a review from clangenb November 8, 2022 14:43
@OverOrion OverOrion force-pushed the szp/teaclave-sdk-1.1.6-testing branch from a214cf8 to 4108018 Compare November 8, 2022 15:19
Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Only one minor issue, we should investigate. Otherwise, it looks very good! Thanks for fighting this war. :)

Cargo.lock Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
enclave-runtime/Cargo.lock Outdated Show resolved Hide resolved
Comment on lines -38 to +41
TlsStream(RustlsStream),
TlsStream(Box<RustlsStream>),
WebSocketHandshake(RustlsMidHandshake),
EstablishedWebsocket(RustlsWebSocket),
EstablishedWebsocket(Box<RustlsWebSocket>),
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this one also because of a big discrepancy between enum variant sizes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will add it to the PR description as well.

Cargo.lock Outdated
Comment on lines 6463 to 6465
name = "sgx_crypto_helper"
version = "1.1.5"
source = "git+https://github.com/apache/incubator-teaclave-sgx-sdk?tag=v1.1.5#d2d339cbb005f676bb700059bd51dc689c025f6b"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should investigate where this dependency comes from. I am a bit wary of using an old one.

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 seems to have come from core/tls-websocket-server/Cargo.toml.
Fixed this in bd750ad.
I will check the patches section of the cargo workspace though, It should have been overridden by that. 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, patches don't work across different versions.

Thanks for fixing it! ❤️

@clangenb
Copy link
Contributor

clangenb commented Nov 8, 2022

Thank you so much for the detailed pr descriptions. It is a bliss. : 🚀

@clangenb clangenb merged commit f5674c4 into integritee-network:master Nov 8, 2022
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" E3-hardmerge PR introduces a lot changes in a lot of files. Requires some effort to merge or rebase to.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request of updating the rust toolchain version
2 participants