fedimint / fedimint

Federated E-Cash Mint
https://fedimint.org/
MIT License
536 stars 210 forks source link

fix: handle write write conflicts in mem db #3989

Closed maan2003 closed 3 months ago

maan2003 commented 4 months ago

related https://github.com/fedimint/fedimint/issues/3988

maan2003 commented 4 months ago

still broken :/

test deadlocks now

m1sterc001guy commented 4 months ago

Without looking too much at the details here, are we sure we want to implement this into our memdb? We're essentially implementing MVCC snapshot isolation into a database that is maintained only for Fedimint. Just wondering if its more implementation and maintenance effort than it is worth.

maan2003 commented 4 months ago

Without looking too much at the details here, are we sure we want to implement this into our memdb? We're essentially implementing MVCC snapshot isolation into a database that is maintained only for Fedimint. Just wondering if its more implementation and maintenance effort than it is worth.

memdb is pretty much only thing we can use from wasm

douglaz commented 4 months ago

I was playing with this PR in the last couple of days. I still don't know what's the issue but I was able to reproduce it with a very small PR: https://github.com/fedimint/fedimint/pull/3995

elsirion commented 4 months ago

I think we can land this PR with a different test suite that tests write-write conflicts. @m1sterc001guy already wrote these tests, e.g. fedimint_core::db::expect_write_conflict, so that should be quick. The deadlock issue can be debugged separately.

maan2003 commented 4 months ago

Now only contains changes related to write conflicts

maan2003 commented 4 months ago

we still have to use memdb partially on wasm

elsirion commented 4 months ago

I restarted CI, either the correct impl triggers some flakiness more effectively or the PR has a subtle bug.

maan2003 commented 4 months ago

checking the old values makes commit_tx() slow enough, now we are able to reproduce same bug as https://github.com/fedimint/fedimint/pull/3995

same test sends_ecash_out_of_band is failing

it just gets stuck on request timeouts in the end.

codecov[bot] commented 4 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (04d32a7) 58.06% compared to head (dea6eb0) 58.11%. Report is 13 commits behind head on master.

Files Patch % Lines
fedimint-core/src/db/mem_impl.rs 84.21% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3989 +/- ## ========================================== + Coverage 58.06% 58.11% +0.04% ========================================== Files 192 192 Lines 42972 42985 +13 ========================================== + Hits 24951 24979 +28 + Misses 18021 18006 -15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dpc commented 4 months ago

It probably can't be done because of something that I'm not aware, but I wish we just wrap whole db into mutex when using memdb instead of trying to grow and maintain our database implementation. The difference in performance is kind of irrelevant.

maan2003 commented 4 months ago

someone holding to database transaction and not commit will block others forever

maan2003 commented 3 months ago

rebased after #4116