Basically, with bdb, we can’t make a write operation while we are traversing the db with the same db handler. These two operations are performed in different txn contexts and cause a deadlock.
Added coverage by using EraseRecords() which is the simplest function that executes this process.
To replicate it, need bdb support and drop the first commit. The test will run forever.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Didn’t look closer at the changes yet, but can confirm that running the newly introduced unit test via
0$ ./src/test/test_bitcoin --log_level=all --run_test=walletdb_tests/walletdb_read_write_deadlock
succeeds on the PR branch head and hangs if the bugfix commit bda562ddba13ee8f94147734ab9901ab07357f2f is reverted.
712@@ -713,7 +713,7 @@ BerkeleyCursor::~BerkeleyCursor()
713 std::unique_ptr<DatabaseCursor> BerkeleyBatch::GetNewCursor()
714 {
715 if (!pdb) return nullptr;
716- return std::make_unique<BerkeleyCursor>(m_database);
717+ return std::make_unique<BerkeleyCursor>(m_database, this);
batch argument required to avoid this kind of issue in the future.
Updated per feedback. Thanks achow101.
Only diff is BerkeleyCursor requiring a WalletBatch ref instead of a pointer.
673 {
674 if (!database.m_db.get()) {
675 throw std::runtime_error(STR_INTERNAL_BUG("BerkeleyDatabase does not exist"));
676 }
677 // Transaction argument to cursor is only needed when using the cursor to
678 // write to the database. Read-only cursors do not need a txn pointer.
batch.txn() returns a nullptr.
If you mix an already open read-only cursor with some write operations, then the code is no longer performing a read-only operation, so it will need to provide the transaction arg (probably by re-arranging the code in the same way as I did with EraseRecords).
1155 DataStream value{};
1156 DatabaseCursor::Status status = cursor->Next(key, value);
1157 if (status == DatabaseCursor::Status::DONE) {
1158 break;
1159 } else if (status == DatabaseCursor::Status::FAIL) {
1160+ cursor.reset(nullptr);
reset a std::unique_ptr instance explicitly instead of using its RAII capabilities.
Yeah, I explained the reason inside the commit description ca1871d48558b356257d2a95b046f63739dfe3a0:
extra note from the Db.cursor doc: “If transaction protection is enabled, cursors must be opened and closed within the context of a transaction”
thus why added a
CloseCursorcall before calling toTxnAbort/TxnCommit.
The CloseCursor call is the ptr reset.
Just extra safety measures.
ACK de202f860b3f79774d5235dec59a140fa5c82fa7, tested on Ubutnu 23.04.
To replicate it, need bdb support and drop the first commit. The test will run forever.
The test is deadlocked with the second commit dropped as well.
If the batch ptr is not passed, the cursor will not use the db active
txn context which could lead to a deadlock if the code tries to modify
the db while it is traversing it.
E.g. the 'EraseRecords()' function.
so we erase all the records atomically or abort the entire
procedure.
and, at the same time, we can share the same db txn context
for the db cursor and the erase functionality.
extra note from the Db.cursor doc:
"If transaction protection is enabled, cursors must be
opened and closed within the context of a transaction"
thus why added a `CloseCursor` call before calling to
`TxnAbort/TxnCommit`.