From cf4d72a75e9603e947b3d47e1f3ac7c7f37bb401 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 8 Feb 2024 12:34:26 -0300 Subject: [PATCH 1/3] wallet: db, introduce 'RunWithinTxn()' helper function 'RunWithinTxn()' provides a way to execute db operations within a transactional context. It avoids writing repetitive boilerplate code for starting and committing the database transaction. --- src/wallet/wallet.cpp | 5 +++-- src/wallet/walletdb.cpp | 24 ++++++++++++++++++++++++ src/wallet/walletdb.h | 14 ++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 11203ca3a40..db5f54cbb74 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2411,8 +2411,9 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s bool CWallet::DelAddressBook(const CTxDestination& address) { - WalletBatch batch(GetDatabase()); - return DelAddressBookWithDB(batch, address); + return RunWithinTxn(GetDatabase(), /*process_desc=*/"address book entry removal", [&](WalletBatch& batch){ + return DelAddressBookWithDB(batch, address); + }); } bool CWallet::DelAddressBookWithDB(WalletBatch& batch, const CTxDestination& address) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 6e37bc2930e..c66b09628eb 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1230,6 +1230,30 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) return result; } +bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const std::function& func) +{ + WalletBatch batch(database); + if (!batch.TxnBegin()) { + LogPrint(BCLog::WALLETDB, "Error: cannot create db txn for %s\n", process_desc); + return false; + } + + // Run procedure + if (!func(batch)) { + LogPrint(BCLog::WALLETDB, "Error: %s failed\n", process_desc); + batch.TxnAbort(); + return false; + } + + if (!batch.TxnCommit()) { + LogPrint(BCLog::WALLETDB, "Error: cannot commit db txn for %s\n", process_desc); + return false; + } + + // All good + return true; +} + void MaybeCompactWalletDB(WalletContext& context) { static std::atomic fOneThread(false); diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 62449eb64ec..9474a59660f 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -294,6 +294,20 @@ private: WalletDatabase& m_database; }; +/** + * Executes the provided function 'func' within a database transaction context. + * + * This function ensures that all db modifications performed within 'func()' are + * atomically committed to the db at the end of the process. And, in case of a + * failure during execution, all performed changes are rolled back. + * + * @param database The db connection instance to perform the transaction on. + * @param process_desc A description of the process being executed, used for logging purposes in the event of a failure. + * @param func The function to be executed within the db txn context. It returns a boolean indicating whether to commit or roll back the txn. + * @return true if the db txn executed successfully, false otherwise. + */ +bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const std::function& func); + //! Compacts BDB state so that wallet.dat is self-contained (if there are changes) void MaybeCompactWalletDB(WalletContext& context); From 33757814ceb04102141d3fd5ef2f87abf0422310 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 6 Feb 2024 16:28:23 -0300 Subject: [PATCH 2/3] wallet: bdb batch 'ErasePrefix', do not create txn internally Transactions are intended to be started on upper layers rather than internally by the bdb batch object. This enables us to consolidate different write operations within a procedure in the same db txn, improving consistency due to the atomic property of the transaction, as well as its performance due to the reduction of disk write operations. Important Note: This approach also ensures that the BerkeleyBatch::ErasePrefix function behaves exactly as the SQLiteBatch::ErasePrefix function, which does not create a db txn internally. Furthermore, since the `BerkeleyBatch::ErasePrefix' implementation erases records one by one (by traversing the db), this change ensures that the function is always called within an active txn context. Without this measure, there's a potential risk to consistency; certain records may be removed while others could persist due to an internal failure during the procedure. --- src/wallet/bdb.cpp | 9 +++++++-- src/wallet/test/db_tests.cpp | 33 ++++++++++++++++++++++++++++++++ src/wallet/test/wallet_tests.cpp | 8 +++++--- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index cbf6c9b1eaa..38cca32f80a 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -887,7 +887,12 @@ bool BerkeleyBatch::HasKey(DataStream&& key) bool BerkeleyBatch::ErasePrefix(Span prefix) { - if (!TxnBegin()) return false; + // Because this function erases records one by one, ensure that it is executed within a txn context. + // Otherwise, consistency is at risk; it's possible that certain records are removed while others + // remain due to an internal failure during the procedure. + // Additionally, the Dbc::del() cursor delete call below would fail without an active transaction. + if (!Assume(activeTxn)) return false; + auto cursor{std::make_unique(m_database, *this)}; // const_cast is safe below even though prefix_key is an in/out parameter, // because we are not using the DB_DBT_USERMEM flag, so BDB will allocate @@ -901,7 +906,7 @@ bool BerkeleyBatch::ErasePrefix(Span prefix) ret = cursor->dbc()->del(0); } cursor.reset(); - return TxnCommit() && (ret == 0 || ret == DB_NOTFOUND); + return ret == 0 || ret == DB_NOTFOUND; } void BerkeleyDatabase::AddRef() diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index adbbb943185..c933366354d 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -228,6 +228,39 @@ BOOST_AUTO_TEST_CASE(db_availability_after_write_error) } } +// Verify 'ErasePrefix' functionality using db keys similar to the ones used by the wallet. +// Keys are in the form of std::pair +BOOST_AUTO_TEST_CASE(erase_prefix) +{ + const std::string key = "key"; + const std::string key2 = "key2"; + const std::string value = "value"; + const std::string value2 = "value_2"; + auto make_key = [](std::string type, std::string id) { return std::make_pair(type, id); }; + + for (const auto& database : TestDatabases(m_path_root)) { + std::unique_ptr batch = database->MakeBatch(); + + // Write two entries with the same key type prefix, a third one with a different prefix + // and a fourth one with the type-id values inverted + BOOST_CHECK(batch->Write(make_key(key, value), value)); + BOOST_CHECK(batch->Write(make_key(key, value2), value2)); + BOOST_CHECK(batch->Write(make_key(key2, value), value)); + BOOST_CHECK(batch->Write(make_key(value, key), value)); + + // Erase the ones with the same prefix and verify result + BOOST_CHECK(batch->TxnBegin()); + BOOST_CHECK(batch->ErasePrefix(DataStream() << key)); + BOOST_CHECK(batch->TxnCommit()); + + BOOST_CHECK(!batch->Exists(make_key(key, value))); + BOOST_CHECK(!batch->Exists(make_key(key, value2))); + // Also verify that entries with a different prefix were not erased + BOOST_CHECK(batch->Exists(make_key(key2, value))); + BOOST_CHECK(batch->Exists(make_key(value, key))); + } +} + #ifdef USE_SQLITE // Test-only statement execution error diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 7396a43c508..27a81b36691 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -445,9 +445,11 @@ BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup) auto requests = wallet->GetAddressReceiveRequests(); auto erequests = {"val_rr11", "val_rr20"}; BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests)); - WalletBatch batch{wallet->GetDatabase()}; - BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false)); - BOOST_CHECK(batch.EraseAddressData(ScriptHash())); + RunWithinTxn(wallet->GetDatabase(), /*process_desc*/"test", [](WalletBatch& batch){ + BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false)); + BOOST_CHECK(batch.EraseAddressData(ScriptHash())); + return true; + }); }); TestLoadWallet(name, format, [](std::shared_ptr wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) { BOOST_CHECK(!wallet->IsAddressPreviouslySpent(PKHash())); From 77331aa2a198b708372a5c6cdf331faf7e2968ef Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 6 Feb 2024 10:25:48 -0300 Subject: [PATCH 3/3] wallet: simplify EraseRecords by using 'ErasePrefix' --- src/wallet/walletdb.cpp | 55 +++++++++-------------------------------- 1 file changed, 12 insertions(+), 43 deletions(-) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index c66b09628eb..45c3e902201 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1230,9 +1230,8 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) return result; } -bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const std::function& func) +static bool RunWithinTxn(WalletBatch& batch, std::string_view process_desc, const std::function& func) { - WalletBatch batch(database); if (!batch.TxnBegin()) { LogPrint(BCLog::WALLETDB, "Error: cannot create db txn for %s\n", process_desc); return false; @@ -1254,6 +1253,12 @@ bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const return true; } +bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const std::function& func) +{ + WalletBatch batch(database); + return RunWithinTxn(batch, process_desc, func); +} + void MaybeCompactWalletDB(WalletContext& context) { static std::atomic fOneThread(false); @@ -1316,47 +1321,11 @@ bool WalletBatch::WriteWalletFlags(const uint64_t flags) bool WalletBatch::EraseRecords(const std::unordered_set& types) { - // Begin db txn - if (!m_batch->TxnBegin()) return false; - - // Get cursor - std::unique_ptr cursor = m_batch->GetNewCursor(); - if (!cursor) - { - return false; - } - - // Iterate the DB and look for any records that have the type prefixes - while (true) { - // Read next record - DataStream key{}; - DataStream value{}; - DatabaseCursor::Status status = cursor->Next(key, value); - if (status == DatabaseCursor::Status::DONE) { - break; - } else if (status == DatabaseCursor::Status::FAIL) { - cursor.reset(nullptr); - m_batch->TxnAbort(); // abort db txn - return false; - } - - // Make a copy of key to avoid data being deleted by the following read of the type - const SerializeData key_data{key.begin(), key.end()}; - - std::string type; - key >> type; - - if (types.count(type) > 0) { - if (!m_batch->Erase(Span{key_data})) { - cursor.reset(nullptr); - m_batch->TxnAbort(); - return false; // erase failed - } - } - } - // Finish db txn - cursor.reset(nullptr); - return m_batch->TxnCommit(); + return RunWithinTxn(*this, "erase records", [&types](WalletBatch& self) { + return std::all_of(types.begin(), types.end(), [&self](const std::string& type) { + return self.m_batch->ErasePrefix(DataStream() << type); + }); + }); } bool WalletBatch::TxnBegin()