fix: panic: Must deleted existing spendable note [broken, don't land]
#5272
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
In one of the recent commits I've added an assertion to make sure a spendable note was actually always removed when we thought it was, as I was investigating a bug causing notes to be lost (fedimint#5195), and it seemed like a good idea. Too bad it causing flakes now: fedimint#5271 So it looks like our reliance on retrying db conflicts (`autocommit`), leads to inability to assert basic expectations about database state, which seems terrible. In PR I'm adding a little lock to prevent write-write conflicts in this area of the code, and I think it's worthwhile, just to be able to ensure database integrity. Moreover, I realized that `autocommit` is probably a bad idea altogether. Application typically require transaction conflict handling, because they are using **shared** databases. But with an embedded databases, there is no sharing with external entities so there *always* should be, or even *must be* a way to structure application and locking to avoid any conflicts, preventing db isolation level issues, inability to assert basic stuff, or just wasting time retrying things.
|
Damn. This doesn't really work, because what the reading-side sees is being swapped at the moment of commit. |
|
right, locking is better than autocommit. just have a lock for a set of keys. |
|
Having to lock more than across the tx commit ruins it though. :( |
dpc
changed the title
panic: Must deleted existing spendable notepanic: Must deleted existing spendable note [broken, don't land]
|
Probably not necessary anymore after fixing #5195 |
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.
Fix #5271
In one of the recent commits I've added an assertion to make sure a spendable note was actually always removed when we thought it was, as I was investigating a bug causing notes to be lost (#5195), and it seemed like a good idea.
Too bad it causing flakes now: #5271
So it looks like our reliance on retrying db conflicts (
autocommit), leads to inability to assert basic expectations about database state, which seems terrible.In PR I'm adding a little lock to prevent write-write conflicts in this area of the code, and I think it's worthwhile, just to be able to ensure database integrity.
Moreover, I realized that
autocommitis probably a bad idea altogether.Application typically require transaction conflict handling, because they are using shared databases. But with an embedded databases, there is no sharing with external entities so there always should be, or even must be a way to structure application and locking to avoid any conflicts, preventing db isolation level issues, inability to assert basic stuff, or just wasting time retrying things.