Auto merge of #1431 - bitcartel:master_1373_taddr_coinbase_error, r=bitcartel
Return a more informative error message when trying to spend coinbase; select non-coinbase inputs when sending to a transparent output if needed For #1373 and #1519 Code change: - Extra parameter added to AvailableCoins to include or exclude Coinbase coins. Default value of parameter is 'true' as current behaviour is to include Coinbase coins. - SelectCoins, used for sending taddr->taddr, will now exclude Coinbase coins. Unit test: Tried to write a test to focus on the extra parameter added to AvailableCoins but could not. Empirical testing on Testnet: Current behaviour is that upstream RPC commands sendfrom and sendtoaddress try to spend coinbase coins returned by AvailableCoins. So the user will see: ``` ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 1000.0 error: {"code":-6,"message":"Insufficient funds"} ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000 error: {"code":-4,"message":"Error: The transaction was rejected! This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here."} ./zcash-cli sendfrom "" mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000 error: {"code":-4,"message":"Error: The transaction was rejected! This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here."} ``` After fix is applied: ``` ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 1000.0 error: {"code":-6,"message":"Insufficient funds"} ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000 error: {"code":-4,"message":"Coinbase funds can only be sent to a zaddr"} ``` When non-coinbase UTXOs exist, they will now be selected and used: ``` ./zcash-cli z_sendmany tnPJZHeVxegCg91utaquBRPEDBGjozfz9iLDHt7zvphFbZdspNgkTVLCGjDcadQBKNyUwKs8pNjDXuEZKrE1aNLpFwHgz4t '[{"address":"mx5fTRhLZwbYE7ZqhAPueZgQGSnwTbdvKU", "amount":0.01}]' ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 1000.0 error: {"code":-6,"message":"Insufficient funds"} ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000 9818e543ac2f689d4ce8b52087607d73fecd771d45d316a1d9db092f0485aff2 ./zcash-cli sendfrom "" mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000 899f2894823f51f15fc73b5e0871ac943edbe0ff88e1635f86906087b72caf30 ```
This commit is contained in:
zkbot
commit
ec8dc3a88a
|
|
@ -11,6 +11,7 @@ export BITCOIND=${REAL_BITCOIND}
|
|||
#Run the tests
|
||||
|
||||
testScripts=(
|
||||
'wallet_protectcoinbase.py'
|
||||
'wallet.py'
|
||||
'wallet_nullifiers.py'
|
||||
'listtransactions.py'
|
||||
|
|
|
|||
|
|
@ -0,0 +1,126 @@
|
|||
#!/usr/bin/env python2
|
||||
# Copyright (c) 2016 The Zcash developers
|
||||
# Distributed under the MIT software license, see the accompanying
|
||||
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
|
||||
|
||||
|
||||
from test_framework.test_framework import BitcoinTestFramework
|
||||
from test_framework.util import *
|
||||
from time import *
|
||||
|
||||
class Wallet2Test (BitcoinTestFramework):
|
||||
|
||||
def setup_chain(self):
|
||||
print("Initializing test directory "+self.options.tmpdir)
|
||||
initialize_chain_clean(self.options.tmpdir, 4)
|
||||
|
||||
# Start nodes with -regtestprotectcoinbase to set fCoinbaseMustBeProtected to true.
|
||||
def setup_network(self, split=False):
|
||||
self.nodes = start_nodes(3, self.options.tmpdir, extra_args=[['-regtestprotectcoinbase']] * 3 )
|
||||
connect_nodes_bi(self.nodes,0,1)
|
||||
connect_nodes_bi(self.nodes,1,2)
|
||||
connect_nodes_bi(self.nodes,0,2)
|
||||
self.is_network_split=False
|
||||
self.sync_all()
|
||||
|
||||
def wait_for_operationd_success(self, myopid):
|
||||
print('waiting for async operation {}'.format(myopid))
|
||||
opids = []
|
||||
opids.append(myopid)
|
||||
timeout = 120
|
||||
status = None
|
||||
for x in xrange(1, timeout):
|
||||
results = self.nodes[0].z_getoperationresult(opids)
|
||||
if len(results)==0:
|
||||
sleep(1)
|
||||
else:
|
||||
status = results[0]["status"]
|
||||
break
|
||||
print('...returned status: {}'.format(status))
|
||||
assert_equal("success", status)
|
||||
|
||||
def run_test (self):
|
||||
print "Mining blocks..."
|
||||
|
||||
self.nodes[0].generate(4)
|
||||
|
||||
walletinfo = self.nodes[0].getwalletinfo()
|
||||
assert_equal(walletinfo['immature_balance'], 40)
|
||||
assert_equal(walletinfo['balance'], 0)
|
||||
|
||||
self.sync_all()
|
||||
self.nodes[1].generate(101)
|
||||
self.sync_all()
|
||||
|
||||
assert_equal(self.nodes[0].getbalance(), 40)
|
||||
assert_equal(self.nodes[1].getbalance(), 10)
|
||||
assert_equal(self.nodes[2].getbalance(), 0)
|
||||
|
||||
# Send will fail because we are enforcing the consensus rule that
|
||||
# coinbase utxos can only be sent to a zaddr.
|
||||
errorString = ""
|
||||
try:
|
||||
self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.0)
|
||||
except JSONRPCException,e:
|
||||
errorString = e.error['message']
|
||||
assert_equal("Coinbase funds can only be sent to a zaddr" in errorString, True)
|
||||
|
||||
# send node 0 taddr to node 0 zaddr
|
||||
mytaddr = self.nodes[0].getnewaddress()
|
||||
myzaddr = self.nodes[0].z_getnewaddress()
|
||||
recipients = []
|
||||
recipients.append({"address":myzaddr, "amount":20.0})
|
||||
myopid = self.nodes[0].z_sendmany(mytaddr, recipients)
|
||||
self.wait_for_operationd_success(myopid)
|
||||
self.nodes[1].generate(1)
|
||||
self.sync_all()
|
||||
|
||||
# check balances (the z_sendmany consumes 3 coinbase utxos)
|
||||
resp = self.nodes[0].z_gettotalbalance()
|
||||
assert_equal(Decimal(resp["transparent"]), Decimal('10.0'))
|
||||
assert_equal(Decimal(resp["private"]), Decimal('29.9999'))
|
||||
assert_equal(Decimal(resp["total"]), Decimal('39.9999'))
|
||||
|
||||
# convert note to transparent funds
|
||||
recipients = []
|
||||
recipients.append({"address":mytaddr, "amount":20.0})
|
||||
myopid = self.nodes[0].z_sendmany(myzaddr, recipients)
|
||||
self.wait_for_operationd_success(myopid)
|
||||
self.nodes[1].generate(1)
|
||||
self.sync_all()
|
||||
|
||||
# check balances
|
||||
resp = self.nodes[0].z_gettotalbalance()
|
||||
assert_equal(Decimal(resp["transparent"]), Decimal('30.0'))
|
||||
assert_equal(Decimal(resp["private"]), Decimal('9.9998'))
|
||||
assert_equal(Decimal(resp["total"]), Decimal('39.9998'))
|
||||
|
||||
# Send will fail because send amount is too big, even when including coinbase utxos
|
||||
errorString = ""
|
||||
try:
|
||||
self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 99999.0)
|
||||
except JSONRPCException,e:
|
||||
errorString = e.error['message']
|
||||
assert_equal("Insufficient funds" in errorString, True)
|
||||
|
||||
# Send will fail because of insufficient funds unless sender uses coinbase utxos
|
||||
try:
|
||||
self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 21.0)
|
||||
except JSONRPCException,e:
|
||||
errorString = e.error['message']
|
||||
assert_equal("Insufficient funds, coinbase funds can only be spent after they have been sent to a zaddr" in errorString, True)
|
||||
|
||||
# Send will succeed because the balance of non-coinbase utxos is 20.0
|
||||
try:
|
||||
self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 19.0)
|
||||
except JSONRPCException:
|
||||
assert(False)
|
||||
|
||||
self.nodes[1].generate(10)
|
||||
self.sync_all()
|
||||
|
||||
# check balance
|
||||
assert_equal(self.nodes[2].getbalance(), Decimal('19'))
|
||||
|
||||
if __name__ == '__main__':
|
||||
Wallet2Test ().main ()
|
||||
|
|
@ -336,6 +336,11 @@ CChainParams &Params(CBaseChainParams::Network network) {
|
|||
void SelectParams(CBaseChainParams::Network network) {
|
||||
SelectBaseParams(network);
|
||||
pCurrentParams = &Params(network);
|
||||
|
||||
// Some python qa rpc tests need to enforce the coinbase consensus rule
|
||||
if (network == CBaseChainParams::REGTEST && mapArgs.count("-regtestprotectcoinbase")) {
|
||||
regTestParams.SetRegTestCoinbaseMustBeProtected();
|
||||
}
|
||||
}
|
||||
|
||||
bool SelectParamsFromCommandLine()
|
||||
|
|
|
|||
|
|
@ -83,6 +83,8 @@ public:
|
|||
std::string GetFoundersRewardAddressAtIndex(int i) const;
|
||||
/** #1398 to return a fixed founders reward script for miner_tests */
|
||||
bool fMinerTestModeForFoundersRewardScript = false;
|
||||
/** Enforce coinbase consensus rule in regtest mode */
|
||||
void SetRegTestCoinbaseMustBeProtected() { consensus.fCoinbaseMustBeProtected = true; }
|
||||
protected:
|
||||
CChainParams() {}
|
||||
|
||||
|
|
|
|||
|
|
@ -715,7 +715,7 @@ bool AsyncRPCOperation_sendmany::find_utxos(bool fAcceptCoinbase=false) {
|
|||
|
||||
LOCK2(cs_main, pwalletMain->cs_wallet);
|
||||
|
||||
pwalletMain->AvailableCoins(vecOutputs, false, NULL, true);
|
||||
pwalletMain->AvailableCoins(vecOutputs, false, NULL, true, fAcceptCoinbase);
|
||||
|
||||
BOOST_FOREACH(const COutput& out, vecOutputs) {
|
||||
if (out.nDepth < mindepth_) {
|
||||
|
|
|
|||
|
|
@ -2102,7 +2102,7 @@ CAmount CWallet::GetImmatureWatchOnlyBalance() const
|
|||
/**
|
||||
* populate vCoins with vector of available COutputs.
|
||||
*/
|
||||
void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const CCoinControl *coinControl, bool fIncludeZeroValue) const
|
||||
void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const CCoinControl *coinControl, bool fIncludeZeroValue, bool fIncludeCoinBase) const
|
||||
{
|
||||
vCoins.clear();
|
||||
|
||||
|
|
@ -2119,6 +2119,9 @@ void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const
|
|||
if (fOnlyConfirmed && !pcoin->IsTrusted())
|
||||
continue;
|
||||
|
||||
if (pcoin->IsCoinBase() && !fIncludeCoinBase)
|
||||
continue;
|
||||
|
||||
if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0)
|
||||
continue;
|
||||
|
||||
|
|
@ -2284,10 +2287,38 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int
|
|||
return true;
|
||||
}
|
||||
|
||||
bool CWallet::SelectCoins(const CAmount& nTargetValue, set<pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet, const CCoinControl* coinControl) const
|
||||
bool CWallet::SelectCoins(const CAmount& nTargetValue, set<pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet, bool& fOnlyCoinbaseCoinsRet, bool& fNeedCoinbaseCoinsRet, const CCoinControl* coinControl) const
|
||||
{
|
||||
vector<COutput> vCoins;
|
||||
AvailableCoins(vCoins, true, coinControl);
|
||||
// Output parameter fOnlyCoinbaseCoinsRet is set to true when the only available coins are coinbase utxos.
|
||||
vector<COutput> vCoinsNoCoinbase, vCoinsWithCoinbase;
|
||||
AvailableCoins(vCoinsNoCoinbase, true, coinControl, false, false);
|
||||
AvailableCoins(vCoinsWithCoinbase, true, coinControl, false, true);
|
||||
fOnlyCoinbaseCoinsRet = vCoinsNoCoinbase.size() == 0 && vCoinsWithCoinbase.size() > 0;
|
||||
|
||||
// If coinbase utxos can only be sent to zaddrs, exclude any coinbase utxos from coin selection.
|
||||
bool fProtectCoinbase = Params().GetConsensus().fCoinbaseMustBeProtected;
|
||||
vector<COutput> vCoins = (fProtectCoinbase) ? vCoinsNoCoinbase : vCoinsWithCoinbase;
|
||||
|
||||
// Output parameter fNeedCoinbaseCoinsRet is set to true if coinbase utxos need to be spent to meet target amount
|
||||
if (fProtectCoinbase && vCoinsWithCoinbase.size() > vCoinsNoCoinbase.size()) {
|
||||
CAmount value = 0;
|
||||
for (const COutput& out : vCoinsNoCoinbase) {
|
||||
if (!out.fSpendable) {
|
||||
continue;
|
||||
}
|
||||
value += out.tx->vout[out.i].nValue;
|
||||
}
|
||||
if (value <= nTargetValue) {
|
||||
CAmount valueWithCoinbase = 0;
|
||||
for (const COutput& out : vCoinsWithCoinbase) {
|
||||
if (!out.fSpendable) {
|
||||
continue;
|
||||
}
|
||||
valueWithCoinbase += out.tx->vout[out.i].nValue;
|
||||
}
|
||||
fNeedCoinbaseCoinsRet = (valueWithCoinbase >= nTargetValue);
|
||||
}
|
||||
}
|
||||
|
||||
// coin control -> return all selected outputs (we want all selected to go into the transaction for sure)
|
||||
if (coinControl && coinControl->HasSelected())
|
||||
|
|
@ -2407,9 +2438,17 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend,
|
|||
// Choose coins to use
|
||||
set<pair<const CWalletTx*,unsigned int> > setCoins;
|
||||
CAmount nValueIn = 0;
|
||||
if (!SelectCoins(nTotalValue, setCoins, nValueIn, coinControl))
|
||||
bool fOnlyCoinbaseCoins = false;
|
||||
bool fNeedCoinbaseCoins = false;
|
||||
if (!SelectCoins(nTotalValue, setCoins, nValueIn, fOnlyCoinbaseCoins, fNeedCoinbaseCoins, coinControl))
|
||||
{
|
||||
strFailReason = _("Insufficient funds");
|
||||
if (fOnlyCoinbaseCoins && Params().GetConsensus().fCoinbaseMustBeProtected) {
|
||||
strFailReason = _("Coinbase funds can only be sent to a zaddr");
|
||||
} else if (fNeedCoinbaseCoins) {
|
||||
strFailReason = _("Insufficient funds, coinbase funds can only be spent after they have been sent to a zaddr");
|
||||
} else {
|
||||
strFailReason = _("Insufficient funds");
|
||||
}
|
||||
return false;
|
||||
}
|
||||
BOOST_FOREACH(PAIRTYPE(const CWalletTx*, unsigned int) pcoin, setCoins)
|
||||
|
|
|
|||
|
|
@ -565,7 +565,7 @@ public:
|
|||
class CWallet : public CCryptoKeyStore, public CValidationInterface
|
||||
{
|
||||
private:
|
||||
bool SelectCoins(const CAmount& nTargetValue, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet, const CCoinControl *coinControl = NULL) const;
|
||||
bool SelectCoins(const CAmount& nTargetValue, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet, bool& fOnlyCoinbaseCoinsRet, bool& fNeedCoinbaseCoinsRet, const CCoinControl *coinControl = NULL) const;
|
||||
|
||||
CWalletDB *pwalletdbEncryption;
|
||||
|
||||
|
|
@ -780,7 +780,7 @@ public:
|
|||
//! check whether we are allowed to upgrade (or already support) to the named feature
|
||||
bool CanSupportFeature(enum WalletFeature wf) { AssertLockHeld(cs_wallet); return nWalletMaxVersion >= wf; }
|
||||
|
||||
void AvailableCoins(std::vector<COutput>& vCoins, bool fOnlyConfirmed=true, const CCoinControl *coinControl = NULL, bool fIncludeZeroValue=false) const;
|
||||
void AvailableCoins(std::vector<COutput>& vCoins, bool fOnlyConfirmed=true, const CCoinControl *coinControl = NULL, bool fIncludeZeroValue=false, bool fIncludeCoinBase=true) const;
|
||||
bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, std::vector<COutput> vCoins, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet) const;
|
||||
|
||||
bool IsSpent(const uint256& hash, unsigned int n) const;
|
||||
|
|
|
|||
Loading…
Reference in New Issue