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

Add confidentiality feature to the EVM module #855

Merged
merged 7 commits into from
May 16, 2022
Merged

Conversation

nhynes
Copy link
Contributor

@nhynes nhynes commented Mar 22, 2022

This PR adds a feature flag, confidential, to the EVM module such that when it's enabled, all stores will be made to confidential storage. If the key manager isn't dispensing keys, the requester will receive zeros.

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #855 (0487bf6) into main (795caf5) will increase coverage by 0.14%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##             main     #855      +/-   ##
==========================================
+ Coverage   67.58%   67.72%   +0.14%     
==========================================
  Files         123      124       +1     
  Lines        9707     9742      +35     
==========================================
+ Hits         6560     6598      +38     
+ Misses       3122     3119       -3     
  Partials       25       25              
Impacted Files Coverage Δ
runtime-sdk/modules/evm/src/lib.rs 47.72% <0.00%> (-0.57%) ⬇️
runtime-sdk/modules/evm/src/raw_tx.rs 89.88% <ø> (ø)
runtime-sdk/modules/evm/src/backend.rs 61.94% <75.00%> (-0.34%) ⬇️
runtime-sdk/modules/evm/src/state.rs 91.17% <91.17%> (ø)
runtime-sdk/modules/evm/src/test.rs 96.98% <100.00%> (+0.15%) ⬆️
runtime-sdk/src/storage/mod.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 795caf5...0487bf6. Read the comment docs.

Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Ok, let's go with the feature flag for now.

Would be great to have some E2E tests as well just to make sure that this actually works.

runtime-sdk/src/storage/empty.rs Outdated Show resolved Hide resolved
@nhynes nhynes force-pushed the nhynes/confidential-evm branch 2 times, most recently from 0628af2 to cd8ff98 Compare May 9, 2022 16:00
@nhynes
Copy link
Contributor Author

nhynes commented May 9, 2022

@kostko want to take a second pass? I made the confidentiality into a config option and added tests. I think we should be good now?

]);
let keypair = kmgr_client
.get_or_create_keys(key_id)
.expect("unable to retrieve confidential storage keys");
Copy link
Member

Choose a reason for hiding this comment

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

It is even better to propagate a proper aborting error (e.g. see the Abort error in the contracts module) as a panic will trigger a runtime restart.

Copy link
Contributor Author

@nhynes nhynes May 16, 2022

Choose a reason for hiding this comment

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

I agree with you, but it doesn't look like that's quite possible given that the evm crate expects fetching storage to be infallible.

If this stateless code is the right approach, I don't want modify the evm crate for this strange requirement, as changing the trait will introduce a lot of noise that might make it hard to rebase onto upstream. Why not modify dispatch_tx to also std::panic::catch_unwind and turn it into a batch failure? That would solve the symptom.

Alternatively, key fetch can be preflighted at the start of the batch where it aborts. That'd rule out permanent failures, but there can still be transients that would cause abort. Is it really that bad?

Copy link
Member

Choose a reason for hiding this comment

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

Why not modify dispatch_tx to also std::panic::catch_unwind and turn it into a batch failure? That would solve the symptom.

Ah yes, that would also work, can you open an issue for this?

runtime-sdk/modules/evm/src/lib.rs Outdated Show resolved Hide resolved
@nhynes nhynes merged commit 9cfbb92 into main May 16, 2022
@nhynes nhynes deleted the nhynes/confidential-evm branch May 16, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants