Conversation
Since dashpay#7639 a node keeps a version 2 asset unlock and awaiting a re-signed instance even after expired height. A restarted node, or one that never saw the instance, rejected it as bad-assetunlock-too-late, orphaned its spends, and ban honest peer with Misbehaving(100). The same tip dependence punished relayers of v1 unlocks a block ahead of or behind the receiver. Both tip-dependent rejections now fail as TX_BAD_SPECIAL, since the same transaction is valid a block earlier or later. Block validation is unchanged: ProcessSpecialTxsInBlock maps either result to BLOCK_CONSENSUS. The mempool validates a version 2 instance at the last height of its own window instead of the tip, so an expired instance is admitted when it was minable then. The miner and the InstantSend signer still validate at the tip, so it stays unmined and unlocked until Platform re-signs it.
|
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughFor version 2 Asset Unlock transactions with stable txids, mempool validation uses the ancestor at the last height of the validity window instead of the current tip. Two validation failures now return Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change implements the intended mempool and relay-policy behavior without introducing an actionable merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
✨ 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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Final validation — Phase 2 only (queue backlog)
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
PR correctly softens tip-dependent unlock rejections and backdates mempool checks without changing consensus. Two in-scope suggestions remain: align the v24 gate with the backdated height and add a regression test for fresh-node admission of expired v2 unlocks.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)
- Degraded mode:
gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-24T12:50:20Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
lowbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — Small 11-line mempool/P2P-scoring fix confined to asset-unlock validation whose correctness is easy to confirm and leaves block consensus unchanged. - Phase 1 reviewers: not run (skipped for throughput: 14 PRs queued, above the 10 limit)
- Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — final-verifier; agentastra-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort medium); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 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/validation.cpp`:
- [SUGGESTION] src/validation.cpp:1067-1074: v24 activation evaluated at tip instead of historical check height
CheckSpecialTx is called with backdated pindex_check (ancestor at expiry-1) for stable-txid unlocks, but is_v24_active is still computed with DeploymentActiveAfter(tip). CheckAssetUnlockTxImpl rejects version 2 when !is_v24_active, so a v2 window that entirely predates v24 was never minable historically yet is admitted now once the tip activates v24. This contradicts the stated intent of admitting only what was minable at the last height of its window. Impact is mempool-only: miner (src/node/miner.cpp) and InstantSend signer (src/instantsend/signing.cpp) still check at the tip so it stays unmined, signature verification still requires a Platform signature, and v2 instances are never expiry-evicted (src/txmempool.cpp), so it only lingers.
- [SUGGESTION] src/validation.cpp:1069-1074: No regression test for fresh admission of expired v2 unlock
The change adds mempool backdating in src/validation.cpp and TX_CONSENSUS to TX_BAD_SPECIAL rescoring in src/evo/assetlocktx.cpp with no test changes. Existing feature_asset_locks.py coverage checks the reject strings and the already-held expired instance lingering, but does not submit an expired v2 instance to a node that never held it (restarted/fresh side) and assert mempool acceptance without Misbehaving, plus child-spend acceptance. That is the exact failure mode in the PR description and needs a functional test that fails without the fix and passes with it.
| const CBlockIndex* tip{m_active_chainstate.m_chain.Tip()}; | ||
| const bool is_v24_active{DeploymentActiveAfter(tip, m_active_chainstate.m_chainman, Consensus::DEPLOYMENT_V24)}; | ||
| if (!m_chain_helper.special_tx->CheckSpecialTx(tx, tip, is_v24_active, m_active_chainstate.CoinsTip(), true, state)) | ||
| // An expired version 2 asset unlock is kept awaiting a re-signed instance, so it is admitted if | ||
| // it was minable at the last height of its window rather than at the tip | ||
| const auto unlock{IsAssetUnlockWithStableTxid(tx) ? GetTxPayload<CAssetUnlockPayload>(tx) : std::nullopt}; | ||
| const CBlockIndex* pindex_check{unlock ? tip->GetAncestor(std::clamp(unlock->getHeightToExpiry() - 1, 0, tip->nHeight)) : tip}; | ||
| if (!m_chain_helper.special_tx->CheckSpecialTx(tx, pindex_check, is_v24_active, m_active_chainstate.CoinsTip(), true, state)) | ||
| return false; |
There was a problem hiding this comment.
🟡 Suggestion: v24 activation evaluated at tip instead of historical check height
CheckSpecialTx is called with backdated pindex_check (ancestor at expiry-1) for stable-txid unlocks, but is_v24_active is still computed with DeploymentActiveAfter(tip). CheckAssetUnlockTxImpl rejects version 2 when !is_v24_active, so a v2 window that entirely predates v24 was never minable historically yet is admitted now once the tip activates v24. This contradicts the stated intent of admitting only what was minable at the last height of its window. Impact is mempool-only: miner (src/node/miner.cpp) and InstantSend signer (src/instantsend/signing.cpp) still check at the tip so it stays unmined, signature verification still requires a Platform signature, and v2 instances are never expiry-evicted (src/txmempool.cpp), so it only lingers.
| const CBlockIndex* tip{m_active_chainstate.m_chain.Tip()}; | |
| const bool is_v24_active{DeploymentActiveAfter(tip, m_active_chainstate.m_chainman, Consensus::DEPLOYMENT_V24)}; | |
| if (!m_chain_helper.special_tx->CheckSpecialTx(tx, tip, is_v24_active, m_active_chainstate.CoinsTip(), true, state)) | |
| // An expired version 2 asset unlock is kept awaiting a re-signed instance, so it is admitted if | |
| // it was minable at the last height of its window rather than at the tip | |
| const auto unlock{IsAssetUnlockWithStableTxid(tx) ? GetTxPayload<CAssetUnlockPayload>(tx) : std::nullopt}; | |
| const CBlockIndex* pindex_check{unlock ? tip->GetAncestor(std::clamp(unlock->getHeightToExpiry() - 1, 0, tip->nHeight)) : tip}; | |
| if (!m_chain_helper.special_tx->CheckSpecialTx(tx, pindex_check, is_v24_active, m_active_chainstate.CoinsTip(), true, state)) | |
| return false; | |
| const CBlockIndex* tip{m_active_chainstate.m_chain.Tip()}; | |
| // An expired version 2 asset unlock is kept awaiting a re-signed instance, so it is admitted if | |
| // it was minable at the last height of its window rather than at the tip | |
| const auto unlock{IsAssetUnlockWithStableTxid(tx) ? GetTxPayload<CAssetUnlockPayload>(tx) : std::nullopt}; | |
| const CBlockIndex* pindex_check{unlock ? tip->GetAncestor(std::clamp(unlock->getHeightToExpiry() - 1, 0, tip->nHeight)) : tip}; | |
| const bool is_v24_active{DeploymentActiveAfter(pindex_check, m_active_chainstate.m_chainman, Consensus::DEPLOYMENT_V24)}; |
source: muse-spark-1.3-contributor (phase2-reviewer: general)
| // An expired version 2 asset unlock is kept awaiting a re-signed instance, so it is admitted if | ||
| // it was minable at the last height of its window rather than at the tip | ||
| const auto unlock{IsAssetUnlockWithStableTxid(tx) ? GetTxPayload<CAssetUnlockPayload>(tx) : std::nullopt}; | ||
| const CBlockIndex* pindex_check{unlock ? tip->GetAncestor(std::clamp(unlock->getHeightToExpiry() - 1, 0, tip->nHeight)) : tip}; | ||
| if (!m_chain_helper.special_tx->CheckSpecialTx(tx, pindex_check, is_v24_active, m_active_chainstate.CoinsTip(), true, state)) | ||
| return false; |
There was a problem hiding this comment.
🟡 Suggestion: No regression test for fresh admission of expired v2 unlock
The change adds mempool backdating in src/validation.cpp and TX_CONSENSUS to TX_BAD_SPECIAL rescoring in src/evo/assetlocktx.cpp with no test changes. Existing feature_asset_locks.py coverage checks the reject strings and the already-held expired instance lingering, but does not submit an expired v2 instance to a node that never held it (restarted/fresh side) and assert mempool acceptance without Misbehaving, plus child-spend acceptance. That is the exact failure mode in the PR description and needs a functional test that fails without the fix and passes with it.
source: muse-spark-1.3-contributor (phase2-reviewer: general)
thepastaclaw
added
the
pastaclaw:commented
|
This pull request has conflicts, please rebase. |
thepastaclaw
removed
the
pastaclaw:commented
|
superseeded by #7744 |
3 participants
Issue being fixed or feature implemented
Since #7639 a node keeps a version 2 asset unlock and awaiting a re-signed instance even after expired height.
A restarted node, or one that never saw the instance, rejected it as bad-assetunlock-too-late, orphaned its spends, and ban honest peer with Misbehaving(100). The same tip dependence punished relayers of v1 unlocks a block ahead of or behind the receiver.
It's alternate simpler solution to #7744
What was done?
Both tip-dependent rejections now fail as TX_BAD_SPECIAL, since the same transaction is valid a block earlier or later. Block validation is unchanged: ProcessSpecialTxsInBlock maps either result to BLOCK_CONSENSUS.
The mempool validates a version 2 instance at the last height of its own window instead of the tip, so an expired instance is admitted when it was minable then. The miner and the InstantSend signer still validate at the tip, so it stays unmined and unlocked until Platform re-signs it.
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
v2 asset unlocks had not been released yet, so, no breaking changes.
Though, there's no changes in consensus: blocks are validated exactly as before.
The changes are to mempool policy and P2P scoring, and they only affect asset unlock transactions.
Checklist: