V72 418 regression fixes

src/chainstate.cpp

@@ -303,7 +303,7 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
                         continue;
                     COutPoint prevout = tx.vin[j].prevout;
                     if (ColorIdentifier(prevout, outColorId.type) == outColorId) {
-                        g_colorid_state->Erase(outColorId);
+                        if (!fDryRun) g_colorid_state->Erase(outColorId);
                         break;
                     }
                 }
@@ -664,6 +664,18 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
             if (!CheckInputs(tx, state, view, fScriptChecks, GetBlockScriptFlags(pindex), fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr, GetBlockScriptFlags(pindex)))
                 // FormatStateMessage is evaluated before DoS() overwrites state, preserving
                 // the per-input detail in the log while enforcing DoS 100 at the block level.
+                //
+                // DoS scoring asymmetry (informational, not a bug):
+                // - Multi-thread (nScriptCheckThreads > 0): script checks are deferred to
+                //   control.Wait(); CheckInputs returns false without entering the per-check
+                //   loop, so the inner DoS(100) in validation.cpp is never reached.  This
+                //   outer DoS(100) is the only one applied — nDoS ends at 100.
+                // - Single-thread (nScriptCheckThreads == 0): CheckInputs executes inline
+                //   and its inner DoS(100) fires first, then this outer DoS(100) adds a
+                //   further 100, landing at nDoS=200.  Both 100 and 200 trigger a peer ban,
+                //   so the behaviour difference is benign.  The inner DoS also serves the
+                //   mempool path (AcceptToMemoryPool) which has no outer scoring of its own,
+                //   which is why it cannot simply be removed.
                 return state.DoS(100, error("ConnectBlock(): CheckInputs on %s failed with %s",
                     tx.GetHashMalFix().ToString(), FormatStateMessage(state)),
                     REJECT_INVALID, "mandatory-script-verify-flag-failed");
@@ -678,7 +690,11 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
         if (!tx.IsCoinBase()) {
             std::set<ColorIdentifier> newIssuances;
             if (!VerifyTokenBalances(tx, state, view, txfee, !fJustCheck ? &newIssuances : nullptr, pindex->nHeight))
-                return false;
+                // FormatStateMessage is evaluated before DoS() overwrites state, preserving
+                // the per-tx detail in the log while enforcing DoS 100 at the block level.
+                return state.DoS(100, error("ConnectBlock(): VerifyTokenBalances on %s failed with %s",
+                    tx.GetHashMalFix().ToString(), FormatStateMessage(state)),
+                    REJECT_INVALID, "bad-txns-token-balance");
             allNewIssuances.insert(newIssuances.begin(), newIssuances.end());
         }
 
@@ -855,22 +871,28 @@ bool CChainState::ConnectTip(CValidationState& state, CBlockIndex* pindexNew, co
             return error("ConnectTip(): ConnectBlock %s failed", pindexNew->GetBlockHash().ToString());
         }
 
-        // if the block was added successfully and it is a federation block,
-        // make sure that the xfield from this block is added to xFieldHistory
+        // Evaluate the xfield condition and build the change value before Flush.
+        // The actual writes (in-memory history + DB) are deferred until after
+        // view.Flush() so the UTXO commit lands first — matching the ordering
+        // used by DisconnectTip and giving a forward-recoverable crash window.
         CXFieldHistory xfieldHistory;
-        if(blockConnecting.xfield.IsValid()
+        bool hasNewXField = blockConnecting.xfield.IsValid()
             && pindexNew->nHeight > 0
-            && IsXFieldNew(blockConnecting.xfield, &xfieldHistory, static_cast<uint32_t>(pindexNew->nHeight)))
-        {
-            XFieldChange newChange(blockConnecting.xfield.xfieldValue, pindexNew->nHeight + 1, blockConnecting.GetHash());
-            xfieldHistory.Add(blockConnecting.xfield.xfieldType, newChange);
-            pblocktree->WriteXField(newChange);
-        }
+            && IsXFieldNew(blockConnecting.xfield, &xfieldHistory, static_cast<uint32_t>(pindexNew->nHeight));
+        XFieldChange newChange;
+        if (hasNewXField)
+            newChange = XFieldChange(blockConnecting.xfield.xfieldValue, pindexNew->nHeight + 1, blockConnecting.GetHash());
 
         nTime3 = GetTimeMicros(); nTimeConnectTotal += nTime3 - nTime2;
         LogPrint(BCLog::BENCH, "  - Connect total: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime3 - nTime2) * MILLI, nTimeConnectTotal * MICRO, nTimeConnectTotal * MILLI / nBlocksTotal);
         bool flushed = view.Flush();
         assert(flushed);
+
+        // Write xfield after UTXO flush: matches DisconnectTip's ordering.
+        if (hasNewXField) {
+            xfieldHistory.Add(blockConnecting.xfield.xfieldType, newChange);
+            pblocktree->WriteXField(newChange);
+        }
     }
 
     int64_t nTime4 = GetTimeMicros(); nTimeFlush += nTime4 - nTime3;

src/init.cpp

@@ -1133,6 +1133,8 @@ bool AppInitParameterInteraction()
         int64_t nArg = gArgs.GetArg("-datacarriersize", (int64_t)nMaxDatacarrierBytes);
         if (nArg < 0)
             return InitError(_("-datacarriersize cannot be configured with a negative value."));
+        if (nArg > std::numeric_limits<unsigned int>::max())
+            return InitError(_("-datacarriersize must be less than 4294967296."));
         nMaxDatacarrierBytes = (unsigned int)nArg;
     }
     fAcceptMultipleDatacarrier = gArgs.GetBoolArg("-datacarriermultiple", DEFAULT_ACCEPT_MULTIPLE_DATACARRIER);

src/script/sign.h

@@ -167,6 +167,12 @@ void DeserializeHDKeypaths(Stream& s, const std::vector<unsigned char>& key, std
 
     // Read in key path
     uint64_t value_len = ReadCompactSize(s);
+    if (value_len < sizeof(uint32_t)) {
+        throw std::ios_base::failure("HD keypath must contain at least a 4-byte fingerprint");
+    }
+    if (value_len % sizeof(uint32_t) != 0) {
+        throw std::ios_base::failure("HD keypath length is not a multiple of 4");
+    }
     std::vector<uint32_t> keypath;
     for (unsigned int i = 0; i < value_len; i += sizeof(uint32_t)) {
         uint32_t index;

src/test/chainstate_tests.cpp

@@ -27,6 +27,10 @@
 #include <file_io.h>
 #include <xfieldhistory.h>
 #include <key.h>
+#include <coloridentifier.h>
+#include <issuedcolorids.h>
+#include <script/interpreter.h>
+#include <hash.h>
 
 #include <boost/test/unit_test.hpp>
 
@@ -715,4 +719,220 @@ BOOST_AUTO_TEST_CASE(check_block_header_orphan_uses_latest_aggpubkey)
     }
 }
 
+/**
+ * Regression test: DisconnectBlock(fDryRun=true) must not erase from g_colorid_state.
+ *
+ * fDryRun was declared in the signature but never checked in the body, so
+ * g_colorid_state->Erase() ran unconditionally.  CVerifyDB calls DisconnectBlock
+ * with fDryRun=true during its level-3 walk; without the fix this corrupted the
+ * live colorId set whenever verifychain was run on a chain that contained
+ * NON_REISSUABLE or NFT issuances.
+ *
+ * The test bypasses the CVerifyDB sandbox so it fails without the one-line fix
+ * and passes after it.
+ */
+BOOST_AUTO_TEST_CASE(disconnect_block_dry_run_preserves_colorid_state)
+{
+    CScript payTo = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
+
+    // Key for a P2PKH intermediate output.
+    CKey key;
+    const unsigned char vchKeyBytes[32] = {
+        1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0
+    };
+    key.Set(vchKeyBytes, vchKeyBytes + 32, true);
+    CPubKey pubkey = key.GetPubKey();
+    std::vector<unsigned char> vchPubKey(pubkey.begin(), pubkey.end());
+    std::vector<unsigned char> pubkeyHash(20);
+    CHash160().Write(pubkey.data(), pubkey.size()).Finalize(pubkeyHash.data());
+
+    // Block 6: spend coinbase[0] into a plain TPC P2PKH output.
+    CMutableTransaction spendTx;
+    spendTx.nFeatures = 1;
+    spendTx.vin.resize(1);
+    spendTx.vout.resize(1);
+    spendTx.vin[0].prevout.hashMalFix = m_coinbase_txns[0]->GetHashMalFix();
+    spendTx.vin[0].prevout.n = 0;
+    spendTx.vout[0].nValue = 100 * CENT;
+    spendTx.vout[0].scriptPubKey = CScript() << OP_DUP << OP_HASH160
+                                             << ToByteVector(pubkeyHash)
+                                             << OP_EQUALVERIFY << OP_CHECKSIG;
+    {
+        std::vector<unsigned char> vchSig;
+        uint256 sigHash = SignatureHash(
+            m_coinbase_txns[0]->vout[0].scriptPubKey, spendTx, 0, SIGHASH_ALL, 0, SigVersion::BASE);
+        coinbaseKey.Sign_Schnorr(sigHash, vchSig);
+        vchSig.push_back(SIGHASH_ALL);
+        spendTx.vin[0].scriptSig = CScript() << vchSig;
+    }
+    CreateAndProcessBlock({spendTx}, payTo);
+
+    // Block 7: issue a NON_REISSUABLE token from the P2PKH UTXO.
+    COutPoint utxo(spendTx.GetHashMalFix(), 0);
+    ColorIdentifier colorid(utxo, TokenTypes::NON_REISSUABLE);
+    CScript colorScript = CScript() << colorid.toVector() << OP_COLOR
+                                    << OP_DUP << OP_HASH160
+                                    << ToByteVector(pubkeyHash)
+                                    << OP_EQUALVERIFY << OP_CHECKSIG;
+
+    CMutableTransaction issueTx;
+    issueTx.nFeatures = 1;
+    issueTx.vin.resize(1);
+    issueTx.vout.resize(1);
+    issueTx.vin[0].prevout.hashMalFix = spendTx.GetHashMalFix();
+    issueTx.vin[0].prevout.n = 0;
+    issueTx.vout[0].nValue = 50 * CENT;
+    issueTx.vout[0].scriptPubKey = colorScript;
+    {
+        std::vector<unsigned char> vchSig;
+        uint256 sigHash = SignatureHash(
+            spendTx.vout[0].scriptPubKey, issueTx, 0, SIGHASH_ALL, 0, SigVersion::BASE);
+        key.Sign_Schnorr(sigHash, vchSig);
+        vchSig.push_back(SIGHASH_ALL);
+        issueTx.vin[0].scriptSig = CScript() << vchSig << vchPubKey;
+    }
+    CBlock issueBlock = CreateAndProcessBlock({issueTx}, payTo);
+
+    // ConnectBlock must have recorded the colorId in g_colorid_state.
+    {
+        LOCK(cs_main);
+        BOOST_REQUIRE(g_colorid_state && g_colorid_state->IsIssued(colorid));
+    }
+
+    // Call DisconnectBlock(fDryRun=true) directly — no CVerifyDB sandbox in place.
+    // Before the fix, this called g_colorid_state->Erase(colorid) unconditionally.
+    {
+        LOCK(cs_main);
+        CBlockIndex* pindex = chainActive.Tip();
+        BOOST_REQUIRE(pindex->GetBlockHash() == issueBlock.GetHash());
+
+        CCoinsViewCache coins(pcoinsTip.get());
+        DisconnectResult res = g_chainstate.DisconnectBlock(
+            issueBlock, pindex, coins, /*fDryRun=*/true);
+        BOOST_CHECK(res != DISCONNECT_FAILED);
+    }
+
+    // g_colorid_state must be unchanged: the colorId survives the dry-run disconnect.
+    {
+        LOCK(cs_main);
+        BOOST_CHECK(g_colorid_state->IsIssued(colorid));
+    }
+}
+
+/**
+ * Regression test: duplicate NON_REISSUABLE issuance is rejected by
+ * CheckColorIdentifierValidity via g_colorid_state->IsIssued().
+ *
+ * Once colorId C is confirmed in g_colorid_state, any further issuance of C
+ * must be blocked even if the defining TPC outpoint appears unspent in the
+ * coins view (e.g. after a reorg that restores the UTXO while a
+ * DisconnectBlock bug — such as H-2 — leaves C in the confirmed set).
+ *
+ * Because getting two distinct outpoints to derive the same NON_REISSUABLE
+ * colorId requires a SHA-256 collision, we exercise the code path directly:
+ * the first issuance goes through the normal block pipeline (populating
+ * g_colorid_state), then we construct a synthetic CCoinsViewCache that
+ * presents the defining TPC outpoint as unspent again and call
+ * CheckColorIdentifierValidity on a re-issuance transaction.
+ */
+BOOST_AUTO_TEST_CASE(duplicate_nonreissuable_issuance_rejected)
+{
+    CScript payTo = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
+
+    CKey key;
+    const unsigned char vchKeyBytes[32] = {
+        2,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0
+    };
+    key.Set(vchKeyBytes, vchKeyBytes + 32, true);
+    CPubKey pubkey = key.GetPubKey();
+    std::vector<unsigned char> vchPubKey(pubkey.begin(), pubkey.end());
+    std::vector<unsigned char> pubkeyHash(20);
+    CHash160().Write(pubkey.data(), pubkey.size()).Finalize(pubkeyHash.data());
+
+    // Block 6: spend coinbase[1] into a plain TPC P2PKH output.
+    CMutableTransaction spendTx;
+    spendTx.nFeatures = 1;
+    spendTx.vin.resize(1);
+    spendTx.vout.resize(1);
+    spendTx.vin[0].prevout.hashMalFix = m_coinbase_txns[1]->GetHashMalFix();
+    spendTx.vin[0].prevout.n = 0;
+    spendTx.vout[0].nValue = 100 * CENT;
+    spendTx.vout[0].scriptPubKey = CScript() << OP_DUP << OP_HASH160
+                                             << ToByteVector(pubkeyHash)
+                                             << OP_EQUALVERIFY << OP_CHECKSIG;
+    {
+        std::vector<unsigned char> vchSig;
+        uint256 sigHash = SignatureHash(
+            m_coinbase_txns[1]->vout[0].scriptPubKey, spendTx, 0, SIGHASH_ALL, 0, SigVersion::BASE);
+        coinbaseKey.Sign_Schnorr(sigHash, vchSig);
+        vchSig.push_back(SIGHASH_ALL);
+        spendTx.vin[0].scriptSig = CScript() << vchSig;
+    }
+    CreateAndProcessBlock({spendTx}, payTo);
+
+    // Block 7: issue a NON_REISSUABLE token from the P2PKH UTXO.
+    COutPoint definingUtxo(spendTx.GetHashMalFix(), 0);
+    ColorIdentifier colorid(definingUtxo, TokenTypes::NON_REISSUABLE);
+    CScript colorScript = CScript() << colorid.toVector() << OP_COLOR
+                                    << OP_DUP << OP_HASH160
+                                    << ToByteVector(pubkeyHash)
+                                    << OP_EQUALVERIFY << OP_CHECKSIG;
+
+    CMutableTransaction issueTx;
+    issueTx.nFeatures = 1;
+    issueTx.vin.resize(1);
+    issueTx.vout.resize(1);
+    issueTx.vin[0].prevout = definingUtxo;
+    issueTx.vout[0].nValue = 50 * CENT;
+    issueTx.vout[0].scriptPubKey = colorScript;
+    {
+        std::vector<unsigned char> vchSig;
+        uint256 sigHash = SignatureHash(
+            spendTx.vout[0].scriptPubKey, issueTx, 0, SIGHASH_ALL, 0, SigVersion::BASE);
+        key.Sign_Schnorr(sigHash, vchSig);
+        vchSig.push_back(SIGHASH_ALL);
+        issueTx.vin[0].scriptSig = CScript() << vchSig << vchPubKey;
+    }
+    CreateAndProcessBlock({issueTx}, payTo);
+
+    // colorId is now in the confirmed g_colorid_state; definingUtxo is spent.
+    {
+        LOCK(cs_main);
+        BOOST_REQUIRE(g_colorid_state && g_colorid_state->IsIssued(colorid));
+    }
+
+    // Build a synthetic CCoinsViewCache that presents definingUtxo as unspent,
+    // simulating a reorg that restored the UTXO while a DisconnectBlock bug
+    // (H-2) left the colorId in g_colorid_state.
+    // CheckColorIdentifierValidity must reject the re-issuance with
+    // "bad-txns-colorid-already-issued".
+    {
+        LOCK(cs_main);
+
+        CCoinsView dummy;
+        CCoinsViewCache syntheticView(&dummy);
+        Coin tpcCoin;
+        tpcCoin.out.nValue    = spendTx.vout[0].nValue;
+        tpcCoin.out.scriptPubKey = spendTx.vout[0].scriptPubKey;
+        tpcCoin.nHeight       = 6;
+        tpcCoin.fCoinBase     = false;
+        syntheticView.AddCoin(definingUtxo, std::move(tpcCoin), /*potential_overwrite=*/false);
+
+        CMutableTransaction reissueTx;
+        reissueTx.nFeatures = 1;
+        reissueTx.vin.resize(1);
+        reissueTx.vout.resize(1);
+        reissueTx.vin[0].prevout   = definingUtxo;
+        reissueTx.vout[0].nValue   = 50 * CENT;
+        reissueTx.vout[0].scriptPubKey = colorScript;
+
+        CValidationState state;
+        bool valid = CheckColorIdentifierValidity(
+            CTransaction(reissueTx), state, syntheticView, chainActive.Tip()->nHeight);
+
+        BOOST_CHECK(!valid);
+        BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-txns-colorid-already-issued");
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/txmempool.cpp

@@ -617,6 +617,11 @@ void CTxMemPool::removeForScriptFlagChange(unsigned int newMempoolScriptFlags, i
             continue;
         CValidationState state;
         PrecomputedTransactionData txdata(tx);
+        // For SCRIPT_VERIFY_CP2SH_COLORED this call is a no-op: the flag is
+        // already in STANDARD_SCRIPT_VERIFY_FLAGS, so every tx in the mempool
+        // was admitted under it and will pass here.  For any future softfork
+        // whose flag is NOT pre-added to STANDARD, this correctly evicts txs
+        // that would be invalid under the new consensus rules.
         if (!CheckInputs(tx, state, view, true, newMempoolScriptFlags, false, false, txdata)) {
             txToRemove.insert(it);
             continue;

src/verifydb.cpp

@@ -89,10 +89,12 @@ bool CVerifyDB::VerifyDB(CCoinsView *coinsview, int nCheckLevel, int nCheckDepth
         // check level 3: check for inconsistencies during memory-only disconnect of tip blocks
         if (nCheckLevel >= 3 && (coins.DynamicMemoryUsage() + pcoinsTip->DynamicMemoryUsage()) <= nCoinCacheUsage) {
             assert(coins.GetBestBlock() == pindex->GetBlockHash());
-            // fDryRun=true: this is a view-only sandbox — do not mutate g_issued_colorids
-            // or the chainstate DB.  chainActive stays at tip; level 4 reconnects via
-            // ConnectBlock which will restore state, but only when -checklevel=4 is set.
-            DisconnectResult res = g_chainstate.DisconnectBlock(block, pindex, coins, /*fDryRun=*/true);
+            // fDryRun=false: the ColorIdSandbox above already swapped in a clone of
+            // g_colorid_state, so DisconnectBlock erases from the clone, not from the
+            // live confirmed set.  Erasing into the clone is intentional: level 4
+            // reconnects via ConnectBlock starting from this sandbox state, which
+            // requires the colorIds to have been removed so ConnectBlock can re-add them.
+            DisconnectResult res = g_chainstate.DisconnectBlock(block, pindex, coins, /*fDryRun=*/false);
             if (res == DISCONNECT_FAILED) {
                 return error("VerifyDB(): *** irrecoverable inconsistency in block data at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
             }