Skip to content

refactor: replace exclusive locks with shared locks in CInstantSendDb #6963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

refactor: replace exclusive locks with shared locks in CInstantSendDb #6963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/instantsend/db.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,14 +236,14 @@ void CInstantSendDb::RemoveBlockInstantSendLocks(const gsl::not_null<std::shared

bool CInstantSendDb::KnownInstantSendLock(const uint256& islockHash) const
{
LOCK(cs_db);
READ_LOCK(cs_db);
return GetInstantSendLockByHashInternal(islockHash) != nullptr ||
db->Exists(std::make_tuple(DB_ARCHIVED_BY_HASH, islockHash));
}

size_t CInstantSendDb::GetInstantSendLockCount() const
{
LOCK(cs_db);
READ_LOCK(cs_db);
Copy link
Collaborator

@knst knst Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is internal db object (leveldb::DB* pdb;) ready for parallel reads?

Copy link
Member Author

@PastaPastaPasta PastaPastaPasta Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

levelDB appears to say it is internally safe: https://github.com/google/leveldb/blob/main/doc/index.md#concurrency

Copy link
Member Author

@PastaPastaPasta PastaPastaPasta Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gpt agrees

'''
Looked at dbwrapper.h to confirm how CDBWrapper uses LevelDB (leveldb::DB* pdb).
Yes: leveldb::DB supports concurrent reads. Multiple threads can call DB::Get and create/use independent iterators concurrently without external locking.
Don’t share iterators across threads. Each thread must use its own leveldb::Iterator.
Reads and writes may run concurrently; writes are serialized internally. For a consistent view across keys during concurrent writes, use snapshots (ReadOptions::snapshot).
In our wrapper, pdb->Get(...), pdb->NewIterator(iteroptions), and pdb->GetApproximateSizes(...) map directly to LevelDB calls and are safe for parallel reads. The shared readoptions/iteroptions structs are read-only.
Caveat: while LevelDB reads are thread-safe, our higher-level caches (e.g., unordered_lru_cache, Uint256LruHashMap) are not thread-safe on their own. They still need protection by cs_db even during read paths that update the caches.
'''

Copy link
Collaborator

@knst knst Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I am reviewing instantsent/db just in case if we can drop somewhere cs_db completely.

It seems as not easy due to islockCache which is used almost everywhere.

Feasible candidates are:

  1. CInstantSendDb::RemoveInstantSendLock in case of keep_cache=true
  2. CInstantSendDb::WriteInstantSendLockMined

Also see #6964

auto it = std::unique_ptr<CDBIterator>(db->NewIterator());
auto firstKey = std::make_tuple(std::string{DB_ISLOCK_BY_HASH}, uint256());

Expand All @@ -266,7 +266,7 @@ size_t CInstantSendDb::GetInstantSendLockCount() const

InstantSendLockPtr CInstantSendDb::GetInstantSendLockByHashInternal(const uint256& hash, bool use_cache) const
{
AssertLockHeld(cs_db);
AssertSharedLockHeld(cs_db);
Copy link

@UdjinM6 UdjinM6 Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you actually do this? we update islockCache in this method

if (hash.IsNull()) {
return nullptr;
}
Expand All @@ -291,7 +291,7 @@ InstantSendLockPtr CInstantSendDb::GetInstantSendLockByHashInternal(const uint25

uint256 CInstantSendDb::GetInstantSendLockHashByTxidInternal(const uint256& txid) const
{
AssertLockHeld(cs_db);
AssertSharedLockHeld(cs_db);
Copy link

@UdjinM6 UdjinM6 Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same but for txidCache

uint256 islockHash;
if (!txidCache.get(txid, islockHash)) {
if (!db->Read(std::make_tuple(DB_HASH_BY_TXID, txid), islockHash)) {
Expand All @@ -304,13 +304,13 @@ uint256 CInstantSendDb::GetInstantSendLockHashByTxidInternal(const uint256& txid

InstantSendLockPtr CInstantSendDb::GetInstantSendLockByTxid(const uint256& txid) const
{
LOCK(cs_db);
READ_LOCK(cs_db);
return GetInstantSendLockByHashInternal(GetInstantSendLockHashByTxidInternal(txid));
}

InstantSendLockPtr CInstantSendDb::GetInstantSendLockByInput(const COutPoint& outpoint) const
{
LOCK(cs_db);
READ_LOCK(cs_db);
uint256 islockHash;
if (!outpointCache.get(outpoint, islockHash)) {
if (!db->Read(std::make_tuple(DB_HASH_BY_OUTPOINT, outpoint), islockHash)) {
Expand All @@ -323,7 +323,7 @@ InstantSendLockPtr CInstantSendDb::GetInstantSendLockByInput(const COutPoint& ou

std::vector<uint256> CInstantSendDb::GetInstantSendLocksByParent(const uint256& parent) const
{
AssertLockHeld(cs_db);
AssertSharedLockHeld(cs_db);
auto it = std::unique_ptr<CDBIterator>(db->NewIterator());
auto firstKey = std::make_tuple(std::string{DB_HASH_BY_OUTPOINT}, COutPoint(parent, 0));
it->Seek(firstKey);
Expand Down
24 changes: 12 additions & 12 deletions src/instantsend/db.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace instantsend {
class CInstantSendDb
{
private:
mutable Mutex cs_db;
mutable SharedMutex cs_db;

static constexpr int CURRENT_VERSION{1};

Expand Down Expand Up @@ -68,17 +68,17 @@ class CInstantSendDb
* @param parent The hash of the parent IS Lock
* @return Returns a vector of IS Lock hashes
*/
std::vector<uint256> GetInstantSendLocksByParent(const uint256& parent) const EXCLUSIVE_LOCKS_REQUIRED(cs_db);
std::vector<uint256> GetInstantSendLocksByParent(const uint256& parent) const SHARED_LOCKS_REQUIRED(cs_db);

/**
* See GetInstantSendLockByHash
*/
InstantSendLockPtr GetInstantSendLockByHashInternal(const uint256& hash, bool use_cache = true) const EXCLUSIVE_LOCKS_REQUIRED(cs_db);
InstantSendLockPtr GetInstantSendLockByHashInternal(const uint256& hash, bool use_cache = true) const SHARED_LOCKS_REQUIRED(cs_db);

/**
* See GetInstantSendLockHashByTxid
*/
uint256 GetInstantSendLockHashByTxidInternal(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_db);
uint256 GetInstantSendLockHashByTxidInternal(const uint256& txid) const SHARED_LOCKS_REQUIRED(cs_db);


void Upgrade(const util::DbWrapperParams& db_params) EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
Expand Down Expand Up @@ -112,45 +112,45 @@ class CInstantSendDb
void RemoveArchivedInstantSendLocks(int nUntilHeight) EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
void WriteBlockInstantSendLocks(const gsl::not_null<std::shared_ptr<const CBlock>>& pblock, gsl::not_null<const CBlockIndex*> pindexConnected) EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
void RemoveBlockInstantSendLocks(const gsl::not_null<std::shared_ptr<const CBlock>>& pblock, gsl::not_null<const CBlockIndex*> pindexDisconnected) EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
bool KnownInstantSendLock(const uint256& islockHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
bool KnownInstantSendLock(const uint256& islockHash) const LOCKS_EXCLUDED(cs_db);
Copy link

@UdjinM6 UdjinM6 Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this was changed? (same for other similar changes below)

/**
* Gets the number of IS Locks which have not been confirmed by a block
* @return size_t value of the number of IS Locks not confirmed by a block
*/
size_t GetInstantSendLockCount() const EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
size_t GetInstantSendLockCount() const LOCKS_EXCLUDED(cs_db);
/**
* Gets a pointer to the IS Lock based on the hash
* @param hash The hash of the IS Lock
* @param use_cache Should we try using the cache first or not
* @return A Pointer object to the IS Lock, returns nullptr if it doesn't exist
*/
InstantSendLockPtr GetInstantSendLockByHash(const uint256& hash, bool use_cache = true) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db)
InstantSendLockPtr GetInstantSendLockByHash(const uint256& hash, bool use_cache = true) const LOCKS_EXCLUDED(cs_db)
{
LOCK(cs_db);
READ_LOCK(cs_db);
return GetInstantSendLockByHashInternal(hash, use_cache);
};
/**
* Gets an IS Lock hash based on the txid the IS Lock is for
* @param txid The txid which is being searched for
* @return Returns the hash the IS Lock of the specified txid, returns uint256() if it doesn't exist
*/
uint256 GetInstantSendLockHashByTxid(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db)
uint256 GetInstantSendLockHashByTxid(const uint256& txid) const LOCKS_EXCLUDED(cs_db)
{
LOCK(cs_db);
READ_LOCK(cs_db);
return GetInstantSendLockHashByTxidInternal(txid);
};
/**
* Gets an IS Lock pointer from the txid given
* @param txid The txid for which the IS Lock Pointer is being returned
* @return Returns the IS Lock Pointer associated with the txid, returns nullptr if it doesn't exist
*/
InstantSendLockPtr GetInstantSendLockByTxid(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
InstantSendLockPtr GetInstantSendLockByTxid(const uint256& txid) const LOCKS_EXCLUDED(cs_db);
/**
* Gets an IS Lock pointer from an input given
* @param outpoint Since all inputs are really just outpoints that are being spent
* @return IS Lock Pointer associated with that input.
*/
InstantSendLockPtr GetInstantSendLockByInput(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
InstantSendLockPtr GetInstantSendLockByInput(const COutPoint& outpoint) const LOCKS_EXCLUDED(cs_db);
/**
* Called when a ChainLock invalidated a IS Lock, removes any chained/children IS Locks and the invalidated IS Lock
* @param islockHash IS Lock hash which has been invalidated
Expand Down
Loading