Spark minting access violation

src/libspark/keys.cpp

@@ -1,5 +1,6 @@
 #include "keys.h"
 #include "../hash.h"
+#include "../support/cleanse.h"
 #include "transcript.h"
 
 namespace spark {
@@ -16,28 +17,29 @@ SpendKey::SpendKey(const Params* params) {
 SpendKey::SpendKey(const Params* params, const Scalar& r_) {
     this->params = params;
     this->r = r_;
-    std::vector<unsigned char> data;
-    data.resize(32);
+    std::vector<unsigned char> data(32);
     r.serialize(data.data());
-    std::vector<unsigned char> result(CSHA256().OUTPUT_SIZE);
+    std::vector<unsigned char> result(CSHA256::OUTPUT_SIZE);
 
     CHash256 hash256;
     std::string prefix1 = "s1_generation";
     hash256.Write(reinterpret_cast<const unsigned char*>(prefix1.c_str()), prefix1.size());
     hash256.Write(data.data(), data.size());
-    hash256.Finalize(&result[0]);
-    this->s1.memberFromSeed(&result[0]);
+    hash256.Finalize(result.data());
+    this->s1.memberFromSeed(result.data());
 
-    data.clear();
-    result.clear();
+    // Reset for s2 generation - use memory_cleanse to securely clear cryptographic material
+    // (memory_cleanse uses OPENSSL_cleanse which is guaranteed not to be optimized away)
+    memory_cleanse(data.data(), data.size());
+    memory_cleanse(result.data(), result.size());
     hash256.Reset();
     s1.serialize(data.data());
 
     std::string prefix2 = "s2_generation";
     hash256.Write(reinterpret_cast<const unsigned char*>(prefix2.c_str()), prefix2.size());
     hash256.Write(data.data(), data.size());
-    hash256.Finalize(&result[0]);
-    this->s2.memberFromSeed(&result[0]);
+    hash256.Finalize(result.data());
+    this->s2.memberFromSeed(result.data());
 }
 
 const Params* SpendKey::get_params() const {
@@ -212,7 +214,10 @@ unsigned char Address::decode(const std::string& str) {
 		throw std::invalid_argument("Bad address encoding");
 	}
 
-	// Check the encoding prefix
+	// Check the hrp length and encoding prefix
+	if (decoded.hrp.size() < 2) {
+		throw std::invalid_argument("Bad address format");
+	}
 	if (decoded.hrp[0] != ADDRESS_ENCODING_PREFIX) {
 		throw std::invalid_argument("Bad address prefix");
 	}

src/miner.cpp

@@ -1108,7 +1108,8 @@ void static FiroMiner(const CChainParams &chainparams) {
         // due to some internal error but also if the keypool is empty.
         // In the latter case, already the pointer is NULL.
         if (!coinbaseScript || coinbaseScript->reserveScript.empty()) {
-            LogPrintf("FiroMiner stop here coinbaseScript=%s, coinbaseScript->reserveScript.empty()=%s\n", coinbaseScript, coinbaseScript->reserveScript.empty());
+            LogPrintf("FiroMiner stop here coinbaseScript=%p, coinbaseScript->reserveScript.empty()=%d\n", 
+                      coinbaseScript.get(), coinbaseScript ? coinbaseScript->reserveScript.empty() : true);
             throw std::runtime_error("No coinbase script available (mining requires a wallet)");
         }
 
@@ -1141,7 +1142,7 @@ void static FiroMiner(const CChainParams &chainparams) {
             unsigned int nTransactionsUpdatedLast = mempool.GetTransactionsUpdated();
             CBlockIndex *pindexPrev = chainActive.Tip();
             if (pindexPrev) {
-                LogPrintf("loop pindexPrev->nHeight=%s\n", pindexPrev->nHeight);
+                LogPrintf("loop pindexPrev->nHeight=%d\n", pindexPrev->nHeight);
             }
             LogPrintf("BEFORE: pblocktemplate\n");
             std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler(Params()).CreateNewBlock(coinbaseScript->reserveScript, {});
@@ -1164,11 +1165,11 @@ void static FiroMiner(const CChainParams &chainparams) {
             arith_uint256 hashTarget = arith_uint256().SetCompact(pblock->nBits);
             LogPrintf("hashTarget: %s\n", hashTarget.ToString());
             LogPrintf("fTestnet: %d\n", fTestNet);
-            LogPrintf("pindexPrev->nHeight: %s\n", pindexPrev->nHeight);
+            LogPrintf("pindexPrev->nHeight: %d\n", pindexPrev->nHeight);
             LogPrintf("pblock: %s\n", pblock->ToString());
-            LogPrintf("pblock->nVersion: %s\n", pblock->nVersion);
-            LogPrintf("pblock->nTime: %s\n", pblock->nTime);
-            LogPrintf("pblock->nNonce: %s\n", &pblock->nNonce);
+            LogPrintf("pblock->nVersion: %d\n", pblock->nVersion);
+            LogPrintf("pblock->nTime: %u\n", pblock->nTime);
+            LogPrintf("pblock->nNonce: %u\n", pblock->nNonce);
             LogPrintf("powLimit: %s\n", Params().GetConsensus().powLimit.ToString());
 
             while (true) {

src/spark/sparkwallet.cpp

@@ -805,9 +805,14 @@ bool CSparkWallet::CreateSparkMintTransactions(
     wtxNew.BindWallet(pwalletMain);
 
     CMutableTransaction txNew;
-    txNew.nLockTime = chainActive.Height();
+    int nHeight = 0;
+    {
+        LOCK(cs_main);
+        nHeight = chainActive.Height();
+    }
+    txNew.nLockTime = nHeight;
 
-    assert(txNew.nLockTime <= (unsigned int) chainActive.Height());
+    assert(txNew.nLockTime <= static_cast<unsigned int>(nHeight));
     assert(txNew.nLockTime < LOCKTIME_THRESHOLD);
     std::vector<spark::MintedCoinData> outputs_ = outputs;
     CAmount valueToMint = 0;
@@ -844,8 +849,8 @@ bool CSparkWallet::CreateSparkMintTransactions(
                 if (GetRandInt(10) == 0)
                     tx.nLockTime = std::max(0, (int) tx.nLockTime - GetRandInt(100));
 
-                auto nFeeRet = 0;
-                LogPrintf("nFeeRet=%s\n", nFeeRet);
+                CAmount nFeeRet = 0;
+                LogPrintf("nFeeRet=%d\n", nFeeRet);
 
                 auto itr = valueAndUTXO.begin();
 
@@ -898,7 +903,7 @@ bool CSparkWallet::CreateSparkMintTransactions(
                         singleTxOutputs.push_back(mintedCoinData);
                     } else {
                         uint64_t remainingMintValue = mintedValue;
-                        while (remainingMintValue > 0){
+                        while (remainingMintValue > 0 && !remainingOutputs.empty()) {
                             // Create the mint data and push into vector
                             uint64_t singleMintValue = std::min(remainingMintValue, remainingOutputs.begin()->v);
                             spark::MintedCoinData mintedCoinData;
@@ -917,23 +922,23 @@ bool CSparkWallet::CreateSparkMintTransactions(
                     }
 
                     if (subtractFeeFromAmount) {
-                        CAmount singleFee = nFeeRet / singleTxOutputs.size();
-                        CAmount reminder = nFeeRet % singleTxOutputs.size();
+                        if (singleTxOutputs.empty()) {
+                            strFailReason = _("Transaction amount too small");
+                            return false;
+                        }
+                        const CAmount outputCount = static_cast<CAmount>(singleTxOutputs.size());
+                        const CAmount singleFee = nFeeRet / outputCount;
+                        const CAmount remainder = nFeeRet % outputCount;
                         for (size_t i = 0; i < singleTxOutputs.size(); ++i) {
-                            if (cmp::less_equal(singleTxOutputs[i].v, singleFee)) {
-                                singleTxOutputs.erase(singleTxOutputs.begin() + i);
-                                reminder += singleTxOutputs[i].v - singleFee;
-                                if (!singleTxOutputs.size()) {
-                                    strFailReason = _("Transaction amount too small");
-                                    return false;
-                                }
-                                --i;
+                            CAmount feeToSubtract = singleFee;
+                            if (i == 0) {
+                                feeToSubtract += remainder;
                             }
-                            singleTxOutputs[i].v -= singleFee;
-                            if (reminder > 0 && singleTxOutputs[i].v > nFeeRet % singleTxOutputs.size()) {// first receiver pays the remainder not divisible by output count
-                                singleTxOutputs[i].v -= reminder;
-                                reminder = 0;
+                            if (cmp::less_equal(singleTxOutputs[i].v, feeToSubtract)) {
+                                strFailReason = _("Transaction amount too small");
+                                return false;
                             }
+                            singleTxOutputs[i].v -= feeToSubtract;
                         }
                     }
 
@@ -954,9 +959,10 @@ bool CSparkWallet::CreateSparkMintTransactions(
                     // Choose coins to use
                     CAmount nValueIn = 0;
                     if (!pwalletMain->SelectCoins(itr->second, nValueToSelect, setCoins, nValueIn, coinControl)) {
-
                         if (nValueIn < nValueToSelect) {
                             strFailReason = _("Insufficient funds");
+                        } else {
+                            strFailReason = _("Unable to select coins for minting");
                         }
                         return false;
                     }
@@ -990,7 +996,7 @@ bool CSparkWallet::CreateSparkMintTransactions(
                             // send change to one of the specified change addresses
                         else if (IsArgSet("-change") && mapMultiArgs.at("-change").size() > 0) {
                             CBitcoinAddress address(
-                                    mapMultiArgs.at("change")[GetRandInt(mapMultiArgs.at("-change").size())]);
+                                    mapMultiArgs.at("-change")[GetRandInt(mapMultiArgs.at("-change").size())]);
                             CKeyID keyID;
                             if (!address.GetKeyID(keyID)) {
                                 strFailReason = _("Bad change address");
@@ -1204,7 +1210,7 @@ bool CSparkWallet::CreateSparkMintTransactions(
                 {
                     CValidationState state;
                     if (!mempool.IsTransactionAllowed(*wtx.tx, state)) {
-                        strFailReason = _("Signing transaction failed");
+                        strFailReason = _("Transaction not allowed in mempool");
                         return false;
                     }
                 }
@@ -1223,12 +1229,14 @@ bool CSparkWallet::CreateSparkMintTransactions(
 
                     bool added = false;
                     for (auto &utxos : valueAndUTXO) {
+                        if (utxos.second.empty())
+                            continue;
                         auto const &o = utxos.second.front();
                         if (o.tx->tx->vout[o.i].scriptPubKey == wtx.tx->vout[nChangePosInOut].scriptPubKey) {
                             utxos.first += val;
                             utxos.second.push_back(out);
-
                             added = true;
+                            break;
                         }
                     }
 
@@ -1248,6 +1256,7 @@ bool CSparkWallet::CreateSparkMintTransactions(
     }
 
     if (!autoMintAll && valueToMint > 0) {
+        strFailReason = _("Unable to mint full amount; only partial minting was possible");
         return false;
     }
 
@@ -1309,7 +1318,7 @@ CWalletTx CSparkWallet::CreateSparkSpendTransaction(
         }
     }
 
-    int nHeight;
+    int nHeight = 0;
     {
         LOCK(cs_main);
         nHeight = chainActive.Height();
@@ -1346,7 +1355,7 @@ CWalletTx CSparkWallet::CreateSparkSpendTransaction(
     // enough, that fee sniping isn't a problem yet, but by implementing a fix
     // now we ensure code won't be written that makes assumptions about
     // nLockTime that preclude a fix later.
-    tx.nLockTime = chainActive.Height();
+    tx.nLockTime = nHeight;
 
     // Secondly occasionally randomly pick a nLockTime even further back, so
     // that transactions that are delayed after signing for whatever reason,
@@ -1356,53 +1365,59 @@ CWalletTx CSparkWallet::CreateSparkSpendTransaction(
         tx.nLockTime = std::max(0, static_cast<int>(tx.nLockTime) - GetRandInt(100));
     }
 
-    assert(tx.nLockTime <= static_cast<unsigned>(chainActive.Height()));
+    assert(tx.nLockTime <= static_cast<unsigned>(nHeight));
     assert(tx.nLockTime < LOCKTIME_THRESHOLD);
-    std::list<CSparkMintMeta> coins = GetAvailableSparkCoins(coinControl);
-
-    std::pair<CAmount, std::vector<CSparkMintMeta>> estimated =
-            SelectSparkCoins(vOut + mintVOut, recipientsToSubtractFee, coins, privateRecipients.size(), recipients.size(), coinControl, additionalTxSize);
-
     std::vector<CRecipient> recipients_ = recipients;
     std::vector<std::pair<spark::OutputCoinData, bool>> privateRecipients_ = privateRecipients;
     {
-        bool remainderSubtracted = false;
-        fee = estimated.first;
-        for (size_t i = 0; i < recipients_.size(); i++) {
-            auto &recipient = recipients_[i];
-
-            if (recipient.fSubtractFeeFromAmount) {
-                // Subtract fee equally from each selected recipient.
-                recipient.nAmount -= fee / recipientsToSubtractFee;
-
-                if (!remainderSubtracted) {
-                    // First receiver pays the remainder not divisible by output count.
-                    recipient.nAmount -= fee % recipientsToSubtractFee;
+        LOCK2(cs_main, pwalletMain->cs_wallet);
+        {
+            std::list<CSparkMintMeta> coins = GetAvailableSparkCoins(coinControl);
+            std::pair<CAmount, std::vector<CSparkMintMeta>> estimated =
+                    SelectSparkCoins(vOut + mintVOut, recipientsToSubtractFee, coins, privateRecipients.size(), recipients.size(), coinControl, additionalTxSize);
+
+            bool remainderSubtracted = false;
+            fee = estimated.first;
+            const CAmount feePerRecipient = recipientsToSubtractFee > 0 ? fee / recipientsToSubtractFee : 0;
+            const CAmount feeRemainder = recipientsToSubtractFee > 0 ? fee % recipientsToSubtractFee : 0;
+            for (size_t i = 0; i < recipients_.size(); i++) {
+                auto &recipient = recipients_[i];
+
+                if (recipient.fSubtractFeeFromAmount) {
+                    CAmount feeToSubtract = feePerRecipient;
+                    if (!remainderSubtracted) {
+                        // First receiver pays the remainder not divisible by output count.
+                        feeToSubtract += feeRemainder;
+                    }
+                    if (cmp::less_equal(recipient.nAmount, feeToSubtract)) {
+                        throw std::runtime_error(boost::str(
+                                boost::format(_("Amount for recipient %1% is too small to send after the fee has been deducted")) % i));
+                    }
+                    // Subtract fee equally from each selected recipient.
+                    recipient.nAmount -= feeToSubtract;
                     remainderSubtracted = true;
                 }
             }
-        }
 
-        for (size_t i = 0; i < privateRecipients_.size(); i++) {
-            auto &privateRecipient = privateRecipients_[i];
-
-            if (privateRecipient.second) {
-                // Subtract fee equally from each selected recipient.
-                privateRecipient.first.v -= fee / recipientsToSubtractFee;
+            for (size_t i = 0; i < privateRecipients_.size(); i++) {
+                auto &privateRecipient = privateRecipients_[i];
 
-                if (!remainderSubtracted) {
-                    // First receiver pays the remainder not divisible by output count.
-                    privateRecipient.first.v -= fee % recipientsToSubtractFee;
+                if (privateRecipient.second) {
+                    CAmount feeToSubtract = feePerRecipient;
+                    if (!remainderSubtracted) {
+                        // First receiver pays the remainder not divisible by output count.
+                        feeToSubtract += feeRemainder;
+                    }
+                    if (cmp::less_equal(privateRecipient.first.v, feeToSubtract)) {
+                        throw std::runtime_error(boost::str(
+                                boost::format(_("Amount for private recipient %1% is too small to send after the fee has been deducted")) % i));
+                    }
+                    // Subtract fee equally from each selected recipient.
+                    privateRecipient.first.v -= feeToSubtract;
                     remainderSubtracted = true;
                 }
             }
-        }
-
-    }
 
-    {
-        LOCK2(cs_main, pwalletMain->cs_wallet);
-        {
             const spark::Params* params = spark::Params::get_default();
             spark::CSparkState *sparkState = spark::CSparkState::GetState();
             spark::SpendKey spendKey(params);

src/txmempool.cpp

@@ -519,7 +519,7 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
     NotifyEntryRemoved(it->GetSharedTx(), reason);
     const uint256 hash = it->GetTx().GetHash();
     if (!it->GetTx().HasPrivateInputs()) {
-        LogPrintf("removeUnchecked txHash=%s, IsSpend()=%s\n", hash.ToString(), it->GetTx().HasPrivateInputs());
+        LogPrintf("removeUnchecked txHash=%s (no private inputs)\n", hash.ToString());
         BOOST_FOREACH(const CTxIn& txin, it->GetTx().vin)
             mapNextTx.erase(txin.prevout);
     }