-
Notifications
You must be signed in to change notification settings - Fork 210
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: snapshot level isolation violations #5279
Conversation
While looking for mistakes in `.snapshot()` use I've noticed that these two (rarely used) methods did not have some existing fixes that the main versions do.
a4d477b
to
59e4a50
Compare
@@ -168,7 +168,7 @@ impl IRawDatabase for RocksDbReadOnly { | |||
impl<'a> IDatabaseTransactionOpsCore for RocksDbTransaction<'a> { | |||
async fn raw_insert_bytes(&mut self, key: &[u8], value: &[u8]) -> Result<Option<Vec<u8>>> { | |||
fedimint_core::runtime::block_in_place(|| { | |||
let val = self.0.get(key).unwrap(); | |||
let val = self.0.snapshot().get(key).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For onlookers....this was the bug :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can probably remove unwrap
here if we want too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boom!
Great work! 🎉 Wow, that's some unintuitive API on the rocksdb side! So if you use the snapshot struct for reading then what it actually does is read from the DB transaction, but setting the snapshot in the read options for each read (I was worried that it would only read from the snapshot at first). |
Successfully created backport PR for |
Fix #5271
Fix #5195