Skip to content

refactor: simplify asset lock validation code #7284

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
knst wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

refactor: simplify asset lock validation code #7284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6277a71
refactor: inline CheckAssetLockUnlockTx function to creditpool
knst Apr 17, 2026
4476d18
refactor: remove re-validation of asset lock transactions for existin…
knst Apr 17, 2026
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
15 changes: 0 additions & 15 deletions src/evo/assetlocktx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,6 @@
using node::BlockManager;


/**
* Common code for Asset Lock and Asset Unlock
*/
bool CheckAssetLockUnlockTx(const BlockManager& blockman, const llmq::CQuorumManager& qman, const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state)
{
switch (tx.nType) {
case TRANSACTION_ASSET_LOCK:
return CheckAssetLockTx(tx, state);
case TRANSACTION_ASSET_UNLOCK:
return CheckAssetUnlockTx(blockman, qman, tx, pindexPrev, indexes, state);
default:
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-not-asset-locks-at-all");
}
}

/**
* Asset Lock Transaction
*/
Expand Down
1 change: 0 additions & 1 deletion src/evo/assetlocktx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ class CAssetUnlockPayload

bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state);
bool CheckAssetUnlockTx(const node::BlockManager& blockman, const llmq::CQuorumManager& qman, const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state);
bool CheckAssetLockUnlockTx(const node::BlockManager& blockman, const llmq::CQuorumManager& qman, const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state);
bool GetAssetUnlockFee(const CTransaction& tx, CAmount& txfee, TxValidationState& state);

#endif // BITCOIN_EVO_ASSETLOCKTX_H
16 changes: 4 additions & 12 deletions src/evo/creditpool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
#include <memory>
#include <stack>

using node::BlockManager;
using node::ReadBlockFromDisk;

// Forward declaration to prevent a new circular dependencies through masternode/payments.h
Expand Down Expand Up @@ -248,8 +247,7 @@ CCreditPoolManager::~CCreditPoolManager() = default;

CCreditPoolDiff::CCreditPoolDiff(CCreditPool starter, const CBlockIndex* pindexPrev,
const Consensus::Params& consensusParams, const CAmount blockSubsidy) :
pool(std::move(starter)),
pindexPrev(pindexPrev)
pool(std::move(starter))
{
assert(pindexPrev);

Expand Down Expand Up @@ -297,15 +295,9 @@ bool CCreditPoolDiff::Unlock(const CTransaction& tx, TxValidationState& state)
return true;
}

bool CCreditPoolDiff::ProcessLockUnlockTransaction(const BlockManager& blockman, const llmq::CQuorumManager& qman, const CTransaction& tx, TxValidationState& state)
bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state)
{
if (!tx.IsSpecialTxVersion()) return true;
Comment thread
knst marked this conversation as resolved.
if (tx.nType != TRANSACTION_ASSET_LOCK && tx.nType != TRANSACTION_ASSET_UNLOCK) return true;

if (!CheckAssetLockUnlockTx(blockman, qman, tx, pindexPrev, pool.indexes, state)) {
// pass the state returned by the function above
return false;
}

Comment thread
knst marked this conversation as resolved.
try {
Comment thread
knst marked this conversation as resolved.
Comment thread
knst marked this conversation as resolved.
switch (tx.nType) {
Comment on lines 300 to 303
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 21, 2026

Choose a reason for hiding this comment

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

P1 Badge Revalidate asset unlock consensus rules before block assembly

ProcessLockUnlockTransaction now applies only credit-pool accounting and no longer runs CheckAssetUnlockTx, so block assembly can include mempool asset-unlock transactions that became invalid after admission (e.g., expired at current tip height or no longer in an active quorum window). addPackageTxs relies on this method to skip invalid special txs, and if one slips through, CreateNewBlock later fails in TestBlockValidity, which can repeatedly break template creation for miners instead of just skipping the stale tx.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

@knst knst Apr 21, 2026

Choose a reason for hiding this comment

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

Credit pool related checks are still there:

    if (!GetDataFromUnlockTx(tx, toUnlock, index, state)) {
        // state is set up inside GetDataFromUnlockTx
        return false;
    }       
    
    if (sessionUnlocked + toUnlock > pool.currentLimit) {
        return state.Invalid(TxValidationResult::TX_CONSENSUS, "failed-creditpool-unlock-too-much");
    }   
    
    if (pool.indexes.Contains(index) || newIndexes.count(index)) {
        return state.Invalid(TxValidationResult::TX_CONSENSUS, "failed-creditpool-unlock-duplicated-index");
    }

mempool validate all other consensus relevant conditions such as pre-v24 activation ; wrong OP_RETURN utxo, etc.

Copy link
Copy Markdown
Collaborator Author

@knst knst Apr 23, 2026

Choose a reason for hiding this comment

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

duplicate with #7284 (comment)

resolved block assembly

Expand All @@ -322,7 +314,7 @@ bool CCreditPoolDiff::ProcessLockUnlockTransaction(const BlockManager& blockman,
}
Comment on lines 302 to 314
Copy link
Copy Markdown

@thepastaclaw thepastaclaw Apr 23, 2026

Choose a reason for hiding this comment

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

💬 Nitpick: try/catch in ProcessLockUnlockTransaction is now effectively dead

With CheckAssetLockUnlockTx gone, the remaining body inside the try is just Lock(tx, state) / Unlock(tx, state). Those paths use GetTxPayload (returns std::optional) and perform arithmetic/range checks — none of them throw std::exception under normal operation, so the catch block is unreachable and now implies a throwing contract that no longer exists. Either drop the try/catch or add a comment noting it is retained as scaffolding for the upcoming v24 two-version asset-lock rules.

source: ['claude']

}
Comment on lines 298 to 315
Copy link
Copy Markdown

@thepastaclaw thepastaclaw Apr 23, 2026

Choose a reason for hiding this comment

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

🔴 Blocking: Mining path loses height/quorum-sensitive asset unlock validation

CCreditPoolDiff::ProcessLockUnlockTransaction() no longer invokes CheckAssetUnlockTx(), which previously ran CAssetUnlockPayload::VerifySig() (src/evo/assetlocktx.cpp:96-140) — the context-sensitive checks against the current tip height and the active quorum set. On the connect-block path this is fine because ProcessSpecialTxsInBlock already calls CheckSpecialTxInner for every tx (src/evo/specialtxman.cpp:579) before reaching CheckCreditPoolDiffForBlock. But the mining path does not: BlockAssembler::addPackageTxs (src/node/miner.cpp:545) and CreateNewBlock's GetCreditPoolDiffForBlock call (src/node/miner.cpp:306) now rely solely on whatever validation occurred at mempool admission. removeExpiredAssetUnlock purges only entries where item.second < nBlockHeight (src/txmempool.cpp:1096-1098), but VerifySig rejects with bad-assetunlock-too-late as soon as pindexTip->nHeight >= getHeightToExpiry(), so an unlock with expiry == tip height survives one block longer than it is valid. There is no mempool purge at all for the bad-assetunlock-too-old-quorum case when a quorum ages out of the active set. The result is that CreateNewBlock can produce a block template containing asset-unlock txs that will be rejected by ProcessSpecialTxsInBlock on connect, causing miners to generate invalid blocks. Before dropping the re-validation, either reintroduce CheckAssetUnlockTx in the mining path (via addPackageTxs or a dedicated pre-filter) or tighten the mempool purge to cover both the expiry boundary and quorum rotation.

source: ['codex']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/evo/creditpool.cpp`:
- [BLOCKING] lines 298-315: Mining path loses height/quorum-sensitive asset unlock validation
  `CCreditPoolDiff::ProcessLockUnlockTransaction()` no longer invokes `CheckAssetUnlockTx()`, which previously ran `CAssetUnlockPayload::VerifySig()` (src/evo/assetlocktx.cpp:96-140) — the context-sensitive checks against the current tip height and the active quorum set. On the connect-block path this is fine because `ProcessSpecialTxsInBlock` already calls `CheckSpecialTxInner` for every tx (src/evo/specialtxman.cpp:579) before reaching `CheckCreditPoolDiffForBlock`. But the mining path does not: `BlockAssembler::addPackageTxs` (src/node/miner.cpp:545) and `CreateNewBlock`'s `GetCreditPoolDiffForBlock` call (src/node/miner.cpp:306) now rely solely on whatever validation occurred at mempool admission. `removeExpiredAssetUnlock` purges only entries where `item.second < nBlockHeight` (src/txmempool.cpp:1096-1098), but `VerifySig` rejects with `bad-assetunlock-too-late` as soon as `pindexTip->nHeight >= getHeightToExpiry()`, so an unlock with `expiry == tip height` survives one block longer than it is valid. There is no mempool purge at all for the `bad-assetunlock-too-old-quorum` case when a quorum ages out of the active set. The result is that `CreateNewBlock` can produce a block template containing asset-unlock txs that will be rejected by `ProcessSpecialTxsInBlock` on connect, causing miners to generate invalid blocks. Before dropping the re-validation, either reintroduce `CheckAssetUnlockTx` in the mining path (via `addPackageTxs` or a dedicated pre-filter) or tighten the mempool purge to cover both the expiry boundary and quorum rotation.

Comment on lines 298 to 315
Copy link
Copy Markdown

@thepastaclaw thepastaclaw Apr 25, 2026

Choose a reason for hiding this comment

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

🟡 Suggestion: Implicit invariant: ProcessLockUnlockTransaction now relies on caller-side validation

After this PR, ProcessLockUnlockTransaction no longer calls CheckAssetLockTx/CheckAssetUnlockTx. Correctness depends on the caller having validated the transaction beforehand: ConnectBlock validates via CheckSpecialTxInner (specialtxman.cpp) before CheckCreditPoolDiffForBlock; the miner relies on mempool acceptance for ASSET_LOCK and re-checks ASSET_UNLOCK explicitly in addPackageTxs. This is a non-obvious precondition. A short comment above ProcessLockUnlockTransaction documenting that callers must ensure CheckAssetLockTx/CheckAssetUnlockTx has run would prevent future regressions where a new caller skips the upstream validation.

source: ['claude']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/evo/creditpool.cpp`:
- [SUGGESTION] lines 298-315: Implicit invariant: ProcessLockUnlockTransaction now relies on caller-side validation
  After this PR, ProcessLockUnlockTransaction no longer calls CheckAssetLockTx/CheckAssetUnlockTx. Correctness depends on the caller having validated the transaction beforehand: ConnectBlock validates via CheckSpecialTxInner (specialtxman.cpp) before CheckCreditPoolDiffForBlock; the miner relies on mempool acceptance for ASSET_LOCK and re-checks ASSET_UNLOCK explicitly in addPackageTxs. This is a non-obvious precondition. A short comment above ProcessLockUnlockTransaction documenting that callers must ensure CheckAssetLockTx/CheckAssetUnlockTx has run would prevent future regressions where a new caller skips the upstream validation.


std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman, const BlockManager& blockman, const llmq::CQuorumManager& qman,
std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman,
const CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams,
const CAmount blockSubsidy, BlockValidationState& state)
{
Expand All @@ -333,7 +325,7 @@ std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpo
for (size_t i = 1; i < block.vtx.size(); ++i) {
const auto& tx = *block.vtx[i];
TxValidationState tx_state;
if (!creditPoolDiff.ProcessLockUnlockTransaction(blockman, qman, tx, tx_state)) {
if (!creditPoolDiff.ProcessLockUnlockTransaction(tx, tx_state)) {
assert(tx_state.GetResult() == TxValidationResult::TX_CONSENSUS);
state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(),
strprintf("Process Lock/Unlock Transaction failed at Credit Pool (tx hash %s) %s", tx.GetHash().ToString(), tx_state.GetDebugMessage()));
Expand Down
12 changes: 4 additions & 8 deletions src/evo/creditpool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@ struct Params;
namespace llmq {
class CQuorumManager;
} // namespace llmq
namespace node {
class BlockManager;
} // namespace node

struct CCreditPool {
CAmount locked{0};
Expand Down Expand Up @@ -70,16 +67,15 @@ struct CCreditPool {
* limits should stay same and depends only on the previous block.
*/
class CCreditPoolDiff {
private:
public:
const CCreditPool pool;
Comment thread
knst marked this conversation as resolved.
private:
std::unordered_set<uint64_t> newIndexes;

CAmount sessionLocked{0};
CAmount sessionUnlocked{0};
CAmount platformReward{0};

const CBlockIndex *pindexPrev{nullptr};

public:
explicit CCreditPoolDiff(CCreditPool starter, const CBlockIndex *pindexPrev,
const Consensus::Params& consensusParams,
Expand All @@ -90,7 +86,7 @@ class CCreditPoolDiff {
* to change amount of credit pool
* @return true if transaction can be included in this block
*/
bool ProcessLockUnlockTransaction(const node::BlockManager& blockman, const llmq::CQuorumManager& qman, const CTransaction& tx, TxValidationState& state);
bool ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state);

/**
* this function returns total amount of credits for the next block
Expand Down Expand Up @@ -147,7 +143,7 @@ class CCreditPoolManager
EXCLUSIVE_LOCKS_REQUIRED(!cache_mutex);
};

std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman, const node::BlockManager& blockman, const llmq::CQuorumManager& qman,
std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman,
const CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams,
const CAmount blockSubsidy, BlockValidationState& state);

Expand Down
2 changes: 1 addition & 1 deletion src/evo/specialtxman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ bool CSpecialTxProcessor::CheckCreditPoolDiffForBlock(const CBlock& block, const

try {
const CAmount blockSubsidy = GetBlockSubsidy(pindex, m_consensus_params);
const auto creditPoolDiff = GetCreditPoolDiffForBlock(m_cpoolman, m_chainman.m_blockman, m_qman, block,
const auto creditPoolDiff = GetCreditPoolDiffForBlock(m_cpoolman, block,
pindex->pprev, m_consensus_params, blockSubsidy, state);
if (!creditPoolDiff.has_value()) return false;

Expand Down
17 changes: 15 additions & 2 deletions src/node/miner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
LogPrintf("CreateNewBlock() h[%d] CbTx failed to find best CL. Inserting null CL\n", nHeight);
}
BlockValidationState state;
const auto creditPoolDiff = GetCreditPoolDiffForBlock(*m_chain_helper.credit_pool_manager, m_blockman, m_qman, *pblock, pindexPrev, chainparams.GetConsensus(), blockSubsidy, state);
const auto creditPoolDiff = GetCreditPoolDiffForBlock(*m_chain_helper.credit_pool_manager, *pblock, pindexPrev, chainparams.GetConsensus(), blockSubsidy, state);
if (creditPoolDiff == std::nullopt) {
throw std::runtime_error(strprintf("%s: GetCreditPoolDiffForBlock failed: %s", __func__, state.ToString()));
}
Expand Down Expand Up @@ -544,7 +544,20 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
// `state` is local here because used only to log info about this specific tx
TxValidationState state;

if (!creditPoolDiff->ProcessLockUnlockTransaction(m_blockman, m_qman, iter->GetTx(), state)) {
if (iter->GetTx().IsSpecialTxVersion() && iter->GetTx().nType == TRANSACTION_ASSET_UNLOCK) {
// ASSET_UNLOCK transactions may expire after being added to mempool
// They should not be included to the block
if (!CheckAssetUnlockTx(m_blockman, m_qman, iter->GetTx(), pindexPrev, creditPoolDiff->pool.indexes, state)) {
if (fUsingModified) {
mapModifiedTx.get<ancestor_score>().erase(modit);
failedTx.insert(iter);
}
LogPrintf("%s: asset unlock tx %s is skipped due %s\n",
__func__, iter->GetTx().GetHash().ToString(), state.ToString());
continue;
}
}
Comment thread
knst marked this conversation as resolved.
if (!creditPoolDiff->ProcessLockUnlockTransaction(iter->GetTx(), state)) {
if (fUsingModified) {
mapModifiedTx.get<ancestor_score>().erase(modit);
failedTx.insert(iter);
Expand Down
Loading