refactor: simplify asset lock validation code#7284
refactor: simplify asset lock validation code#7284knst wants to merge 2 commits intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 4476d18) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ecdaf5cbc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!tx.IsSpecialTxVersion()) return true; | ||
| 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; | ||
| } | ||
|
|
||
| try { | ||
| switch (tx.nType) { |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
duplicate with #7284 (comment)
resolved block assembly
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR removes the Sequence Diagram(s)sequenceDiagram
participant Miner as Miner / BlockAssembler
participant CPoolMgr as CCreditPoolManager
participant CPoolDiff as CCreditPoolDiff
participant Tx as Transaction
participant Validator as CheckAssetLockTx / CheckAssetUnlockTx
Miner->>CPoolMgr: GetCreditPoolDiffForBlock(block, pindexPrev, ...)
CPoolMgr->>CPoolDiff: construct diff (no blockman/qman)
Miner->>CPoolDiff: iterate transactions -> ProcessLockUnlockTransaction(tx)
CPoolDiff->>Tx: inspect tx.nType
alt nType == ASSET_LOCK
CPoolDiff->>Validator: CheckAssetLockTx(tx)
else nType == ASSET_UNLOCK
CPoolDiff->>Validator: CheckAssetUnlockTx(tx)
else
CPoolDiff-->>Miner: return true / ignore
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/node/miner.cpp (1)
541-555:⚠️ Potential issue | 🟠 MajorVerify or add full asset lock/unlock validation at candidate selection before AddToBlock.
Line 547 calls
ProcessLockUnlockTransaction()which only validates pool limits and duplicate indexes. A stale or invalid asset lock/unlock transaction can pass this accounting check and be added to the block candidate, then fail during full special-tx validation inTestBlockValidity()(viaProcessSpecialTxsInBlock()→CheckSpecialTxInner()→CheckAssetLockTx()/CheckAssetUnlockTx()), aborting the entire block template instead of skipping the candidate. Either callCheckAssetLockTx()/CheckAssetUnlockTx()checks here beforeAddToBlock(), or confirm mempool revalidation prevents stale asset locks/unlocks from persisting across reorg and fork boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/miner.cpp` around lines 541 - 555, The current code only calls ProcessLockUnlockTransaction() which does lightweight accounting checks and can let stale/invalid asset lock/unlock TXs reach AddToBlock and later abort TestBlockValidity; update the candidate-selection path (where creditPoolDiff is used) to perform the full special-tx validation by invoking CheckAssetLockTx()/CheckAssetUnlockTx() (or the common CheckSpecialTxInner/ProcessSpecialTxsInBlock helper) for the iter->GetTx() before adding the TX to the block candidate, and if those checks fail log the state, remove the TX from mapModifiedTx/failedTx like the existing branch and continue so invalid special TXs are skipped instead of causing block-template abortion. Ensure you reference ProcessLockUnlockTransaction, CheckAssetLockTx/CheckAssetUnlockTx and AddToBlock when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/node/miner.cpp`:
- Around line 541-555: The current code only calls
ProcessLockUnlockTransaction() which does lightweight accounting checks and can
let stale/invalid asset lock/unlock TXs reach AddToBlock and later abort
TestBlockValidity; update the candidate-selection path (where creditPoolDiff is
used) to perform the full special-tx validation by invoking
CheckAssetLockTx()/CheckAssetUnlockTx() (or the common
CheckSpecialTxInner/ProcessSpecialTxsInBlock helper) for the iter->GetTx()
before adding the TX to the block candidate, and if those checks fail log the
state, remove the TX from mapModifiedTx/failedTx like the existing branch and
continue so invalid special TXs are skipped instead of causing block-template
abortion. Ensure you reference ProcessLockUnlockTransaction,
CheckAssetLockTx/CheckAssetUnlockTx and AddToBlock when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 46434f4a-faed-4845-aead-f38c9feeb213
📒 Files selected for processing (6)
src/evo/assetlocktx.cppsrc/evo/assetlocktx.hsrc/evo/creditpool.cppsrc/evo/creditpool.hsrc/evo/specialtxman.cppsrc/node/miner.cpp
💤 Files with no reviewable changes (2)
- src/evo/assetlocktx.cpp
- src/evo/assetlocktx.h
knst
force-pushed
the
refactor-asset-locks-v2
branch
from
5ecdaf5 to
2d5b71a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/evo/creditpool.h (1)
33-38: Stale forward declarations.With
ProcessLockUnlockTransactionandGetCreditPoolDiffForBlockno longer takingnode::BlockManagerorllmq::CQuorumManager, the forward declarations on lines 33–38 appear unused in this header. Consider removing them for cleanup (and drop the now-unneeded<gsl/pointers.h>only if verified unused elsewhere —gsl::not_nullis still used at line 144, so keep that one).♻️ Proposed cleanup
-namespace llmq { -class CQuorumManager; -} // namespace llmq -namespace node { -class BlockManager; -} // namespace node🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/evo/creditpool.h` around lines 33 - 38, Remove the now-stale forward declarations for llmq::CQuorumManager and node::BlockManager from the header (they are no longer referenced by ProcessLockUnlockTransaction or GetCreditPoolDiffForBlock); also evaluate and remove the `#include` <gsl/pointers.h> only if it is truly unused elsewhere (note gsl::not_null is still used at the symbol referenced on line 144, so keep gsl headers that define not_null). Update header by deleting the unused forward-declare block for llmq and node and then run a build to confirm no remaining references before removing the gsl include.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/evo/creditpool.h`:
- Around line 33-38: Remove the now-stale forward declarations for
llmq::CQuorumManager and node::BlockManager from the header (they are no longer
referenced by ProcessLockUnlockTransaction or GetCreditPoolDiffForBlock); also
evaluate and remove the `#include` <gsl/pointers.h> only if it is truly unused
elsewhere (note gsl::not_null is still used at the symbol referenced on line
144, so keep gsl headers that define not_null). Update header by deleting the
unused forward-declare block for llmq and node and then run a build to confirm
no remaining references before removing the gsl include.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ebab6f00-7927-479b-abcb-6f73cdbd65ca
📒 Files selected for processing (4)
src/evo/creditpool.cppsrc/evo/creditpool.hsrc/evo/specialtxman.cppsrc/node/miner.cpp
✅ Files skipped from review due to trivial changes (1)
- src/evo/specialtxman.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/node/miner.cpp
- src/evo/creditpool.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d5b71aadc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
knst
force-pushed
the
refactor-asset-locks-v2
branch
from
2d5b71a to
3ddcc9b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ddcc9b9c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/node/miner.cpp (1)
541-555:⚠️ Potential issue | 🟠 MajorRe-validate asset lock/unlock transactions in mining before AddToBlock.
The code calls
ProcessLockUnlockTransaction(credit pool accounting only), but asset unlock transactions have time-dependent validity constraints. Specifically,CheckAssetUnlockTxverifies thatpindexTip->nHeight < requestedHeight + 48; a transaction valid in mempool can become stale if the chain advances before the block template is created. Currently, invalid transactions are only caught byTestBlockValidity, which aborts the entire block template instead of skipping the individual transaction.Either preserve direct
CheckAssetUnlockTxvalidation during mining, or document the invariant that mempool asset lock/unlock entries remain valid between acceptance and block assembly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/miner.cpp` around lines 541 - 555, The mining loop currently only calls ProcessLockUnlockTransaction but must also re-check time-dependent asset unlock validity to avoid aborting the whole block in TestBlockValidity; add a direct call to CheckAssetUnlockTx (or the same validation logic) for the candidate tx right before AddToBlock handling in the loop that processes lock/unlock txs (the same place where ProcessLockUnlockTransaction is invoked), and if CheckAssetUnlockTx fails treat it like other skipped txs: remove from mapModifiedTx<ancestor_score> if fUsingModified, insert into failedTx, LogPrintf a clear message referencing the tx hash and the validation failure, and continue; ensure the call uses current chain tip height (pindexTip->nHeight) and the tx’s requestedHeight logic (requestedHeight+48) so stale unlocks get skipped rather than causing TestBlockValidity to abort.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/node/miner.cpp`:
- Around line 541-555: The mining loop currently only calls
ProcessLockUnlockTransaction but must also re-check time-dependent asset unlock
validity to avoid aborting the whole block in TestBlockValidity; add a direct
call to CheckAssetUnlockTx (or the same validation logic) for the candidate tx
right before AddToBlock handling in the loop that processes lock/unlock txs (the
same place where ProcessLockUnlockTransaction is invoked), and if
CheckAssetUnlockTx fails treat it like other skipped txs: remove from
mapModifiedTx<ancestor_score> if fUsingModified, insert into failedTx, LogPrintf
a clear message referencing the tx hash and the validation failure, and
continue; ensure the call uses current chain tip height (pindexTip->nHeight) and
the tx’s requestedHeight logic (requestedHeight+48) so stale unlocks get skipped
rather than causing TestBlockValidity to abort.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7ad1e57c-0e52-466a-8694-a5626a932f0e
📒 Files selected for processing (4)
src/evo/creditpool.cppsrc/evo/creditpool.hsrc/evo/specialtxman.cppsrc/node/miner.cpp
✅ Files skipped from review due to trivial changes (1)
- src/evo/specialtxman.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/evo/creditpool.h
knst
force-pushed
the
refactor-asset-locks-v2
branch
from
3ddcc9b to
765002c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/evo/creditpool.h (1)
33-38: Remove stale forward declarations after signature simplification.With
llmq::CQuorumManagerandnode::BlockManagerno longer referenced by any function signatures in this header, these forward declarations are unused. Consider removing them to keep the API surface minimal.♻️ Proposed cleanup
class TxValidationState; namespace Consensus { struct Params; } // namespace Consensus -namespace llmq { -class CQuorumManager; -} // namespace llmq -namespace node { -class BlockManager; -} // namespace node🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/evo/creditpool.h` around lines 33 - 38, Remove the now-unused forward declarations for llmq::CQuorumManager and node::BlockManager from the header (they appear as "class CQuorumManager;" and "class BlockManager;") since signature simplification no longer references these types; delete those declaration lines and verify no remaining usages of CQuorumManager or BlockManager in this header or its inline functions, then rebuild to ensure no broken references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/evo/creditpool.h`:
- Around line 33-38: Remove the now-unused forward declarations for
llmq::CQuorumManager and node::BlockManager from the header (they appear as
"class CQuorumManager;" and "class BlockManager;") since signature
simplification no longer references these types; delete those declaration lines
and verify no remaining usages of CQuorumManager or BlockManager in this header
or its inline functions, then rebuild to ensure no broken references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 716b0b66-bbf9-4e68-b88d-cc8ab3a4af40
📒 Files selected for processing (4)
src/evo/creditpool.cppsrc/evo/creditpool.hsrc/evo/specialtxman.cppsrc/node/miner.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/evo/specialtxman.cpp
- src/evo/creditpool.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765002c588
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
knst
force-pushed
the
refactor-asset-locks-v2
branch
from
765002c to
8c0b150
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/evo/creditpool.h (1)
33-38: Nit: stale forward declarations can be dropped.After this refactor, neither
llmq::CQuorumManagernornode::BlockManageris referenced anywhere else in this header (they were only needed by the removed parameters ofProcessLockUnlockTransaction/GetCreditPoolDiffForBlock). The forward-declaration namespaces can be removed for cleanliness.♻️ Suggested cleanup
class TxValidationState; namespace Consensus { struct Params; } // namespace Consensus -namespace llmq { -class CQuorumManager; -} // namespace llmq -namespace node { -class BlockManager; -} // namespace node🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/evo/creditpool.h` around lines 33 - 38, Remove the now-stale forward declarations for llmq::CQuorumManager and node::BlockManager from this header: since the parameters that referenced them in ProcessLockUnlockTransaction and GetCreditPoolDiffForBlock were removed, drop the entire namespace blocks "namespace llmq { class CQuorumManager; }" and "namespace node { class BlockManager; }" to clean up unused declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/evo/creditpool.h`:
- Around line 33-38: Remove the now-stale forward declarations for
llmq::CQuorumManager and node::BlockManager from this header: since the
parameters that referenced them in ProcessLockUnlockTransaction and
GetCreditPoolDiffForBlock were removed, drop the entire namespace blocks
"namespace llmq { class CQuorumManager; }" and "namespace node { class
BlockManager; }" to clean up unused declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5ae9187-ed1c-4536-98d8-f1bc6f8d0d8f
📒 Files selected for processing (5)
src/evo/creditpool.cppsrc/evo/creditpool.hsrc/evo/specialtxman.cppsrc/node/miner.cppsrc/node/miner.h
💤 Files with no reviewable changes (1)
- src/node/miner.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/evo/creditpool.cpp
knst
force-pushed
the
refactor-asset-locks-v2
branch
from
8c0b150 to
6829d83
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6829d835de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The refactor correctly removes redundant validation for already-validated blocks on the connect-block path, but it also strips context-sensitive asset-unlock checks from the mining path in BlockAssembler, where no earlier CheckSpecialTxInner pass exists. Mempool expiration is not strict enough to preserve the prior guarantee. One blocking issue and one minor nit.
Reviewed commit: 6829d83
🔴 1 blocking | 💬 1 nitpick(s)
🤖 Prompt for all review comments 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.
| @@ -322,7 +314,7 @@ bool CCreditPoolDiff::ProcessLockUnlockTransaction(const BlockManager& blockman, | |||
| } | |||
| } | |||
There was a problem hiding this comment.
🔴 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.
| @@ -322,7 +314,7 @@ bool CCreditPoolDiff::ProcessLockUnlockTransaction(const BlockManager& blockman, | |||
| } | |||
There was a problem hiding this comment.
💬 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']
knst
force-pushed
the
refactor-asset-locks-v2
branch
2 times, most recently
from
0ee6e1a to
ea5d7c5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea5d7c5a2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| TxValidationState state; | ||
|
|
||
| if (!creditPoolDiff->ProcessLockUnlockTransaction(m_blockman, m_qman, iter->GetTx(), state)) { | ||
| if (!iter->GetTx().IsSpecialTxVersion() && iter->GetTx().nType == TRANSACTION_ASSET_UNLOCK) { |
There was a problem hiding this comment.
The new guard in addPackageTxs is inverted: TRANSACTION_ASSET_UNLOCK transactions are special-version transactions, so !iter->GetTx().IsSpecialTxVersion() && iter->GetTx().nType == TRANSACTION_ASSET_UNLOCK is never true and CheckAssetUnlockTx(...) never runs. Fresh evidence in this revision is the newly added predicate itself, which makes the intended revalidation path unreachable. Since CCreditPoolDiff::ProcessLockUnlockTransaction no longer performs CheckAssetUnlockTx, tip-dependent unlock checks (expiry/quorum-window/signature context) are skipped during template assembly, allowing stale mempool unlocks to be selected and fail later during full block validation.
Useful? React with 👍 / 👎.
…g blocks in the chain
knst
force-pushed
the
refactor-asset-locks-v2
branch
from
ea5d7c5 to
4476d18
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I traced the changed asset-lock / credit-pool validation paths on the exact checked-out SHA and did not find a concrete correctness regression from this refactor. The removed re-validation in GetCreditPoolDiffForBlock() appears to be redundant with the per-transaction special-tx checks that already run during block validation, while the remaining credit-pool diff logic still enforces the stateful unlock/index/limit rules needed for balance reconstruction. I did not identify a blocker or actionable suggestion on this version of the branch.
Reviewed commit: 4476d18
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean refactor that removes redundant asset lock/unlock validation from CCreditPoolDiff::ProcessLockUnlockTransaction (already enforced earlier by CheckSpecialTxInner) and adds a targeted CheckAssetUnlockTx call in BlockAssembler::addPackageTxs to handle the mempool-expiration case. Both reviewers agree no consensus or correctness issue exists. Only minor architecture and documentation suggestions remain.
Reviewed commit: 4476d18
🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments 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.h`:
- [SUGGESTION] lines 70-71: Making CCreditPoolDiff::pool public weakens encapsulation
`pool` was previously private and is now public solely so `BlockAssembler::addPackageTxs` can read `creditPoolDiff->pool.indexes` (src/node/miner.cpp:550). This exposes the entire CCreditPool struct (locked, currentLimit, latelyUnlocked, indexes) to all callers when only `indexes` is needed. A narrow const accessor would preserve encapsulation while addressing the same need. Minor, but worth considering since this class is consensus-adjacent.
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.
| @@ -322,7 +314,7 @@ bool CCreditPoolDiff::ProcessLockUnlockTransaction(const BlockManager& blockman, | |||
| } | |||
| } | |||
There was a problem hiding this comment.
🟡 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.
2 participants
Issue being fixed or feature implemented
This PR is a refactoring to simplify asset lock validation and preparation for 2 version of assset locks which is going to be validated with different rules on pre-v24 fork and after v24 fork.
What was done?
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: