Skip to content

perf: cache block data for CreditPool for asset unlock limits calculation #6658

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

Merged
PastaPastaPasta merged 9 commits into from
Jun 18, 2025
Merged

perf: cache block data for CreditPool for asset unlock limits calculation #6658

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f7749db
refactor: use helper GetDataFromUnlockTxes to get credit pool amount
knst May 5, 2025
afd7f84
perf: use GetAncestor() to jump blocks back for CreditPool
knst May 5, 2025
e3d3783
refactor: combine GetDataFromUnlockTxes and GetBlockForCreditPool to …
knst May 5, 2025
1706270
perf: cache block data for credit pool for calculation asset unlock l…
knst May 5, 2025
34e06ec
fmt: apply clang-format suggestions
knst May 5, 2025
52ae3aa
refactor: use CreditPoolPeriodBlocks for block_data_cache
knst May 27, 2025
9c5e451
fix: typo in error message for GetDataFromUnlockTx
knst May 27, 2025
095b2d8
refactor: use if statement feature
knst Jun 2, 2025
72cfb93
feat: bail out if GetTxPayload failed
knst Jun 2, 2025
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
109 changes: 56 additions & 53 deletions src/evo/creditpool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#include <deploymentstatus.h>
#include <logging.h>
#include <node/blockstorage.h>
#include <util/irange.h>
#include <validation.h>

#include <algorithm>
Expand Down Expand Up @@ -47,29 +46,63 @@ static bool GetDataFromUnlockTx(const CTransaction& tx, CAmount& toUnlock, uint6
}

namespace {
struct UnlockDataPerBlock {
CAmount unlocked{0};
std::unordered_set<uint64_t> indexes;
};
struct CreditPoolDataPerBlock {
CAmount credit_pool{0};
CAmount unlocked{0};
std::unordered_set<uint64_t> indexes;
};
} // anonymous namespace

// it throws exception if anything went wrong
static UnlockDataPerBlock GetDataFromUnlockTxes(const std::vector<CTransactionRef>& vtx)
static std::optional<CreditPoolDataPerBlock> GetCreditDataFromBlock(const gsl::not_null<const CBlockIndex*> block_index,
const Consensus::Params& consensusParams)
{
UnlockDataPerBlock blockData;
// There's no CbTx before DIP0003 activation
if (!DeploymentActiveAt(*block_index, Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0003)) {
return std::nullopt;
}

CreditPoolDataPerBlock blockData;

static Mutex cache_mutex;
static unordered_lru_cache<uint256, CreditPoolDataPerBlock, StaticSaltedHasher> block_data_cache GUARDED_BY(
Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta Jun 2, 2025

Choose a reason for hiding this comment

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

missing include for unordered_lru_cache

Copy link
Copy Markdown
Collaborator Author

@knst knst Jun 2, 2025

Choose a reason for hiding this comment

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

This include is already presented in the header

dash/src/evo/creditpool.h

Line 14 in 9c5e451

#include <unordered_lru_cache.h>

Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta Jun 2, 2025

Choose a reason for hiding this comment

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

I believe IWYU (or the concept behind it), would be to include it in the cpp too, but nit

cache_mutex){static_cast<size_t>(Params().CreditPoolPeriodBlocks()) * 2};
if (LOCK(cache_mutex); block_data_cache.get(block_index->GetBlockHash(), blockData)) {
return blockData;
}

CBlock block;
if (!ReadBlockFromDisk(block, block_index, consensusParams)) {
throw std::runtime_error("failed-getcbforblock-read");
}

for (CTransactionRef tx : vtx) {
if (block.vtx.empty() || block.vtx[0]->vExtraPayload.empty() || !block.vtx[0]->IsSpecialTxVersion()) {
LogPrintf("%s: ERROR: empty CbTx for CreditPool at height=%d\n", __func__, block_index->nHeight);
return std::nullopt;
}


if (const auto opt_cbTx = GetTxPayload<CCbTx>(block.vtx[0]->vExtraPayload); opt_cbTx) {
blockData.credit_pool = opt_cbTx->creditPoolBalance;
} else {
LogPrintf("%s: WARNING: No valid CbTx at height=%d\n", __func__, block_index->nHeight);
return std::nullopt;
}
for (CTransactionRef tx : block.vtx) {
if (!tx->IsSpecialTxVersion() || tx->nType != TRANSACTION_ASSET_UNLOCK) continue;

CAmount unlocked{0};
TxValidationState tx_state;
uint64_t index{0};
if (!GetDataFromUnlockTx(*tx, unlocked, index, tx_state)) {
throw std::runtime_error(strprintf("%s: CCreditPoolManager::GetDataFromUnlockTxes failed: %s", __func__, tx_state.ToString()));
throw std::runtime_error(strprintf("%s: GetDataFromUnlockTx failed: %s", __func__, tx_state.ToString()));
}
blockData.unlocked += unlocked;
blockData.indexes.insert(index);
}

LOCK(cache_mutex);
block_data_cache.insert(block_index->GetBlockHash(), blockData);
return blockData;
}

Expand Down Expand Up @@ -112,32 +145,11 @@ void CCreditPoolManager::AddToCache(const uint256& block_hash, int height, const
}
}

static std::optional<CBlock> GetBlockForCreditPool(const gsl::not_null<const CBlockIndex*> block_index,
const Consensus::Params& consensusParams)
{
// There's no CbTx before DIP0003 activation
if (!DeploymentActiveAt(*block_index, Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0003)) {
return std::nullopt;
}

CBlock block;
if (!ReadBlockFromDisk(block, block_index, consensusParams)) {
throw std::runtime_error("failed-getcbforblock-read");
}

if (block.vtx.empty() || block.vtx[0]->vExtraPayload.empty() || !block.vtx[0]->IsSpecialTxVersion()) {
LogPrintf("%s: ERROR: empty CbTx for CreditPool at height=%d\n", __func__, block_index->nHeight);
return std::nullopt;
}

return block;
}

CCreditPool CCreditPoolManager::ConstructCreditPool(const gsl::not_null<const CBlockIndex*> block_index,
CCreditPool prev, const Consensus::Params& consensusParams)
{
std::optional<CBlock> block = GetBlockForCreditPool(block_index, consensusParams);
if (!block) {
std::optional<CreditPoolDataPerBlock> opt_block_data = GetCreditDataFromBlock(block_index, consensusParams);
if (!opt_block_data) {
// If reading of previous block is not successfully, but
// prev contains credit pool related data, something strange happened
if (prev.locked != 0) {
Expand All @@ -151,63 +163,54 @@ CCreditPool CCreditPoolManager::ConstructCreditPool(const gsl::not_null<const CB
AddToCache(block_index->GetBlockHash(), block_index->nHeight, emptyPool);
return emptyPool;
}
CAmount locked = [&, func=__func__]() {
const auto opt_cbTx = GetTxPayload<CCbTx>(block->vtx[0]->vExtraPayload);
if (!opt_cbTx) {
throw std::runtime_error(strprintf("%s: failed-getcreditpool-cbtx-payload", func));
}
return opt_cbTx->creditPoolBalance;
}();
const CreditPoolDataPerBlock& blockData{*opt_block_data};

// We use here sliding window with Params().CreditPoolPeriodBlocks to determine
// current limits for asset unlock transactions.
// Indexes should not be duplicated since genesis block, but the Unlock Amount
// of withdrawal transaction is limited only by this window
UnlockDataPerBlock blockData = GetDataFromUnlockTxes(block->vtx);
CRangesSet indexes{std::move(prev.indexes)};
if (std::any_of(blockData.indexes.begin(), blockData.indexes.end(), [&](const uint64_t index) { return !indexes.Add(index); })) {
throw std::runtime_error(strprintf("%s: failed-getcreditpool-index-duplicated", __func__));
}

const CBlockIndex* distant_block_index = block_index;
for ([[maybe_unused]] auto _ : irange::range(Params().CreditPoolPeriodBlocks())) {
distant_block_index = distant_block_index->pprev;
if (distant_block_index == nullptr) break;
}
const CBlockIndex* distant_block_index{
block_index->GetAncestor(block_index->nHeight - Params().CreditPoolPeriodBlocks())};
CAmount distantUnlocked{0};
if (distant_block_index) {
if (std::optional<CBlock> distant_block = GetBlockForCreditPool(distant_block_index, consensusParams); distant_block) {
distantUnlocked = GetDataFromUnlockTxes(distant_block->vtx).unlocked;
if (std::optional<CreditPoolDataPerBlock> distant_block{GetCreditDataFromBlock(distant_block_index, consensusParams)};
distant_block) {
distantUnlocked = distant_block->unlocked;
}
}
Comment on lines +177 to 185
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

GetAncestor call can underflow for early blocks

block_index->nHeight - Params().CreditPoolPeriodBlocks() will be
negative for the first N blocks. GetAncestor() asserts on heights
< 0 in several call-sites, which would crash the node.

-const CBlockIndex* distant_block_index{
-    block_index->GetAncestor(block_index->nHeight - Params().CreditPoolPeriodBlocks())};
+const int distant_height = block_index->nHeight - Params().CreditPoolPeriodBlocks();
+const CBlockIndex* distant_block_index =
+        distant_height >= 0 ? block_index->GetAncestor(distant_height) : nullptr;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const CBlockIndex* distant_block_index{
block_index->GetAncestor(block_index->nHeight - Params().CreditPoolPeriodBlocks())};
CAmount distantUnlocked{0};
if (distant_block_index) {
if (std::optional<CBlock> distant_block = GetBlockForCreditPool(distant_block_index, consensusParams); distant_block) {
distantUnlocked = GetDataFromUnlockTxes(distant_block->vtx).unlocked;
if (std::optional<CreditPoolDataPerBlock> distant_block{GetCreditDataFromBlock(distant_block_index, consensusParams)};
distant_block) {
distantUnlocked = distant_block->unlocked;
}
}
const int distant_height = block_index->nHeight - Params().CreditPoolPeriodBlocks();
const CBlockIndex* distant_block_index =
distant_height >= 0 ? block_index->GetAncestor(distant_height) : nullptr;
CAmount distantUnlocked{0};
if (distant_block_index) {
if (std::optional<CreditPoolDataPerBlock> distant_block{GetCreditDataFromBlock(distant_block_index, consensusParams)};
distant_block) {
distantUnlocked = distant_block->unlocked;
}
}

Copy link
Copy Markdown
Collaborator Author

@knst knst May 5, 2025

Choose a reason for hiding this comment

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

no, there's no asserts:

    if (height > nHeight || height < 0) {
        return nullptr; 
    }

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 5, 2025

Choose a reason for hiding this comment

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

@knst You're absolutely right, and I made a mistake in my review. Looking at the implementation of GetAncestor, it indeed handles negative heights gracefully by returning nullptr:

if (height > nHeight || height < 0) {
    return nullptr; 
}

And the existing code already properly handles this case with the null check:

if (distant_block_index) {
    // ...
}

I apologize for the incorrect suggestion. The current implementation is safe as written.

✏️ Learnings added 
Learnt from: knst
PR: dashpay/dash#6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.755Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


CAmount currentLimit = locked;
CAmount currentLimit = blockData.credit_pool;
const CAmount latelyUnlocked = prev.latelyUnlocked + blockData.unlocked - distantUnlocked;
if (DeploymentActiveAt(*block_index, Params().GetConsensus(), Consensus::DEPLOYMENT_WITHDRAWALS)) {
currentLimit = std::min(currentLimit, LimitAmountV22);
} else {
// Unlock limits in pre-v22 are max(100, min(.10 * assetlockpool, 1000)) inside window
if (currentLimit + latelyUnlocked > LimitAmountLow) {
currentLimit = std::max(LimitAmountLow, locked / 10) - latelyUnlocked;
currentLimit = std::max(LimitAmountLow, blockData.credit_pool / 10) - latelyUnlocked;
if (currentLimit < 0) currentLimit = 0;
}
currentLimit = std::min(currentLimit, LimitAmountHigh - latelyUnlocked);
}

if (currentLimit != 0 || latelyUnlocked > 0 || locked > 0) {
if (currentLimit != 0 || latelyUnlocked > 0 || blockData.credit_pool > 0) {
LogPrint(BCLog::CREDITPOOL, /* Continued */
"CCreditPoolManager: asset unlock limits on height: %d locked: %d.%08d limit: %d.%08d "
"unlocked-in-window: %d.%08d\n",
block_index->nHeight, locked / COIN, locked % COIN, currentLimit / COIN, currentLimit % COIN,
latelyUnlocked / COIN, latelyUnlocked % COIN);
block_index->nHeight, blockData.credit_pool / COIN, blockData.credit_pool % COIN, currentLimit / COIN,
currentLimit % COIN, latelyUnlocked / COIN, latelyUnlocked % COIN);
}

if (currentLimit < 0) {
throw std::runtime_error(
strprintf("Negative limit for CreditPool: %d.%08d\n", currentLimit / COIN, currentLimit % COIN));
}

CCreditPool pool{locked, currentLimit, latelyUnlocked, indexes};
CCreditPool pool{blockData.credit_pool, currentLimit, latelyUnlocked, indexes};
AddToCache(block_index->GetBlockHash(), block_index->nHeight, pool);
return pool;

Expand Down
Loading