Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions src/Makefile.test.include
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ BITCOIN_TESTS =\
test/txvalidation_tests.cpp \
test/txvalidationcache_tests.cpp \
test/uint256_tests.cpp \
test/unordered_lru_cache_tests.cpp \
test/util_tests.cpp \
test/validation_block_tests.cpp \
test/validation_chainstate_tests.cpp \
Expand Down
4 changes: 3 additions & 1 deletion src/evo/cbtx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ auto CachedGetQcHashesQcIndexedHashes(const CBlockIndex* pindexPrev, const llmq:
uint256 block_hash{blockIndex->GetBlockHash()};

std::pair<uint256, int> qc_hash;
if (!qc_hashes_cached[llmqType].get(block_hash, qc_hash)) {
if (auto cached = qc_hashes_cached[llmqType].get(block_hash)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this particular case is not good anymore; here's 1 extra constructor of uint256 is called now.

once - default constructor.
After that one more in get()
and 3rd one is 'qc_hash=*cached'

Used to be only 2 constructors:

  1. default for qc_hash
  2. assignment in get()

std::optional supposed to decrease its amount, not increase!

Consider use std::optional's feature value_or

qc_hash = *cached;
} else {
auto [pqc, dummy_hash] = quorum_block_processor.GetMinedCommitment(llmqType, block_hash);
if (dummy_hash == uint256::ZERO) {
// this should never happen
Expand Down
8 changes: 4 additions & 4 deletions src/evo/creditpool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ static std::optional<CreditPoolDataPerBlock> GetCreditDataFromBlock(const gsl::n
static Mutex cache_mutex;
static Uint256LruHashMap<CreditPoolDataPerBlock> block_data_cache GUARDED_BY(cache_mutex){
static_cast<size_t>(Params().CreditPoolPeriodBlocks()) * 2};
if (LOCK(cache_mutex); block_data_cache.get(block_index->GetBlockHash(), blockData)) {
return blockData;
if (LOCK(cache_mutex); auto cached = block_data_cache.get(block_index->GetBlockHash())) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit(perf): should reduce scope of blockData here to avoid empty object creation

return *cached;
}

CBlock block;
Expand Down Expand Up @@ -123,8 +123,8 @@ std::optional<CCreditPool> CCreditPoolManager::GetFromCache(const CBlockIndex& b
CCreditPool pool;
{
LOCK(cache_mutex);
if (creditPoolCache.get(block_hash, pool)) {
return pool;
if (auto cached = creditPoolCache.get(block_hash)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should reduce scope of pool here

return *cached;
}
}
if (block_index.nHeight % DISK_SNAPSHOT_PERIOD == 0) {
Expand Down
4 changes: 2 additions & 2 deletions src/evo/mnhftx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,8 @@ std::optional<CMNHFManager::Signals> CMNHFManager::GetFromCache(const CBlockInde
const uint256& blockHash = pindex->GetBlockHash();
{
LOCK(cs_cache);
if (mnhfCache.get(blockHash, signals)) {
return signals;
if (auto cached = mnhfCache.get(blockHash)) {
return *cached;
}
}
{
Expand Down
31 changes: 18 additions & 13 deletions src/instantsend/db.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -271,11 +271,14 @@ InstantSendLockPtr CInstantSendDb::GetInstantSendLockByHashInternal(const uint25
return nullptr;
}

InstantSendLockPtr ret;
if (use_cache && islockCache.get(hash, ret)) {
return ret;
if (use_cache) {
if (auto cached = islockCache.get(hash)) {
return *cached;
}
}

InstantSendLockPtr ret;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

-    InstantSendLockPtr ret;
-    ret = std::make_shared<InstantSendLock>();
+     InstantSendLockPtr ret{std::make_shared<InstantSendLock>()};


ret = std::make_shared<InstantSendLock>();
bool exists = db->Read(std::make_tuple(DB_ISLOCK_BY_HASH, hash), *ret);
if (!exists || (::SerializeHash(*ret) != hash)) {
Expand All @@ -292,13 +295,14 @@ InstantSendLockPtr CInstantSendDb::GetInstantSendLockByHashInternal(const uint25
uint256 CInstantSendDb::GetInstantSendLockHashByTxidInternal(const uint256& txid) const
{
AssertLockHeld(cs_db);
if (auto cached = txidCache.get(txid)) {
return *cached;
}
uint256 islockHash;
if (!txidCache.get(txid, islockHash)) {
if (!db->Read(std::make_tuple(DB_HASH_BY_TXID, txid), islockHash)) {
return {};
}
txidCache.insert(txid, islockHash);
if (!db->Read(std::make_tuple(DB_HASH_BY_TXID, txid), islockHash)) {
return {};
}
txidCache.insert(txid, islockHash);
return islockHash;
}

Expand All @@ -311,13 +315,14 @@ InstantSendLockPtr CInstantSendDb::GetInstantSendLockByTxid(const uint256& txid)
InstantSendLockPtr CInstantSendDb::GetInstantSendLockByInput(const COutPoint& outpoint) const
{
LOCK(cs_db);
if (auto cached = outpointCache.get(outpoint)) {
return GetInstantSendLockByHashInternal(*cached);
}
uint256 islockHash;
if (!outpointCache.get(outpoint, islockHash)) {
if (!db->Read(std::make_tuple(DB_HASH_BY_OUTPOINT, outpoint), islockHash)) {
return nullptr;
}
outpointCache.insert(outpoint, islockHash);
if (!db->Read(std::make_tuple(DB_HASH_BY_OUTPOINT, outpoint), islockHash)) {
return nullptr;
}
outpointCache.insert(outpoint, islockHash);
return GetInstantSendLockByHashInternal(islockHash);
}

Expand Down
7 changes: 4 additions & 3 deletions src/llmq/blockprocessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -513,11 +513,12 @@ uint256 CQuorumBlockProcessor::GetQuorumBlockHash(const Consensus::LLMQParams& l

bool CQuorumBlockProcessor::HasMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash) const
{
bool fExists;
if (LOCK(minableCommitmentsCs); mapHasMinedCommitmentCache[llmqType].get(quorumHash, fExists)) {
return fExists;
if (LOCK(minableCommitmentsCs); auto cached = mapHasMinedCommitmentCache[llmqType].get(quorumHash)) {
return *cached;
}

bool fExists;

fExists = m_evoDb.Exists(std::make_pair(DB_MINED_COMMITMENT, std::make_pair(llmqType, quorumHash)));
Comment on lines +520 to 522

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💬 Nitpick: Combine split declaration and assignment of fExists

The declaration bool fExists; on line 520 is followed by a blank line and then assigned on line 522. This split was meaningful when fExists was an out-parameter to the old get() API, but after the refactor it can be a single initialized declaration, matching the pattern used in other call sites updated by this PR (e.g. signing.cpp).

💡 Suggested change
Suggested change
bool fExists;
fExists = m_evoDb.Exists(std::make_pair(DB_MINED_COMMITMENT, std::make_pair(llmqType, quorumHash)));
bool fExists = m_evoDb.Exists(std::make_pair(DB_MINED_COMMITMENT, std::make_pair(llmqType, quorumHash)));

source: ['claude']


LOCK(minableCommitmentsCs);
Expand Down
4 changes: 3 additions & 1 deletion src/llmq/dkgsessionmgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,9 @@ MessageProcessingResult CDKGSessionManager::ProcessMessage(CNode& pfrom, bool is
if (indexedQuorumsCache.empty()) {
utils::InitQuorumsCache(indexedQuorumsCache);
}
indexedQuorumsCache[llmqType].get(quorumHash, quorumIndex);
if (auto cached = indexedQuorumsCache[llmqType].get(quorumHash)) {
quorumIndex = *cached;
}
}

// No luck, try to compute
Expand Down
24 changes: 14 additions & 10 deletions src/llmq/quorums.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -571,8 +571,8 @@ std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp
}
}
auto& cache = scanQuorumsCache[llmqType];
bool fCacheExists = cache.get(pindexStore->GetBlockHash(), vecResultQuorums);
if (fCacheExists) {
if (auto cached = cache.get(pindexStore->GetBlockHash())) {
vecResultQuorums = *cached;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

consider using std::optional's feature value_or

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

why? / how?

@knst knst Nov 12, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

indeed, this place is not feasible; not easy to do due to if (...) and extra code inside

Consider vecResultQuorums = std::move(*cached)

@coderabbitai, check that std::move is working with std::optional's values?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

uint256 is trivially copyable; there's no gain to move really

// We have exactly what requested so just return it
if (vecResultQuorums.size() == nCountRequested) {
return vecResultQuorums;
Expand Down Expand Up @@ -642,12 +642,15 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint25
// We cannot hold cs_quorumBaseBlockIndexCache the whole time as that creates lock-order inversion with cs_main;
// We cannot acquire cs_main if we have cs_quorumBaseBlockIndexCache held
const CBlockIndex* pindex;
if (!WITH_LOCK(cs_quorumBaseBlockIndexCache, return quorumBaseBlockIndexCache.get(quorumHash, pindex))) {
auto cached = WITH_LOCK(cs_quorumBaseBlockIndexCache, return quorumBaseBlockIndexCache.get(quorumHash));
if (!cached.has_value()) {
pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(quorumHash));
if (pindex) {
LOCK(cs_quorumBaseBlockIndexCache);
quorumBaseBlockIndexCache.insert(quorumHash, pindex);
}
} else {
pindex = *cached;
}
return pindex;
}();
Expand All @@ -668,9 +671,8 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, gsl::not_nul
return nullptr;
}

CQuorumPtr pQuorum;
if (LOCK(cs_map_quorums); mapQuorumsCache[llmqType].get(quorumHash, pQuorum)) {
return pQuorum;
if (LOCK(cs_map_quorums); auto cached = mapQuorumsCache[llmqType].get(quorumHash)) {
return *cached;
}

return BuildQuorumFromCommitment(llmqType, pQuorumBaseBlockIndex, populate_cache);
Expand Down Expand Up @@ -833,9 +835,12 @@ MessageProcessingResult CQuorumManager::ProcessMessage(CNode& pfrom, CConnman& c

CQuorumPtr pQuorum;
{
if (LOCK(cs_map_quorums); !mapQuorumsCache[request.GetLLMQType()].get(request.GetQuorumHash(), pQuorum)) {
LOCK(cs_map_quorums);
auto cached = mapQuorumsCache[request.GetLLMQType()].get(request.GetQuorumHash());
if (!cached.has_value()) {
return errorHandler("Quorum not found", 0); // Don't bump score because we asked for it
}
pQuorum = *cached;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

use std::optional's feature value_or

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

why / how?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why?

I gave a 2nd look, it just shared_ptr; so, copy is quite fast, no allocation, one atomic is relatively cheap.

Though, it could be:

-        CQuorumPtr pQuorum;
-        {
-            LOCK(cs_map_quorums);
-            auto cached = mapQuorumsCache[request.GetLLMQType()].get(request.GetQuorumHash());
-            if (!cached.has_value()) {
-                return errorHandler("Quorum not found", 0); // Don't bump score because we asked for it
-            }
-            pQuorum = *cached;
+        CQuorumPtr pQuorum = WITH_LOCK(cs_map_quorums, return mapQuorumsCache[request.GetLLMQType()].get(request.GetQuorumHash())).value_or(nullptr);
+        if (pQuorum == nullptr) {
+            return errorHandler("Quorum not found", 0); // Don't bump score because we asked for it
         }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I believe compiler may optimize away this copy anyway as the obj was not initialized prior.

}

// Check if request has QUORUM_VERIFICATION_VECTOR data
Expand Down Expand Up @@ -1118,9 +1123,8 @@ void CQuorumManager::StartCleanupOldQuorumDataThread(const CBlockIndex* pIndex)
const CBlockIndex* pindex_loop{pIndex};
std::set<uint256> quorum_keys;
while (pindex_loop != nullptr && pIndex->nHeight - pindex_loop->nHeight < params.max_store_depth()) {
uint256 quorum_key;
if (cache.get(pindex_loop->GetBlockHash(), quorum_key)) {
quorum_keys.insert(quorum_key);
if (auto quorum_key = cache.get(pindex_loop->GetBlockHash())) {
quorum_keys.insert(*quorum_key);
if (quorum_keys.size() >= static_cast<size_t>(params.keepOldKeys)) break; // extra safety belt
}
pindex_loop = pindex_loop->pprev;
Expand Down
21 changes: 9 additions & 12 deletions src/llmq/signing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,16 @@ bool CRecoveredSigsDb::HasRecoveredSig(Consensus::LLMQType llmqType, const uint2
bool CRecoveredSigsDb::HasRecoveredSigForId(Consensus::LLMQType llmqType, const uint256& id) const
{
auto cacheKey = std::make_pair(llmqType, id);
bool ret;
{
LOCK(cs_cache);
if (hasSigForIdCache.get(cacheKey, ret)) {
return ret;
if (auto cached = hasSigForIdCache.get(cacheKey)) {
return *cached;
}
}


auto k = std::make_tuple(std::string("rs_r"), llmqType, id);
ret = db->Exists(k);
bool ret = db->Exists(k);

LOCK(cs_cache);
hasSigForIdCache.insert(cacheKey, ret);
Expand All @@ -62,16 +61,15 @@ bool CRecoveredSigsDb::HasRecoveredSigForId(Consensus::LLMQType llmqType, const

bool CRecoveredSigsDb::HasRecoveredSigForSession(const uint256& signHash) const
{
bool ret;
{
LOCK(cs_cache);
if (hasSigForSessionCache.get(signHash, ret)) {
return ret;
if (auto cached = hasSigForSessionCache.get(signHash)) {
return *cached;
}
}

auto k = std::make_tuple(std::string("rs_s"), signHash);
ret = db->Exists(k);
bool ret = db->Exists(k);

LOCK(cs_cache);
hasSigForSessionCache.insert(signHash, ret);
Expand All @@ -80,16 +78,15 @@ bool CRecoveredSigsDb::HasRecoveredSigForSession(const uint256& signHash) const

bool CRecoveredSigsDb::HasRecoveredSigForHash(const uint256& hash) const
{
bool ret;
{
LOCK(cs_cache);
if (hasSigForHashCache.get(hash, ret)) {
return ret;
if (auto cached = hasSigForHashCache.get(hash)) {
return *cached;
}
}

auto k = std::make_tuple(std::string("rs_h"), hash);
ret = db->Exists(k);
bool ret = db->Exists(k);

LOCK(cs_cache);
hasSigForHashCache.insert(hash, ret);
Expand Down
4 changes: 2 additions & 2 deletions src/llmq/snapshot.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -352,8 +352,8 @@ std::optional<CQuorumSnapshot> CQuorumSnapshotManager::GetSnapshotForBlock(const

LOCK(snapshotCacheCs);
// try using cache before reading from disk
if (quorumSnapshotCache.get(snapshotHash, snapshot)) {
return snapshot;
if (auto cached = quorumSnapshotCache.get(snapshotHash)) {
return *cached;
}
if (m_evoDb.Read(std::make_pair(DB_QUORUM_SNAPSHOT, snapshotHash), snapshot)) {
quorumSnapshotCache.insert(snapshotHash, snapshot);
Expand Down
7 changes: 4 additions & 3 deletions src/llmq/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,8 @@ std::vector<CDeterministicMNCPtr> GetAllQuorumMembers(Consensus::LLMQType llmqTy
}
if (reset_cache) {
mapQuorumMembers[llmqType].clear();
} else if (mapQuorumMembers[llmqType].get(pQuorumBaseBlockIndex->GetBlockHash(), quorumMembers)) {
return quorumMembers;
} else if (auto cached = mapQuorumMembers[llmqType].get(pQuorumBaseBlockIndex->GetBlockHash())) {
return *cached;
}
}

Expand Down Expand Up @@ -251,7 +251,8 @@ std::vector<CDeterministicMNCPtr> GetAllQuorumMembers(Consensus::LLMQType llmqTy
if (reset_cache) {
LOCK(cs_indexed_members);
mapIndexedQuorumMembers[llmqType].clear();
} else if (LOCK(cs_indexed_members); mapIndexedQuorumMembers[llmqType].get(std::pair(pCycleQuorumBaseBlockIndex->GetBlockHash(), quorumIndex), quorumMembers)) {
} else if (LOCK(cs_indexed_members); auto cached = mapIndexedQuorumMembers[llmqType].get(std::pair(pCycleQuorumBaseBlockIndex->GetBlockHash(), quorumIndex))) {
quorumMembers = *cached;
LOCK(cs_members);
mapQuorumMembers[llmqType].insert(pQuorumBaseBlockIndex->GetBlockHash(), quorumMembers);
return quorumMembers;
Expand Down
8 changes: 3 additions & 5 deletions src/masternode/meta.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,10 @@ bool CMasternodeMetaMan::AlreadyHavePlatformBan(const uint256& inv_hash) const
std::optional<PlatformBanMessage> CMasternodeMetaMan::GetPlatformBan(const uint256& inv_hash) const
{
LOCK(cs);
PlatformBanMessage ret;
if (!m_seen_platform_bans.get(inv_hash, ret)) {
return std::nullopt;
if (auto cached = m_seen_platform_bans.get(inv_hash)) {
return *cached;
}

return ret;
return std::nullopt;
}

void CMasternodeMetaMan::RememberPlatformBan(const uint256& inv_hash, PlatformBanMessage&& msg)
Expand Down
Loading
Loading