fix(mining): validate cumulative special transaction state per package

doc/release-notes-7570.md

@@ -0,0 +1,5 @@
+Bug Fixes
+---------
+
+- Block template creation now skips Asset Unlock transaction packages that
+  exceed credit pool limits instead of failing to create a template. (#7570)

src/evo/creditpool.cpp

@@ -281,7 +281,7 @@ bool CCreditPoolDiff::Lock(const CTransaction& tx, TxValidationState& state)
     return state.Invalid(TxValidationResult::TX_CONSENSUS, "failed-creditpool-lock-invalid");
 }
 
-bool CCreditPoolDiff::Unlock(const CTransaction& tx, TxValidationState& state)
+bool CCreditPoolDiff::Unlock(const CTransaction& tx, TxValidationState& state, std::optional<uint64_t>* inserted_index)
 {
     uint64_t index{0};
     CAmount toUnlock{0};
@@ -299,11 +299,13 @@ bool CCreditPoolDiff::Unlock(const CTransaction& tx, TxValidationState& state)
     }
 
     newIndexes.insert(index);
+    if (inserted_index) *inserted_index = index;
     sessionUnlocked += toUnlock;
     return true;
 }
 
-bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state)
+bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state,
+                                                   std::optional<uint64_t>* inserted_index)
 {
     if (!tx.IsSpecialTxVersion()) return true;
 
@@ -312,7 +314,7 @@ bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxVal
         case TRANSACTION_ASSET_LOCK:
             return Lock(tx, state);
         case TRANSACTION_ASSET_UNLOCK:
-            return Unlock(tx, state);
+            return Unlock(tx, state, inserted_index);
         default:
             return true;
         }
@@ -322,6 +324,32 @@ bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxVal
     }
 }
 
+bool CCreditPoolDiff::ProcessLockUnlockTransactions(const std::vector<CTransactionRef>& txs, TxValidationState& state)
+{
+    const auto initialLocked = sessionLocked;
+    const auto initialUnlocked = sessionUnlocked;
+    std::vector<uint64_t> insertedIndexes;
+
+    for (const auto& tx : txs) {
+        std::optional<uint64_t> inserted_index;
+        if (ProcessLockUnlockTransaction(*tx, state, &inserted_index)) {
+            if (inserted_index) insertedIndexes.push_back(*inserted_index);
+            continue;
+        }
+
+        // Roll back exactly what this invocation changed: amounts are scalar
+        // snapshots, and only the indexes inserted above are erased so that
+        // state committed by earlier packages is left untouched.
+        for (const uint64_t index : insertedIndexes) {
+            newIndexes.erase(index);
+        }
+        sessionLocked = initialLocked;
+        sessionUnlocked = initialUnlocked;
+        return false;
+    }
+    return true;
+}
+
 std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman,
                                                          const CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams,
                                                          const CAmount blockSubsidy, BlockValidationState& state)

src/evo/creditpool.h

@@ -20,6 +20,7 @@
 
 #include <optional>
 #include <unordered_set>
+#include <vector>
 
 class BlockValidationState;
 class CBlock;
@@ -84,9 +85,17 @@ class CCreditPoolDiff {
     /**
      * This function should be called for each Asset Lock/Unlock tx
      * to change amount of credit pool
+     * @param inserted_index if non-null, receives the unlock index recorded for this tx (if any)
      * @return true if transaction can be included in this block
      */
-    bool ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state);
+    bool ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state,
+                                      std::optional<uint64_t>* inserted_index = nullptr);
+
+    /**
+     * Process a package of Asset Lock/Unlock transactions atomically.
+     * @return true if all transactions can be included in this block
+     */
+    bool ProcessLockUnlockTransactions(const std::vector<CTransactionRef>& txs, TxValidationState& state);
 
     /**
      * this function returns total amount of credits for the next block
@@ -101,7 +110,7 @@ class CCreditPoolDiff {
 
 private:
     bool Lock(const CTransaction& tx, TxValidationState& state);
-    bool Unlock(const CTransaction& tx, TxValidationState& state);
+    bool Unlock(const CTransaction& tx, TxValidationState& state, std::optional<uint64_t>* inserted_index = nullptr);
 };
 
 class CCreditPoolManager

src/node/miner.cpp

@@ -69,18 +69,17 @@ BlockAssembler::Options::Options()
     nBlockMaxSize = DEFAULT_BLOCK_MAX_SIZE;
 }
 
-BlockAssembler::BlockAssembler(Chainstate& chainstate, const NodeContext& node, const CTxMemPool* mempool, const Options& options) :
-      m_blockman(chainstate.m_blockman),
-      m_chain_helper(chainstate.ChainHelper()),
-      m_chainstate(chainstate),
-      m_evoDb(*Assert(node.evodb)),
-      m_chainlocks(*Assert(node.chainlocks)),
-      m_clhandler(*Assert(node.clhandler)),
-      m_isman(*Assert(Assert(node.llmq_ctx)->isman)),
-      chainparams(chainstate.m_chainman.GetParams()),
-      m_mempool(mempool),
-      m_quorum_block_processor(*Assert(Assert(node.llmq_ctx)->quorum_block_processor)),
-      m_qman(*Assert(Assert(node.llmq_ctx)->qman))
+BlockAssembler::BlockAssembler(Chainstate& chainstate, const NodeContext& node, const CTxMemPool* mempool,
+                               const Options& options) :
+    m_chain_helper(chainstate.ChainHelper()),
+    m_chainstate(chainstate),
+    m_evoDb(*Assert(node.evodb)),
+    m_chainlocks(*Assert(node.chainlocks)),
+    m_clhandler(*Assert(node.clhandler)),
+    m_isman(*Assert(Assert(node.llmq_ctx)->isman)),
+    chainparams(chainstate.m_chainman.GetParams()),
+    m_mempool(mempool),
+    m_quorum_block_processor(*Assert(Assert(node.llmq_ctx)->quorum_block_processor))
 {
     blockMinFeeRate = options.blockMinFeeRate;
     nBlockMaxSize = options.nBlockMaxSize;
@@ -556,48 +555,6 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
             }
         }
 
-        if (creditPoolDiff != std::nullopt) {
-            // If one transaction is skipped due to limits, it is not a reason to interrupt
-            // whole process of adding transactions.
-            // `state` is local here because used only to log info about this specific tx
-            TxValidationState 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;
-                }
-            }
-            if (!creditPoolDiff->ProcessLockUnlockTransaction(iter->GetTx(), state)) {
-                if (fUsingModified) {
-                    mapModifiedTx.get<ancestor_score>().erase(modit);
-                    failedTx.insert(iter);
-                }
-                LogPrintf("%s: asset-locks tx %s skipped due %s\n",
-                          __func__, iter->GetTx().GetHash().ToString(), state.ToString());
-                continue;
-            }
-        }
-        if (std::optional<uint8_t> signal = extractEHFSignal(iter->GetTx()); signal != std::nullopt) {
-            if (signals.find(*signal) != signals.end()) {
-                if (fUsingModified) {
-                    mapModifiedTx.get<ancestor_score>().erase(modit);
-                    failedTx.insert(iter);
-                }
-                LogPrintf("%s: ehf signal tx %s skipped due to duplicate %d\n",
-                          __func__, iter->GetTx().GetHash().ToString(), *signal);
-                continue;
-            }
-            signals.insert({*signal, 0});
-        }
-
         // We skip mapTx entries that are inBlock, and mapModifiedTx shouldn't
         // contain anything that is inBlock.
         assert(!inBlock.count(iter));
@@ -657,6 +614,41 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
         std::vector<CTxMemPool::txiter> sortedEntries;
         SortForBlock(ancestors, sortedEntries);
 
+        auto packageSignals = signals;
+        std::vector<CTransactionRef> creditPoolTransactions;
+        bool validPackage{true};
+        for (const auto& entry : sortedEntries) {
+            const auto& tx = entry->GetTx();
+            if (std::optional<uint8_t> signal = extractEHFSignal(tx); signal != std::nullopt) {
+                if (!packageSignals.emplace(*signal, 0).second) {
+                    LogPrintf("%s: package tx %s skipped due to duplicate EHF signal %d\n", __func__,
+                              tx.GetHash().ToString(), *signal);
+                    validPackage = false;
+                    break;
+                }
+            }
+            if (tx.IsSpecialTxVersion() && (tx.nType == TRANSACTION_ASSET_LOCK || tx.nType == TRANSACTION_ASSET_UNLOCK)) {
+                creditPoolTransactions.emplace_back(entry->GetSharedTx());
+            }
+        }
+
+        if (validPackage && creditPoolDiff != std::nullopt && !creditPoolTransactions.empty()) {
+            TxValidationState state;
+            if (!creditPoolDiff->ProcessLockUnlockTransactions(creditPoolTransactions, state)) {
+                LogPrintf("%s: package tx %s skipped due to credit pool state: %s\n", __func__,
+                          iter->GetTx().GetHash().ToString(), state.ToString());
+                validPackage = false;
+            }
+        }
+        if (!validPackage) {
+            if (fUsingModified) {
+                mapModifiedTx.get<ancestor_score>().erase(modit);
+                failedTx.insert(iter);
+            }
+            continue;
+        }
+        signals = std::move(packageSignals);
+
         for (size_t i = 0; i < sortedEntries.size(); ++i) {
             AddToBlock(sortedEntries[i]);
             // Erase from the modified set, if present

src/node/miner.h

@@ -36,11 +36,9 @@ namespace Consensus { struct Params; };
 namespace llmq {
 class CInstantSendManager;
 class CQuorumBlockProcessor;
-class CQuorumManager;
 } // namespace llmq
 
 namespace node {
-class BlockManager;
 struct NodeContext;
 
 static const bool DEFAULT_PRINTPRIORITY = false;
@@ -168,7 +166,6 @@ class BlockAssembler
     int nHeight;
     int64_t m_lock_time_cutoff;
 
-    BlockManager& m_blockman;
     CChainstateHelper& m_chain_helper;
     Chainstate& m_chainstate;
     CEvoDB& m_evoDb;
@@ -178,7 +175,6 @@ class BlockAssembler
     const CChainParams& chainparams;
     const CTxMemPool* const m_mempool;
     const llmq::CQuorumBlockProcessor& m_quorum_block_processor;
-    const llmq::CQuorumManager& m_qman;
 
 public:
     struct Options {

src/test/evo_assetlocks_tests.cpp

@@ -8,6 +8,7 @@
 #include <consensus/tx_check.h>
 #include <consensus/validation.h>
 #include <evo/assetlocktx.h>
+#include <evo/creditpool.h>
 #include <evo/specialtx.h>
 #include <llmq/context.h>
 #include <policy/policy.h>
@@ -124,6 +125,16 @@ static CMutableTransaction CreateAssetUnlockTx(FillableSigningProvider& keystore
     return tx;
 }
 
+static CTransactionRef CreateCreditPoolUnlockTx(uint64_t index, CAmount amount)
+{
+    CMutableTransaction tx;
+    tx.nVersion = 3;
+    tx.nType = TRANSACTION_ASSET_UNLOCK;
+    tx.vout.emplace_back(amount, CScript{});
+    SetTxPayload(tx, CAssetUnlockPayload{1, index, 0, 0, {}, {}});
+    return MakeTransactionRef(std::move(tx));
+}
+
 BOOST_FIXTURE_TEST_SUITE(evo_assetlocks_tests, TestChain100Setup)
 
 static void CheckAssetLockCommon(uint8_t version, bool is_v24_active)
@@ -507,4 +518,41 @@ BOOST_FIXTURE_TEST_CASE(evo_assetunlock, TestChain100Setup)
 
 }
 
+BOOST_FIXTURE_TEST_CASE(credit_pool_package_atomicity, TestChain100Setup)
+{
+    LOCK(cs_main);
+    const auto unlock_seven = CreateCreditPoolUnlockTx(1, 7 * COIN);
+    const auto unlock_four = CreateCreditPoolUnlockTx(2, 4 * COIN);
+    const auto* tip = m_node.chainman->ActiveChain().Tip();
+    CCreditPoolDiff diff{CCreditPool{100 * COIN, 10 * COIN}, tip, Params().GetConsensus(), 0};
+
+    TxValidationState package_state;
+    BOOST_CHECK(!diff.ProcessLockUnlockTransactions({unlock_seven, unlock_four}, package_state));
+    BOOST_CHECK_EQUAL(package_state.GetRejectReason(), "failed-creditpool-unlock-too-much");
+    BOOST_CHECK_EQUAL(diff.GetTotalLocked(), 100 * COIN);
+
+    TxValidationState retry_state;
+    BOOST_CHECK(diff.ProcessLockUnlockTransaction(*unlock_seven, retry_state));
+    BOOST_CHECK_EQUAL(diff.GetTotalLocked(), 93 * COIN);
+
+    // A later package's rollback must erase only its own indexes: index 1 was
+    // committed above and must survive the failed package below.
+    const auto unlock_two = CreateCreditPoolUnlockTx(3, 2 * COIN);
+    const auto unlock_dup = CreateCreditPoolUnlockTx(1, COIN);
+    TxValidationState dup_state;
+    BOOST_CHECK(!diff.ProcessLockUnlockTransactions({unlock_two, unlock_dup}, dup_state));
+    BOOST_CHECK_EQUAL(dup_state.GetRejectReason(), "failed-creditpool-unlock-duplicated-index");
+    BOOST_CHECK_EQUAL(diff.GetTotalLocked(), 93 * COIN);
+
+    // index 1 must still be known as used...
+    TxValidationState still_dup_state;
+    BOOST_CHECK(!diff.ProcessLockUnlockTransactions({unlock_dup}, still_dup_state));
+    BOOST_CHECK_EQUAL(still_dup_state.GetRejectReason(), "failed-creditpool-unlock-duplicated-index");
+
+    // ...while index 3 was rolled back and is usable again
+    TxValidationState reuse_state;
+    BOOST_CHECK(diff.ProcessLockUnlockTransactions({unlock_two}, reuse_state));
+    BOOST_CHECK_EQUAL(diff.GetTotalLocked(), 91 * COIN);
+}
+
 BOOST_AUTO_TEST_SUITE_END()

test/functional/feature_asset_locks.py

@@ -748,13 +748,15 @@ def test_v24_fork(self, node_wallet, node, pubkey):
         self.mine_quorum_2_nodes()
         self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': False, 'reject-reason': 'bad-assetunlock-too-old-quorum'})
 
+        self.test_admissible_asset_unlock_ancestor_package(node_wallet, pubkey)
+
         asset_unlock_tx = self.create_assetunlock(620, 4000 * COIN + 1, pubkey)
         txid_in_block = self.send_tx(asset_unlock_tx)
         self.log.info(f"{txid_in_block} should not be mined")
         tip_hash = self.generate(node, 1)[0]
         assert txid_in_block not in node.getblock(tip_hash)['tx']
 
-        asset_unlock_tx = self.create_assetunlock(621, 4000 * COIN, pubkey)
+        asset_unlock_tx = self.create_assetunlock(621, 3999 * COIN, pubkey)
         txid_in_block = self.send_tx(asset_unlock_tx)
         self.log.info(f"{txid_in_block} should be mined")
         tip_hash = self.generate(node, 1)[0]
@@ -766,6 +768,45 @@ def test_v24_fork(self, node_wallet, node, pubkey):
         tip_hash = self.generate(node, 1)[0]
         assert txid_in_block not in node.getblock(tip_hash)['tx']
 
+        self.test_asset_unlock_ancestor_package(node_wallet, asset_unlock_tx, txid_in_block)
+
+    def create_asset_unlock_child(self, node_wallet, asset_unlock_tx, asset_unlock_txid):
+        child_value = Decimal(asset_unlock_tx.vout[0].nValue - tiny_amount) / COIN
+        child_hex = node_wallet.createrawtransaction(
+            [{'txid': asset_unlock_txid, 'vout': 0}],
+            {node_wallet.getnewaddress(): child_value})
+        signed_child = node_wallet.signrawtransactionwithwallet(child_hex)
+        assert signed_child['complete']
+        child_txid = node_wallet.sendrawtransaction(signed_child['hex'])
+
+        # Ensure package selection considers the child before its Asset Unlock
+        # ancestor is considered on its own.
+        node_wallet.prioritisetransaction(child_txid, COIN)
+        return child_txid
+
+    def test_admissible_asset_unlock_ancestor_package(self, node_wallet, pubkey):
+        self.log.info("Test an admissible Asset Unlock ancestor package")
+        asset_unlock_tx = self.create_assetunlock(619, COIN, pubkey)
+        asset_unlock_txid = self.send_tx(asset_unlock_tx)
+        child_txid = self.create_asset_unlock_child(node_wallet, asset_unlock_tx, asset_unlock_txid)
+
+        template_txids = {tx_from_hex(tx['data']).rehash() for tx in node_wallet.getblocktemplate()['transactions']}
+        assert asset_unlock_txid in template_txids
+        assert child_txid in template_txids
+
+        tip_hash = self.generate(node_wallet, 1)[0]
+        mined_txids = node_wallet.getblock(tip_hash)['tx']
+        assert asset_unlock_txid in mined_txids
+        assert child_txid in mined_txids
+
+    def test_asset_unlock_ancestor_package(self, node_wallet, asset_unlock_tx, asset_unlock_txid):
+        self.log.info("Test an Asset Unlock that exceeds the current limit as an ancestor package")
+        child_txid = self.create_asset_unlock_child(node_wallet, asset_unlock_tx, asset_unlock_txid)
+
+        template_txids = {tx_from_hex(tx['data']).rehash() for tx in node_wallet.getblocktemplate()['transactions']}
+        assert asset_unlock_txid not in template_txids
+        assert child_txid not in template_txids
+
     def test_asset_locks_v2_pre_v24(self, node_wallet, node, pubkey):
         self.log.info("Testing asset lock v2 rejection before v24 activation...")
         assert not softfork_active(node_wallet, 'v24')