From 692b827683c024749a459cca6c936d310b751edb Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 26 May 2020 16:44:00 +0200 Subject: [PATCH 01/24] Add DummySignatureCreator which just creates zeroed sigs - backports bitcoin@9b4e7d9a5ec0f69c175d23dc9c94ed723147cf45 --- src/script/sign.cpp | 36 ++++++++++++++++++++++++++++++++++++ src/script/sign.h | 8 ++++++++ 2 files changed, 44 insertions(+) diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 3b594dafb012..7e3eb74e989d 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -296,3 +296,39 @@ CScript CombineSignatures(const CScript& scriptPubKey, const BaseSignatureChecke return CombineSignatures(scriptPubKey, checker, txType, vSolutions, stack1, stack2); } + +namespace { +/** Dummy signature checker which accepts all signatures. */ +class DummySignatureChecker : public BaseSignatureChecker +{ +public: + DummySignatureChecker() {} + + bool CheckSig(const std::vector& scriptSig, const std::vector& vchPubKey, const CScript& scriptCode) const + { + return true; + } +}; +const DummySignatureChecker dummyChecker; +} + +const BaseSignatureChecker& DummySignatureCreator::Checker() const +{ + return dummyChecker; +} + +bool DummySignatureCreator::CreateSig(std::vector& vchSig, const CKeyID& keyid, const CScript& scriptCode) const +{ + // Create a dummy signature that is a valid DER-encoding + vchSig.assign(72, '\000'); + vchSig[0] = 0x30; + vchSig[1] = 69; + vchSig[2] = 0x02; + vchSig[3] = 33; + vchSig[4] = 0x01; + vchSig[4 + 33] = 0x02; + vchSig[5 + 33] = 32; + vchSig[6 + 33] = 0x01; + vchSig[6 + 33 + 32] = SIGHASH_ALL; + return true; +} diff --git a/src/script/sign.h b/src/script/sign.h index f2dac17de7c2..f11f9be89c79 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -44,6 +44,14 @@ class TransactionSignatureCreator : public BaseSignatureCreator { bool CreateSig(std::vector& vchSig, const CKeyID& keyid, const CScript& scriptCode) const; }; +/** A signature creator that just produces 72-byte empty signatyres. */ +class DummySignatureCreator : public BaseSignatureCreator { +public: + DummySignatureCreator(const CKeyStore* keystoreIn) : BaseSignatureCreator(keystoreIn) {} + const BaseSignatureChecker& Checker() const; + bool CreateSig(std::vector& vchSig, const CKeyID& keyid, const CScript& scriptCode) const; +}; + /** Produce a script signature using a generic signature creator. */ bool ProduceSignature(const BaseSignatureCreator& creator, const CScript& scriptPubKey, CScript& scriptSig, bool fColdStake); From ccb18dd9cb0a03586b2129d62588e5e7651f3cfc Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 26 May 2020 21:43:15 +0200 Subject: [PATCH 02/24] Add FundTransaction method to wallet - backports bitcoin@1e0d1a2ff02982d6ffe61e70b027c56f3bf22d0b Some code stolen from Jonas Schnelli --- src/coincontrol.h | 4 +- src/qt/walletmodel.cpp | 9 +- src/wallet/rpcwallet.cpp | 11 +-- src/wallet/wallet.cpp | 178 +++++++++++++++++++++++++++++++-------- src/wallet/wallet.h | 14 ++- 5 files changed, 167 insertions(+), 49 deletions(-) diff --git a/src/coincontrol.h b/src/coincontrol.h index c7005cd5611e..f00191456a81 100644 --- a/src/coincontrol.h +++ b/src/coincontrol.h @@ -68,12 +68,12 @@ class CCoinControl setSelected.clear(); } - void ListSelected(std::vector& vOutpoints) + void ListSelected(std::vector& vOutpoints) const { vOutpoints.assign(setSelected.begin(), setSelected.end()); } - unsigned int QuantitySelected() + unsigned int QuantitySelected() const { return setSelected.size(); } diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index ce117bce3ee0..9067907d4bab 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -389,7 +389,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact { CAmount total = 0; QList recipients = transaction.getRecipients(); - std::vector > vecSend; + std::vector vecSend; if (recipients.empty()) { return OK; @@ -413,7 +413,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact subtotal += out.amount(); const unsigned char* scriptStr = (const unsigned char*)out.script().data(); CScript scriptPubKey(scriptStr, scriptStr + out.script().size()); - vecSend.push_back(std::pair(scriptPubKey, out.amount())); + vecSend.push_back(CRecipient{scriptPubKey, static_cast(out.amount()), false}); } if (subtotal <= 0) { return InvalidAmount; @@ -453,7 +453,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact // Regular P2PK or P2PKH scriptPubKey = GetScriptForDestination(out); } - vecSend.push_back(std::pair(scriptPubKey, rcp.amount)); + vecSend.push_back(CRecipient{scriptPubKey, rcp.amount, false}); total += rcp.amount; } @@ -473,6 +473,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact transaction.newPossibleKeyChange(wallet); CAmount nFeeRequired = 0; + int nChangePosRet; std::string strFailReason; CWalletTx* newTx = transaction.getTransaction(); @@ -489,9 +490,11 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact *newTx, *keyChange, nFeeRequired, + nChangePosRet, strFailReason, coinControl, recipients[0].inputType, + true, recipients[0].useSwiftTX, 0, fIncludeDelegations); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 9d39e7d6233e..3ad287d061a5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -764,7 +764,7 @@ void SendMoney(const CTxDestination& address, CAmount nValue, CWalletTx& wtxNew, // Create and send the transaction CReserveKey reservekey(pwalletMain); CAmount nFeeRequired; - if (!pwalletMain->CreateTransaction(scriptPubKey, nValue, wtxNew, reservekey, nFeeRequired, strError, NULL, ALL_COINS, fUseIX, (CAmount)0)) { + if (!pwalletMain->CreateTransaction(scriptPubKey, nValue, wtxNew, reservekey, nFeeRequired, strError, nullptr, ALL_COINS, fUseIX, (CAmount)0)) { if (nValue + nFeeRequired > pwalletMain->GetBalance()) strError = strprintf("Error: This transaction requires a transaction fee of at least %s because of its amount, complexity, or use of recently received funds!", FormatMoney(nFeeRequired)); LogPrintf("SendMoney() : %s\n", strError); @@ -905,7 +905,7 @@ UniValue CreateColdStakeDelegation(const UniValue& params, CWalletTx& wtxNew, CR // Create the transaction CAmount nFeeRequired; - if (!pwalletMain->CreateTransaction(scriptPubKey, nValue, wtxNew, reservekey, nFeeRequired, strError, NULL, ALL_COINS, /*fUseIX*/ false, (CAmount)0, fUseDelegated)) { + if (!pwalletMain->CreateTransaction(scriptPubKey, nValue, wtxNew, reservekey, nFeeRequired, strError, nullptr, ALL_COINS, /*fUseIX*/ false, (CAmount)0, fUseDelegated)) { if (nValue + nFeeRequired > currBalance) strError = strprintf("Error: This transaction requires a transaction fee of at least %s because of its amount, complexity, or use of recently received funds!", FormatMoney(nFeeRequired)); LogPrintf("%s : %s\n", __func__, strError); @@ -1653,7 +1653,7 @@ UniValue sendmany(const UniValue& params, bool fHelp) wtx.mapValue["comment"] = params[3].get_str(); std::set setAddress; - std::vector > vecSend; + std::vector vecSend; CAmount totalAmount = 0; std::vector keys = sendTo.getKeys(); @@ -1671,7 +1671,7 @@ UniValue sendmany(const UniValue& params, bool fHelp) CAmount nAmount = AmountFromValue(sendTo[name_]); totalAmount += nAmount; - vecSend.push_back(std::make_pair(scriptPubKey, nAmount)); + vecSend.push_back(CRecipient{scriptPubKey, nAmount, false}); } isminefilter filter = ISMINE_SPENDABLE; @@ -1689,7 +1689,8 @@ UniValue sendmany(const UniValue& params, bool fHelp) CReserveKey keyChange(pwalletMain); CAmount nFeeRequired = 0; std::string strFailReason; - bool fCreated = pwalletMain->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, strFailReason); + int nChangePosRet; + bool fCreated = pwalletMain->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, nChangePosRet, strFailReason); if (!fCreated) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason); const CWallet::CommitResult& res = pwalletMain->CommitTransaction(wtx, keyChange); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 20845d1f5364..28210eacfa9c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2274,7 +2274,7 @@ bool CWallet::SelectCoinsToSpend(const CAmount& nTargetValue, std::set return all selected outputs (we want all selected to go into the transaction for sure) - if (coinControl && coinControl->HasSelected()) { + if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs) { for (const COutput& out : vCoins) { if (!out.fSpendable) continue; @@ -2285,9 +2285,46 @@ bool CWallet::SelectCoinsToSpend(const CAmount& nTargetValue, std::set= nTargetValue); } - return (SelectCoinsMinConf(nTargetValue, 1, 6, vCoins, setCoinsRet, nValueRet) || - SelectCoinsMinConf(nTargetValue, 1, 1, vCoins, setCoinsRet, nValueRet) || - (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue, 0, 1, vCoins, setCoinsRet, nValueRet))); + // calculate value from preset inputs and store them + std::set > setPresetCoins; + CAmount nValueFromPresetInputs = 0; + + std::vector vPresetInputs; + if (coinControl) + coinControl->ListSelected(vPresetInputs); + for (const COutPoint& outpoint : vPresetInputs) { + std::map::const_iterator it = mapWallet.find(outpoint.hash); + if (it != mapWallet.end()) { + const CWalletTx* pcoin = &it->second; + // Clearly invalid input, fail + if (pcoin->vout.size() <= outpoint.n) + return false; + nValueFromPresetInputs += pcoin->vout[outpoint.n].nValue; + setPresetCoins.insert(std::make_pair(pcoin, outpoint.n)); + } else + return false; // TODO: Allow non-wallet inputs + } + + // remove preset inputs from vCoins + for (std::vector::iterator it = vCoins.begin(); it != vCoins.end() && coinControl && coinControl->HasSelected();) { + if (setPresetCoins.count(std::make_pair(it->tx, it->i))) + it = vCoins.erase(it); + else + ++it; + } + + bool res = nTargetValue <= nValueFromPresetInputs || + SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 6, vCoins, setCoinsRet, nValueRet) || + SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 1, vCoins, setCoinsRet, nValueRet) || + (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, vCoins, setCoinsRet, nValueRet)); + + // because SelectCoinsMinConf clears the setCoinsRet, we now add the possible inputs to the coinset + setCoinsRet.insert(setPresetCoins.begin(), setPresetCoins.end()); + + // add preset inputs to the total value selected + nValueRet += nValueFromPresetInputs; + + return res; } bool CWallet::GetBudgetSystemCollateralTX(CWalletTx& tx, uint256 hash, bool useIX) @@ -2300,11 +2337,12 @@ bool CWallet::GetBudgetSystemCollateralTX(CWalletTx& tx, uint256 hash, bool useI CAmount nFeeRet = 0; std::string strFail = ""; - std::vector > vecSend; - vecSend.push_back(std::make_pair(scriptChange, BUDGET_FEE_TX_OLD)); // Old 50 PIV collateral + std::vector vecSend; + vecSend.push_back(CRecipient{scriptChange, BUDGET_FEE_TX_OLD, false}); // Old 50 PIV collateral CCoinControl* coinControl = NULL; - bool success = CreateTransaction(vecSend, tx, reservekey, nFeeRet, strFail, coinControl, ALL_COINS, useIX, (CAmount)0); + int nChangePosRet; + bool success = CreateTransaction(vecSend, tx, reservekey, nFeeRet, nChangePosRet, strFail, coinControl, ALL_COINS, true, useIX, (CAmount)0); if (!success) { LogPrintf("GetBudgetSystemCollateralTX: Error - %s\n", strFail); return false; @@ -2323,11 +2361,12 @@ bool CWallet::GetBudgetFinalizationCollateralTX(CWalletTx& tx, uint256 hash, boo CAmount nFeeRet = 0; std::string strFail = ""; - std::vector > vecSend; - vecSend.push_back(std::make_pair(scriptChange, BUDGET_FEE_TX)); // New 5 PIV collateral + std::vector vecSend; + vecSend.push_back(CRecipient{scriptChange, BUDGET_FEE_TX, false}); // New 5 PIV collateral CCoinControl* coinControl = NULL; - bool success = CreateTransaction(vecSend, tx, reservekey, nFeeRet, strFail, coinControl, ALL_COINS, useIX, (CAmount)0); + int nChangePosRet; + bool success = CreateTransaction(vecSend, tx, reservekey, nFeeRet, nChangePosRet, strFail, coinControl, ALL_COINS, true, useIX, (CAmount)0); if (!success) { LogPrintf("GetBudgetSystemCollateralTX: Error - %s\n", strFail); return false; @@ -2336,27 +2375,69 @@ bool CWallet::GetBudgetFinalizationCollateralTX(CWalletTx& tx, uint256 hash, boo return true; } -bool CWallet::CreateTransaction(const std::vector >& vecSend, +bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosRet, std::string& strFailReason) +{ + std::vector vecSend; + + // Turn the txout set into a CRecipient vector + for (const CTxOut& txOut : tx.vout) { + CRecipient recipient = {txOut.scriptPubKey, txOut.nValue, false}; + vecSend.push_back(recipient); + } + + CCoinControl coinControl; + coinControl.fAllowOtherInputs = true; + for (const CTxIn& txin : tx.vin) + coinControl.Select(txin.prevout); + + CReserveKey reservekey(this); + CWalletTx wtx; + if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosRet, strFailReason, &coinControl, ALL_COINS, false)) + return false; + + if (nChangePosRet != -1) + tx.vout.insert(tx.vout.begin() + nChangePosRet, wtx.vout[nChangePosRet]); + + // Add new txins (keeping original txin scriptSig/order) + for (const CTxIn& txin : wtx.vin) { + bool found = false; + for (const CTxIn& origTxIn : tx.vin) { + if (txin.prevout.hash == origTxIn.prevout.hash && txin.prevout.n == origTxIn.prevout.n) { + found = true; + break; + } + } + if (!found) + tx.vin.push_back(txin); + } + + return true; +} + +bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, + int& nChangePosRet, std::string& strFailReason, const CCoinControl* coinControl, AvailableCoinsType coin_type, + bool sign, bool useIX, CAmount nFeePay, bool fIncludeDelegated) { if (useIX && nFeePay < CENT) nFeePay = CENT; + nChangePosRet = -1; CAmount nValue = 0; - for (const PAIRTYPE(CScript, CAmount) & s : vecSend) { - if (nValue < 0) { + for (const CRecipient& rec : vecSend) { + if (rec.nAmount < 0) { strFailReason = _("Transaction amounts must be positive"); return false; } - nValue += s.second; + nValue += rec.nAmount; } if (vecSend.empty() || nValue < 0) { strFailReason = _("Transaction amounts must be positive"); @@ -2382,8 +2463,8 @@ bool CWallet::CreateTransaction(const std::vector >& // vouts to the payees if (coinControl && !coinControl->fSplitBlock) { - for (const PAIRTYPE(CScript, CAmount) & s : vecSend) { - CTxOut txout(s.second, s.first); + for (const CRecipient& rec : vecSend) { + CTxOut txout(rec.nAmount, rec.scriptPubKey); if (txout.IsDust(::minRelayTxFee)) { strFailReason = _("Transaction amount too small"); return false; @@ -2399,13 +2480,13 @@ bool CWallet::CreateTransaction(const std::vector >& else nSplitBlock = 1; - for (const PAIRTYPE(CScript, CAmount) & s : vecSend) { + for (const CRecipient& rec : vecSend) { for (int i = 0; i < nSplitBlock; i++) { if (i == nSplitBlock - 1) { - uint64_t nRemainder = s.second % nSplitBlock; - txNew.vout.push_back(CTxOut((s.second / nSplitBlock) + nRemainder, s.first)); + uint64_t nRemainder = rec.nAmount % nSplitBlock; + txNew.vout.push_back(CTxOut((rec.nAmount / nSplitBlock) + nRemainder, rec.scriptPubKey)); } else - txNew.vout.push_back(CTxOut(s.second / nSplitBlock, s.first)); + txNew.vout.push_back(CTxOut(rec.nAmount / nSplitBlock, rec.scriptPubKey)); } } } @@ -2497,7 +2578,8 @@ bool CWallet::CreateTransaction(const std::vector >& reservekey.ReturnKey(); } else { // Insert change txn at random position: - std::vector::iterator position = txNew.vout.begin() + GetRandInt(txNew.vout.size() + 1); + nChangePosRet = GetRandInt(txNew.vout.size() + 1); + std::vector::iterator position = txNew.vout.begin() + nChangePosRet; txNew.vout.insert(position, newTxOut); } } @@ -2510,21 +2592,40 @@ bool CWallet::CreateTransaction(const std::vector >& // Sign int nIn = 0; - for (const PAIRTYPE(const CWalletTx*, unsigned int) & coin : setCoins) - if (!SignSignature(*this, *coin.first, txNew, nIn++)) { + CTransaction txNewConst(txNew); + for (const PAIRTYPE(const CWalletTx*, unsigned int) & coin : setCoins) { + bool signSuccess; + const CScript& scriptPubKey = coin.first->vout[coin.second].scriptPubKey; + CScript& scriptSigRes = txNew.vin[nIn].scriptSig; + if (sign) + signSuccess = ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, SIGHASH_ALL), scriptPubKey, scriptSigRes, false); + else + signSuccess = ProduceSignature(DummySignatureCreator(this), scriptPubKey, scriptSigRes, false); + + if (!signSuccess) { strFailReason = _("Signing transaction failed"); return false; } + nIn++; + } + + unsigned int nBytes = ::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION); + + // Remove scriptSigs if we used dummy signatures for fee calculation + if (!sign) { + for (CTxIn& vin : txNew.vin) + vin.scriptSig = CScript(); + } // Embed the constructed transaction data in wtxNew. *static_cast(&wtxNew) = CTransaction(txNew); // Limit size - unsigned int nBytes = ::GetSerializeSize(*(CTransaction*)&wtxNew, SER_NETWORK, PROTOCOL_VERSION); if (nBytes >= MAX_STANDARD_TX_SIZE) { strFailReason = _("Transaction too large"); return false; } + dPriority = wtxNew.ComputePriority(dPriority, nBytes); // Can we complete this as a free transaction? @@ -2563,9 +2664,10 @@ bool CWallet::CreateTransaction(const std::vector >& bool CWallet::CreateTransaction(CScript scriptPubKey, const CAmount& nValue, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, std::string& strFailReason, const CCoinControl* coinControl, AvailableCoinsType coin_type, bool useIX, CAmount nFeePay, bool fIncludeDelegated) { - std::vector > vecSend; - vecSend.push_back(std::make_pair(scriptPubKey, nValue)); - return CreateTransaction(vecSend, wtxNew, reservekey, nFeeRet, strFailReason, coinControl, coin_type, useIX, nFeePay, fIncludeDelegated); + std::vector vecSend; + vecSend.push_back(CRecipient{scriptPubKey, nValue, false}); + int nChangePosRet; + return CreateTransaction(vecSend, wtxNew, reservekey, nFeeRet, nChangePosRet, strFailReason, coinControl, coin_type, true, useIX, nFeePay, fIncludeDelegated); } bool CWallet::CreateCoinStake( @@ -3439,9 +3541,9 @@ void CWallet::AutoCombineDust() if (vRewardCoins.size() <= 1) continue; - std::vector > vecSend; + std::vector vecSend; CScript scriptPubKey = GetScriptForDestination(it->first.Get()); - vecSend.push_back(std::make_pair(scriptPubKey, nTotalRewardsValue)); + vecSend.push_back(CRecipient{scriptPubKey, nTotalRewardsValue, false}); //Send change to same address CTxDestination destMyAddress; @@ -3456,11 +3558,12 @@ void CWallet::AutoCombineDust() CReserveKey keyChange(this); // this change address does not end up being used, because change is returned with coin control switch std::string strErr; CAmount nFeeRet = 0; + int nChangePosRet; // 10% safety margin to avoid "Insufficient funds" errors - vecSend[0].second = nTotalRewardsValue - (nTotalRewardsValue / 10); + vecSend[0].nAmount = nTotalRewardsValue - (nTotalRewardsValue / 10); - if (!CreateTransaction(vecSend, wtx, keyChange, nFeeRet, strErr, coinControl, ALL_COINS, false, CAmount(0))) { + if (!CreateTransaction(vecSend, wtx, keyChange, nFeeRet, nChangePosRet, strErr, coinControl, ALL_COINS, true, false, CAmount(0))) { LogPrintf("AutoCombineDust createtransaction failed, reason: %s\n", strErr); continue; } @@ -3542,7 +3645,7 @@ bool CWallet::MultiSend() CWalletTx wtx; CReserveKey keyChange(this); // this change address does not end up being used, because change is returned with coin control switch CAmount nFeeRet = 0; - std::vector > vecSend; + std::vector vecSend; // loop through multisend vector and add amounts and addresses to the sending vector const isminefilter filter = ISMINE_SPENDABLE; @@ -3553,22 +3656,23 @@ bool CWallet::MultiSend() CBitcoinAddress strAddSend(vMultiSend[i].first); CScript scriptPubKey; scriptPubKey = GetScriptForDestination(strAddSend.Get()); - vecSend.push_back(std::make_pair(scriptPubKey, nAmount)); + vecSend.push_back(CRecipient{scriptPubKey, nAmount, false}); } //get the fee amount CWalletTx wtxdummy; std::string strErr; - CreateTransaction(vecSend, wtxdummy, keyChange, nFeeRet, strErr, &cControl, ALL_COINS, false, CAmount(0)); - CAmount nLastSendAmount = vecSend[vecSend.size() - 1].second; + int nChangePosRet; + CreateTransaction(vecSend, wtxdummy, keyChange, nFeeRet, nChangePosRet, strErr, &cControl, ALL_COINS, true, false, CAmount(0)); + CAmount nLastSendAmount = vecSend[vecSend.size() - 1].nAmount; if (nLastSendAmount < nFeeRet + 500) { LogPrintf("%s: fee of %d is too large to insert into last output\n", __func__, nFeeRet + 500); return false; } - vecSend[vecSend.size() - 1].second = nLastSendAmount - nFeeRet - 500; + vecSend[vecSend.size() - 1].nAmount = nLastSendAmount - nFeeRet - 500; // Create the transaction and commit it to the network - if (!CreateTransaction(vecSend, wtx, keyChange, nFeeRet, strErr, &cControl, ALL_COINS, false, CAmount(0))) { + if (!CreateTransaction(vecSend, wtx, keyChange, nFeeRet, int nChangePosRet, strErr, &cControl, ALL_COINS, true, false, CAmount(0))) { LogPrintf("MultiSend createtransaction failed\n"); return false; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 4ba4fd57dc54..1951a976821e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -207,6 +207,14 @@ class CStakerStatus bool IsActive() const { return (nTime + 30) >= GetTime(); } }; +struct CRecipient +{ + CScript scriptPubKey; + CAmount nAmount; + bool fSubtractFeeFromAmount; +}; + + /** * A CWallet is an extension of a keystore, which also maintains a set of transactions and balances, * and provides the ability to create new transactions. @@ -452,13 +460,16 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface CAmount GetUnconfirmedWatchOnlyBalance() const; CAmount GetImmatureWatchOnlyBalance() const; CAmount GetLockedWatchOnlyBalance() const; - bool CreateTransaction(const std::vector >& vecSend, + bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosRet, std::string& strFailReason); + bool CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, + int& nChangePosRet, std::string& strFailReason, const CCoinControl* coinControl = NULL, AvailableCoinsType coin_type = ALL_COINS, + bool sign = true, bool useIX = false, CAmount nFeePay = 0, bool fIncludeDelegated = false); @@ -645,7 +656,6 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface static bool ParameterInteraction(); }; - /** A key allocated from the key pool. */ class CReserveKey { From 761e60e1fbc6132063768403c3dbcba9c7105124 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Mon, 1 Jun 2020 19:20:50 +0200 Subject: [PATCH 03/24] Add fundrawtransaction RPC method - backports bitcoin/bitcoin@21bbd920e5cc02dae5e75795c1f0bbfba9a41b53 --- src/rpc/client.cpp | 1 + src/rpc/rawtransaction.cpp | 54 ++++++++++++++++++++++++++++++++++++++ src/rpc/server.cpp | 1 + src/rpc/server.h | 1 + 4 files changed, 57 insertions(+) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 7ea69ecdf9e6..57434ead1ca5 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -99,6 +99,7 @@ static const CRPCConvertParam vRPCConvertParams[] = {"createrawtransaction", 0}, {"createrawtransaction", 1}, {"createrawtransaction", 2}, + {"fundrawtransaction", 1}, {"signrawtransaction", 1}, {"signrawtransaction", 2}, {"sendrawtransaction", 1}, diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index a10917c6b249..65bd312ff236 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -553,6 +553,60 @@ static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std:: vErrorsRet.push_back(entry); } +UniValue fundrawtransaction(const UniValue& params, bool fHelp) +{ + if (fHelp || params.size() != 1) + throw std::runtime_error( + "fundrawtransaction \"hexstring\"\n" + "\nAdd inputs to a transaction until it has enough in value to meet its out value.\n" + "This will not modify existing inputs, and will add one change output to the outputs.\n" + "Note that inputs which were signed may need to be resigned after completion since in/outputs have been added.\n" + "The inputs added will not be signed, use signrawtransaction for that.\n" + "\nArguments:\n" + "1. \"hexstring\" (string, required) The hex string of the raw transaction\n" + "\nResult:\n" + "{\n" + " \"hex\": \"value\", (string) The resulting raw transaction (hex-encoded string)\n" + " \"fee\": n, (numeric) The fee added to the transaction\n" + " \"changepos\": n (numeric) The position of the added change output, or -1\n" + "}\n" + "\"hex\" \n" + "\nExamples:\n" + "\nCreate a transaction with no inputs\n" + + HelpExampleCli("createrawtransaction", "\"[]\" \"{\\\"myaddress\\\":0.01}\"") + + "\nAdd sufficient unsigned inputs to meet the output value\n" + + HelpExampleCli("fundrawtransaction", "\"rawtransactionhex\"") + + "\nSign the transaction\n" + + HelpExampleCli("signrawtransaction", "\"fundedtransactionhex\"") + + "\nSend the transaction\n" + + HelpExampleCli("sendrawtransaction", "\"signedtransactionhex\"") + ); + + if (!pwalletMain) + throw std::runtime_error("wallet not initialized"); + + RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)); + + // parse hex string from parameter + CTransaction origTx; + if (!DecodeHexTx(origTx, params[0].get_str())) + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + + CMutableTransaction tx(origTx); + CAmount nFee; + std::string strFailReason; + int nChangePos = -1; + if(!pwalletMain->FundTransaction(tx, nFee, nChangePos, strFailReason)) + throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason); + + UniValue result(UniValue::VOBJ); + result.push_back(Pair("hex", EncodeHexTx(tx))); + result.push_back(Pair("changepos", nChangePos)); + result.push_back(Pair("fee", ValueFromAmount(nFee))); + + return result; +} + UniValue signrawtransaction(const UniValue& params, bool fHelp) { if (fHelp || params.size() < 1 || params.size() > 4) diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 5a6f8a0614ae..3e1553ce5dac 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -343,6 +343,7 @@ static const CRPCCommand vRPCCommands[] = {"rawtransactions", "decoderawtransaction", &decoderawtransaction, true, false, false}, {"rawtransactions", "decodescript", &decodescript, true, false, false}, {"rawtransactions", "getrawtransaction", &getrawtransaction, true, false, false}, + {"rawtransactions", "fundrawtransaction", &fundrawtransaction, false, false, false}, {"rawtransactions", "sendrawtransaction", &sendrawtransaction, false, false, false}, {"rawtransactions", "signrawtransaction", &signrawtransaction, false, false, false}, /* uses wallet if enabled */ diff --git a/src/rpc/server.h b/src/rpc/server.h index 046cd885c303..7fa3c2e17bdc 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -291,6 +291,7 @@ extern UniValue listlockunspent(const UniValue& params, bool fHelp); extern UniValue createrawtransaction(const UniValue& params, bool fHelp); extern UniValue decoderawtransaction(const UniValue& params, bool fHelp); extern UniValue decodescript(const UniValue& params, bool fHelp); +extern UniValue fundrawtransaction(const UniValue& params, bool fHelp); extern UniValue signrawtransaction(const UniValue& params, bool fHelp); extern UniValue sendrawtransaction(const UniValue& params, bool fHelp); extern UniValue createrawzerocoinspend(const UniValue& params, bool fHelp); From a2f80711b461448496d4c709ef20c7d7e5bfeba3 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 3 Jun 2020 15:33:39 +0200 Subject: [PATCH 04/24] [wallet] CreateTransaction: simplify change address check - backports bitcoin/bitcoin@709f8685ac37510aa145ac259753583c82280038 --- src/wallet/wallet.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 28210eacfa9c..22370dac8f28 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -125,7 +125,7 @@ PairResult CWallet::getNewAddress(CTxDestination& ret, const std::string address if (!GetKeyFromPool(newKey, type)) { // inform the user to top-up the keypool or unlock the wallet return PairResult(false, new std::string( - "Keypool ran out, please call keypoolrefill first, or unlock the wallet.")); + _("Keypool ran out, please call keypoolrefill first, or unlock the wallet."))); } CKeyID keyID = newKey.GetID(); @@ -2560,9 +2560,10 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, // Reserve a new key pair from key pool CPubKey vchPubKey; - bool ret; - ret = reservekey.GetReservedKey(vchPubKey, true); - assert(ret); // should never fail, as we just unlocked + if (!reservekey.GetReservedKey(vchPubKey, true)) { + strFailReason = _("Can't generate a change-address key. Please call keypoolrefill first."); + return false; + } scriptChange = GetScriptForDestination(vchPubKey.GetID()); } From bc44ba0734aed357ab0223938bf5542c9c0ceda0 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 3 Jun 2020 15:44:17 +0200 Subject: [PATCH 05/24] [wallet] allow transaction without change if keypool is empty - backports bitcoin/bitcoin@92bcd70808b9cac56b184903aa6d37baf9641b37 --- src/wallet/wallet.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 22370dac8f28..43c4589149fc 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2447,6 +2447,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, wtxNew.fTimeReceivedIsTxTime = true; wtxNew.BindWallet(this); CMutableTransaction txNew; + CScript scriptChange; { LOCK2(cs_main, cs_wallet); @@ -2529,7 +2530,6 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, // Fill a vout to ourself // TODO: pass in scriptChange instead of reservekey so // change transaction isn't always pay-to-pivx-address - CScript scriptChange; bool combineChange = false; // coin control: send change to custom address @@ -2558,14 +2558,14 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, // rediscover unknown transactions that were written with keys of ours to recover // post-backup change. - // Reserve a new key pair from key pool + // Reserve a new key pair from key pool. If it fails, provide a dummy CPubKey vchPubKey; if (!reservekey.GetReservedKey(vchPubKey, true)) { strFailReason = _("Can't generate a change-address key. Please call keypoolrefill first."); - return false; + scriptChange = CScript(); + } else { + scriptChange = GetScriptForDestination(vchPubKey.GetID()); } - - scriptChange = GetScriptForDestination(vchPubKey.GetID()); } if (!combineChange) { @@ -2658,6 +2658,11 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, nFeeRet = nFeeNeeded; continue; } + + // Give up if change keypool ran out and we failed to find a solution without change: + if (scriptChange.empty() && nChangePosRet != -1) { + return false; + } } } return true; From ab407ff566adf7452d1c636982c21299ab3e3f64 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 2 Jun 2020 23:28:09 +0200 Subject: [PATCH 06/24] [Tests] Fix and enable fundrawtransaction functional tests --- test/functional/rpc_fundrawtransaction.py | 799 ++++++++-------------- test/functional/test_runner.py | 2 +- 2 files changed, 273 insertions(+), 528 deletions(-) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 906d59d26cd6..2170769beb2a 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -6,11 +6,13 @@ from test_framework.test_framework import PivxTestFramework from test_framework.util import ( assert_equal, + assert_raises_rpc_error, assert_fee_amount, assert_greater_than, count_bytes, connect_nodes, Decimal, + DecimalAmt, JSONRPCException, ) @@ -21,33 +23,44 @@ def get_unspent(listunspent, amount): return utx raise AssertionError('Could not find unspent with amount={}'.format(amount)) +def check_outputs(outputs, dec_tx): + for out in outputs: + found = False + for x in dec_tx['vout']: + if x['scriptPubKey']['addresses'][0] == out and \ + DecimalAmt(float(x['value'])) == DecimalAmt(outputs[out]): + found = True + break + if not found: + return False + return True + class RawTransactionsTest(PivxTestFramework): - def __init__(self): - super().__init__() + def set_test_params(self): + self.num_nodes = 3 self.setup_clean_chain = True - self.num_nodes = 4 - - def setup_network(self, split=False): - self.nodes = self.start_nodes() - connect_nodes(self.nodes[0], 1) - connect_nodes(self.nodes[1], 2) - connect_nodes(self.nodes[0], 2) - connect_nodes(self.nodes[0], 3) + def create_and_fund(self, nodeid, inputs, outputs): + rawtx = self.nodes[nodeid].createrawtransaction(inputs, outputs) + rawtxfund = self.nodes[nodeid].fundrawtransaction(rawtx) + for vin in self.nodes[nodeid].decoderawtransaction(rawtxfund['hex'])['vin']: + assert_equal(vin['scriptSig']['hex'], '') # not signed + rawtxsigned = self.nodes[nodeid].signrawtransaction(rawtxfund['hex']) + assert rawtxsigned['complete'] + return self.nodes[nodeid].decoderawtransaction(rawtxsigned['hex']), rawtxfund['fee'], rawtxfund['changepos'] - self.is_network_split=False - self.sync_all() + def get_tot_balance(self, nodeid): + wi = self.nodes[nodeid].getwalletinfo() + return wi['balance'] + wi['immature_balance'] def run_test(self): - print("Mining blocks...") - min_relay_tx_fee = self.nodes[0].getnetworkinfo()['relayfee'] # This test is not meant to test fee estimation and we'd like # to be sure all txs are sent at a consistent desired feerate for node in self.nodes: - node.settxfee(min_relay_tx_fee) + node.settxfee(float(min_relay_tx_fee)) # if the fee's positive delta is higher than this value tests will fail, # neg. delta always fail the tests. @@ -55,492 +68,297 @@ def run_test(self): # than a minimum sized signature. # = 2 bytes * minRelayTxFeePerByte - feeTolerance = 2 * min_relay_tx_fee/1000 + self.fee_tolerance = 2 * min_relay_tx_fee/1000 - self.nodes[2].generate(1) + print("Mining blocks...") + self.nodes[1].generate(1) self.sync_all() self.nodes[0].generate(121) self.sync_all() - - watchonly_address = self.nodes[0].getnewaddress() - watchonly_pubkey = self.nodes[0].validateaddress(watchonly_address)["pubkey"] - watchonly_amount = Decimal(200) - self.nodes[3].importpubkey(watchonly_pubkey, "", True) - watchonly_txid = self.nodes[0].sendtoaddress(watchonly_address, watchonly_amount) - self.nodes[0].sendtoaddress(self.nodes[3].getnewaddress(), watchonly_amount / 10) - self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.5) self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.0) self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 5.0) - + self.sync_all() self.nodes[0].generate(1) self.sync_all() - ############### - # simple test # - ############### - inputs = [ ] - outputs = { self.nodes[0].getnewaddress() : 1.0 } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) - fee = rawtxfund['fee'] - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - assert(len(dec_tx['vin']) > 0) #test if we have enought inputs - - ############################## - # simple test with two coins # - ############################## - inputs = [ ] - outputs = { self.nodes[0].getnewaddress() : 2.2 } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) - fee = rawtxfund['fee'] - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - assert(len(dec_tx['vin']) > 0) #test if we have enough inputs - - ############################## - # simple test with two coins # - ############################## - inputs = [ ] - outputs = { self.nodes[0].getnewaddress() : 2.6 } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) - fee = rawtxfund['fee'] - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - assert(len(dec_tx['vin']) > 0) - assert_equal(dec_tx['vin'][0]['scriptSig']['hex'], '') - - - ################################ - # simple test with two outputs # - ################################ - inputs = [ ] - outputs = { self.nodes[0].getnewaddress() : 2.6, self.nodes[1].getnewaddress() : 2.5 } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) - fee = rawtxfund['fee'] - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - totalOut = 0 - for out in dec_tx['vout']: - totalOut += out['value'] - - assert(len(dec_tx['vin']) > 0) - assert_equal(dec_tx['vin'][0]['scriptSig']['hex'], '') - - - ######################################################################### - # test a fundrawtransaction with a VIN greater than the required amount # - ######################################################################### + self.test_simple() + self.test_simple_two_coins() + self.test_simple_one_coin() + self.test_simple_two_outputs() + self.test_change() + self.test_no_change() + self.test_coin_selection() + self.test_two_vin() + self.test_two_vin_two_vout() + self.test_invalid_input() + self.test_fee_p2pkh() + self.test_fee_p2pkh_multi_out() + self.test_fee_p2sh() + self.test_fee_4of5() + self.test_spend_2of2() + self.test_locked_wallet() + self.test_many_inputs_fee() + self.test_many_inputs() + self.test_op_return() + + + def test_simple(self): + self.log.info("simple test") + dec_tx, fee, changepos = self.create_and_fund(2, [], {self.nodes[0].getnewaddress(): 1.0}) + assert_equal(len(dec_tx['vin']) > 0, True) # test if we have enought inputs + assert_greater_than(changepos, -1) # check change + assert_equal(Decimal(dec_tx['vout'][changepos]['value']) + fee, DecimalAmt(0.5)) + + def test_simple_two_coins(self): + self.log.info("simple test with two coins") + dec_tx, fee, changepos = self.create_and_fund(2, [], {self.nodes[0].getnewaddress(): 2.2}) + assert_equal(len(dec_tx['vin']) > 0, True) # test if we have enought inputs + assert_greater_than(changepos, -1) # check change + assert_equal(Decimal(dec_tx['vout'][changepos]['value']) + fee, DecimalAmt(0.3)) + + def test_simple_one_coin(self): + self.log.info("simple test with one coin") + dec_tx, fee, changepos = self.create_and_fund(2, [], {self.nodes[0].getnewaddress(): 2.6}) + assert_equal(len(dec_tx['vin']) > 0, True) # test if we have enought inputs + assert_greater_than(changepos, -1) # check change + assert_equal(Decimal(dec_tx['vout'][changepos]['value']) + fee, DecimalAmt(2.4)) + + def test_simple_two_outputs(self): + self.log.info("simple test with two outputs") + outputs = {self.nodes[0].getnewaddress(): 2.6, self.nodes[1].getnewaddress(): 2.5} + dec_tx, fee, changepos = self.create_and_fund(2, [], outputs) + assert_equal(len(dec_tx['vin']) > 0, True) # test if we have enought inputs + assert_greater_than(changepos, -1) # check change + assert_equal(Decimal(dec_tx['vout'][changepos]['value']) + fee, DecimalAmt(0.9)) + assert check_outputs(outputs, dec_tx) # check outputs + + def test_change(self): + self.log.info("test with a vin > required amount") utx = get_unspent(self.nodes[2].listunspent(), 5) - - inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']}] - outputs = { self.nodes[0].getnewaddress() : 1.0 } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) - fee = rawtxfund['fee'] - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - totalOut = 0 - for out in dec_tx['vout']: - totalOut += out['value'] - - assert_equal(fee + totalOut, utx['amount']) #compare vin total and totalout+fee - - - ##################################################################### - # test a fundrawtransaction with which will not get a change output # - ##################################################################### + inputs = [{'txid': utx['txid'], 'vout': utx['vout']}] + outputs = {self.nodes[0].getnewaddress(): 1.0} + dec_tx, fee, changepos = self.create_and_fund(2, inputs, outputs) + assert_equal(len(dec_tx['vin']) > 0, True) # test if we have enought inputs + assert_greater_than(changepos, -1) # check change + assert_equal(Decimal(dec_tx['vout'][changepos]['value']) + fee, DecimalAmt(4.0)) + assert check_outputs(outputs, dec_tx) # check outputs + self.test_no_change_fee = fee # Use the same fee for the next tx + + def test_no_change(self): + self.log.info("test with no change output") utx = get_unspent(self.nodes[2].listunspent(), 5) - - inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']}] - outputs = { self.nodes[0].getnewaddress() : Decimal(5.0) - fee - feeTolerance } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) - fee = rawtxfund['fee'] - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - totalOut = 0 - for out in dec_tx['vout']: - totalOut += out['value'] - - assert_equal(rawtxfund['changepos'], -1) - assert_equal(fee + totalOut, utx['amount']) #compare vin total and totalout+fee - - - #################################################### - # test a fundrawtransaction with an invalid option # - #################################################### - utx = get_unspent(self.nodes[2].listunspent(), 5) - - inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']} ] - outputs = { self.nodes[0].getnewaddress() : Decimal(4.0) } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - - try: - self.nodes[2].fundrawtransaction(rawtx, {'foo': 'bar'}) - raise AssertionError("Accepted invalid option foo") - except JSONRPCException as e: - assert("Unexpected key foo" in e.error['message']) - - - ############################################################ - # test a fundrawtransaction with an invalid change address # - ############################################################ - utx = get_unspent(self.nodes[2].listunspent(), 5) - - inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']} ] - outputs = { self.nodes[0].getnewaddress() : Decimal(4.0) } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - - try: - self.nodes[2].fundrawtransaction(rawtx, {'changeAddress': 'foobar'}) - raise AssertionError("Accepted invalid pivx address") - except JSONRPCException as e: - assert("changeAddress must be a valid pivx address" in e.error['message']) - - - ############################################################ - # test a fundrawtransaction with a provided change address # - ############################################################ - utx = get_unspent(self.nodes[2].listunspent(), 5) - - inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']} ] - outputs = { self.nodes[0].getnewaddress() : Decimal(4.0) } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - - change = self.nodes[2].getnewaddress() - try: - rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {'changeAddress': change, 'changePosition': 2}) - except JSONRPCException as e: - assert('changePosition out of bounds' == e.error['message']) - else: - assert(False) - rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {'changeAddress': change, 'changePosition': 0}) - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - out = dec_tx['vout'][0]; - assert_equal(change, out['scriptPubKey']['addresses'][0]) - - - ######################################################################### - # test a fundrawtransaction with a VIN smaller than the required amount # - ######################################################################### + inputs = [{'txid': utx['txid'], 'vout': utx['vout']}] + outputs = {self.nodes[0].getnewaddress(): 5.0 - float(self.test_no_change_fee) - float(self.fee_tolerance)} + dec_tx, fee, changepos = self.create_and_fund(2, inputs, outputs) + assert_equal(len(dec_tx['vin']) > 0, True) # test if we have enought inputs + assert_equal(changepos, -1) # check (no) change + assert check_outputs(outputs, dec_tx) # check outputs + + def test_coin_selection(self): + self.log.info("test with a vin < required amount") utx = get_unspent(self.nodes[2].listunspent(), 1) - - inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']}] - outputs = { self.nodes[0].getnewaddress() : 1.0 } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) + inputs = [{'txid': utx['txid'], 'vout': utx['vout']}] + outputs = {self.nodes[0].getnewaddress(): 1.0} + rawtx = self.nodes[2].createrawtransaction(inputs, outputs) # 4-byte version + 1-byte vin count + 36-byte prevout then script_len rawtx = rawtx[:82] + "0100" + rawtx[84:] - dec_tx = self.nodes[2].decoderawtransaction(rawtx) + dec_tx = self.nodes[2].decoderawtransaction(rawtx) assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) assert_equal("00", dec_tx['vin'][0]['scriptSig']['hex']) rawtxfund = self.nodes[2].fundrawtransaction(rawtx) fee = rawtxfund['fee'] - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - totalOut = 0 - matchingOuts = 0 - for i, out in enumerate(dec_tx['vout']): - totalOut += out['value'] - if out['scriptPubKey']['addresses'][0] in outputs: - matchingOuts+=1 - else: - assert_equal(i, rawtxfund['changepos']) - - assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - assert_equal("00", dec_tx['vin'][0]['scriptSig']['hex']) - - assert_equal(matchingOuts, 1) + changepos = rawtxfund['changepos'] + dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) + assert_equal(len(dec_tx['vin']) > 0, True) # test if we have enought inputs + assert_equal("00", dec_tx['vin'][0]['scriptSig']['hex']) # check vin sig again assert_equal(len(dec_tx['vout']), 2) + assert_greater_than(changepos, -1) # check change + assert_equal(Decimal(dec_tx['vout'][changepos]['value']) + fee, DecimalAmt(1.5)) + assert check_outputs(outputs, dec_tx) # check outputs - - ########################################### - # test a fundrawtransaction with two VINs # - ########################################### + def test_two_vin(self): + self.log.info("test with 2 vins") utx = get_unspent(self.nodes[2].listunspent(), 1) utx2 = get_unspent(self.nodes[2].listunspent(), 5) - - inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']},{'txid' : utx2['txid'], 'vout' : utx2['vout']} ] - outputs = { self.nodes[0].getnewaddress() : 6.0 } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) - fee = rawtxfund['fee'] - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - totalOut = 0 - matchingOuts = 0 - for out in dec_tx['vout']: - totalOut += out['value'] - if out['scriptPubKey']['addresses'][0] in outputs: - matchingOuts+=1 - - assert_equal(matchingOuts, 1) + inputs = [{'txid': utx['txid'], 'vout': utx['vout']}, {'txid': utx2['txid'], 'vout': utx2['vout']}] + outputs = {self.nodes[0].getnewaddress(): 6.0} + dec_tx, fee, changepos = self.create_and_fund(2, inputs, outputs) + assert_equal(len(dec_tx['vin']) > 0, True) # test if we have enought inputs + assert check_outputs(outputs, dec_tx) # check outputs assert_equal(len(dec_tx['vout']), 2) + matchingIns = len([x for x in dec_tx['vin'] if x['txid'] in [y['txid'] for y in inputs]]) + assert_equal(matchingIns, 2) # match 2 vins given as params + assert_greater_than(changepos, -1) # check change + assert_equal(Decimal(dec_tx['vout'][changepos]['value']) + fee, DecimalAmt(1.5)) - matchingIns = 0 - for vinOut in dec_tx['vin']: - for vinIn in inputs: - if vinIn['txid'] == vinOut['txid']: - matchingIns+=1 - - assert_equal(matchingIns, 2) #we now must see two vins identical to vins given as params - - ######################################################### - # test a fundrawtransaction with two VINs and two vOUTs # - ######################################################### + def test_two_vin_two_vout(self): + self.log.info("test with 2 vins and 2 vouts") utx = get_unspent(self.nodes[2].listunspent(), 1) utx2 = get_unspent(self.nodes[2].listunspent(), 5) - - inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']},{'txid' : utx2['txid'], 'vout' : utx2['vout']} ] - outputs = { self.nodes[0].getnewaddress() : 6.0, self.nodes[0].getnewaddress() : 1.0 } - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) - fee = rawtxfund['fee'] - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - totalOut = 0 - matchingOuts = 0 - for out in dec_tx['vout']: - totalOut += out['value'] - if out['scriptPubKey']['addresses'][0] in outputs: - matchingOuts+=1 - - assert_equal(matchingOuts, 2) + inputs = [{'txid': utx['txid'], 'vout': utx['vout']}, {'txid': utx2['txid'], 'vout': utx2['vout']}] + outputs = {self.nodes[0].getnewaddress(): 6.0, self.nodes[0].getnewaddress(): 1.0} + dec_tx, fee, changepos = self.create_and_fund(2, inputs, outputs) + assert_equal(len(dec_tx['vin']) > 0, True) # test if we have enought inputs + assert check_outputs(outputs, dec_tx) # check outputs assert_equal(len(dec_tx['vout']), 3) - - ############################################## - # test a fundrawtransaction with invalid vin # - ############################################## - listunspent = self.nodes[2].listunspent() - inputs = [ {'txid' : "1c7f966dab21119bac53213a2bc7532bff1fa844c124fd750a7d0b1332440bd1", 'vout' : 0} ] #invalid vin! - outputs = { self.nodes[0].getnewaddress() : 1.0} - rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - dec_tx = self.nodes[2].decoderawtransaction(rawtx) - - try: - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) - raise AssertionError("Spent more than available") - except JSONRPCException as e: - assert("Insufficient" in e.error['message']) - - - ############################################################ - #compare fee of a standard pubkeyhash transaction + matchingIns = len([x for x in dec_tx['vin'] if x['txid'] in [y['txid'] for y in inputs]]) + assert_equal(matchingIns, 2) # match 2 vins given as params + assert_greater_than(changepos, -1) # check change + assert_equal(Decimal(dec_tx['vout'][changepos]['value']) + fee, DecimalAmt(0.5)) + + def test_invalid_input(self): + self.log.info("test with invalid vin") + inputs = [{'txid': "1c7f966dab21119bac53213a2bc7532bff1fa844c124fd750a7d0b1332440bd1", 'vout': 0}] #invalid vin! + outputs = {self.nodes[0].getnewaddress(): 1.0} + rawtx = self.nodes[2].createrawtransaction(inputs, outputs) + assert_raises_rpc_error(-32603, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx) + + def test_fee_p2pkh(self): + """Compare fee of a standard pubkeyhash transaction.""" + self.log.info("test p2pkh fee") inputs = [] - outputs = {self.nodes[1].getnewaddress():1.1} - rawTx = self.nodes[0].createrawtransaction(inputs, outputs) - fundedTx = self.nodes[0].fundrawtransaction(rawTx) + outputs = {self.nodes[1].getnewaddress(): 1.1} + dec_tx, fee, changepos = self.create_and_fund(0, inputs, outputs) #create same transaction over sendtoaddress txId = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 1.1) signedFee = self.nodes[0].getrawmempool(True)[txId]['fee'] #compare fee - feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) - assert(feeDelta >= 0 and feeDelta <= feeTolerance) - ############################################################ + feeDelta = Decimal(fee) - Decimal(signedFee) + assert feeDelta >= 0 and feeDelta <= self.fee_tolerance + + def test_fee_p2pkh_multi_out(self): + """Compare fee of a standard pubkeyhash transaction with multiple outputs.""" + self.log.info("test p2pkh fee with multiple outputs") + outputs = {self.nodes[1].getnewaddress(): 1.1, + self.nodes[1].getnewaddress(): 1.2, + self.nodes[1].getnewaddress(): 0.1, + self.nodes[1].getnewaddress(): 1.3, + self.nodes[1].getnewaddress(): 0.2, + self.nodes[1].getnewaddress(): 0.3} + dec_tx, fee, changepos = self.create_and_fund(0, [], outputs) - ############################################################ - #compare fee of a standard pubkeyhash transaction with multiple outputs - inputs = [] - outputs = {self.nodes[1].getnewaddress():1.1,self.nodes[1].getnewaddress():1.2,self.nodes[1].getnewaddress():0.1,self.nodes[1].getnewaddress():1.3,self.nodes[1].getnewaddress():0.2,self.nodes[1].getnewaddress():0.3} - rawTx = self.nodes[0].createrawtransaction(inputs, outputs) - fundedTx = self.nodes[0].fundrawtransaction(rawTx) #create same transaction over sendtoaddress txId = self.nodes[0].sendmany("", outputs) signedFee = self.nodes[0].getrawmempool(True)[txId]['fee'] #compare fee - feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) - assert(feeDelta >= 0 and feeDelta <= feeTolerance) - ############################################################ - - - ############################################################ - #compare fee of a 2of2 multisig p2sh transaction - - # create 2of2 addr - addr1 = self.nodes[1].getnewaddress() - addr2 = self.nodes[1].getnewaddress() - - addr1Obj = self.nodes[1].validateaddress(addr1) - addr2Obj = self.nodes[1].validateaddress(addr2) - - mSigObj = self.nodes[1].addmultisigaddress(2, [addr1Obj['pubkey'], addr2Obj['pubkey']]) - - inputs = [] - outputs = {mSigObj:1.1} - rawTx = self.nodes[0].createrawtransaction(inputs, outputs) - fundedTx = self.nodes[0].fundrawtransaction(rawTx) - - #create same transaction over sendtoaddress - txId = self.nodes[0].sendtoaddress(mSigObj, 1.1) + feeDelta = Decimal(fee) - Decimal(signedFee) + assert feeDelta >= 0 and feeDelta <= self.fee_tolerance + + def test_fee_p2sh(self): + """Compare fee of a 2-of-2 multisig p2sh transaction.""" + self.log.info("test 2-of-2 multisig p2sh fee") + addrs = [self.nodes[1].getnewaddress() for _ in range(2)] + mSigAddr = self.nodes[1].addmultisigaddress(2, addrs) + outputs = {mSigAddr: 1.1} + dec_tx, fee, changepos = self.create_and_fund(0, [], outputs) + + # Create same transaction over sendtoaddress. + txId = self.nodes[0].sendtoaddress(mSigAddr, 1.1) signedFee = self.nodes[0].getrawmempool(True)[txId]['fee'] - #compare fee - feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) - assert(feeDelta >= 0 and feeDelta <= feeTolerance) - ############################################################ - - - ############################################################ - #compare fee of a standard pubkeyhash transaction - - # create 4of5 addr - addr1 = self.nodes[1].getnewaddress() - addr2 = self.nodes[1].getnewaddress() - addr3 = self.nodes[1].getnewaddress() - addr4 = self.nodes[1].getnewaddress() - addr5 = self.nodes[1].getnewaddress() - - addr1Obj = self.nodes[1].validateaddress(addr1) - addr2Obj = self.nodes[1].validateaddress(addr2) - addr3Obj = self.nodes[1].validateaddress(addr3) - addr4Obj = self.nodes[1].validateaddress(addr4) - addr5Obj = self.nodes[1].validateaddress(addr5) + # Compare fee. + feeDelta = Decimal(fee) - Decimal(signedFee) + assert feeDelta >= 0 and feeDelta <= self.fee_tolerance - mSigObj = self.nodes[1].addmultisigaddress(4, [addr1Obj['pubkey'], addr2Obj['pubkey'], addr3Obj['pubkey'], addr4Obj['pubkey'], addr5Obj['pubkey']]) + def test_fee_4of5(self): + """Compare fee of a a 4-of-5 multisig p2sh transaction.""" + self.log.info("test 4-of-5 addresses multisig fee") + addrs = [self.nodes[1].getnewaddress() for _ in range(5)] + mSigAddr = self.nodes[1].addmultisigaddress(4, addrs) + outputs = {mSigAddr: 1.1} + dec_tx, fee, changepos = self.create_and_fund(0, [], outputs) - inputs = [] - outputs = {mSigObj:1.1} - rawTx = self.nodes[0].createrawtransaction(inputs, outputs) - fundedTx = self.nodes[0].fundrawtransaction(rawTx) - - #create same transaction over sendtoaddress - txId = self.nodes[0].sendtoaddress(mSigObj, 1.1) + # Create same transaction over sendtoaddress. + txId = self.nodes[0].sendtoaddress(mSigAddr, 1.1) signedFee = self.nodes[0].getrawmempool(True)[txId]['fee'] - #compare fee - feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) - assert(feeDelta >= 0 and feeDelta <= feeTolerance) - ############################################################ - + # Compare fee. + feeDelta = Decimal(fee) - Decimal(signedFee) + assert feeDelta >= 0 and feeDelta <= self.fee_tolerance - ############################################################ - # spend a 2of2 multisig transaction over fundraw + def test_spend_2of2(self): + """Spend a 2-of-2 multisig transaction over fundraw.""" + self.log.info("test spending from 2-of-2 multisig") + addrs = [self.nodes[2].getnewaddress() for _ in range(2)] + mSigAddr = self.nodes[2].addmultisigaddress(2, addrs) - # create 2of2 addr - addr1 = self.nodes[2].getnewaddress() - addr2 = self.nodes[2].getnewaddress() - - addr1Obj = self.nodes[2].validateaddress(addr1) - addr2Obj = self.nodes[2].validateaddress(addr2) - - mSigObj = self.nodes[2].addmultisigaddress(2, [addr1Obj['pubkey'], addr2Obj['pubkey']]) - - - # send 1.2 BTC to msig addr - txId = self.nodes[0].sendtoaddress(mSigObj, 1.2) - self.sync_all() - self.nodes[1].generate(1) + # Send 50.1 PIV to mSigAddr. + self.nodes[0].sendtoaddress(mSigAddr, 50.1) + self.nodes[0].generate(1) self.sync_all() + # fund transaction with msigAddr fund oldBalance = self.nodes[1].getbalance() - inputs = [] - outputs = {self.nodes[1].getnewaddress():1.1} - rawTx = self.nodes[2].createrawtransaction(inputs, outputs) - fundedTx = self.nodes[2].fundrawtransaction(rawTx) - - signedTx = self.nodes[2].signrawtransaction(fundedTx['hex']) - txId = self.nodes[2].sendrawtransaction(signedTx['hex']) - self.sync_all() - self.nodes[1].generate(1) + outputs = {self.nodes[1].getnewaddress(): 50.0} + dec_tx, fee, changepos = self.create_and_fund(2, [], outputs) + self.nodes[2].sendrawtransaction(dec_tx['hex']) + self.nodes[2].generate(1) self.sync_all() - # make sure funds are received at node1 - assert_equal(oldBalance+Decimal('1.10000000'), self.nodes[1].getbalance()) - - ############################################################ - # locked wallet test - self.nodes[1].encryptwallet("test") - self.nodes.pop(1) - self.stop_nodes() + # Make sure funds are received at node1. + assert_equal(oldBalance + DecimalAmt(50.0), self.nodes[1].getbalance()) - self.nodes = self.start_nodes() - # This test is not meant to test fee estimation and we'd like - # to be sure all txs are sent at a consistent desired feerate - for node in self.nodes: - node.settxfee(min_relay_tx_fee) + def test_locked_wallet(self): + self.log.info("test locked wallet") + self.nodes[1].node_encrypt_wallet("test") + self.start_node(1) connect_nodes(self.nodes[0], 1) connect_nodes(self.nodes[1], 2) - connect_nodes(self.nodes[0], 2) - connect_nodes(self.nodes[0], 3) - self.is_network_split=False self.sync_all() - # drain the keypool + # Drain the keypool. self.nodes[1].getnewaddress() - inputs = [] - outputs = {self.nodes[0].getnewaddress():1.1} - rawTx = self.nodes[1].createrawtransaction(inputs, outputs) + self.nodes[1].getrawchangeaddress() + utx = get_unspent(self.nodes[1].listunspent(), DecimalAmt(250.0)) + inputs = [{'txid': utx['txid'], 'vout': utx['vout']}] + outputs = {self.nodes[0].getnewaddress(): 250.0 - float(self.test_no_change_fee) - float(self.fee_tolerance)} + rawtx = self.nodes[1].createrawtransaction(inputs, outputs) + # fund a transaction that does not require a new key for the change output + self.nodes[1].fundrawtransaction(rawtx) + # fund a transaction that requires a new key for the change output # creating the key must be impossible because the wallet is locked - try: - fundedTx = self.nodes[1].fundrawtransaction(rawTx) - raise AssertionError("Wallet unlocked without passphrase") - except JSONRPCException as e: - assert('Keypool ran out' in e.error['message']) + outputs = {self.nodes[0].getnewaddress(): 250.0} + rawtx = self.nodes[1].createrawtransaction([], outputs) + assert_raises_rpc_error(-32603, "Can't generate a change-address key. Please call keypoolrefill first.", self.nodes[1].fundrawtransaction, rawtx) - #refill the keypool + # Refill the keypool. self.nodes[1].walletpassphrase("test", 100) + self.nodes[1].keypoolrefill(8) #need to refill the keypool to get an internal change address self.nodes[1].walletlock() - try: - self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 1.2) - raise AssertionError("Wallet unlocked without passphrase") - except JSONRPCException as e: - assert('walletpassphrase' in e.error['message']) + assert_raises_rpc_error(-13, "walletpassphrase", self.nodes[1].sendtoaddress, self.nodes[0].getnewaddress(), 1.2) - oldBalance = self.nodes[0].getbalance() + oldBalance = self.get_tot_balance(0) + rawtx = self.nodes[1].createrawtransaction([], outputs) + fundedTx = self.nodes[1].fundrawtransaction(rawtx) - inputs = [] - outputs = {self.nodes[0].getnewaddress():1.1} - rawTx = self.nodes[1].createrawtransaction(inputs, outputs) - fundedTx = self.nodes[1].fundrawtransaction(rawTx) - - #now we need to unlock - self.nodes[1].walletpassphrase("test", 100) + # Now we need to unlock. + self.nodes[1].walletpassphrase("test", 600) signedTx = self.nodes[1].signrawtransaction(fundedTx['hex']) - txId = self.nodes[1].sendrawtransaction(signedTx['hex']) + self.nodes[1].sendrawtransaction(signedTx['hex']) self.nodes[1].generate(1) self.sync_all() - # make sure funds are received at node1 - assert_equal(oldBalance+Decimal('51.10000000'), self.nodes[0].getbalance()) + # Make sure funds are received at node0. + assert_equal(oldBalance + DecimalAmt(250.0), self.get_tot_balance(0)) + def test_many_inputs_int(self, fCompareFee): + """Multiple (~19) inputs tx test | Compare fee.""" + info = "compare fee" if fCompareFee else "send" + self.log.info("test with many inputs (%s)" % info) - ############################################### - # multiple (~19) inputs tx test | Compare fee # - ############################################### - - #empty node1, send some small coins from node0 to node1 - self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), self.nodes[1].getbalance(), "", "", True) - self.sync_all() - self.nodes[0].generate(1) + # Empty node1, send some small coins from node0 to node1. + self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), float(self.nodes[1].getbalance()) - 0.001) + self.nodes[1].generate(1) self.sync_all() for i in range(0,20): @@ -548,125 +366,52 @@ def run_test(self): self.nodes[0].generate(1) self.sync_all() - #fund a tx with ~20 small inputs - inputs = [] - outputs = {self.nodes[0].getnewaddress():0.15,self.nodes[0].getnewaddress():0.04} - rawTx = self.nodes[1].createrawtransaction(inputs, outputs) - fundedTx = self.nodes[1].fundrawtransaction(rawTx) + # Fund a tx with ~20 small inputs. + outputs = {self.nodes[0].getnewaddress():0.15, self.nodes[0].getnewaddress(): 0.04} + rawtx = self.nodes[1].createrawtransaction([], outputs) + fundedTx = self.nodes[1].fundrawtransaction(rawtx) - #create same transaction over sendtoaddress - txId = self.nodes[1].sendmany("", outputs) - signedFee = self.nodes[1].getrawmempool(True)[txId]['fee'] - - #compare fee - feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) - assert(feeDelta >= 0 and feeDelta <= feeTolerance*19) #~19 inputs - - - ############################################# - # multiple (~19) inputs tx test | sign/send # - ############################################# + if fCompareFee: + # Create same transaction over sendtoaddress. + txId = self.nodes[1].sendmany("", outputs) + signedFee = self.nodes[1].getrawmempool(True)[txId]['fee'] - #again, empty node1, send some small coins from node0 to node1 - self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), self.nodes[1].getbalance(), "", "", True) - self.sync_all() - self.nodes[0].generate(1) - self.sync_all() + # Compare fee. + feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) + assert feeDelta >= 0 and feeDelta <= self.fee_tolerance * 19 #~19 inputs - for i in range(0,20): - self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 0.01) - self.nodes[0].generate(1) - self.sync_all() + else: + # Sign and send the rawtransaction + oldBalance = self.get_tot_balance(0) + fundedAndSignedTx = self.nodes[1].signrawtransaction(fundedTx['hex']) + self.nodes[1].sendrawtransaction(fundedAndSignedTx['hex']) + self.nodes[1].generate(1) + self.sync_all() + assert_equal(oldBalance + DecimalAmt(0.19), self.get_tot_balance(0)) - #fund a tx with ~20 small inputs - oldBalance = self.nodes[0].getbalance() + def test_many_inputs_fee(self): + return self.test_many_inputs_int(True) - inputs = [] - outputs = {self.nodes[0].getnewaddress():0.15,self.nodes[0].getnewaddress():0.04} - rawTx = self.nodes[1].createrawtransaction(inputs, outputs) - fundedTx = self.nodes[1].fundrawtransaction(rawTx) - fundedAndSignedTx = self.nodes[1].signrawtransaction(fundedTx['hex']) - txId = self.nodes[1].sendrawtransaction(fundedAndSignedTx['hex']) - self.sync_all() - self.nodes[0].generate(1) - self.sync_all() - assert_equal(oldBalance+Decimal('50.19000000'), self.nodes[0].getbalance()) #0.19+block reward + def test_many_inputs(self): + return self.test_many_inputs_int(False) - ##################################################### - # test fundrawtransaction with OP_RETURN and no vin # - ##################################################### + def test_op_return(self): + self.log.info("test fundrawtxn with OP_RETURN and no vin") + # just add 0.001 PIV since we don't allow 0-value outputs + rawtx = "010000000001a086010000000000066a047465737400000000" - rawtx = "0100000000010000000000000000066a047465737400000000" - dec_tx = self.nodes[2].decoderawtransaction(rawtx) + dec_tx = self.nodes[2].decoderawtransaction(rawtx) assert_equal(len(dec_tx['vin']), 0) assert_equal(len(dec_tx['vout']), 1) rawtxfund = self.nodes[2].fundrawtransaction(rawtx) - dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - - assert_greater_than(len(dec_tx['vin']), 0) # at least one vin - assert_equal(len(dec_tx['vout']), 2) # one change output added - - - ################################################## - # test a fundrawtransaction using only watchonly # - ################################################## + dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) - inputs = [] - outputs = {self.nodes[2].getnewaddress() : watchonly_amount / 2} - rawtx = self.nodes[3].createrawtransaction(inputs, outputs) - - result = self.nodes[3].fundrawtransaction(rawtx, {'includeWatching': True }) - res_dec = self.nodes[0].decoderawtransaction(result["hex"]) - assert_equal(len(res_dec["vin"]), 1) - assert_equal(res_dec["vin"][0]["txid"], watchonly_txid) - - assert("fee" in result.keys()) - assert_greater_than(result["changepos"], -1) - - ############################################################### - # test fundrawtransaction using the entirety of watched funds # - ############################################################### - - inputs = [] - outputs = {self.nodes[2].getnewaddress() : watchonly_amount} - rawtx = self.nodes[3].createrawtransaction(inputs, outputs) - - # Backward compatibility test (2nd param is includeWatching) - result = self.nodes[3].fundrawtransaction(rawtx, True) - res_dec = self.nodes[0].decoderawtransaction(result["hex"]) - assert_equal(len(res_dec["vin"]), 2) - assert(res_dec["vin"][0]["txid"] == watchonly_txid or res_dec["vin"][1]["txid"] == watchonly_txid) - - assert_greater_than(result["fee"], 0) - assert_greater_than(result["changepos"], -1) - assert_equal(result["fee"] + res_dec["vout"][result["changepos"]]["value"], watchonly_amount / 10) - - signedtx = self.nodes[3].signrawtransaction(result["hex"]) - assert(not signedtx["complete"]) - signedtx = self.nodes[0].signrawtransaction(signedtx["hex"]) - assert(signedtx["complete"]) - self.nodes[0].sendrawtransaction(signedtx["hex"]) - self.nodes[0].generate(1) - self.sync_all() + assert_greater_than(len(dec_tx['vin']), 0) # at least one vin + assert_equal(len(dec_tx['vout']), 2) # one change output added - ####################### - # Test feeRate option # - ####################### - # Make sure there is exactly one input so coin selection can't skew the result - assert_equal(len(self.nodes[3].listunspent(1)), 1) - - inputs = [] - outputs = {self.nodes[2].getnewaddress() : 1} - rawtx = self.nodes[3].createrawtransaction(inputs, outputs) - result = self.nodes[3].fundrawtransaction(rawtx) # uses min_relay_tx_fee (set by settxfee) - result2 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2*min_relay_tx_fee}) - result3 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 10*min_relay_tx_fee}) - result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) - assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) - assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) if __name__ == '__main__': RawTransactionsTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 025835011c16..19e980065572 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -62,6 +62,7 @@ # vv Tests less than 5m vv 'wallet_zapwallettxes.py', # ~ 300 sec 'p2p_time_offset.py', # ~ 267 sec + 'rpc_fundrawtransaction.py', # ~ 222 sec 'mining_pos_coldStaking.py', # ~ 215 sec 'mining_pos_reorg.py', # ~ 212 sec 'wallet_abandonconflict.py', # ~ 212 sec @@ -110,7 +111,6 @@ # Don't append tests at the end to avoid merge conflicts # Put them in a random line within the section that fits their approximate run-time # 'feature_block.py', - # 'rpc_fundrawtransaction.py', # 'wallet_importmulti.py', # 'mempool_limit.py', # We currently don't limit our mempool_reorg # 'interface_zmq.py', From fab65563a2ac417323ae2f44abf9dfbcd6149c48 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 3 Jun 2020 23:05:23 +0200 Subject: [PATCH 07/24] Exempt unspendable transaction outputs from dust checks - backports bitcoin/bitcoin@0aad1f13b2430165062bf9436036c1222a8724da Since unspendable outputs can't be spent, there is no threshold at which it would be uneconomic to spend them. This primarily targets transaction outputs with `OP_RETURN`. --- src/primitives/transaction.h | 27 +++++++++++++++-------- test/functional/rpc_fundrawtransaction.py | 3 +-- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 5ef83e0c28fc..bae812a76ecd 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -167,17 +167,26 @@ class CTxOut uint256 GetHash() const; bool GetKeyIDFromUTXO(CKeyID& keyIDRet) const; - bool IsDust(CFeeRate minRelayTxFee) const + CAmount GetDustThreshold(const CFeeRate &minRelayTxFee) const { - // "Dust" is defined in terms of CTransaction::minRelayTxFee, which has units upiv-per-kilobyte. - // If you'd pay more than 1/3 in fees to spend something, then we consider it dust. - // A typical txout is 34 bytes big, and will need a CTxIn of at least 148 bytes to spend - // i.e. total is 148 + 32 = 182 bytes. Default -minrelaytxfee is 10000 upiv per kB - // and that means that fee per txout is 182 * 10000 / 1000 = 1820 upiv. - // So dust is a txout less than 1820 *3 = 5460 upiv - // with default -minrelaytxfee = minRelayTxFee = 10000 upiv per kB. + // "Dust" is defined in terms of CTransaction::minRelayTxFee, + // which has units satoshis-per-kilobyte. + // If you'd pay more than 1/3 in fees + // to spend something, then we consider it dust. + // A typical spendable txout is 34 bytes big, and will + // need a CTxIn of at least 148 bytes to spend: + // so dust is a spendable txout less than 546 satoshis + // with default minRelayTxFee. + if (scriptPubKey.IsUnspendable()) + return 0; + size_t nSize = GetSerializeSize(*this, SER_DISK, 0) + 148u; - return (nValue < 3*minRelayTxFee.GetFee(nSize)); + return 3 * minRelayTxFee.GetFee(nSize); + } + + bool IsDust(CFeeRate minRelayTxFee) const + { + return (nValue < GetDustThreshold(minRelayTxFee)); } bool IsZerocoinMint() const; diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 2170769beb2a..9dcee8e860e5 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -397,8 +397,7 @@ def test_many_inputs(self): def test_op_return(self): self.log.info("test fundrawtxn with OP_RETURN and no vin") - # just add 0.001 PIV since we don't allow 0-value outputs - rawtx = "010000000001a086010000000000066a047465737400000000" + rawtx = "0100000000010000000000000000066a047465737400000000" dec_tx = self.nodes[2].decoderawtransaction(rawtx) From 12b38b05bb488d78542bfb30a5253fe102eac0a1 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 02:38:09 +0200 Subject: [PATCH 08/24] Add have-pubkey distinction to ISMINE flags - backports bitcoin/bitcoin@d3354c52d7c0c6446cad4074c1d0e04bb1b3d84e This indicates that, eg, we have a public key for a key which may be used as a pay-to-pubkey-hash. It generally means that we can create a valid scriptSig except for missing private key(s) with which to create signatures. - backports bitcoin/bitcoin@428a898acd37e1c0afa21623a8fe5728859067be [squash commit] --- src/qt/transactiondesc.cpp | 4 ++-- src/wallet/wallet_ismine.cpp | 10 +++++++--- src/wallet/wallet_ismine.h | 5 ++++- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp index 8964497e4c0e..0450485b1287 100644 --- a/src/qt/transactiondesc.cpp +++ b/src/qt/transactiondesc.cpp @@ -178,7 +178,7 @@ QString TransactionDesc::toHTML(CWallet* wallet, CWalletTx& wtx, TransactionReco } if (fAllFromMe) { - if (fAllFromMe == ISMINE_WATCH_ONLY) + if (fAllFromMe & ISMINE_WATCH_ONLY) strHTML += "" + tr("From") + ": " + tr("watch-only") + "
"; // @@ -200,7 +200,7 @@ QString TransactionDesc::toHTML(CWallet* wallet, CWalletTx& wtx, TransactionReco strHTML += GUIUtil::HtmlEscape(EncodeDestination(address)); if (toSelf == ISMINE_SPENDABLE) strHTML += " (own address)"; - else if (toSelf == ISMINE_WATCH_ONLY) + else if (toSelf & ISMINE_WATCH_ONLY) strHTML += " (watch-only)"; strHTML += "
"; } diff --git a/src/wallet/wallet_ismine.cpp b/src/wallet/wallet_ismine.cpp index cbfda9b900da..092146b4927f 100644 --- a/src/wallet/wallet_ismine.cpp +++ b/src/wallet/wallet_ismine.cpp @@ -9,6 +9,7 @@ #include "key.h" #include "keystore.h" #include "script/script.h" +#include "script/sign.h" #include "script/standard.h" #include "util.h" @@ -39,7 +40,7 @@ isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey) txnouttype whichType; if(!Solver(scriptPubKey, whichType, vSolutions)) { if(keystore.HaveWatchOnly(scriptPubKey)) - return ISMINE_WATCH_ONLY; + return ISMINE_WATCH_UNSOLVABLE; return ISMINE_NO; } @@ -97,8 +98,11 @@ isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey) } } - if(keystore.HaveWatchOnly(scriptPubKey)) - return ISMINE_WATCH_ONLY; + if (keystore.HaveWatchOnly(scriptPubKey)) { + // TODO: This could be optimized some by doing some work after the above solver + CScript scriptSig; + return ProduceSignature(DummySignatureCreator(&keystore), scriptPubKey, scriptSig, false) ? ISMINE_WATCH_SOLVABLE : ISMINE_WATCH_UNSOLVABLE; + } return ISMINE_NO; } diff --git a/src/wallet/wallet_ismine.h b/src/wallet/wallet_ismine.h index a95067ee50ae..1b549b5fd576 100644 --- a/src/wallet/wallet_ismine.h +++ b/src/wallet/wallet_ismine.h @@ -17,7 +17,10 @@ class CScript; enum isminetype { ISMINE_NO = 0, //! Indicates that we dont know how to create a scriptSig that would solve this if we were given the appropriate private keys - ISMINE_WATCH_ONLY = 1, + ISMINE_WATCH_UNSOLVABLE = 1, + //! Indicates that we know how to create a scriptSig that would solve this if we were given the appropriate private keys + ISMINE_WATCH_SOLVABLE = 2, + ISMINE_WATCH_ONLY = ISMINE_WATCH_SOLVABLE | ISMINE_WATCH_UNSOLVABLE, ISMINE_SPENDABLE = 4, //! Indicates that we have the staking key of a P2CS ISMINE_COLD = 8, From cbffa8063f46a7d13ff772b3d5716ccf99897470 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 03:10:03 +0200 Subject: [PATCH 09/24] Add logic to track pubkeys as watch-only, not just scripts - backports bitcoin/bitcoin@cfc3dd34284357262bcc7eef2714a210891276c0 --- src/crypter.cpp | 5 ++++- src/keystore.cpp | 42 +++++++++++++++++++++++++++++++++++------- src/keystore.h | 5 ++++- 3 files changed, 43 insertions(+), 9 deletions(-) diff --git a/src/crypter.cpp b/src/crypter.cpp index abe7e9db88d3..6c81bc283cf5 100644 --- a/src/crypter.cpp +++ b/src/crypter.cpp @@ -199,13 +199,16 @@ bool CCryptoKeyStore::GetPubKey(const CKeyID& address, CPubKey& vchPubKeyOut) co { LOCK(cs_KeyStore); if (!IsCrypted()) - return CKeyStore::GetPubKey(address, vchPubKeyOut); + return CBasicKeyStore::GetPubKey(address, vchPubKeyOut); CryptedKeyMap::const_iterator mi = mapCryptedKeys.find(address); if (mi != mapCryptedKeys.end()) { vchPubKeyOut = (*mi).second.first; return true; } + + // Check for watch-only pubkeys + return CBasicKeyStore::GetPubKey(address, vchPubKeyOut); } return false; } diff --git a/src/keystore.cpp b/src/keystore.cpp index b9dc12454d6a..87d2015be5e6 100644 --- a/src/keystore.cpp +++ b/src/keystore.cpp @@ -13,20 +13,26 @@ #include "util.h" -bool CKeyStore::GetPubKey(const CKeyID& address, CPubKey& vchPubKeyOut) const +bool CKeyStore::AddKey(const CKey& key) +{ + return AddKeyPubKey(key, key.GetPubKey()); +} + +bool CBasicKeyStore::GetPubKey(const CKeyID &address, CPubKey &vchPubKeyOut) const { CKey key; - if (!GetKey(address, key)) + if (!GetKey(address, key)) { + WatchKeyMap::const_iterator it = mapWatchKeys.find(address); + if (it != mapWatchKeys.end()) { + vchPubKeyOut = it->second; + return true; + } return false; + } vchPubKeyOut = key.GetPubKey(); return true; } -bool CKeyStore::AddKey(const CKey& key) -{ - return AddKeyPubKey(key, key.GetPubKey()); -} - bool CBasicKeyStore::AddKeyPubKey(const CKey& key, const CPubKey& pubkey) { LOCK(cs_KeyStore); @@ -61,10 +67,29 @@ bool CBasicKeyStore::GetCScript(const CScriptID& hash, CScript& redeemScriptOut) return false; } +static bool ExtractPubKey(const CScript &dest, CPubKey& pubKeyOut) +{ + //TODO: Use Solver to extract this? + CScript::const_iterator pc = dest.begin(); + opcodetype opcode; + std::vector vch; + if (!dest.GetOp(pc, opcode, vch) || vch.size() < 33 || vch.size() > 65) + return false; + pubKeyOut = CPubKey(vch); + if (!pubKeyOut.IsFullyValid()) + return false; + if (!dest.GetOp(pc, opcode, vch) || opcode != OP_CHECKSIG || dest.GetOp(pc, opcode, vch)) + return false; + return true; +} + bool CBasicKeyStore::AddWatchOnly(const CScript& dest) { LOCK(cs_KeyStore); setWatchOnly.insert(dest); + CPubKey pubKey; + if (ExtractPubKey(dest, pubKey)) + mapWatchKeys[pubKey.GetID()] = pubKey; return true; } @@ -72,6 +97,9 @@ bool CBasicKeyStore::RemoveWatchOnly(const CScript& dest) { LOCK(cs_KeyStore); setWatchOnly.erase(dest); + CPubKey pubKey; + if (ExtractPubKey(dest, pubKey)) + mapWatchKeys.erase(pubKey.GetID()); return true; } diff --git a/src/keystore.h b/src/keystore.h index 3b72e59816ef..832c7137bb45 100644 --- a/src/keystore.h +++ b/src/keystore.h @@ -34,7 +34,7 @@ class CKeyStore virtual bool HaveKey(const CKeyID& address) const = 0; virtual bool GetKey(const CKeyID& address, CKey& keyOut) const = 0; virtual void GetKeys(std::set& setAddress) const = 0; - virtual bool GetPubKey(const CKeyID& address, CPubKey& vchPubKeyOut) const; + virtual bool GetPubKey(const CKeyID& address, CPubKey& vchPubKeyOut) const = 0; //! Support for BIP 0013 : see https://github.com/bitcoin/bips/blob/master/bip-0013.mediawiki virtual bool AddCScript(const CScript& redeemScript) = 0; @@ -49,6 +49,7 @@ class CKeyStore }; typedef std::map KeyMap; +typedef std::map WatchKeyMap; typedef std::map ScriptMap; typedef std::set WatchOnlySet; @@ -57,11 +58,13 @@ class CBasicKeyStore : public CKeyStore { protected: KeyMap mapKeys; + WatchKeyMap mapWatchKeys; ScriptMap mapScripts; WatchOnlySet setWatchOnly; public: bool AddKeyPubKey(const CKey& key, const CPubKey& pubkey); + bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const; bool HaveKey(const CKeyID& address) const; void GetKeys(std::set& setAddress) const; bool GetKey(const CKeyID& address, CKey& keyOut) const; From 60a20a458fb916884255a4b4f3156170be6016d4 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 03:31:40 +0200 Subject: [PATCH 10/24] Split up importaddress into helper functions - backports bitcoin/bitcoin@983d2d90af1b517bee51170d2ea059e68d09be35 --- src/wallet/rpcdump.cpp | 75 +++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 542ce39adaa9..9010b4805a73 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -151,6 +151,27 @@ UniValue importprivkey(const UniValue& params, bool fHelp) return NullUniValue; } +void ImportScript(const CScript& script) +{ + if (::IsMine(*pwalletMain, script) == ISMINE_SPENDABLE) + throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script"); + + pwalletMain->MarkDirty(); + + if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script)) + throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); +} + +void ImportAddress(const CTxDestination& dest, const std::string& strLabel, const std::string& strPurpose) +{ + CScript script = GetScriptForDestination(dest); + ImportScript(script); + // add to address book or update label + if (IsValidDestination(dest)) { + pwalletMain->SetAddressBook(dest, strLabel, strPurpose); + } +} + UniValue importaddress(const UniValue& params, bool fHelp) { if (fHelp || params.size() < 1 || params.size() > 3) @@ -173,22 +194,6 @@ UniValue importaddress(const UniValue& params, bool fHelp) "\nAs a JSON-RPC call\n" + HelpExampleRpc("importaddress", "\"myaddress\", \"testing\", false")); - LOCK2(cs_main, pwalletMain->cs_wallet); - - CScript script; - - bool isStakingAddress = false; - CTxDestination dest = DecodeDestination(params[0].get_str(), isStakingAddress); - bool isAddressValid = IsValidDestination(dest); - if (isAddressValid) { - script = GetScriptForDestination(dest); - } else if (IsHex(params[0].get_str())) { - std::vector data(ParseHex(params[0].get_str())); - script = CScript(data.begin(), data.end()); - } else { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid PIVX address or script"); - } - std::string strLabel = ""; if (params.size() > 1) strLabel = params[1].get_str(); @@ -198,31 +203,27 @@ UniValue importaddress(const UniValue& params, bool fHelp) if (params.size() > 2) fRescan = params[2].get_bool(); - { - if (::IsMine(*pwalletMain, script) & ISMINE_SPENDABLE_ALL) - throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script"); - - // add to address book or update label - if (isAddressValid) { - pwalletMain->SetAddressBook(dest, strLabel, - (isStakingAddress ? - AddressBook::AddressBookPurpose::COLD_STAKING : - AddressBook::AddressBookPurpose::RECEIVE)); - } + LOCK2(cs_main, pwalletMain->cs_wallet); - // Don't throw error in case an address is already there - if (pwalletMain->HaveWatchOnly(script)) - return NullUniValue; + bool isStakingAddress = false; + CTxDestination dest = DecodeDestination(params[0].get_str(), isStakingAddress); - pwalletMain->MarkDirty(); + if (IsValidDestination(dest)) { + ImportAddress(dest, strLabel, isStakingAddress ? + AddressBook::AddressBookPurpose::COLD_STAKING : + AddressBook::AddressBookPurpose::RECEIVE); - if (!pwalletMain->AddWatchOnly(script)) - throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); + } else if (IsHex(params[0].get_str())) { + std::vector data(ParseHex(params[0].get_str())); + ImportScript(CScript(data.begin(), data.end())); - if (fRescan) { - pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true); - pwalletMain->ReacceptWalletTransactions(); - } + } else { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid PIVX address or script"); + } + + if (fRescan) { + pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true); + pwalletMain->ReacceptWalletTransactions(); } return NullUniValue; From 816dabbb9f8a9e68f2081be7024df75ca33a70de Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 03:49:03 +0200 Subject: [PATCH 11/24] Add p2sh option to importaddress to import redeemScripts --- src/rpc/client.cpp | 1 + src/wallet/rpcdump.cpp | 32 +++++++++++++++---------- test/functional/rpc_listtransactions.py | 10 ++++++++ 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 57434ead1ca5..f1d2b96bc1fe 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -112,6 +112,7 @@ static const CRPCConvertParam vRPCConvertParams[] = {"importprivkey", 2}, {"importprivkey", 3}, {"importaddress", 2}, + {"importaddress", 3}, {"verifychain", 0}, {"verifychain", 1}, {"keypoolrefill", 0}, diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 9010b4805a73..aebeefeb0a22 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -151,21 +151,29 @@ UniValue importprivkey(const UniValue& params, bool fHelp) return NullUniValue; } -void ImportScript(const CScript& script) +void ImportAddress(const CTxDestination& dest, const std::string& strLabel, const std::string& strPurpose); + +void ImportScript(const CScript& script, const std::string& strLabel, bool isRedeemScript) { - if (::IsMine(*pwalletMain, script) == ISMINE_SPENDABLE) + if (!isRedeemScript && ::IsMine(*pwalletMain, script) == ISMINE_SPENDABLE) throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script"); pwalletMain->MarkDirty(); if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script)) throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); + + if (isRedeemScript) { + if (!pwalletMain->HaveCScript(script) && !pwalletMain->AddCScript(script)) + throw JSONRPCError(RPC_WALLET_ERROR, "Error adding p2sh redeemScript to wallet"); + ImportAddress(CScriptID(script), strLabel, "receive"); + } } void ImportAddress(const CTxDestination& dest, const std::string& strLabel, const std::string& strPurpose) { CScript script = GetScriptForDestination(dest); - ImportScript(script); + ImportScript(script, strLabel, false); // add to address book or update label if (IsValidDestination(dest)) { pwalletMain->SetAddressBook(dest, strLabel, strPurpose); @@ -174,7 +182,7 @@ void ImportAddress(const CTxDestination& dest, const std::string& strLabel, cons UniValue importaddress(const UniValue& params, bool fHelp) { - if (fHelp || params.size() < 1 || params.size() > 3) + if (fHelp || params.size() < 1 || params.size() > 4) throw std::runtime_error( "importaddress \"address\" ( \"label\" rescan )\n" "\nAdds an address or script (in hex) that can be watched as if it were in your wallet but cannot be used to spend.\n" @@ -183,6 +191,7 @@ UniValue importaddress(const UniValue& params, bool fHelp) "1. \"address\" (string, required) The address\n" "2. \"label\" (string, optional, default=\"\") An optional label\n" "3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n" + "4. p2sh (boolean, optional, default=false) Add the P2SH version of the script as well\n" "\nNote: This call can take minutes to complete if rescan is true.\n" @@ -194,14 +203,11 @@ UniValue importaddress(const UniValue& params, bool fHelp) "\nAs a JSON-RPC call\n" + HelpExampleRpc("importaddress", "\"myaddress\", \"testing\", false")); - std::string strLabel = ""; - if (params.size() > 1) - strLabel = params[1].get_str(); - + const std::string strLabel = (params.size() > 1 ? params[1].get_str() : ""); // Whether to perform rescan after import - bool fRescan = true; - if (params.size() > 2) - fRescan = params[2].get_bool(); + const bool fRescan = (params.size() > 2 ? params[2].get_bool() : true); + // Whether to import a p2sh version, too + const bool fP2SH = (params.size() > 3 ? params[3].get_bool() : false); LOCK2(cs_main, pwalletMain->cs_wallet); @@ -209,13 +215,15 @@ UniValue importaddress(const UniValue& params, bool fHelp) CTxDestination dest = DecodeDestination(params[0].get_str(), isStakingAddress); if (IsValidDestination(dest)) { + if (fP2SH) + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Cannot use the p2sh flag with an address - use a script instead"); ImportAddress(dest, strLabel, isStakingAddress ? AddressBook::AddressBookPurpose::COLD_STAKING : AddressBook::AddressBookPurpose::RECEIVE); } else if (IsHex(params[0].get_str())) { std::vector data(ParseHex(params[0].get_str())); - ImportScript(CScript(data.begin(), data.end())); + ImportScript(CScript(data.begin(), data.end()), strLabel, fP2SH); } else { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid PIVX address or script"); diff --git a/test/functional/rpc_listtransactions.py b/test/functional/rpc_listtransactions.py index 969be8227178..c5153841473d 100755 --- a/test/functional/rpc_listtransactions.py +++ b/test/functional/rpc_listtransactions.py @@ -81,6 +81,16 @@ def run_test(self): {"category":"receive","amount":Decimal("0.44")}, {"txid":txid, "account" : "toself"} ) + multisig = self.nodes[1].createmultisig(1, [self.nodes[1].getnewaddress()]) + self.nodes[0].importaddress(multisig["redeemScript"], "watchonly", False, True) + txid = self.nodes[1].sendtoaddress(multisig["address"], 0.1) + self.nodes[1].generate(1) + self.sync_all() + assert(len(self.nodes[0].listtransactions("watchonly", 100, 0, False)) == 0) + assert_array_result(self.nodes[0].listtransactions("watchonly", 100, 0, True), + {"category": "receive", "amount": Decimal("0.1")}, + {"txid": txid, "account": "watchonly"}) + if __name__ == '__main__': ListTransactionsTest().main() From 7b4eb6d1ef8bb459349a4e625d981ee0f95db70b Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 03:58:12 +0200 Subject: [PATCH 12/24] Add importpubkey method to import a watch-only pubkey - backports bitcoin/bitcoin@a1d7df32360605724d8f0ea4b7aebfa7aea24c97 --- src/rpc/client.cpp | 1 + src/rpc/server.cpp | 1 + src/rpc/server.h | 1 + src/wallet/rpcdump.cpp | 44 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 47 insertions(+) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index f1d2b96bc1fe..fb1fd2d3b269 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -113,6 +113,7 @@ static const CRPCConvertParam vRPCConvertParams[] = {"importprivkey", 3}, {"importaddress", 2}, {"importaddress", 3}, + {"importpubkey", 2}, {"verifychain", 0}, {"verifychain", 1}, {"keypoolrefill", 0}, diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 3e1553ce5dac..453b7fff0fda 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -425,6 +425,7 @@ static const CRPCCommand vRPCCommands[] = {"wallet", "importprivkey", &importprivkey, true, false, true}, {"wallet", "importwallet", &importwallet, true, false, true}, {"wallet", "importaddress", &importaddress, true, false, true}, + {"wallet", "importpubkey", &importpubkey, true, false, true}, {"wallet", "keypoolrefill", &keypoolrefill, true, false, true}, {"wallet", "listaccounts", &listaccounts, false, false, true}, {"wallet", "listdelegators", &listdelegators, false, false, true}, diff --git a/src/rpc/server.h b/src/rpc/server.h index 7fa3c2e17bdc..a536557314a4 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -195,6 +195,7 @@ extern UniValue clearbanned(const UniValue& params, bool fHelp); extern UniValue dumpprivkey(const UniValue& params, bool fHelp); // in rpcdump.cpp extern UniValue importprivkey(const UniValue& params, bool fHelp); extern UniValue importaddress(const UniValue& params, bool fHelp); +extern UniValue importpubkey(const UniValue& params, bool fHelp); extern UniValue dumpwallet(const UniValue& params, bool fHelp); extern UniValue importwallet(const UniValue& params, bool fHelp); extern UniValue bip38encrypt(const UniValue& params, bool fHelp); diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index aebeefeb0a22..c0ff8f52e59b 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -237,6 +237,50 @@ UniValue importaddress(const UniValue& params, bool fHelp) return NullUniValue; } +UniValue importpubkey(const UniValue& params, bool fHelp) +{ + if (fHelp || params.size() < 1 || params.size() > 4) + throw std::runtime_error( + "importpubkey \"pubkey\" ( \"label\" rescan )\n" + "\nAdds a public key (in hex) that can be watched as if it were in your wallet but cannot be used to spend.\n" + "\nArguments:\n" + "1. \"pubkey\" (string, required) The hex-encoded public key\n" + "2. \"label\" (string, optional, default=\"\") An optional label\n" + "3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n" + "\nNote: This call can take minutes to complete if rescan is true.\n" + "\nExamples:\n" + "\nImport a public key with rescan\n" + + HelpExampleCli("importpubkey", "\"mypubkey\"") + + "\nImport using a label without rescan\n" + + HelpExampleCli("importpubkey", "\"mypubkey\" \"testing\" false") + + "\nAs a JSON-RPC call\n" + + HelpExampleRpc("importpubkey", "\"mypubkey\", \"testing\", false") + ); + + const std::string strLabel = (params.size() > 1 ? params[1].get_str() : ""); + // Whether to perform rescan after import + const bool fRescan = (params.size() > 2 ? params[2].get_bool() : true); + + if (!IsHex(params[0].get_str())) + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey must be a hex string"); + std::vector data(ParseHex(params[0].get_str())); + CPubKey pubKey(data.begin(), data.end()); + if (!pubKey.IsFullyValid()) + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key"); + + LOCK2(cs_main, pwalletMain->cs_wallet); + + ImportAddress(pubKey.GetID(), strLabel, "receive"); + ImportScript(GetScriptForRawPubKey(pubKey), strLabel, false); + + if (fRescan) { + pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true); + pwalletMain->ReacceptWalletTransactions(); + } + + return NullUniValue; +} + // TODO: Needs further review over the HD flow, staking addresses and multisig import. UniValue importwallet(const UniValue& params, bool fHelp) { From 1b153e5cddf13ebe3ae8f2bf07bcb37338275288 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 04:00:20 +0200 Subject: [PATCH 13/24] Update importaddress help to push its use to script-only - backports bitcoin/bitcoin@5c17059872c9b63a1e05c7aa8aea32a03c3ec73a --- src/wallet/rpcdump.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index c0ff8f52e59b..73bbaeb3fddb 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -184,24 +184,25 @@ UniValue importaddress(const UniValue& params, bool fHelp) { if (fHelp || params.size() < 1 || params.size() > 4) throw std::runtime_error( - "importaddress \"address\" ( \"label\" rescan )\n" - "\nAdds an address or script (in hex) that can be watched as if it were in your wallet but cannot be used to spend.\n" + "importaddress \"script\" ( \"label\" rescan )\n" + "\nAdds a script (in hex), or address, that can be watched as if it were in your wallet but cannot be used to spend.\n" "\nArguments:\n" - "1. \"address\" (string, required) The address\n" + "1. \"script\" (string, required) hex-encoded script (or address)\n" "2. \"label\" (string, optional, default=\"\") An optional label\n" "3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n" "4. p2sh (boolean, optional, default=false) Add the P2SH version of the script as well\n" "\nNote: This call can take minutes to complete if rescan is true.\n" + "If you have the full public key, you should call importpublickey instead of this.\n" "\nExamples:\n" - "\nImport an address with rescan\n" + - HelpExampleCli("importaddress", "\"myaddress\"") + + "\nImport a script with rescan\n" + + HelpExampleCli("importaddress", "\"myscript\"") + "\nImport using a label without rescan\n" + - HelpExampleCli("importaddress", "\"myaddress\" \"testing\" false") + + HelpExampleCli("importaddress", "\"myscript\" \"testing\" false") + "\nAs a JSON-RPC call\n" + - HelpExampleRpc("importaddress", "\"myaddress\", \"testing\", false")); + HelpExampleRpc("importaddress", "\"myscript\", \"testing\", false")); const std::string strLabel = (params.size() > 1 ? params[1].get_str() : ""); // Whether to perform rescan after import From 134c5d2c1a13f51f14b3b25938ae56e52ee0b2e5 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 04:14:44 +0200 Subject: [PATCH 14/24] Implement watchonly support in fundrawtransaction - backports bitcoin/bitcoin@6bdb474dc9dd34e1a5b13ce9494a936cba77e027 Some code and test cases stolen from Bryan Bishop (pull bitcoin/bitcoin#5524). - backports bitcoin/bitcoin@d04285430d1b54b3ce3d50ffa67b6098157e7c14 [squash commit] --- src/coincontrol.h | 2 +- src/rpc/rawtransaction.cpp | 16 ++++++++++++---- src/wallet/wallet.cpp | 4 +++- src/wallet/wallet.h | 2 +- 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/coincontrol.h b/src/coincontrol.h index f00191456a81..8973fa7406a6 100644 --- a/src/coincontrol.h +++ b/src/coincontrol.h @@ -36,7 +36,7 @@ class CCoinControl setSelected.clear(); useSwiftTX = false; fAllowOtherInputs = false; - fAllowWatchOnly = true; + fAllowWatchOnly = false; nMinimumTotalFee = 0; fSplitBlock = false; nSplitBlock = 1; diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 65bd312ff236..a865d7ad17aa 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -555,15 +555,21 @@ static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std:: UniValue fundrawtransaction(const UniValue& params, bool fHelp) { - if (fHelp || params.size() != 1) + if (fHelp || params.size() < 1 || params.size() > 2) throw std::runtime_error( - "fundrawtransaction \"hexstring\"\n" + "fundrawtransaction \"hexstring\" ( includeWatching )\n" "\nAdd inputs to a transaction until it has enough in value to meet its out value.\n" "This will not modify existing inputs, and will add one change output to the outputs.\n" "Note that inputs which were signed may need to be resigned after completion since in/outputs have been added.\n" "The inputs added will not be signed, use signrawtransaction for that.\n" + "Note that all existing inputs must have their previous output transaction be in the wallet.\n" + "Note that all inputs selected must be of standard form and P2SH scripts must be" + "in the wallet using importaddress or addmultisigaddress (to calculate fees).\n" + "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n" + "\nArguments:\n" "1. \"hexstring\" (string, required) The hex string of the raw transaction\n" + "2. includeWatching (boolean, optional, default false) Also select inputs which are watch only\n" "\nResult:\n" "{\n" " \"hex\": \"value\", (string) The resulting raw transaction (hex-encoded string)\n" @@ -585,18 +591,20 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) if (!pwalletMain) throw std::runtime_error("wallet not initialized"); - RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)); + RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VBOOL)); // parse hex string from parameter CTransaction origTx; if (!DecodeHexTx(origTx, params[0].get_str())) throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + const bool includeWatching = params.size() > 1 && params[1].get_bool(); + CMutableTransaction tx(origTx); CAmount nFee; std::string strFailReason; int nChangePos = -1; - if(!pwalletMain->FundTransaction(tx, nFee, nChangePos, strFailReason)) + if(!pwalletMain->FundTransaction(tx, nFee, nChangePos, strFailReason, includeWatching)) throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason); UniValue result(UniValue::VOBJ); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 43c4589149fc..c28ca90f2f10 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2078,6 +2078,7 @@ bool CWallet::AvailableCoins(std::vector* pCoins, // --> populates bool fIsValid = ( ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || + (coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO) || ((mine & ((fIncludeColdStaking ? ISMINE_COLD : ISMINE_NO) | (fIncludeDelegated ? ISMINE_SPENDABLE_DELEGATED : ISMINE_NO) )) != ISMINE_NO)); @@ -2375,7 +2376,7 @@ bool CWallet::GetBudgetFinalizationCollateralTX(CWalletTx& tx, uint256 hash, boo return true; } -bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosRet, std::string& strFailReason) +bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosRet, std::string& strFailReason, bool includeWatching) { std::vector vecSend; @@ -2387,6 +2388,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nC CCoinControl coinControl; coinControl.fAllowOtherInputs = true; + coinControl.fAllowWatchOnly = includeWatching; for (const CTxIn& txin : tx.vin) coinControl.Select(txin.prevout); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1951a976821e..d9aae620495b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -460,7 +460,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface CAmount GetUnconfirmedWatchOnlyBalance() const; CAmount GetImmatureWatchOnlyBalance() const; CAmount GetLockedWatchOnlyBalance() const; - bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosRet, std::string& strFailReason); + bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosRet, std::string& strFailReason, bool includeWatching); bool CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, From 76c8d54d8a5c8a3114a134b9ff41f78640627924 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 04:47:16 +0200 Subject: [PATCH 15/24] [QA] Test watchonly addrs in fundrawtransaction tests --- test/functional/rpc_fundrawtransaction.py | 59 +++++++++++++++++++++-- test/functional/test_framework/util.py | 11 +++++ test/functional/test_runner.py | 2 +- 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 9dcee8e860e5..369cb708a827 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -7,13 +7,11 @@ from test_framework.util import ( assert_equal, assert_raises_rpc_error, - assert_fee_amount, assert_greater_than, - count_bytes, connect_nodes, + find_vout_for_address, Decimal, DecimalAmt, - JSONRPCException, ) @@ -39,7 +37,7 @@ def check_outputs(outputs, dec_tx): class RawTransactionsTest(PivxTestFramework): def set_test_params(self): - self.num_nodes = 3 + self.num_nodes = 4 self.setup_clean_chain = True def create_and_fund(self, nodeid, inputs, outputs): @@ -75,6 +73,19 @@ def run_test(self): self.sync_all() self.nodes[0].generate(121) self.sync_all() + + watchonly_address = self.nodes[0].getnewaddress() + watchonly_pubkey = self.nodes[0].validateaddress(watchonly_address)["pubkey"] + self.watchonly_amount = DecimalAmt(200.0) + self.nodes[3].importpubkey(watchonly_pubkey, "", True) + self.watchonly_txid = self.nodes[0].sendtoaddress(watchonly_address, float(self.watchonly_amount)) + + # Lock UTXO so nodes[0] doesn't accidentally spend it + self.watchonly_vout = find_vout_for_address(self.nodes[0], self.watchonly_txid, watchonly_address) + self.nodes[0].lockunspent(False, [{"txid": self.watchonly_txid, "vout": self.watchonly_vout}]) + + self.nodes[0].sendtoaddress(self.nodes[3].getnewaddress(), float(self.watchonly_amount) / 10) + self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.5) self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.0) self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 5.0) @@ -101,6 +112,8 @@ def run_test(self): self.test_many_inputs_fee() self.test_many_inputs() self.test_op_return() + self.test_watchonly() + self.test_all_watched_funds() def test_simple(self): @@ -410,6 +423,44 @@ def test_op_return(self): assert_greater_than(len(dec_tx['vin']), 0) # at least one vin assert_equal(len(dec_tx['vout']), 2) # one change output added + def test_watchonly(self): + self.log.info("test using only watchonly") + + outputs = {self.nodes[2].getnewaddress(): float(self.watchonly_amount) / 2} + rawtx = self.nodes[3].createrawtransaction([], outputs) + + result = self.nodes[3].fundrawtransaction(rawtx, True) + res_dec = self.nodes[0].decoderawtransaction(result["hex"]) + assert_equal(len(res_dec["vin"]), 1) + assert_equal(res_dec["vin"][0]["txid"], self.watchonly_txid) + + assert "fee" in result.keys() + assert_greater_than(result["changepos"], -1) + + def test_all_watched_funds(self): + self.log.info("test using entirety of watched funds") + + outputs = {self.nodes[2].getnewaddress(): float(self.watchonly_amount)} + rawtx = self.nodes[3].createrawtransaction([], outputs) + + # Backward compatibility test (2nd param is includeWatching). + result = self.nodes[3].fundrawtransaction(rawtx, True) + res_dec = self.nodes[0].decoderawtransaction(result["hex"]) + assert_equal(len(res_dec["vin"]), 2) + assert res_dec["vin"][0]["txid"] == self.watchonly_txid or res_dec["vin"][1]["txid"] == self.watchonly_txid + + assert_greater_than(result["fee"], 0) + assert_greater_than(result["changepos"], -1) + assert_equal(result["fee"] + res_dec["vout"][result["changepos"]]["value"], float(self.watchonly_amount) / 10) + + signedtx = self.nodes[3].signrawtransaction(result["hex"]) + assert not signedtx["complete"] + signedtx = self.nodes[0].signrawtransaction(signedtx["hex"]) + assert signedtx["complete"] + self.nodes[0].sendrawtransaction(signedtx["hex"]) + self.nodes[0].generate(1) + self.sync_all() + if __name__ == '__main__': diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 5484bcff1828..7db33468f844 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -591,6 +591,17 @@ def mine_large_block(node, utxos=None): create_lots_of_big_transactions(node, txouts, utxos, num, fee=fee) node.generate(1) +def find_vout_for_address(node, txid, addr): + """ + Locate the vout index of the given transaction sending to the + given address. Raises runtime error exception if not found. + """ + tx = node.getrawtransaction(txid, True) + for i in range(len(tx["vout"])): + if any([addr == a for a in tx["vout"][i]["scriptPubKey"]["addresses"]]): + return i + raise RuntimeError("Vout not found for address: txid=%s, addr=%s" % (txid, addr)) + ### PIVX specific utils ### vZC_DENOMS = [1, 5, 10, 50, 100, 500, 1000, 5000] DEFAULT_FEE = 0.01 diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 19e980065572..a52e5a53f59d 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -62,7 +62,7 @@ # vv Tests less than 5m vv 'wallet_zapwallettxes.py', # ~ 300 sec 'p2p_time_offset.py', # ~ 267 sec - 'rpc_fundrawtransaction.py', # ~ 222 sec + 'rpc_fundrawtransaction.py', # ~ 260 sec 'mining_pos_coldStaking.py', # ~ 215 sec 'mining_pos_reorg.py', # ~ 212 sec 'wallet_abandonconflict.py', # ~ 212 sec From d655b427dea579aa4c6a5bb8df250047f4a55d9c Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 07:06:28 +0200 Subject: [PATCH 16/24] Use CCoinControl selection in CWallet::FundTransaction - backports bitcoin@d6cc6a1830bb7e03701488ca30c46457434dec6c --- src/coincontrol.h | 5 ++--- src/qt/coincontroldialog.cpp | 2 +- src/wallet/wallet.cpp | 11 ++--------- 3 files changed, 5 insertions(+), 13 deletions(-) diff --git a/src/coincontrol.h b/src/coincontrol.h index 8973fa7406a6..a2a7f4f13ddc 100644 --- a/src/coincontrol.h +++ b/src/coincontrol.h @@ -47,10 +47,9 @@ class CCoinControl return (!setSelected.empty()); } - bool IsSelected(const uint256& hash, unsigned int n) const + bool IsSelected(const COutPoint& output) const { - COutPoint outpt(hash, n); - return (setSelected.count(outpt) > 0); + return (setSelected.count(output) > 0); } void Select(const COutPoint& output) diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index 19982d81a21b..db2125af24d8 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -901,7 +901,7 @@ void CoinControlDialog::updateView() } // set checkbox - if (coinControl->IsSelected(txhash, out.i)) + if (coinControl->IsSelected(COutPoint(txhash, out.i))) itemOutput->setCheckState(COLUMN_CHECKBOX, Qt::Checked); // outputs delegated (for cold staking) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c28ca90f2f10..37da9a655769 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2065,7 +2065,7 @@ bool CWallet::AvailableCoins(std::vector* pCoins, // --> populates (mine == ISMINE_WATCH_ONLY && nWatchonlyConfig == 1) || (IsLockedCoin((*it).first, i) && nCoinType != ONLY_10000) || (pcoin->vout[i].nValue <= 0 && !fIncludeZeroValue) || - (fCoinsSelected && !coinControl->fAllowOtherInputs && !coinControl->IsSelected((*it).first, i)) + (fCoinsSelected && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(COutPoint((*it).first, i))) ) continue; // --Skip P2CS outputs @@ -2402,14 +2402,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nC // Add new txins (keeping original txin scriptSig/order) for (const CTxIn& txin : wtx.vin) { - bool found = false; - for (const CTxIn& origTxIn : tx.vin) { - if (txin.prevout.hash == origTxIn.prevout.hash && txin.prevout.n == origTxIn.prevout.n) { - found = true; - break; - } - } - if (!found) + if (!coinControl.IsSelected(txin.prevout)) tx.vin.push_back(txin); } From 0c1f7ba2e1dca93779de18fe13fc1b9fbff1ed2d Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 05:44:35 +0200 Subject: [PATCH 17/24] Add strict flag to RPCTypeCheckObj - backports bitcoin/bitcoin@f7dc1d32bbd23ff593e63f575a2d446a96e1ea15 Strict flag forces type check on all object keys. --- src/rpc/server.cpp | 12 +++++++++++- src/rpc/server.h | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 453b7fff0fda..e4d44519e27c 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -92,7 +92,8 @@ void RPCTypeCheck(const UniValue& params, void RPCTypeCheckObj(const UniValue& o, const std::map& typesExpected, - bool fAllowNull) + bool fAllowNull, + bool fStrict) { for (const PAIRTYPE(std::string, UniValue::VType)& t : typesExpected) { const UniValue& v = find_value(o, t.first); @@ -105,6 +106,15 @@ void RPCTypeCheckObj(const UniValue& o, throw JSONRPCError(RPC_TYPE_ERROR, err); } } + + if (fStrict) { + for (const std::string& k : o.getKeys()) { + if (typesExpected.count(k) == 0) { + std::string err = strprintf("Unexpected key %s", k); + throw JSONRPCError(RPC_TYPE_ERROR, err); + } + } + } } static inline int64_t roundint64(double d) diff --git a/src/rpc/server.h b/src/rpc/server.h index a536557314a4..48cbf64cf40d 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -70,7 +70,7 @@ void RPCTypeCheck(const UniValue& params, * Use like: RPCTypeCheckObj(object, boost::assign::map_list_of("name", str_type)("value", int_type)); */ void RPCTypeCheckObj(const UniValue& o, - const std::map& typesExpected, bool fAllowNull=false); + const std::map& typesExpected, bool fAllowNull=false, bool fStrict=false); /** Opaque base class for timers returned by NewTimerFunc. * This provides no methods at the moment, but makes sure that delete From a3ac19174681d9c0265c510e3699c844524846d8 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 06:51:19 +0200 Subject: [PATCH 18/24] Add change options to fundrawtransaction - backports bitcoin/bitcoin@db992eadbcbde0ca14df66c120a5868938e61e83 [PIVX: Don't keep backward compatibility (as this is introduced and updated in the same PR)] --- src/qt/walletmodel.cpp | 4 +- src/rpc/rawtransaction.cpp | 45 ++++++++++++++++---- src/wallet/rpcwallet.cpp | 4 +- src/wallet/wallet.cpp | 50 +++++++++++++---------- src/wallet/wallet.h | 9 +++- test/functional/rpc_fundrawtransaction.py | 5 +-- 6 files changed, 80 insertions(+), 37 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 9067907d4bab..e1d795ddd1fb 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -473,7 +473,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact transaction.newPossibleKeyChange(wallet); CAmount nFeeRequired = 0; - int nChangePosRet; + int nChangePosInOut = -1; std::string strFailReason; CWalletTx* newTx = transaction.getTransaction(); @@ -490,7 +490,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact *newTx, *keyChange, nFeeRequired, - nChangePosRet, + nChangePosInOut, strFailReason, coinControl, recipients[0].inputType, diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index a865d7ad17aa..32f51af147c6 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -557,7 +557,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) { if (fHelp || params.size() < 1 || params.size() > 2) throw std::runtime_error( - "fundrawtransaction \"hexstring\" ( includeWatching )\n" + "fundrawtransaction \"hexstring\" ( options )\n" "\nAdd inputs to a transaction until it has enough in value to meet its out value.\n" "This will not modify existing inputs, and will add one change output to the outputs.\n" "Note that inputs which were signed may need to be resigned after completion since in/outputs have been added.\n" @@ -569,7 +569,12 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) "\nArguments:\n" "1. \"hexstring\" (string, required) The hex string of the raw transaction\n" - "2. includeWatching (boolean, optional, default false) Also select inputs which are watch only\n" + "2. options (object, optional)\n" + " {\n" + " \"changeAddress\" (string, optional, default pool address) The PIVX address to receive the change\n" + " \"changePosition\" (numeric, optional, default random) The index of the change output\n" + " \"includeWatching\" (boolean, optional, default false) Also select inputs which are watch only\n" + " }\n" "\nResult:\n" "{\n" " \"hex\": \"value\", (string) The resulting raw transaction (hex-encoded string)\n" @@ -591,25 +596,51 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) if (!pwalletMain) throw std::runtime_error("wallet not initialized"); - RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VBOOL)); + RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)); + + CTxDestination changeAddress = CNoDestination(); + int changePosition = -1; + bool includeWatching = false; + + if (params.size() > 1) { + RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VOBJ)); + UniValue options = params[1]; + RPCTypeCheckObj(options, boost::assign::map_list_of("changeAddress", UniValue::VSTR)("changePosition", UniValue::VNUM)("includeWatching", UniValue::VBOOL), true, true); + + if (options.exists("changeAddress")) { + changeAddress = DecodeDestination(options["changeAddress"].get_str()); + + if (!IsValidDestination(changeAddress)) + throw JSONRPCError(RPC_INVALID_PARAMETER, "changeAddress must be a valid PIVX address"); + } + + if (options.exists("changePosition")) + changePosition = options["changePosition"].get_int(); + + if (options.exists("includeWatching")) + includeWatching = options["includeWatching"].get_bool(); + } // parse hex string from parameter CTransaction origTx; if (!DecodeHexTx(origTx, params[0].get_str())) throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); - const bool includeWatching = params.size() > 1 && params[1].get_bool(); + if (origTx.vout.size() == 0) + throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output"); + + if (changePosition != -1 && (changePosition < 0 || (unsigned int) changePosition > origTx.vout.size())) + throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); CMutableTransaction tx(origTx); CAmount nFee; std::string strFailReason; - int nChangePos = -1; - if(!pwalletMain->FundTransaction(tx, nFee, nChangePos, strFailReason, includeWatching)) + if(!pwalletMain->FundTransaction(tx, nFee, changePosition, strFailReason, includeWatching, changeAddress)) throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason); UniValue result(UniValue::VOBJ); result.push_back(Pair("hex", EncodeHexTx(tx))); - result.push_back(Pair("changepos", nChangePos)); + result.push_back(Pair("changepos", changePosition)); result.push_back(Pair("fee", ValueFromAmount(nFee))); return result; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 3ad287d061a5..5cb2d3fe8d94 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1689,8 +1689,8 @@ UniValue sendmany(const UniValue& params, bool fHelp) CReserveKey keyChange(pwalletMain); CAmount nFeeRequired = 0; std::string strFailReason; - int nChangePosRet; - bool fCreated = pwalletMain->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, nChangePosRet, strFailReason); + int nChangePosInOut = -1; + bool fCreated = pwalletMain->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, nChangePosInOut, strFailReason); if (!fCreated) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason); const CWallet::CommitResult& res = pwalletMain->CommitTransaction(wtx, keyChange); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 37da9a655769..c3aecbb76a5a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2342,8 +2342,8 @@ bool CWallet::GetBudgetSystemCollateralTX(CWalletTx& tx, uint256 hash, bool useI vecSend.push_back(CRecipient{scriptChange, BUDGET_FEE_TX_OLD, false}); // Old 50 PIV collateral CCoinControl* coinControl = NULL; - int nChangePosRet; - bool success = CreateTransaction(vecSend, tx, reservekey, nFeeRet, nChangePosRet, strFail, coinControl, ALL_COINS, true, useIX, (CAmount)0); + int nChangePosInOut = -1; + bool success = CreateTransaction(vecSend, tx, reservekey, nFeeRet, nChangePosInOut, strFail, coinControl, ALL_COINS, true, useIX, (CAmount)0); if (!success) { LogPrintf("GetBudgetSystemCollateralTX: Error - %s\n", strFail); return false; @@ -2366,8 +2366,8 @@ bool CWallet::GetBudgetFinalizationCollateralTX(CWalletTx& tx, uint256 hash, boo vecSend.push_back(CRecipient{scriptChange, BUDGET_FEE_TX, false}); // New 5 PIV collateral CCoinControl* coinControl = NULL; - int nChangePosRet; - bool success = CreateTransaction(vecSend, tx, reservekey, nFeeRet, nChangePosRet, strFail, coinControl, ALL_COINS, true, useIX, (CAmount)0); + int nChangePosInOut = -1; + bool success = CreateTransaction(vecSend, tx, reservekey, nFeeRet, nChangePosInOut, strFail, coinControl, ALL_COINS, true, useIX, (CAmount)0); if (!success) { LogPrintf("GetBudgetSystemCollateralTX: Error - %s\n", strFail); return false; @@ -2376,7 +2376,7 @@ bool CWallet::GetBudgetFinalizationCollateralTX(CWalletTx& tx, uint256 hash, boo return true; } -bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosRet, std::string& strFailReason, bool includeWatching) +bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, const CTxDestination& destChange) { std::vector vecSend; @@ -2387,6 +2387,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nC } CCoinControl coinControl; + coinControl.destChange = destChange; coinControl.fAllowOtherInputs = true; coinControl.fAllowWatchOnly = includeWatching; for (const CTxIn& txin : tx.vin) @@ -2394,11 +2395,11 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nC CReserveKey reservekey(this); CWalletTx wtx; - if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosRet, strFailReason, &coinControl, ALL_COINS, false)) + if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, &coinControl, ALL_COINS, false)) return false; - if (nChangePosRet != -1) - tx.vout.insert(tx.vout.begin() + nChangePosRet, wtx.vout[nChangePosRet]); + if (nChangePosInOut != -1) + tx.vout.insert(tx.vout.begin() + nChangePosInOut, wtx.vout[nChangePosInOut]); // Add new txins (keeping original txin scriptSig/order) for (const CTxIn& txin : wtx.vin) { @@ -2413,7 +2414,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, - int& nChangePosRet, + int& nChangePosInOut, std::string& strFailReason, const CCoinControl* coinControl, AvailableCoinsType coin_type, @@ -2423,9 +2424,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, bool fIncludeDelegated) { if (useIX && nFeePay < CENT) nFeePay = CENT; - nChangePosRet = -1; CAmount nValue = 0; + int nChangePosRequest = nChangePosInOut; for (const CRecipient& rec : vecSend) { if (rec.nAmount < 0) { @@ -2450,6 +2451,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, nFeeRet = 0; if (nFeePay > 0) nFeeRet = nFeePay; while (true) { + nChangePosInOut = nChangePosRequest; txNew.vin.clear(); txNew.vout.clear(); wtxNew.fFromMe = true; @@ -2572,10 +2574,16 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, nFeeRet += nChange; nChange = 0; reservekey.ReturnKey(); + nChangePosInOut = -1; } else { - // Insert change txn at random position: - nChangePosRet = GetRandInt(txNew.vout.size() + 1); - std::vector::iterator position = txNew.vout.begin() + nChangePosRet; + if (nChangePosInOut == -1) { + // Insert change txn at random position: + nChangePosInOut = GetRandInt(txNew.vout.size()+1); + } else if (nChangePosInOut < 0 || (unsigned int) nChangePosInOut > txNew.vout.size()) { + strFailReason = _("Change index out of range"); + return false; + } + std::vector::iterator position = txNew.vout.begin() + nChangePosInOut; txNew.vout.insert(position, newTxOut); } } @@ -2655,7 +2663,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, } // Give up if change keypool ran out and we failed to find a solution without change: - if (scriptChange.empty() && nChangePosRet != -1) { + if (scriptChange.empty() && nChangePosInOut != -1) { return false; } } @@ -2667,8 +2675,8 @@ bool CWallet::CreateTransaction(CScript scriptPubKey, const CAmount& nValue, CWa { std::vector vecSend; vecSend.push_back(CRecipient{scriptPubKey, nValue, false}); - int nChangePosRet; - return CreateTransaction(vecSend, wtxNew, reservekey, nFeeRet, nChangePosRet, strFailReason, coinControl, coin_type, true, useIX, nFeePay, fIncludeDelegated); + int nChangePosInOut = -1; + return CreateTransaction(vecSend, wtxNew, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, coin_type, true, useIX, nFeePay, fIncludeDelegated); } bool CWallet::CreateCoinStake( @@ -3559,12 +3567,12 @@ void CWallet::AutoCombineDust() CReserveKey keyChange(this); // this change address does not end up being used, because change is returned with coin control switch std::string strErr; CAmount nFeeRet = 0; - int nChangePosRet; + int nChangePosInOut = -1; // 10% safety margin to avoid "Insufficient funds" errors vecSend[0].nAmount = nTotalRewardsValue - (nTotalRewardsValue / 10); - if (!CreateTransaction(vecSend, wtx, keyChange, nFeeRet, nChangePosRet, strErr, coinControl, ALL_COINS, true, false, CAmount(0))) { + if (!CreateTransaction(vecSend, wtx, keyChange, nFeeRet, nChangePosInOut, strErr, coinControl, ALL_COINS, true, false, CAmount(0))) { LogPrintf("AutoCombineDust createtransaction failed, reason: %s\n", strErr); continue; } @@ -3663,8 +3671,8 @@ bool CWallet::MultiSend() //get the fee amount CWalletTx wtxdummy; std::string strErr; - int nChangePosRet; - CreateTransaction(vecSend, wtxdummy, keyChange, nFeeRet, nChangePosRet, strErr, &cControl, ALL_COINS, true, false, CAmount(0)); + int nChangePosInOut = -1; + CreateTransaction(vecSend, wtxdummy, keyChange, nFeeRet, nChangePosInOut, strErr, &cControl, ALL_COINS, true, false, CAmount(0)); CAmount nLastSendAmount = vecSend[vecSend.size() - 1].nAmount; if (nLastSendAmount < nFeeRet + 500) { LogPrintf("%s: fee of %d is too large to insert into last output\n", __func__, nFeeRet + 500); @@ -3673,7 +3681,7 @@ bool CWallet::MultiSend() vecSend[vecSend.size() - 1].nAmount = nLastSendAmount - nFeeRet - 500; // Create the transaction and commit it to the network - if (!CreateTransaction(vecSend, wtx, keyChange, nFeeRet, int nChangePosRet, strErr, &cControl, ALL_COINS, true, false, CAmount(0))) { + if (!CreateTransaction(vecSend, wtx, keyChange, nFeeRet, int nChangePosInOut, strErr, &cControl, ALL_COINS, true, false, CAmount(0))) { LogPrintf("MultiSend createtransaction failed\n"); return false; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d9aae620495b..3d4741ba4922 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -460,12 +460,17 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface CAmount GetUnconfirmedWatchOnlyBalance() const; CAmount GetImmatureWatchOnlyBalance() const; CAmount GetLockedWatchOnlyBalance() const; - bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosRet, std::string& strFailReason, bool includeWatching); + bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, const CTxDestination& destChange = CNoDestination()); + /** + * Create a new transaction paying the recipients with a set of coins + * selected by SelectCoins(); Also create the change output, when needed + * @note passing nChangePosInOut as -1 will result in setting a random position + */ bool CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, - int& nChangePosRet, + int& nChangePosInOut, std::string& strFailReason, const CCoinControl* coinControl = NULL, AvailableCoinsType coin_type = ALL_COINS, diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 369cb708a827..4aa37fac8022 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -429,7 +429,7 @@ def test_watchonly(self): outputs = {self.nodes[2].getnewaddress(): float(self.watchonly_amount) / 2} rawtx = self.nodes[3].createrawtransaction([], outputs) - result = self.nodes[3].fundrawtransaction(rawtx, True) + result = self.nodes[3].fundrawtransaction(rawtx, {"includeWatching": True}) res_dec = self.nodes[0].decoderawtransaction(result["hex"]) assert_equal(len(res_dec["vin"]), 1) assert_equal(res_dec["vin"][0]["txid"], self.watchonly_txid) @@ -443,8 +443,7 @@ def test_all_watched_funds(self): outputs = {self.nodes[2].getnewaddress(): float(self.watchonly_amount)} rawtx = self.nodes[3].createrawtransaction([], outputs) - # Backward compatibility test (2nd param is includeWatching). - result = self.nodes[3].fundrawtransaction(rawtx, True) + result = self.nodes[3].fundrawtransaction(rawtx, {"includeWatching": True}) res_dec = self.nodes[0].decoderawtransaction(result["hex"]) assert_equal(len(res_dec["vin"]), 2) assert res_dec["vin"][0]["txid"] == self.watchonly_txid or res_dec["vin"][1]["txid"] == self.watchonly_txid From bc9dc67f606e85a3c62ed7fa8cf0bef88d352486 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 07:17:55 +0200 Subject: [PATCH 19/24] Add lockUnspents option to fundrawtransaction - backports bitcoin/bitcoin@18be394cd8cf5b021e25fff34066f1a3a002858c --- src/rpc/rawtransaction.cpp | 9 +++++++-- src/wallet/wallet.cpp | 10 ++++++++-- src/wallet/wallet.h | 2 +- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 32f51af147c6..369587495cbe 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -574,6 +574,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) " \"changeAddress\" (string, optional, default pool address) The PIVX address to receive the change\n" " \"changePosition\" (numeric, optional, default random) The index of the change output\n" " \"includeWatching\" (boolean, optional, default false) Also select inputs which are watch only\n" + " \"lockUnspents\" (boolean, optional, default false) Lock selected unspent outputs\n" " }\n" "\nResult:\n" "{\n" @@ -601,11 +602,12 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) CTxDestination changeAddress = CNoDestination(); int changePosition = -1; bool includeWatching = false; + bool lockUnspents = false; if (params.size() > 1) { RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VOBJ)); UniValue options = params[1]; - RPCTypeCheckObj(options, boost::assign::map_list_of("changeAddress", UniValue::VSTR)("changePosition", UniValue::VNUM)("includeWatching", UniValue::VBOOL), true, true); + RPCTypeCheckObj(options, boost::assign::map_list_of("changeAddress", UniValue::VSTR)("changePosition", UniValue::VNUM)("includeWatching", UniValue::VBOOL)("lockUnspents", UniValue::VBOOL), true, true); if (options.exists("changeAddress")) { changeAddress = DecodeDestination(options["changeAddress"].get_str()); @@ -619,6 +621,9 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) if (options.exists("includeWatching")) includeWatching = options["includeWatching"].get_bool(); + + if (options.exists("lockUnspents")) + lockUnspents = options["lockUnspents"].get_bool(); } // parse hex string from parameter @@ -635,7 +640,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) CMutableTransaction tx(origTx); CAmount nFee; std::string strFailReason; - if(!pwalletMain->FundTransaction(tx, nFee, changePosition, strFailReason, includeWatching, changeAddress)) + if(!pwalletMain->FundTransaction(tx, nFee, changePosition, strFailReason, includeWatching, lockUnspents, changeAddress)) throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason); UniValue result(UniValue::VOBJ); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c3aecbb76a5a..ed6dd228d61d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2376,7 +2376,7 @@ bool CWallet::GetBudgetFinalizationCollateralTX(CWalletTx& tx, uint256 hash, boo return true; } -bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, const CTxDestination& destChange) +bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange) { std::vector vecSend; @@ -2403,8 +2403,14 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC // Add new txins (keeping original txin scriptSig/order) for (const CTxIn& txin : wtx.vin) { - if (!coinControl.IsSelected(txin.prevout)) + if (!coinControl.IsSelected(txin.prevout)) { tx.vin.push_back(txin); + + if (lockUnspents) { + LOCK2(cs_main, cs_wallet); + LockCoin(txin.prevout); + } + } } return true; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 3d4741ba4922..0769051bdae8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -460,7 +460,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface CAmount GetUnconfirmedWatchOnlyBalance() const; CAmount GetImmatureWatchOnlyBalance() const; CAmount GetLockedWatchOnlyBalance() const; - bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, const CTxDestination& destChange = CNoDestination()); + bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange = CNoDestination()); /** * Create a new transaction paying the recipients with a set of coins * selected by SelectCoins(); Also create the change output, when needed From 87dbdf889fbd6f0129468f9efa4740860551f74b Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 07:39:37 +0200 Subject: [PATCH 20/24] [QA] Test new options in rpc_fundrawtransaction functional test --- test/functional/rpc_fundrawtransaction.py | 30 +++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 4aa37fac8022..a5ddb09501ab 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -99,6 +99,9 @@ def run_test(self): self.test_simple_two_outputs() self.test_change() self.test_no_change() + self.test_invalid_option() + self.test_invalid_change_address() + self.test_valid_change_address() self.test_coin_selection() self.test_two_vin() self.test_two_vin_two_vout() @@ -168,6 +171,33 @@ def test_no_change(self): assert_equal(changepos, -1) # check (no) change assert check_outputs(outputs, dec_tx) # check outputs + def test_invalid_option(self): + self.log.info("test with an invalid option") + outputs = {self.nodes[0].getnewaddress(): 1.0} + rawtx = self.nodes[2].createrawtransaction([], outputs) + assert_raises_rpc_error(-3, "Unexpected key foo", + self.nodes[2].fundrawtransaction, rawtx, {'foo': 'bar'}) + + def test_invalid_change_address(self): + self.log.info("test with an invalid change address") + outputs = {self.nodes[0].getnewaddress(): 1.0} + rawtx = self.nodes[2].createrawtransaction([], outputs) + assert_raises_rpc_error(-8, "changeAddress must be a valid PIVX address", + self.nodes[2].fundrawtransaction, rawtx, {'changeAddress': 'foobar'}) + + def test_valid_change_address(self): + self.log.info("test with a provided change address") + outputs = {self.nodes[0].getnewaddress(): 1.0} + rawtx = self.nodes[2].createrawtransaction([], outputs) + change = self.nodes[2].getnewaddress() + assert_raises_rpc_error(-8, "changePosition out of bounds", + self.nodes[2].fundrawtransaction, rawtx, {'changeAddress': change, + 'changePosition': 2}) + rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {'changeAddress': change, 'changePosition': 0}) + dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) + out = dec_tx['vout'][0] + assert_equal(change, out['scriptPubKey']['addresses'][0]) + def test_coin_selection(self): self.log.info("test with a vin < required amount") utx = get_unspent(self.nodes[2].listunspent(), 1) From 169bc3b4d0fb925f05133a231a477d291fb5d48a Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 07:54:54 +0200 Subject: [PATCH 21/24] [RPC] add feerate option to fundrawtransaction - backports bitcoin/bitcoin@3b35e4896b5b8be9ffd6dacddb081f69a5b77903 --- src/coincontrol.h | 3 +++ src/rpc/rawtransaction.cpp | 15 ++++++++++----- src/wallet/wallet.cpp | 10 +++++++++- src/wallet/wallet.h | 2 +- 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/coincontrol.h b/src/coincontrol.h index a2a7f4f13ddc..c753c547f8fe 100644 --- a/src/coincontrol.h +++ b/src/coincontrol.h @@ -24,6 +24,8 @@ class CCoinControl bool fAllowWatchOnly; //! Minimum absolute fee (not per kilobyte) CAmount nMinimumTotalFee; + //! Feerate to use (0 = estimate fee with payTxFee fallback) + CFeeRate nFeeRate; CCoinControl() { @@ -38,6 +40,7 @@ class CCoinControl fAllowOtherInputs = false; fAllowWatchOnly = false; nMinimumTotalFee = 0; + nFeeRate = CFeeRate(0); fSplitBlock = false; nSplitBlock = 1; } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 369587495cbe..643760206800 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -563,7 +563,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) "Note that inputs which were signed may need to be resigned after completion since in/outputs have been added.\n" "The inputs added will not be signed, use signrawtransaction for that.\n" "Note that all existing inputs must have their previous output transaction be in the wallet.\n" - "Note that all inputs selected must be of standard form and P2SH scripts must be" + "Note that all inputs selected must be of standard form and P2SH scripts must be " "in the wallet using importaddress or addmultisigaddress (to calculate fees).\n" "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n" @@ -575,6 +575,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) " \"changePosition\" (numeric, optional, default random) The index of the change output\n" " \"includeWatching\" (boolean, optional, default false) Also select inputs which are watch only\n" " \"lockUnspents\" (boolean, optional, default false) Lock selected unspent outputs\n" + " \"feeRate\" (numeric, optional, default 0=estimate) Set a specific feerate (PIV per KB)\n" " }\n" "\nResult:\n" "{\n" @@ -603,11 +604,12 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) int changePosition = -1; bool includeWatching = false; bool lockUnspents = false; + CFeeRate feeRate = CFeeRate(0); if (params.size() > 1) { RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VOBJ)); UniValue options = params[1]; - RPCTypeCheckObj(options, boost::assign::map_list_of("changeAddress", UniValue::VSTR)("changePosition", UniValue::VNUM)("includeWatching", UniValue::VBOOL)("lockUnspents", UniValue::VBOOL), true, true); + RPCTypeCheckObj(options, boost::assign::map_list_of("changeAddress", UniValue::VSTR)("changePosition", UniValue::VNUM)("includeWatching", UniValue::VBOOL)("lockUnspents", UniValue::VBOOL)("feeRate", UniValue::VNUM), true, true); if (options.exists("changeAddress")) { changeAddress = DecodeDestination(options["changeAddress"].get_str()); @@ -624,6 +626,9 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) if (options.exists("lockUnspents")) lockUnspents = options["lockUnspents"].get_bool(); + + if (options.exists("feeRate")) + feeRate = CFeeRate(AmountFromValue(options["feeRate"])); } // parse hex string from parameter @@ -638,15 +643,15 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); CMutableTransaction tx(origTx); - CAmount nFee; + CAmount nFeeOut; std::string strFailReason; - if(!pwalletMain->FundTransaction(tx, nFee, changePosition, strFailReason, includeWatching, lockUnspents, changeAddress)) + if(!pwalletMain->FundTransaction(tx, nFeeOut, feeRate, changePosition, strFailReason, includeWatching, lockUnspents, changeAddress)) throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason); UniValue result(UniValue::VOBJ); result.push_back(Pair("hex", EncodeHexTx(tx))); result.push_back(Pair("changepos", changePosition)); - result.push_back(Pair("fee", ValueFromAmount(nFee))); + result.push_back(Pair("fee", ValueFromAmount(nFeeOut))); return result; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ed6dd228d61d..e8e53e03838a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2376,7 +2376,7 @@ bool CWallet::GetBudgetFinalizationCollateralTX(CWalletTx& tx, uint256 hash, boo return true; } -bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange) +bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange) { std::vector vecSend; @@ -2390,6 +2390,8 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC coinControl.destChange = destChange; coinControl.fAllowOtherInputs = true; coinControl.fAllowWatchOnly = includeWatching; + coinControl.nFeeRate = specificFeeRate; + for (const CTxIn& txin : tx.vin) coinControl.Select(txin.prevout); @@ -2653,6 +2655,12 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CAmount nFeeNeeded = std::max(nFeePay, GetMinimumFee(nBytes, nTxConfirmTarget, mempool)); + if (coinControl && nFeeNeeded > 0 && coinControl->nMinimumTotalFee > nFeeNeeded) { + nFeeNeeded = coinControl->nMinimumTotalFee; + } + if (coinControl && coinControl->nFeeRate > CFeeRate(0)) + nFeeNeeded = coinControl->nFeeRate.GetFee(nBytes); + // If we made it here and we aren't even able to meet the relay fee on the next pass, give up // because we must be at the maximum allowed fee. if (nFeeNeeded < ::minRelayTxFee.GetFee(nBytes)) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 0769051bdae8..8b534e16c8b3 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -460,7 +460,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface CAmount GetUnconfirmedWatchOnlyBalance() const; CAmount GetImmatureWatchOnlyBalance() const; CAmount GetLockedWatchOnlyBalance() const; - bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange = CNoDestination()); + bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange = CNoDestination()); /** * Create a new transaction paying the recipients with a set of coins * selected by SelectCoins(); Also create the change output, when needed From 5bca4f41b719136075d58ba3e2dbc9a535f33c1a Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 08:03:01 +0200 Subject: [PATCH 22/24] Add more clear interface for CoinControl.h regarding individual feerate - backports bitcoin/bitcoin@04eaa9095813b854c4299027c595fb9ebaf6f934 --- src/coincontrol.h | 5 ++++- src/rpc/rawtransaction.cpp | 7 +++++-- src/wallet/wallet.cpp | 5 +++-- src/wallet/wallet.h | 2 +- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/coincontrol.h b/src/coincontrol.h index c753c547f8fe..0af8e54806c9 100644 --- a/src/coincontrol.h +++ b/src/coincontrol.h @@ -24,7 +24,9 @@ class CCoinControl bool fAllowWatchOnly; //! Minimum absolute fee (not per kilobyte) CAmount nMinimumTotalFee; - //! Feerate to use (0 = estimate fee with payTxFee fallback) + //! Override estimated feerate + bool fOverrideFeeRate; + //! Feerate to use if overrideFeeRate is true CFeeRate nFeeRate; CCoinControl() @@ -41,6 +43,7 @@ class CCoinControl fAllowWatchOnly = false; nMinimumTotalFee = 0; nFeeRate = CFeeRate(0); + fOverrideFeeRate = false; fSplitBlock = false; nSplitBlock = 1; } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 643760206800..92de375d5616 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -605,6 +605,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) bool includeWatching = false; bool lockUnspents = false; CFeeRate feeRate = CFeeRate(0); + bool overrideEstimatedFeerate = false; if (params.size() > 1) { RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VOBJ)); @@ -627,8 +628,10 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) if (options.exists("lockUnspents")) lockUnspents = options["lockUnspents"].get_bool(); - if (options.exists("feeRate")) + if (options.exists("feeRate")) { feeRate = CFeeRate(AmountFromValue(options["feeRate"])); + overrideEstimatedFeerate = true; + } } // parse hex string from parameter @@ -645,7 +648,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) CMutableTransaction tx(origTx); CAmount nFeeOut; std::string strFailReason; - if(!pwalletMain->FundTransaction(tx, nFeeOut, feeRate, changePosition, strFailReason, includeWatching, lockUnspents, changeAddress)) + if(!pwalletMain->FundTransaction(tx, nFeeOut, overrideEstimatedFeerate, feeRate, changePosition, strFailReason, includeWatching, lockUnspents, changeAddress)) throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason); UniValue result(UniValue::VOBJ); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e8e53e03838a..0e95eaa03984 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2376,7 +2376,7 @@ bool CWallet::GetBudgetFinalizationCollateralTX(CWalletTx& tx, uint256 hash, boo return true; } -bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange) +bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange) { std::vector vecSend; @@ -2390,6 +2390,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, const C coinControl.destChange = destChange; coinControl.fAllowOtherInputs = true; coinControl.fAllowWatchOnly = includeWatching; + coinControl.fOverrideFeeRate = overrideEstimatedFeeRate; coinControl.nFeeRate = specificFeeRate; for (const CTxIn& txin : tx.vin) @@ -2658,7 +2659,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, if (coinControl && nFeeNeeded > 0 && coinControl->nMinimumTotalFee > nFeeNeeded) { nFeeNeeded = coinControl->nMinimumTotalFee; } - if (coinControl && coinControl->nFeeRate > CFeeRate(0)) + if (coinControl && coinControl->fOverrideFeeRate) nFeeNeeded = coinControl->nFeeRate.GetFee(nBytes); // If we made it here and we aren't even able to meet the relay fee on the next pass, give up diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8b534e16c8b3..b6dc0342b0e5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -460,7 +460,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface CAmount GetUnconfirmedWatchOnlyBalance() const; CAmount GetImmatureWatchOnlyBalance() const; CAmount GetLockedWatchOnlyBalance() const; - bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange = CNoDestination()); + bool FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange = CNoDestination()); /** * Create a new transaction paying the recipients with a set of coins * selected by SelectCoins(); Also create the change output, when needed From dd357605458ecc81d886159c1642c635ec04bfdf Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 4 Jun 2020 09:43:17 +0200 Subject: [PATCH 23/24] [QA] Add test_option_feerate to rpc_fundrawtransaction functional test --- test/functional/rpc_fundrawtransaction.py | 21 ++++++++++++++++++--- test/functional/test_runner.py | 1 + 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index a5ddb09501ab..9d3f5dc9c2d2 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -5,10 +5,12 @@ from test_framework.test_framework import PivxTestFramework from test_framework.util import ( + assert_fee_amount, assert_equal, assert_raises_rpc_error, assert_greater_than, connect_nodes, + count_bytes, find_vout_for_address, Decimal, DecimalAmt, @@ -54,11 +56,11 @@ def get_tot_balance(self, nodeid): return wi['balance'] + wi['immature_balance'] def run_test(self): - min_relay_tx_fee = self.nodes[0].getnetworkinfo()['relayfee'] + self.min_relay_tx_fee = self.nodes[0].getnetworkinfo()['relayfee'] # This test is not meant to test fee estimation and we'd like # to be sure all txs are sent at a consistent desired feerate for node in self.nodes: - node.settxfee(float(min_relay_tx_fee)) + node.settxfee(float(self.min_relay_tx_fee)) # if the fee's positive delta is higher than this value tests will fail, # neg. delta always fail the tests. @@ -66,7 +68,7 @@ def run_test(self): # than a minimum sized signature. # = 2 bytes * minRelayTxFeePerByte - self.fee_tolerance = 2 * min_relay_tx_fee/1000 + self.fee_tolerance = 2 * self.min_relay_tx_fee/1000 print("Mining blocks...") self.nodes[1].generate(1) @@ -117,6 +119,7 @@ def run_test(self): self.test_op_return() self.test_watchonly() self.test_all_watched_funds() + self.test_option_feerate() def test_simple(self): @@ -490,6 +493,18 @@ def test_all_watched_funds(self): self.nodes[0].generate(1) self.sync_all() + def test_option_feerate(self): + self.log.info("test feeRate option") + # Make sure there is exactly one input so coin selection can't skew the result. + assert_equal(len(self.nodes[1].listunspent()), 1) + rawtx = self.nodes[1].createrawtransaction([], {self.nodes[1].getnewaddress(): 0.001}) + result = self.nodes[1].fundrawtransaction(rawtx) # uses self.min_relay_tx_fee (set by settxfee) + result2 = self.nodes[1].fundrawtransaction(rawtx, {"feeRate": 2 * float(self.min_relay_tx_fee)}) + result3 = self.nodes[1].fundrawtransaction(rawtx, {"feeRate": 10 * float(self.min_relay_tx_fee)}) + result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) + assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) + assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) + if __name__ == '__main__': diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index a52e5a53f59d..057530d8df88 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -171,6 +171,7 @@ 'rpc_blockchain.py', 'rpc_budget.py', 'rpc_decodescript.py', + 'rpc_fundrawtransaction.py', 'rpc_net.py', 'rpc_signmessage.py', 'rpc_spork.py', From fc81158bedbe9ae231d5e41feccfd649e4b02984 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Sun, 28 Jun 2020 12:27:15 +0200 Subject: [PATCH 24/24] [QA] Add test_change_position case to rpc_fundrawtransaction.py --- test/functional/rpc_fundrawtransaction.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 9d3f5dc9c2d2..6ed6cb0b3eac 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -101,6 +101,7 @@ def run_test(self): self.test_simple_two_outputs() self.test_change() self.test_no_change() + self.test_change_position() self.test_invalid_option() self.test_invalid_change_address() self.test_valid_change_address() @@ -121,7 +122,6 @@ def run_test(self): self.test_all_watched_funds() self.test_option_feerate() - def test_simple(self): self.log.info("simple test") dec_tx, fee, changepos = self.create_and_fund(2, [], {self.nodes[0].getnewaddress(): 1.0}) @@ -174,6 +174,16 @@ def test_no_change(self): assert_equal(changepos, -1) # check (no) change assert check_outputs(outputs, dec_tx) # check outputs + def test_change_position(self): + """Ensure setting changePosition in fundraw with an exact match is handled properly.""" + self.log.info("test not-used changePosition option") + utx = get_unspent(self.nodes[2].listunspent(), 5) + inputs = [{'txid': utx['txid'], 'vout': utx['vout']}] + outputs = {self.nodes[0].getnewaddress(): 5.0 - float(self.test_no_change_fee) - float(self.fee_tolerance)} + rawtx = self.nodes[2].createrawtransaction(inputs, outputs) + rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"changePosition": 0}) + assert_equal(rawtxfund["changepos"], -1) + def test_invalid_option(self): self.log.info("test with an invalid option") outputs = {self.nodes[0].getnewaddress(): 1.0}