From 41e835dd50d358c127bab17a1f2872471dbdfe9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Wed, 30 Mar 2016 00:59:29 +0100 Subject: [PATCH 1/3] Add strict flag to RPCTypeCheckObj Strict flag forces type check on all object keys. --- src/rpc/server.cpp | 15 ++++++++++++++- src/rpc/server.h | 2 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 8326fe14d..d06a9142b 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -89,7 +89,8 @@ void RPCTypeCheck(const UniValue& params, void RPCTypeCheckObj(const UniValue& o, const map& typesExpected, - bool fAllowNull) + bool fAllowNull, + bool fStrict) { BOOST_FOREACH(const PAIRTYPE(string, UniValue::VType)& t, typesExpected) { @@ -104,6 +105,18 @@ void RPCTypeCheckObj(const UniValue& o, throw JSONRPCError(RPC_TYPE_ERROR, err); } } + + if (fStrict) + { + BOOST_FOREACH(const string& k, o.getKeys()) + { + if (typesExpected.count(k) == 0) + { + string err = strprintf("Unexpected key %s", k); + throw JSONRPCError(RPC_TYPE_ERROR, err); + } + } + } } CAmount AmountFromValue(const UniValue& value) diff --git a/src/rpc/server.h b/src/rpc/server.h index a7ed710ce..b47133661 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 af4fe7fd126eff2dd1942276ea91c8ab9dd717c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Wed, 30 Mar 2016 02:04:22 +0100 Subject: [PATCH 2/3] Add change options to fundrawtransaction --- qa/rpc-tests/fundrawtransaction.py | 80 +++++++++++++++++++++++++++++- src/wallet/rpcwallet.cpp | 58 ++++++++++++++++++---- src/wallet/wallet.cpp | 34 +++++++++---- src/wallet/wallet.h | 5 +- 4 files changed, 153 insertions(+), 24 deletions(-) diff --git a/qa/rpc-tests/fundrawtransaction.py b/qa/rpc-tests/fundrawtransaction.py index 4492ea398..496c7fe8b 100755 --- a/qa/rpc-tests/fundrawtransaction.py +++ b/qa/rpc-tests/fundrawtransaction.py @@ -177,6 +177,83 @@ class RawTransactionsTest(BitcoinTestFramework): assert_equal(fee + totalOut, utx['amount']) #compare vin total and totalout+fee + #################################################### + # test a fundrawtransaction with an invalid option # + #################################################### + utx = False + listunspent = self.nodes[2].listunspent() + for aUtx in listunspent: + if aUtx['amount'] == 5.0: + utx = aUtx + break + + assert_equal(utx!=False, True) + + 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,e: + assert("Unexpected key foo" in e.error['message']) + + + ############################################################ + # test a fundrawtransaction with an invalid change address # + ############################################################ + utx = False + listunspent = self.nodes[2].listunspent() + for aUtx in listunspent: + if aUtx['amount'] == 5.0: + utx = aUtx + break + + assert_equal(utx!=False, True) + + 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 bitcoin address") + except JSONRPCException,e: + assert("changeAddress must be a valid bitcoin address" in e.error['message']) + + + + ############################################################ + # test a fundrawtransaction with a provided change address # + ############################################################ + utx = False + listunspent = self.nodes[2].listunspent() + for aUtx in listunspent: + if aUtx['amount'] == 5.0: + utx = aUtx + break + + assert_equal(utx!=False, True) + + 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() + 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 # ######################################################################### @@ -568,7 +645,7 @@ class RawTransactionsTest(BitcoinTestFramework): outputs = {self.nodes[2].getnewaddress() : watchonly_amount / 2} rawtx = self.nodes[3].createrawtransaction(inputs, 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"], watchonly_txid) @@ -584,6 +661,7 @@ class RawTransactionsTest(BitcoinTestFramework): 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) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 5511e9d3a..5cd57fc38 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2437,7 +2437,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) if (fHelp || params.size() < 1 || params.size() > 2) throw 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" @@ -2447,8 +2447,14 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) "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" + "1. \"hexstring\" (string, required) The hex string of the raw transaction\n" + "2. options (object, optional)\n" + " {\n" + " \"changeAddress\" (string, optional, default pool address) The bitcoin 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" + " for backward compatibility: passing in a true instzead of an object will result in {\"includeWatching\":true}\n" "\nResult:\n" "{\n" " \"hex\": \"value\", (string) The resulting raw transaction (hex-encoded string)\n" @@ -2467,7 +2473,40 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) + HelpExampleCli("sendrawtransaction", "\"signedtransactionhex\"") ); - 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) { + if (params[1].type() == UniValue::VBOOL) { + // backward compatibility bool only fallback + includeWatching = params[1].get_bool(); + } + else { + 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")) { + CBitcoinAddress address(options["changeAddress"].get_str()); + + if (!address.IsValid()) + throw JSONRPCError(RPC_INVALID_PARAMETER, "changeAddress must be a valid bitcoin address"); + + changeAddress = address.Get(); + } + + 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; @@ -2477,20 +2516,19 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) if (origTx.vout.size() == 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output"); - bool includeWatching = false; - if (params.size() > 1) - includeWatching = params[1].get_bool(); + if (changePosition != -1 && (changePosition < 0 || changePosition > origTx.vout.size())) + throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); CMutableTransaction tx(origTx); CAmount nFee; 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/wallet.cpp b/src/wallet/wallet.cpp index e8c946671..ee9255216 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1932,7 +1932,7 @@ bool CWallet::SelectCoins(const vector& vAvailableCoins, const CAmount& return res; } -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) { vector vecSend; @@ -1944,6 +1944,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nC } CCoinControl coinControl; + coinControl.destChange = destChange; coinControl.fAllowOtherInputs = true; coinControl.fAllowWatchOnly = includeWatching; BOOST_FOREACH(const CTxIn& txin, tx.vin) @@ -1951,11 +1952,11 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nC CReserveKey reservekey(this); CWalletTx wtx; - if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosRet, strFailReason, &coinControl, false)) + if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, &coinControl, 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) BOOST_FOREACH(const CTxIn& txin, wtx.vin) @@ -1968,9 +1969,10 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nC } bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, - int& nChangePosRet, std::string& strFailReason, const CCoinControl* coinControl, bool sign) + int& nChangePosInOut, std::string& strFailReason, const CCoinControl* coinControl, bool sign) { CAmount nValue = 0; + int nChangePosRequest = nChangePosInOut; unsigned int nSubtractFeeFromAmount = 0; BOOST_FOREACH (const CRecipient& recipient, vecSend) { @@ -2036,10 +2038,10 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt // Start with no fee and loop until there is enough fee while (true) { + nChangePosInOut = nChangePosRequest; txNew.vin.clear(); txNew.vout.clear(); wtxNew.fFromMe = true; - nChangePosRet = -1; bool fFirst = true; CAmount nValueToSelect = nValue; @@ -2159,14 +2161,24 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt // add the dust to the fee. if (newTxOut.IsDust(::minRelayTxFee)) { + nChangePosInOut = -1; nFeeRet += nChange; reservekey.ReturnKey(); } else { - // Insert change txn at random position: - nChangePosRet = GetRandInt(txNew.vout.size()+1); - 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 > txNew.vout.size()) + { + strFailReason = _("Change index out of range"); + return false; + } + + vector::iterator position = txNew.vout.begin()+nChangePosInOut; txNew.vout.insert(position, newTxOut); } } @@ -2842,13 +2854,13 @@ void CWallet::GetScriptForMining(boost::shared_ptr &script) script->reserveScript = CScript() << ToByteVector(pubkey) << OP_CHECKSIG; } -void CWallet::LockCoin(COutPoint& output) +void CWallet::LockCoin(const COutPoint& output) { AssertLockHeld(cs_wallet); // setLockedCoins setLockedCoins.insert(output); } -void CWallet::UnlockCoin(COutPoint& output) +void CWallet::UnlockCoin(const COutPoint& output) { AssertLockHeld(cs_wallet); // setLockedCoins setLockedCoins.erase(output); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 96d5d4e1e..b0781e917 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -739,13 +739,14 @@ public: * Insert additional inputs into the transaction by * calling CreateTransaction(); */ - 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, + bool CreateTransaction(const std::vector& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl *coinControl = NULL, bool sign = true); bool CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey); From f2d0944eb372838e05c666ce9b3df119d7da5594 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Wed, 6 Apr 2016 15:56:14 +0100 Subject: [PATCH 3/3] Add lockUnspents option to fundrawtransaction --- src/wallet/rpcwallet.cpp | 9 +++++++-- src/wallet/wallet.cpp | 10 +++++++++- src/wallet/wallet.h | 6 +++--- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 5cd57fc38..3078cebd4 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2453,6 +2453,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) " \"changeAddress\" (string, optional, default pool address) The bitcoin 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" " for backward compatibility: passing in a true instzead of an object will result in {\"includeWatching\":true}\n" "\nResult:\n" @@ -2478,6 +2479,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) CTxDestination changeAddress = CNoDestination(); int changePosition = -1; bool includeWatching = false; + bool lockUnspents = false; if (params.size() > 1) { if (params[1].type() == UniValue::VBOOL) { @@ -2489,7 +2491,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) 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")) { CBitcoinAddress address(options["changeAddress"].get_str()); @@ -2505,6 +2507,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(); } } @@ -2523,7 +2528,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp) CAmount nFee; 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 ee9255216..8161c659a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1932,7 +1932,7 @@ bool CWallet::SelectCoins(const vector& vAvailableCoins, const CAmount& return res; } -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) { vector vecSend; @@ -1962,7 +1962,15 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC BOOST_FOREACH(const CTxIn& txin, wtx.vin) { 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 b0781e917..aab4b217c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -667,8 +667,8 @@ public: bool IsSpent(const uint256& hash, unsigned int n) const; bool IsLockedCoin(uint256 hash, unsigned int n) const; - void LockCoin(COutPoint& output); - void UnlockCoin(COutPoint& output); + void LockCoin(const COutPoint& output); + void UnlockCoin(const COutPoint& output); void UnlockAllCoins(); void ListLockedCoins(std::vector& vOutpts); @@ -739,7 +739,7 @@ public: * Insert additional inputs into the transaction by * calling CreateTransaction(); */ - 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