From 31ea70682020815815b337b238921263e2254388 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 27 Jan 2026 10:20:28 +0000 Subject: [PATCH 01/10] Fix format specifier crashes in spark minting and other logging This commit includes the PR #1766 fixes plus additional format specifier corrections to prevent EXCEPTION_ACCESS_VIOLATION crashes: Spark minting fixes (from PR #1766): - Fix nFeeRet format specifier from %s to %d in sparkwallet.cpp - Change nFeeRet type from auto to CAmount for proper formatting - Fix nFeeRet format specifier in wallet.cpp CreateLelantusMintTransactions - Add locking for chainActive.Height() access - Rework fee subtraction logic to prevent underflows - Add validation for fee subtraction in spark spend Additional format specifier fixes: - Fix payTxFee.GetFeePerK()=%s to %d in wallet.cpp (2 locations) - Fix listOwnCoins.size()=%s to %zu in wallet.cpp - Fix CheckFinalTx and IsTrusted boolean format from %s to %d - Fix nDepth=%s to %d in wallet.cpp - Fix pcoin->tx->vout.size()=%s to %zu in wallet.cpp - Fix nHeight format specifiers in validation.cpp (6 locations) - Fix isVerifyDB format from %s to %d in validation.cpp - Fix coinbaseScript pointer and boolean format in miner.cpp - Fix HasPrivateInputs boolean format in txmempool.cpp - Fix vecOutputs.size() and coins.size() in rpcwallet.cpp The root cause was using %s format specifier with non-string types (int, size_t, bool, CAmount), causing tinyformat to interpret integer values as memory addresses, leading to access violations. Co-authored-by: reuben --- src/miner.cpp | 4 +- src/spark/sparkwallet.cpp | 91 ++++++++++++++++++++++----------------- src/txmempool.cpp | 2 +- src/validation.cpp | 12 +++--- src/wallet/rpcwallet.cpp | 4 +- src/wallet/wallet.cpp | 36 ++++++++++------ 6 files changed, 85 insertions(+), 64 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index 661e959ebd..f73fae973e 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -1108,7 +1108,7 @@ 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, coinbaseScript->reserveScript.empty()); throw std::runtime_error("No coinbase script available (mining requires a wallet)"); } @@ -1141,7 +1141,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 pblocktemplate = BlockAssembler(Params()).CreateNewBlock(coinbaseScript->reserveScript, {}); diff --git a/src/spark/sparkwallet.cpp b/src/spark/sparkwallet.cpp index 27587ea6b8..55992dcb0d 100644 --- a/src/spark/sparkwallet.cpp +++ b/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(nHeight)); assert(txNew.nLockTime < LOCKTIME_THRESHOLD); std::vector 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(); @@ -1309,7 +1314,7 @@ CWalletTx CSparkWallet::CreateSparkSpendTransaction( } } - int nHeight; + int nHeight = 0; { LOCK(cs_main); nHeight = chainActive.Height(); @@ -1346,7 +1351,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 +1361,59 @@ CWalletTx CSparkWallet::CreateSparkSpendTransaction( tx.nLockTime = std::max(0, static_cast(tx.nLockTime) - GetRandInt(100)); } - assert(tx.nLockTime <= static_cast(chainActive.Height())); + assert(tx.nLockTime <= static_cast(nHeight)); assert(tx.nLockTime < LOCKTIME_THRESHOLD); - std::list coins = GetAvailableSparkCoins(coinControl); - - std::pair> estimated = - SelectSparkCoins(vOut + mintVOut, recipientsToSubtractFee, coins, privateRecipients.size(), recipients.size(), coinControl, additionalTxSize); - std::vector recipients_ = recipients; std::vector> 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 coins = GetAvailableSparkCoins(coinControl); + std::pair> 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); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index a84627be56..7043ae7a57 100644 --- a/src/txmempool.cpp +++ b/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, IsSpend()=%d\n", hash.ToString(), it->GetTx().HasPrivateInputs()); BOOST_FOREACH(const CTxIn& txin, it->GetTx().vin) mapNextTx.erase(txin.prevout); } diff --git a/src/validation.cpp b/src/validation.cpp index 030eedeb8e..3f89eea07d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2707,7 +2707,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin int64_t nTimeStart = GetTimeMicros(); //btzc: update nHeight, isVerifyDB // Check it again in case a previous version let a bad block in - LogPrintf("ConnectBlock nHeight=%s, hash=%s\n", pindex->nHeight, block.GetHash().ToString()); + LogPrintf("ConnectBlock nHeight=%d, hash=%s\n", pindex->nHeight, block.GetHash().ToString()); if (!CheckBlock(block, state, chainparams.GetConsensus(), !fJustCheck, !fJustCheck, pindex->nHeight, false)) { LogPrintf("--> failed\n"); return error("%s: Consensus::CheckBlock: %s", __func__, FormatStateMessage(state)); @@ -3390,7 +3390,7 @@ void PruneAndFlush() { /** Update chainActive and related internal data structures. */ void static UpdateTip(CBlockIndex *pindexNew, const CChainParams &chainParams) { - LogPrintf("UpdateTip() pindexNew.nHeight=%s\n", pindexNew->nHeight); + LogPrintf("UpdateTip() pindexNew.nHeight=%d\n", pindexNew->nHeight); chainActive.SetTip(pindexNew); // New best block @@ -3640,7 +3640,7 @@ struct ConnectTrace { */ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr& pblock, ConnectTrace& connectTrace) { - LogPrintf("ConnectTip() nHeight=%s\n", pindexNew->nHeight); + LogPrintf("ConnectTip() nHeight=%d\n", pindexNew->nHeight); assert(pindexNew->pprev == chainActive.Tip()); // Read block from disk. int64_t nTime1 = GetTimeMicros(); @@ -4430,7 +4430,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P if (!block.sparkTxInfo) block.sparkTxInfo = std::make_shared(); - LogPrintf("CheckBlock() nHeight=%s, blockHash= %s, isVerifyDB = %s\n", nHeight, block.GetHash().ToString(), isVerifyDB); + LogPrintf("CheckBlock() nHeight=%d, blockHash= %s, isVerifyDB = %d\n", nHeight, block.GetHash().ToString(), isVerifyDB); // These are checks that are independent of context. @@ -4888,7 +4888,7 @@ static bool AcceptBlock(const std::shared_ptr& pblock, CValidation if (!AcceptBlockHeader(block, state, chainparams, &pindex)) return false; - LogPrintf("AcceptBlock nHeight=%s\n", pindex->nHeight); + LogPrintf("AcceptBlock nHeight=%d\n", pindex->nHeight); // Try to process all requested blocks that we don't have, but only // process an unrequested block if it's new and has enough work to // advance our tip, and isn't too many blocks ahead. @@ -5408,7 +5408,7 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview, // check level 0: read from disk if (!ReadBlockFromDisk(block, pindex, chainparams.GetConsensus())) return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); - LogPrintf("VerifyDB->CheckBlock() nHeight=%s\n", pindex->nHeight); + LogPrintf("VerifyDB->CheckBlock() nHeight=%d\n", pindex->nHeight); // check level 1: verify block validity if (nCheckLevel >= 1 && !CheckBlock(block, state, chainparams.GetConsensus(), true, true, pindex->nHeight, true)) return error("%s: *** found bad block at %d, hash=%s (%s)\n", __func__, diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index e4d28e436f..a06ea6661a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3832,7 +3832,7 @@ UniValue listunspentlelantusmints(const JSONRPCRequest& request) { std::vector vecOutputs; assert(pwallet != NULL); pwallet->ListAvailableLelantusMintCoins(vecOutputs, false); - LogPrintf("vecOutputs.size()=%s\n", vecOutputs.size()); + LogPrintf("vecOutputs.size()=%zu\n", vecOutputs.size()); BOOST_FOREACH(const COutput &out, vecOutputs) { if (out.nDepth < nMinDepth || out.nDepth > nMaxDepth) @@ -3887,7 +3887,7 @@ UniValue listunspentsparkmints(const JSONRPCRequest& request) { std::list coins = pwallet->sparkWallet->GetAvailableSparkCoins(); - LogPrintf("coins.size()=%s\n", coins.size()); + LogPrintf("coins.size()=%zu\n", coins.size()); BOOST_FOREACH(const auto& coin, coins) { UniValue entry(UniValue::VOBJ); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0c4ce96ef5..7c11c11182 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3541,17 +3541,17 @@ void CWallet::ListAvailableLelantusMintCoins(std::vector &vCoins, bool std::list listOwnCoins; CWalletDB walletdb(pwalletMain->strWalletFile); listOwnCoins = zwallet->GetTracker().MintsAsLelantusEntries(true, false); - LogPrintf("listOwnCoins.size()=%s\n", listOwnCoins.size()); + LogPrintf("listOwnCoins.size()=%zu\n", listOwnCoins.size()); for (std::map::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) { const CWalletTx *pcoin = &(*it).second; // LogPrintf("pcoin=%s\n", pcoin->GetHash().ToString()); if (!CheckFinalTx(*pcoin)) { - LogPrintf("!CheckFinalTx(*pcoin)=%s\n", !CheckFinalTx(*pcoin)); + LogPrintf("!CheckFinalTx(*pcoin)=%d\n", !CheckFinalTx(*pcoin)); continue; } if (fOnlyConfirmed && !pcoin->IsTrusted()) { - LogPrintf("fOnlyConfirmed = %s, !pcoin->IsTrusted() = %s\n", fOnlyConfirmed, !pcoin->IsTrusted()); + LogPrintf("fOnlyConfirmed = %d, !pcoin->IsTrusted() = %d\n", fOnlyConfirmed, !pcoin->IsTrusted()); continue; } @@ -3562,10 +3562,10 @@ void CWallet::ListAvailableLelantusMintCoins(std::vector &vCoins, bool int nDepth = pcoin->GetDepthInMainChain(); if (nDepth < 0) { - LogPrintf("nDepth=%s\n", nDepth); + LogPrintf("nDepth=%d\n", nDepth); continue; } - LogPrintf("pcoin->tx->vout.size()=%s\n", pcoin->tx->vout.size()); + LogPrintf("pcoin->tx->vout.size()=%zu\n", pcoin->tx->vout.size()); for (unsigned int i = 0; i < pcoin->tx->vout.size(); i++) { if (pcoin->tx->vout[i].scriptPubKey.IsLelantusMint() || pcoin->tx->vout[i].scriptPubKey.IsLelantusJMint()) { @@ -3937,7 +3937,12 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // now we ensure code won't be written that makes assumptions about // nLockTime that preclude a fix later. - txNew.nLockTime = chainActive.Height(); + int nHeight = 0; + { + LOCK(cs_main); + nHeight = chainActive.Height(); + } + txNew.nLockTime = nHeight; // Secondly occasionally randomly pick a nLockTime even further back, so // that transactions that are delayed after signing for whatever reason, @@ -3946,7 +3951,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT if (GetRandInt(10) == 0) txNew.nLockTime = std::max(0, (int)txNew.nLockTime - GetRandInt(100)); - assert(txNew.nLockTime <= (unsigned int)chainActive.Height()); + assert(txNew.nLockTime <= static_cast(nHeight)); assert(txNew.nLockTime < LOCKTIME_THRESHOLD); { @@ -4348,9 +4353,14 @@ bool CWallet::CreateLelantusMintTransactions( wtxNew.BindWallet(this); 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(nHeight)); assert(txNew.nLockTime < LOCKTIME_THRESHOLD); { @@ -4379,8 +4389,8 @@ bool CWallet::CreateLelantusMintTransactions( CHDMint dMint; - auto nFeeRet = 0; - LogPrintf("nFeeRet=%s\n", nFeeRet); + CAmount nFeeRet = 0; + LogPrintf("nFeeRet=%d\n", nFeeRet); auto itr = valueAndUTXO.begin(); @@ -4740,7 +4750,7 @@ std::string CWallet::MintAndStoreLelantus(const CAmount& value, if ((value + payTxFee.GetFeePerK()) > GetBalance()) return _("Insufficient funds"); - LogPrintf("payTxFee.GetFeePerK()=%s\n", payTxFee.GetFeePerK()); + LogPrintf("payTxFee.GetFeePerK()=%d\n", payTxFee.GetFeePerK()); int64_t nFeeRequired = 0; int nChangePosRet = -1; @@ -4809,7 +4819,7 @@ std::string CWallet::MintAndStoreSpark( if (cmp::greater((value + payTxFee.GetFeePerK()), GetBalance())) return _("Insufficient funds"); - LogPrintf("payTxFee.GetFeePerK()=%s\n", payTxFee.GetFeePerK()); + LogPrintf("payTxFee.GetFeePerK()=%d\n", payTxFee.GetFeePerK()); int64_t nFeeRequired = 0; int nChangePosRet = -1; From 27b2302314ea2a4d928169d058aae3dd99c89cb4 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 27 Jan 2026 10:36:03 +0000 Subject: [PATCH 02/10] Fix null pointer dereference in miner coinbaseScript logging When coinbaseScript is NULL, the LogPrintf was unconditionally calling coinbaseScript->reserveScript.empty() which would crash. Use a ternary operator to only dereference when coinbaseScript is non-null. Co-authored-by: reuben --- src/miner.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/miner.cpp b/src/miner.cpp index f73fae973e..770e2f5f6e 100644 --- a/src/miner.cpp +++ b/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=%p, coinbaseScript->reserveScript.empty()=%d\n", coinbaseScript, coinbaseScript->reserveScript.empty()); + LogPrintf("FiroMiner stop here coinbaseScript=%p, coinbaseScript->reserveScript.empty()=%d\n", + coinbaseScript, coinbaseScript ? coinbaseScript->reserveScript.empty() : true); throw std::runtime_error("No coinbase script available (mining requires a wallet)"); } From e0d94a0c09ca72be6ebc9aa70deb0bdb23b06ef7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 27 Jan 2026 10:37:07 +0000 Subject: [PATCH 03/10] Fix misleading log message in mempool removeUnchecked The log was printing 'IsSpend()=%d' but actually calling HasPrivateInputs(), and since we're inside the !HasPrivateInputs() block, the value was always false. Simplified to a clearer message that describes the branch condition. Co-authored-by: reuben --- src/txmempool.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 7043ae7a57..93e8cf9d07 100644 --- a/src/txmempool.cpp +++ b/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()=%d\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); } From e854998ff3730eb895d170775886e3148d5e4394 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 27 Jan 2026 11:45:23 +0000 Subject: [PATCH 04/10] Fix shared_ptr formatting crash in miner logging coinbaseScript is a boost::shared_ptr, not a raw pointer. Using %p with a shared_ptr causes tinyformat to fail. Use .get() to extract the raw pointer for logging. Co-authored-by: reuben --- src/miner.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/miner.cpp b/src/miner.cpp index 770e2f5f6e..bea6c954da 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -1109,7 +1109,7 @@ void static FiroMiner(const CChainParams &chainparams) { // In the latter case, already the pointer is NULL. if (!coinbaseScript || coinbaseScript->reserveScript.empty()) { LogPrintf("FiroMiner stop here coinbaseScript=%p, coinbaseScript->reserveScript.empty()=%d\n", - coinbaseScript, coinbaseScript ? coinbaseScript->reserveScript.empty() : true); + coinbaseScript.get(), coinbaseScript ? coinbaseScript->reserveScript.empty() : true); throw std::runtime_error("No coinbase script available (mining requires a wallet)"); } From 6e65bdd368c8d5a21f21f2574ddd3fd3be308123 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 27 Jan 2026 11:47:55 +0000 Subject: [PATCH 05/10] Fix additional format specifier issues in miner logging - pindexPrev->nHeight: %s -> %d (int) - pblock->nVersion: %s -> %d (int) - pblock->nTime: %s -> %u (uint32_t) - pblock->nNonce: %s with &address -> %u with value (was passing address of nNonce!) Co-authored-by: reuben --- src/miner.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index bea6c954da..86817cf6de 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -1165,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) { From ebe917941ecd7b91a98e9cb34c0d0f0b5e4e4ad3 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 27 Jan 2026 11:52:09 +0000 Subject: [PATCH 06/10] Fix critical erase-while-iterating bug in CreateSparkMintTransactions The fee subtraction loop had multiple bugs: 1. After erase(), singleTxOutputs[i] referred to the wrong element (shifted) 2. If erasing the last element, singleTxOutputs[i] was out of bounds 3. The subtraction singleTxOutputs[i].v -= singleFee ran even after erase This caused undefined behavior (memory corruption/crashes) when: - Clicking 'Anonymize all' button - Sending from transparent balance to Spark address Fixed by using the same safe pattern as CreateSparkSpendTransaction: - Check for empty vector upfront - Calculate fee per output and remainder - First output pays the remainder - Validate each amount before subtraction (no erasing mid-loop) Co-authored-by: reuben --- src/spark/sparkwallet.cpp | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/spark/sparkwallet.cpp b/src/spark/sparkwallet.cpp index 55992dcb0d..ee23512642 100644 --- a/src/spark/sparkwallet.cpp +++ b/src/spark/sparkwallet.cpp @@ -922,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(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; } } From b414dacfbb7d02b83e7dd0850e989841ed55ad12 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 27 Jan 2026 11:52:52 +0000 Subject: [PATCH 07/10] Add safety check for empty remainingOutputs in mint loop Prevent potential crash if remainingOutputs becomes empty while remainingMintValue is still > 0. This could happen in edge cases and would cause dereferencing of an invalid iterator. Co-authored-by: reuben --- src/spark/sparkwallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/spark/sparkwallet.cpp b/src/spark/sparkwallet.cpp index ee23512642..f888d28eb4 100644 --- a/src/spark/sparkwallet.cpp +++ b/src/spark/sparkwallet.cpp @@ -903,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; From 2de1f24f927e559216220a8890a91333093251a5 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 27 Jan 2026 16:26:13 +0000 Subject: [PATCH 08/10] Fix multiple bugs in CreateSparkMintTransactions Bug fixes in sparkwallet.cpp: 1. Fix typo: mapMultiArgs.at("change") -> mapMultiArgs.at("-change") This would cause std::out_of_range exception when -change arg is used. 2. Add empty check before utxos.second.front() access to prevent undefined behavior if any entry has an empty vector. 3. Add break after finding matching UTXO entry to avoid unnecessary iterations and potential issues. 4. Fix missing strFailReason when SelectCoins fails for reasons other than insufficient funds - now sets "Unable to select coins for minting". 5. Fix missing strFailReason when function fails because valueToMint > 0 at end - now sets descriptive error message. 6. Fix misleading error message: "Signing transaction failed" changed to "Transaction not allowed in mempool" for mempool rejection. Bug fix in wallet.cpp: - Same typo fix for mapMultiArgs.at("change") in CreateLelantusMintTransactions These bugs could potentially cause crashes (especially #1 and #2) or make debugging difficult due to missing/wrong error messages. Co-authored-by: reuben --- src/spark/sparkwallet.cpp | 12 ++++++++---- src/wallet/wallet.cpp | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/spark/sparkwallet.cpp b/src/spark/sparkwallet.cpp index f888d28eb4..0f1e8f296d 100644 --- a/src/spark/sparkwallet.cpp +++ b/src/spark/sparkwallet.cpp @@ -959,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; } @@ -995,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"); @@ -1209,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; } } @@ -1228,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; } } @@ -1253,6 +1256,7 @@ bool CSparkWallet::CreateSparkMintTransactions( } if (!autoMintAll && valueToMint > 0) { + strFailReason = _("Unable to mint full amount; only partial minting was possible"); return false; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7c11c11182..c9d71264da 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4487,7 +4487,7 @@ bool CWallet::CreateLelantusMintTransactions( // 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"); From d1be4ec7ba88c6196f1f71fff71b06ce810f7631 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 27 Jan 2026 16:39:28 +0000 Subject: [PATCH 09/10] Fix critical memory corruption bugs in spark library 1. SpendKey constructor: After data.clear() and result.clear(), the code was accessing data.data() and result[0] on empty vectors, causing undefined behavior. This could corrupt memory and cause crashes with bizarre stack traces (explaining why boost::shared_ptr appeared in spark minting crash traces - corrupted memory was being misinterpreted). Fix: Use std::fill() to zero the buffers instead of clear(), maintaining the vector size and valid memory. 2. Address::decode: Accessing decoded.hrp[0] and decoded.hrp[1] without checking if hrp has at least 2 characters could cause out-of-bounds access. Fix: Add size check before accessing hrp elements. These bugs could cause heap/stack corruption that manifests as crashes during seemingly unrelated operations (like Spark minting), with corrupted stack traces showing wrong function names and types. Co-authored-by: reuben --- src/libspark/keys.cpp | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/libspark/keys.cpp b/src/libspark/keys.cpp index 456e45e203..1de251994b 100644 --- a/src/libspark/keys.cpp +++ b/src/libspark/keys.cpp @@ -16,28 +16,28 @@ SpendKey::SpendKey(const Params* params) { SpendKey::SpendKey(const Params* params, const Scalar& r_) { this->params = params; this->r = r_; - std::vector data; - data.resize(32); + std::vector data(32); r.serialize(data.data()); - std::vector result(CSHA256().OUTPUT_SIZE); + std::vector result(CSHA256::OUTPUT_SIZE); CHash256 hash256; std::string prefix1 = "s1_generation"; hash256.Write(reinterpret_cast(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 - resize instead of clear to maintain buffer capacity + std::fill(data.begin(), data.end(), 0); + std::fill(result.begin(), result.end(), 0); hash256.Reset(); s1.serialize(data.data()); std::string prefix2 = "s2_generation"; hash256.Write(reinterpret_cast(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 +212,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"); } From 7146c975966cb94595c82a617d3fa4b554f7cee4 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 28 Jan 2026 05:01:49 +0000 Subject: [PATCH 10/10] Use memory_cleanse for secure cryptographic key buffer clearing Replace std::fill with memory_cleanse in SpendKey constructor to securely clear cryptographic key material. memory_cleanse uses OPENSSL_cleanse which is guaranteed not to be optimized away by the compiler, providing proper protection against cold boot attacks and memory dump analysis. Co-authored-by: reuben --- src/libspark/keys.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libspark/keys.cpp b/src/libspark/keys.cpp index 1de251994b..74440fa012 100644 --- a/src/libspark/keys.cpp +++ b/src/libspark/keys.cpp @@ -1,5 +1,6 @@ #include "keys.h" #include "../hash.h" +#include "../support/cleanse.h" #include "transcript.h" namespace spark { @@ -27,9 +28,10 @@ SpendKey::SpendKey(const Params* params, const Scalar& r_) { hash256.Finalize(result.data()); this->s1.memberFromSeed(result.data()); - // Reset for s2 generation - resize instead of clear to maintain buffer capacity - std::fill(data.begin(), data.end(), 0); - std::fill(result.begin(), result.end(), 0); + // 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());