Database Transactions #581

m1sterc001guy · 2022-09-15T19:39:57Z

This PR is a work in progress. It adds a new trait for a Database Transaction and implements the database transaction for RocksDB. I have integrated the API into the federation module. The tests are still broken and I am currently testing the changes.

The main difference between this PR and the current code is that in this PR a single Database Transaction and passed around to subsequent functions via a reference. The old code use a "Transaction batch" concept and each function either had a copy of the parent transaction, or a "subtransaction" (which is just a copy of the parent).

Open Questions.

Do we want the transaction API to differentiate between new insertions and insertions that potentially could overwrite a key? (current code does support this).
Passing a reference to a single transaction instead of a transaction batch changes the semantics of "subtransactions". In this PR I have replaced all commits with checkpoints (which internally just set the savepoint on the transaction), but I am still evaluating if these are the correct semantics.
I am open to suggestions on scoping as this change touches lots of code. This PR only updates the federation code and leaves the client alone.

jkitman · 2022-09-15T19:51:11Z

One thing I've been meaning to do is to run with a real database in our integration tests (versus in-memory mock). This could be a way we can test the RocksDB implementation works.

m1sterc001guy · 2022-09-15T21:30:09Z

One thing I've been meaning to do is to run with a real database in our integration tests (versus in-memory mock). This could be a way we can test the RocksDB implementation works.

I can work on this as part of getting this change checked in.

dpc · 2022-09-15T22:23:07Z

fedimint-api/src/db/mod.rs

@@ -46,6 +46,24 @@ pub trait Database: Send + Sync {
    fn raw_find_by_prefix(&self, key_prefix: &[u8]) -> PrefixIter<'_>;

    fn raw_apply_batch(&self, batch: DbBatch) -> Result<()>;
+
+    fn new_transaction<'a>(&'a self) -> Box<dyn DbTransaction<'a> + 'a>;


I would advise to wrap Box<dyn DbTransaction<'a> + 'a> into a neat newtype that Derefs to &dyn DBTransaction` to avoid typing all that all the time. We're going to be using that pattern all over the modularization framework as well. Example: https://github.com/fedimint/fedimint/pull/546/files#diff-de9b10a6ed1a7ca9958bd8722504beb2edd17137c8ded2c261379e3fee1f9357R26

m1sterc001guy · 2022-09-16T19:08:24Z

Appreciate the feedback on this. I have been thinking about the questions above and come to some conclusions.

I think it is worth preserving the new vs overwrite semantics. This can potentially catch errors if the code is expecting an insert to be new but its not for some reason. I will add this.
I don't think we need to preserve the subtransaction semantics. These subtransactions were really just all one transaction anyway, so I think its better to make it explicit in the code.
My proposal will be to introduce the new trait and API and make a small change to integrate transactions. Subsequent PRs can refactor the code to use it everywhere.

Additionally, longer term I think we should migrate away from Optimistic Transactions and support transactions using DB locks. This will catch conflicts at write/read time rather than at commit time, which potentially could allow us to reject a write instead of throwing away the whole db transaction. This is a large change so I'll also leave this for another PR.

Will get started working on integrating the feedback above.

dpc · 2022-09-16T19:36:14Z

Additionally, longer term I think we should migrate away from Optimistic Transactions and support transactions using DB locks.

dpc · 2022-09-16T21:40:52Z

Sorry, I only meant to close the #428 and must have got confused.

m1sterc001guy · 2022-09-30T21:01:55Z

Pushed a new version of the code that rebased on top of master, reduced the scope of the PR, and added a unit test for transactions for the memory db, rocksdb, and sled db.

dpc · 2022-10-01T06:04:20Z

fedimint-api/src/db/mem_impl.rs

+        Box::new(MemDbIter { data })
+    }
+
+    fn commit_tx(self: Box<Self>) -> Result<()> {


How does this deal with write isolation what are our requirements of transaction isolation anyway?

Basically:

transaction A starts (gets a copy of data)

transaction B starts (gets a copy of data)

transaction A deletes a key X - it works, in the copy of the data

transaction B deletes a key X - it works, in the copy of the data

transaction A commits, it works, X gets deleted

transaction B commits, it should fail as X is deleted and there was a conflict

Seems like rocksdb is implementing a snapshot level transaction isolation? https://www.sobyte.net/post/2022-01/rocksdb-tx/

So to implement it we would need to check which keys were modified since the tx started? Or have some shared map with a per-key lock, so we can lock per any modified key? https://en.wikipedia.org/wiki/Multiversion_concurrency_control basically, I guess, maybe (I'm not an expert)?

Honestly, I would consider not implementing/using any in-memory database, and just use an ephemeral (temporary, random named file) real database instance in tests. In my experience, database integration is so important and easy to isolate that it's better to just not mock/fake it, take a tiny performance hit and run more tests against the real thing.

We chatted offline but I'll respond here for transparency. Yes the above description is exactly right, rocksdb Optimistic Concurrency has snapshot isolation and allows for transactions to read their own writes. The above example with the deletion of the key is how rocksdb would operate.

I very much agree that re-implementing snapshot isolation for an in-memory key value store is not worth our effort. I just didn't want to regress the existing database implementations (sled and in-memory), so essentially I added some hacky code that allows those implementations to operate on a snapshot, but they won't do conflict resolution during commit (because as you point out, that would require MVCC). Since @elsirion mentioned that we currently shouldn't have any write-write conflicts, I think this is ok for now.

I agree that the proper fix is to use rocksdb for the mock tests as well as the real tests and just remove the in-memory and sled support altogether.

dpc · 2022-10-03T17:21:04Z

Fixed merge conflict.

codecov-commenter · 2022-10-03T18:15:30Z

Codecov Report

Base: 66.61% // Head: 67.28% // Increases project coverage by +0.67% 🎉

Coverage data is based on head (769abf0) compared to base (69d05b8).
Patch coverage: 84.24% of modified lines in pull request are covered.

❗ Current head 769abf0 differs from pull request most recent head 81beea6. Consider uploading reports for the commit 81beea6 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #581      +/-   ##
==========================================
+ Coverage   66.61%   67.28%   +0.67%     
==========================================
  Files          87       87              
  Lines       12771    13112     +341     
==========================================
+ Hits         8507     8823     +316     
- Misses       4264     4289      +25

Impacted Files	Coverage Δ
fedimint-api/src/module/mod.rs	`92.85% <ø> (ø)`
fedimint-api/src/macros.rs	`7.84% <39.24%> (+0.72%)`	⬆️
fedimint-api/src/db/mod.rs	`80.44% <81.88%> (+1.27%)`	⬆️
fedimint-sled/src/lib.rs	`78.48% <97.05%> (+27.05%)`	⬆️
fedimint-api/src/db/mem_impl.rs	`91.80% <98.43%> (+7.32%)`	⬆️
fedimint-api/src/module/testing.rs	`93.46% <100.00%> (+0.13%)`	⬆️
fedimint-rocksdb/src/lib.rs	`70.47% <100.00%> (+13.53%)`	⬆️
fedimint-server/src/consensus/mod.rs	`93.07% <100.00%> (+0.31%)`	⬆️
modules/fedimint-ln/src/lib.rs	`93.02% <100.00%> (+0.03%)`	⬆️
modules/fedimint-mint/src/lib.rs	`90.90% <100.00%> (-0.22%)`	⬇️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

* Add support for lifetimes * `Box<...>` does not have to be `Sync` (that's kind of a whole purspose of using it over `Arc`, I guess)

okjodom · 2022-10-04T18:41:27Z

integrationtests/tests/tests.rs

@@ -317,6 +317,7 @@ async fn drop_peers_who_contribute_bad_sigs() {
    assert!(fed.subset_peers(&[0, 1, 2]).has_dropped_peer(3));
 }

+/*


@m1sterc001guy , why do we have to disable this test with the DB change?

PS: disabling by comment is super easy to miss in reviews. I think we should use #[ignore] macro

#[tokio::test(flavor = "multi_thread")] #[ignore] async fn my_test_case() { ... }

Oops apologies, didn't mean to keep that in there. I will revert.

Will use ignore in the future, thanks for the tip.

Re-enable the test: #743

The test is stalling for me locally (with and without my change) but I haven't had time to dig into why.

dpc reviewed Sep 15, 2022

View reviewed changes

dpc mentioned this pull request Sep 15, 2022

Introduce and enable transactions support for db. #428

Closed

elsirion mentioned this pull request Sep 16, 2022

Retain Payment Information in API DB (Payment History) #573

Closed

jkitman mentioned this pull request Sep 16, 2022

test RocksDB in the integration tests #582

Merged

dpc closed this Sep 16, 2022

dpc reopened this Sep 16, 2022

m1sterc001guy force-pushed the basictxs2 branch from 96d52bc to afe9755 Compare September 30, 2022 20:59

m1sterc001guy changed the title ~~[Draft] Example implementation of Database Transactions~~ Database Transactions Sep 30, 2022

m1sterc001guy marked this pull request as ready for review September 30, 2022 21:01

dpc reviewed Oct 1, 2022

View reviewed changes

dpc mentioned this pull request Oct 3, 2022

Database replaces an element #726

Closed

dpc force-pushed the basictxs2 branch from e5b532a to 769abf0 Compare October 3, 2022 18:56

dpc and others added 4 commits October 3, 2022 11:58

Improve dyn_newtype_define macro

1266c83

* Add support for lifetimes * `Box<...>` does not have to be `Sync` (that's kind of a whole purspose of using it over `Arc`, I guess)

Add database transactions

3936169

Fix lints

8d54489

Add "dyn newtype" for DatabaseTransaction

81beea6

dpc force-pushed the basictxs2 branch from 769abf0 to 81beea6 Compare October 3, 2022 19:00

dpc enabled auto-merge October 4, 2022 17:35

dpc approved these changes Oct 4, 2022

View reviewed changes

dpc merged commit e7a257d into fedimint:master Oct 4, 2022

okjodom reviewed Oct 4, 2022

View reviewed changes

m1sterc001guy mentioned this pull request Oct 4, 2022

Re-enable lightning_gateway_pays_internal_invoice #743

Merged

m1sterc001guy deleted the basictxs2 branch January 4, 2023 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Database Transactions #581

Database Transactions #581

m1sterc001guy commented Sep 15, 2022

jkitman commented Sep 15, 2022

m1sterc001guy commented Sep 15, 2022 •

edited

Loading

dpc Sep 15, 2022 •

edited

Loading

m1sterc001guy commented Sep 16, 2022

dpc commented Sep 16, 2022

dpc commented Sep 16, 2022

m1sterc001guy commented Sep 30, 2022

dpc Oct 1, 2022 •

edited

Loading

m1sterc001guy Oct 3, 2022

dpc commented Oct 3, 2022

codecov-commenter commented Oct 3, 2022 •

edited

Loading

okjodom Oct 4, 2022

okjodom Oct 4, 2022

m1sterc001guy Oct 4, 2022

m1sterc001guy Oct 4, 2022

Database Transactions #581

Database Transactions #581

Conversation

m1sterc001guy commented Sep 15, 2022

jkitman commented Sep 15, 2022

m1sterc001guy commented Sep 15, 2022 • edited Loading

dpc Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

m1sterc001guy commented Sep 16, 2022

dpc commented Sep 16, 2022

dpc commented Sep 16, 2022

m1sterc001guy commented Sep 30, 2022

dpc Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

m1sterc001guy Oct 3, 2022

Choose a reason for hiding this comment

dpc commented Oct 3, 2022

codecov-commenter commented Oct 3, 2022 • edited Loading

Codecov Report

okjodom Oct 4, 2022

Choose a reason for hiding this comment

okjodom Oct 4, 2022

Choose a reason for hiding this comment

m1sterc001guy Oct 4, 2022

Choose a reason for hiding this comment

m1sterc001guy Oct 4, 2022

Choose a reason for hiding this comment

m1sterc001guy commented Sep 15, 2022 •

edited

Loading

dpc Sep 15, 2022 •

edited

Loading

dpc Oct 1, 2022 •

edited

Loading

codecov-commenter commented Oct 3, 2022 •

edited

Loading