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

fix: improve key handling #4994

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

AaronFeickert
Copy link
Collaborator

Description

Updates the use of CoreTransactionAEADKey and CommsNoiseKey to be populated in place. Switches CommsNoiseKey to use a SafeArray under the hood.

Extends the work of PR 4967.

Motivation and Context

Earlier work updates the use of keys for value encryption and Noise. Value encryption keys use a SafeArray type in a Hidden wrapper, and Noise keys use a Hidden array. However, in both cases, a copy of the hash output used to populate the keys is left in memory.

This work mitigates the problem. Because the hashing API now supports in-place output finalization via finalize_into and finalize_into_reset, we can populate keys directly by mutable reference.

It also renames CoreTransactionAEADKey to EncryptedValueKey for clarity, and switches CommsNoiseKey to be a SafeArray under the hood. There is discussion on whether CommsNoiseKey needs to be a SafeArray; while the reasoning makes sense, I still think it's good practice to take advantage of the benefits of SafeArray for array-like key types unless there's a compelling performance reason otherwise. Discussion on this is welcome!

How Has This Been Tested?

Existing tests pass.

@AaronFeickert
Copy link
Collaborator Author

Before merging, it should be decided whether or not the domain separation labels for the relevant hashers should be updated to reflect the reverse-domain notation used elsewhere in the codebase. I think they probably should, but this will be a breaking change.

@stringhandler stringhandler merged commit f069b14 into tari-project:development Dec 5, 2022
@AaronFeickert AaronFeickert deleted the key-updates branch December 5, 2022 16:09
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.

2 participants