feat: refactor key-related field operations to be atomic #5178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| connection | ||
| .transaction::<_, Error, _>(|| { | ||
| secondary_key_version = WalletSettingSql::get(&DbKey::SecondaryKeyVersion, connection) | ||
| .map_err(|_| Error::RollbackTransaction)?; |
There was a problem hiding this comment.
This mapping is not actually needed. Any Err will rollback the transaction. The docs on diesel.rs don't reflect this in v1.4, but have been updated in the 2.0 docs on docs.rs. In either case, I checked the source and it rolls back on Err.
So it's better to just return the error with ? and have the original error find their way to the logs
There was a problem hiding this comment.
I had tried this initially, but the compiler complained that the internal error couldn't be converted to a diesel error.
| .transaction::<_, Error, _>(|| { | ||
| WalletSettingSql::new(DbKey::SecondaryKeyVersion, self.secondary_key_version.to_string()) | ||
| .set(connection) | ||
| .map_err(|_| Error::RollbackTransaction)?; |
AaronFeickert
dismissed stale reviews from hansieodendaal and SWvheerden
via
280f98e
stringhandler
approved these changes
Feb 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Refactors key-related database field operations to be atomic.
Closes issue 5177.
Motivation and Context
Key-related database fields always travel together. We need a consistent set of secondary key version identifier, secondary key salt, and encrypted main key in order to set up the
XChaCha20-Poly1305cipher used for database encryption operations and passphrase changes.While a recent PR ensures that write operations for these fields are done atomically via a write transaction, there is no corresponding read transaction. It's therefore possible that those fields are inconsistent. While this should only result in an error and require the user to load their wallet again, it seemed like a smart idea to ensure that reads are consistent for any future use cases.
This PR refactors the handling of those fields to reduce redundancy and ensure atomicity for reads and writes. It introduces a new
DatabaseKeyFieldsstruct that handles reads and writes, and additionally takes care of encoding and decoding of the underlying data.It also makes the handling of the three fields more consistent. Previously, individual reads and writes required the use of a complex
matchto handle different states. This functionality has been mostly moved intoDatabaseKeyFieldsto make these states more apparent.How Has This Been Tested?
Existing unit tests pass. Manually tested the following operations:
It does not seem possible to directly test read operation inconsistency caused by a simultaneous write operation.