Auto merge of #4843 - furszy:2020_improving_sendmany_redudant_loops_first_round, r=daira

Improving asyncOp_sendmany redundancies (first round)

Work over async sendmany operation flow. Specifically on the utxo finding, utxo selection, total amount calculation and the tx inputs appending process.  Removed several unneeded for loops over the entire utxo set, re structuring and cleaning the flow.
This commit is contained in:
Homu 2020-11-18 13:54:13 +00:00
commit 2cc836680a
6 changed files with 171 additions and 183 deletions

View File

@ -84,7 +84,7 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
recipients= [{"address":myzaddr, "amount": Decimal('1')}]
myopid = self.nodes[3].z_sendmany(mytaddr, recipients)
wait_and_assert_operationid_status(self.nodes[3], myopid, "failed", "Insufficient funds, no UTXOs found for taddr from address.", 10)
wait_and_assert_operationid_status(self.nodes[3], myopid, "failed", "Insufficient transparent funds, no UTXOs found for taddr from address.", 10)
# This send will fail because our wallet does not allow any change when shielding a coinbase utxo,
# as it's currently not possible to specify a change address in z_sendmany.

View File

@ -198,6 +198,15 @@ void AsyncRPCOperation_sendmany::main() {
// !!! Payment disclosure END
}
struct TxValues {
CAmount t_inputs_total{0};
CAmount z_inputs_total{0};
CAmount t_outputs_total{0};
CAmount z_outputs_total{0};
CAmount targetAmount{0};
bool selectedUTXOCoinbase{false};
};
// Notes:
// 1. #1159 Currently there is no limit set on the number of joinsplits, so size of tx could be invalid.
// 2. #1360 Note selection is not optimal
@ -210,18 +219,31 @@ bool AsyncRPCOperation_sendmany::main_impl() {
bool isMultipleZaddrOutput = (t_outputs_.size()==0 && z_outputs_.size()>=1);
bool isPureTaddrOnlyTx = (isfromtaddr_ && z_outputs_.size() == 0);
CAmount minersFee = fee_;
TxValues txValues;
// First calculate the target
for (SendManyRecipient & t : t_outputs_) {
txValues.t_outputs_total += t.amount;
}
for (SendManyRecipient & t : z_outputs_) {
txValues.z_outputs_total += t.amount;
}
CAmount sendAmount = txValues.z_outputs_total + txValues.t_outputs_total;
txValues.targetAmount = sendAmount + minersFee;
// When spending coinbase utxos, you can only specify a single zaddr as the change must go somewhere
// and if there are multiple zaddrs, we don't know where to send it.
if (isfromtaddr_) {
// Only select coinbase if we are spending from a single t-address to a single z-address.
if (!useanyutxo_ && isSingleZaddrOutput) {
bool b = find_utxos(true);
bool b = find_utxos(true, txValues);
if (!b) {
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds, no UTXOs found for taddr from address.");
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient transparent funds, no UTXOs found for taddr from address.");
}
} else {
bool b = find_utxos(false);
bool b = find_utxos(false, txValues);
if (!b) {
if (isMultipleZaddrOutput) {
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Could not find any non-coinbase UTXOs to spend. Coinbase UTXOs can only be sent to a single zaddr recipient from a single taddr.");
@ -239,115 +261,33 @@ bool AsyncRPCOperation_sendmany::main_impl() {
// At least one of z_sprout_inputs_ and z_sapling_inputs_ must be empty by design
assert(z_sprout_inputs_.empty() || z_sapling_inputs_.empty());
CAmount t_inputs_total = 0;
for (SendManyInputUTXO & t : t_inputs_) {
t_inputs_total += t.amount;
}
CAmount z_inputs_total = 0;
for (SendManyInputJSOP & t : z_sprout_inputs_) {
z_inputs_total += t.amount;
txValues.z_inputs_total += t.amount;
}
for (auto t : z_sapling_inputs_) {
z_inputs_total += t.note.value();
txValues.z_inputs_total += t.note.value();
}
CAmount t_outputs_total = 0;
for (SendManyRecipient & t : t_outputs_) {
t_outputs_total += t.amount;
}
CAmount z_outputs_total = 0;
for (SendManyRecipient & t : z_outputs_) {
z_outputs_total += t.amount;
}
CAmount sendAmount = z_outputs_total + t_outputs_total;
CAmount targetAmount = sendAmount + minersFee;
assert(!isfromtaddr_ || z_inputs_total == 0);
assert(!isfromzaddr_ || t_inputs_total == 0);
if (isfromtaddr_ && (t_inputs_total < targetAmount)) {
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS,
strprintf("Insufficient transparent funds, have %s, need %s",
FormatMoney(t_inputs_total), FormatMoney(targetAmount)));
}
assert(!isfromtaddr_ || txValues.z_inputs_total == 0);
assert(!isfromzaddr_ || txValues.t_inputs_total == 0);
if (isfromzaddr_ && (z_inputs_total < targetAmount)) {
if (isfromzaddr_ && (txValues.z_inputs_total < txValues.targetAmount)) {
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS,
strprintf("Insufficient shielded funds, have %s, need %s",
FormatMoney(z_inputs_total), FormatMoney(targetAmount)));
}
// If from address is a taddr, select UTXOs to spend
CAmount selectedUTXOAmount = 0;
bool selectedUTXOCoinbase = false;
if (isfromtaddr_) {
// Get dust threshold
CKey secret;
secret.MakeNewKey(true);
CScript scriptPubKey = GetScriptForDestination(secret.GetPubKey().GetID());
CTxOut out(CAmount(1), scriptPubKey);
CAmount dustThreshold = out.GetDustThreshold(minRelayTxFee);
CAmount dustChange = -1;
std::vector<SendManyInputUTXO> selectedTInputs;
for (SendManyInputUTXO & t : t_inputs_) {
bool b = t.coinbase;
if (b) {
selectedUTXOCoinbase = true;
}
selectedUTXOAmount += t.amount;
selectedTInputs.push_back(t);
if (selectedUTXOAmount >= targetAmount) {
// Select another utxo if there is change less than the dust threshold.
dustChange = selectedUTXOAmount - targetAmount;
if (dustChange == 0 || dustChange >= dustThreshold) {
break;
}
}
}
// If there is transparent change, is it valid or is it dust?
if (dustChange < dustThreshold && dustChange != 0) {
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS,
strprintf("Insufficient transparent funds, have %s, need %s more to avoid creating invalid change output %s (dust threshold is %s)",
FormatMoney(t_inputs_total), FormatMoney(dustThreshold - dustChange), FormatMoney(dustChange), FormatMoney(dustThreshold)));
}
t_inputs_ = selectedTInputs;
t_inputs_total = selectedUTXOAmount;
// update the transaction with these inputs
if (isUsingBuilder_) {
for (auto t : t_inputs_) {
builder_.AddTransparentInput(COutPoint(t.txid, t.vout), t.scriptPubKey, t.amount);
}
} else {
CMutableTransaction rawTx(tx_);
for (SendManyInputUTXO & t : t_inputs_) {
uint256 txid = t.txid;
int vout = t.vout;
CAmount amount = t.amount;
CTxIn in(COutPoint(txid, vout));
rawTx.vin.push_back(in);
}
tx_ = CTransaction(rawTx);
}
FormatMoney(txValues.z_inputs_total), FormatMoney(txValues.targetAmount)));
}
if (isfromtaddr_) {
LogPrint("zrpc", "%s: spending %s to send %s with fee %s\n",
getId(), FormatMoney(targetAmount), FormatMoney(sendAmount), FormatMoney(minersFee));
getId(), FormatMoney(txValues.targetAmount), FormatMoney(sendAmount), FormatMoney(minersFee));
} else {
LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n",
getId(), FormatMoney(targetAmount), FormatMoney(sendAmount), FormatMoney(minersFee));
getId(), FormatMoney(txValues.targetAmount), FormatMoney(sendAmount), FormatMoney(minersFee));
}
LogPrint("zrpc", "%s: transparent input: %s (to choose from)\n", getId(), FormatMoney(t_inputs_total));
LogPrint("zrpcunsafe", "%s: private input: %s (to choose from)\n", getId(), FormatMoney(z_inputs_total));
LogPrint("zrpc", "%s: transparent output: %s\n", getId(), FormatMoney(t_outputs_total));
LogPrint("zrpcunsafe", "%s: private output: %s\n", getId(), FormatMoney(z_outputs_total));
LogPrint("zrpc", "%s: transparent input: %s (to choose from)\n", getId(), FormatMoney(txValues.t_inputs_total));
LogPrint("zrpcunsafe", "%s: private input: %s (to choose from)\n", getId(), FormatMoney(txValues.z_inputs_total));
LogPrint("zrpc", "%s: transparent output: %s\n", getId(), FormatMoney(txValues.t_outputs_total));
LogPrint("zrpcunsafe", "%s: private output: %s\n", getId(), FormatMoney(txValues.z_outputs_total));
LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(minersFee));
KeyIO keyIO(Params());
@ -405,7 +345,7 @@ bool AsyncRPCOperation_sendmany::main_impl() {
ops.push_back(t.op);
notes.push_back(t.note);
sum += t.note.value();
if (sum >= targetAmount) {
if (sum >= txValues.targetAmount) {
break;
}
}
@ -479,8 +419,8 @@ bool AsyncRPCOperation_sendmany::main_impl() {
if (isPureTaddrOnlyTx) {
add_taddr_outputs_to_tx();
CAmount funds = selectedUTXOAmount;
CAmount fundsSpent = t_outputs_total + minersFee;
CAmount funds = txValues.t_inputs_total;
CAmount fundsSpent = txValues.t_outputs_total + minersFee;
CAmount change = funds - fundsSpent;
CReserveKey keyChange(pwalletMain);
@ -517,7 +457,7 @@ bool AsyncRPCOperation_sendmany::main_impl() {
for (auto o : z_sprout_inputs_) {
zInputsDeque.push_back(o);
tmp += o.amount;
if (tmp >= targetAmount) {
if (tmp >= txValues.targetAmount) {
break;
}
}
@ -556,13 +496,13 @@ bool AsyncRPCOperation_sendmany::main_impl() {
if (isfromtaddr_) {
add_taddr_outputs_to_tx();
CAmount funds = selectedUTXOAmount;
CAmount fundsSpent = t_outputs_total + minersFee + z_outputs_total;
CAmount funds = txValues.t_inputs_total;
CAmount fundsSpent = txValues.t_outputs_total + minersFee + txValues.z_outputs_total;
CAmount change = funds - fundsSpent;
CReserveKey keyChange(pwalletMain);
if (change > 0) {
if (selectedUTXOCoinbase) {
if (txValues.selectedUTXOCoinbase) {
assert(isSingleZaddrOutput);
throw JSONRPCError(RPC_WALLET_ERROR, strprintf(
"Change %s not allowed. When shielding coinbase funds, the wallet does not "
@ -630,9 +570,9 @@ bool AsyncRPCOperation_sendmany::main_impl() {
int changeOutputIndex = -1; // this is updated after each joinsplit if jsChange > 0
bool vpubNewProcessed = false; // updated when vpub_new for miner fee and taddr outputs is set in last joinsplit
CAmount vpubNewTarget = minersFee;
if (t_outputs_total > 0) {
if (txValues.t_outputs_total > 0) {
add_taddr_outputs_to_tx();
vpubNewTarget += t_outputs_total;
vpubNewTarget += txValues.t_outputs_total;
}
// Keep track of treestate within this transaction
@ -825,7 +765,7 @@ bool AsyncRPCOperation_sendmany::main_impl() {
if (jsInputValue < vpubNewTarget) {
throw JSONRPCError(RPC_WALLET_ERROR,
strprintf("Insufficient funds for vpub_new %s (miners fee %s, taddr outputs %s)",
FormatMoney(vpubNewTarget), FormatMoney(minersFee), FormatMoney(t_outputs_total)));
FormatMoney(vpubNewTarget), FormatMoney(minersFee), FormatMoney(txValues.t_outputs_total)));
}
outAmount += vpubNewTarget;
info.vpub_new += vpubNewTarget; // funds flowing back to public pool
@ -891,65 +831,96 @@ bool AsyncRPCOperation_sendmany::main_impl() {
}
bool AsyncRPCOperation_sendmany::find_utxos(bool fAcceptCoinbase=false) {
bool AsyncRPCOperation_sendmany::find_utxos(bool fAcceptCoinbase, TxValues& txValues) {
std::set<CTxDestination> destinations;
if (!useanyutxo_) {
destinations.insert(fromtaddr_);
}
vector<COutput> vecOutputs;
LOCK2(cs_main, pwalletMain->cs_wallet);
pwalletMain->AvailableCoins(vecOutputs, false, NULL, true, fAcceptCoinbase);
BOOST_FOREACH(const COutput& out, vecOutputs) {
if (!out.fSpendable) {
continue;
}
if (out.nDepth < mindepth_) {
continue;
}
if (destinations.size()) {
CTxDestination address;
if (!ExtractDestination(out.tx->vout[out.i].scriptPubKey, address)) {
continue;
}
if (!destinations.count(address)) {
continue;
}
}
// By default we ignore coinbase outputs
bool isCoinbase = out.tx->IsCoinBase();
if (isCoinbase && fAcceptCoinbase==false) {
continue;
}
CScript scriptPubKey = out.tx->vout[out.i].scriptPubKey;
CAmount nValue = out.tx->vout[out.i].nValue;
SendManyInputUTXO utxo(out.tx->GetHash(), out.i, scriptPubKey, nValue, isCoinbase);
t_inputs_.push_back(utxo);
}
pwalletMain->AvailableCoins(
t_inputs_,
false, // fOnlyConfirmed
nullptr, // coinControl
true, // fIncludeZeroValue
fAcceptCoinbase, // fIncludeCoinBase
true, // fOnlySpendable
mindepth_, // nMinDepth
&destinations); // onlyFilterByDests
if (t_inputs_.empty()) return false;
// sort in ascending order, so smaller utxos appear first
std::sort(t_inputs_.begin(), t_inputs_.end(), [](SendManyInputUTXO i, SendManyInputUTXO j) -> bool {
return i.amount < j.amount;
std::sort(t_inputs_.begin(), t_inputs_.end(), [](const COutput& i, const COutput& j) -> bool {
return i.Value() < j.Value();
});
// Load transparent inputs
load_inputs(txValues);
return t_inputs_.size() > 0;
}
bool AsyncRPCOperation_sendmany::load_inputs(TxValues& txValues) {
// If from address is a taddr, select UTXOs to spend
CAmount selectedUTXOAmount = 0;
// Get dust threshold
CKey secret;
secret.MakeNewKey(true);
CScript scriptPubKey = GetScriptForDestination(secret.GetPubKey().GetID());
CTxOut out(CAmount(1), scriptPubKey);
CAmount dustThreshold = out.GetDustThreshold(minRelayTxFee);
CAmount dustChange = -1;
std::vector<COutput> selectedTInputs;
for (const COutput& out : t_inputs_) {
if (out.fIsCoinbase) {
txValues.selectedUTXOCoinbase = true;
}
selectedUTXOAmount += out.Value();
selectedTInputs.emplace_back(out);
if (selectedUTXOAmount >= txValues.targetAmount) {
// Select another utxo if there is change less than the dust threshold.
dustChange = selectedUTXOAmount - txValues.targetAmount;
if (dustChange == 0 || dustChange >= dustThreshold) {
break;
}
}
}
t_inputs_ = selectedTInputs;
txValues.t_inputs_total = selectedUTXOAmount;
if (txValues.t_inputs_total < txValues.targetAmount) {
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS,
strprintf("Insufficient transparent funds, have %s, need %s",
FormatMoney(txValues.t_inputs_total), FormatMoney(txValues.targetAmount)));
}
// If there is transparent change, is it valid or is it dust?
if (dustChange < dustThreshold && dustChange != 0) {
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS,
strprintf("Insufficient transparent funds, have %s, need %s more to avoid creating invalid change output %s (dust threshold is %s)",
FormatMoney(txValues.t_inputs_total), FormatMoney(dustThreshold - dustChange), FormatMoney(dustChange), FormatMoney(dustThreshold)));
}
// update the transaction with these inputs
if (isUsingBuilder_) {
for (const auto& out : t_inputs_) {
const CTxOut& txOut = out.tx->vout[out.i];
builder_.AddTransparentInput(COutPoint(out.tx->GetHash(), out.i), txOut.scriptPubKey, txOut.nValue);
}
} else {
CMutableTransaction rawTx(tx_);
for (const auto& out : t_inputs_) {
rawTx.vin.push_back(CTxIn(COutPoint(out.tx->GetHash(), out.i)));
}
tx_ = CTransaction(rawTx);
}
return true;
}
bool AsyncRPCOperation_sendmany::find_unspent_notes() {
std::vector<SproutNoteEntry> sproutEntries;
std::vector<SaplingNoteEntry> saplingEntries;
{
LOCK2(cs_main, pwalletMain->cs_wallet);
pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, fromaddress_, mindepth_);
}
pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, fromaddress_, mindepth_);
// If using the TransactionBuilder, we only want Sapling notes.
// If not using it, we only want Sprout notes.

View File

@ -26,6 +26,7 @@
#define ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE 10000
using namespace libzcash;
class TxValues;
class SendManyRecipient {
public:
@ -37,18 +38,6 @@ public:
address(address_), amount(amount_), memo(memo_) {}
};
class SendManyInputUTXO {
public:
uint256 txid;
int vout;
CScript scriptPubKey;
CAmount amount;
bool coinbase;
SendManyInputUTXO(uint256 txid_, int vout_, CScript scriptPubKey_, CAmount amount_, bool coinbase_) :
txid(txid_), vout(vout_), scriptPubKey(scriptPubKey_), amount(amount_), coinbase(coinbase_) {}
};
class SendManyInputJSOP {
public:
JSOutPoint point;
@ -127,7 +116,7 @@ private:
std::vector<SendManyRecipient> t_outputs_;
std::vector<SendManyRecipient> z_outputs_;
std::vector<SendManyInputUTXO> t_inputs_;
std::vector<COutput> t_inputs_;
std::vector<SendManyInputJSOP> z_sprout_inputs_;
std::vector<SaplingNoteEntry> z_sapling_inputs_;
@ -137,7 +126,9 @@ private:
void add_taddr_change_output_to_tx(CReserveKey& keyChange, CAmount amount);
void add_taddr_outputs_to_tx();
bool find_unspent_notes();
bool find_utxos(bool fAcceptCoinbase);
bool find_utxos(bool fAcceptCoinbase, TxValues& txValues);
// Load transparent inputs into the transaction or the transactionBuilder (in case of have it)
bool load_inputs(TxValues& txValues);
std::array<unsigned char, ZC_MEMO_SIZE> get_memo_from_hex_string(std::string s);
bool main_impl();
@ -186,10 +177,6 @@ public:
bool find_unspent_notes() {
return delegate->find_unspent_notes();
}
bool find_utxos(bool fAcceptCoinbase) {
return delegate->find_utxos(fAcceptCoinbase);
}
std::array<unsigned char, ZC_MEMO_SIZE> get_memo_from_hex_string(std::string s) {
return delegate->get_memo_from_hex_string(s);

View File

@ -1209,7 +1209,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals)
operation->main();
BOOST_CHECK(operation->isFailed());
std::string msg = operation->getErrorMessage();
BOOST_CHECK( msg.find("Insufficient funds, no UTXOs found") != string::npos);
BOOST_CHECK( msg.find("Insufficient transparent funds") != string::npos);
}
// minconf cannot be zero when sending from zaddr

View File

@ -3209,8 +3209,16 @@ CAmount CWallet::GetImmatureWatchOnlyBalance() const
return nTotal;
}
void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const CCoinControl *coinControl, bool fIncludeZeroValue, bool fIncludeCoinBase) const
void CWallet::AvailableCoins(vector<COutput>& vCoins,
bool fOnlyConfirmed,
const CCoinControl *coinControl,
bool fIncludeZeroValue,
bool fIncludeCoinBase,
bool fOnlySpendable,
int nMinDepth,
std::set<CTxDestination>* onlyFilterByDests) const
{
assert(nMinDepth >= 0);
vCoins.clear();
{
@ -3229,21 +3237,36 @@ void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const
if (pcoin->IsCoinBase() && !fIncludeCoinBase)
continue;
if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0)
bool isCoinbase = pcoin->IsCoinBase();
if (isCoinbase && pcoin->GetBlocksToMaturity() > 0)
continue;
int nDepth = pcoin->GetDepthInMainChain();
if (nDepth < 0)
if (nDepth < nMinDepth)
continue;
for (unsigned int i = 0; i < pcoin->vout.size(); i++) {
isminetype mine = IsMine(pcoin->vout[i]);
const auto& output = pcoin->vout[i];
isminetype mine = IsMine(output);
bool isSpendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) ||
(coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO);
if (fOnlySpendable && !isSpendable)
continue;
// Filter by specific destinations if needed
if (onlyFilterByDests && !onlyFilterByDests->empty()) {
CTxDestination address;
if (!ExtractDestination(output.scriptPubKey, address) || onlyFilterByDests->count(address) == 0) {
continue;
}
}
if (!(IsSpent(wtxid, i)) && mine != ISMINE_NO &&
!IsLockedCoin((*it).first, i) && (pcoin->vout[i].nValue > 0 || fIncludeZeroValue) &&
(!coinControl || !coinControl->HasSelected() || coinControl->fAllowOtherInputs || coinControl->IsSelected((*it).first, i)))
vCoins.push_back(COutput(pcoin, i, nDepth,
((mine & ISMINE_SPENDABLE) != ISMINE_NO) ||
(coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO)));
vCoins.push_back(COutput(pcoin, i, nDepth, isSpendable, isCoinbase));
}
}
}

View File

@ -620,12 +620,12 @@ public:
int i;
int nDepth;
bool fSpendable;
bool fIsCoinbase;
COutput(const CWalletTx *txIn, int iIn, int nDepthIn, bool fSpendableIn)
{
tx = txIn; i = iIn; nDepth = nDepthIn; fSpendable = fSpendableIn;
}
COutput(const CWalletTx *txIn, int iIn, int nDepthIn, bool fSpendableIn, bool fIsCoinbaseIn = false) :
tx(txIn), i(iIn), nDepth(nDepthIn), fSpendable(fSpendableIn), fIsCoinbase(fIsCoinbaseIn){ }
CAmount Value() const { return tx->vout[i].nValue; }
std::string ToString() const;
};
@ -1010,7 +1010,14 @@ public:
/**
* populate vCoins with vector of available COutputs.
*/
void AvailableCoins(std::vector<COutput>& vCoins, bool fOnlyConfirmed=true, const CCoinControl *coinControl = NULL, bool fIncludeZeroValue=false, bool fIncludeCoinBase=true) const;
void AvailableCoins(std::vector<COutput>& vCoins,
bool fOnlyConfirmed=true,
const CCoinControl *coinControl = NULL,
bool fIncludeZeroValue=false,
bool fIncludeCoinBase=true,
bool fOnlySpendable=false,
int nMinDepth = 0,
std::set<CTxDestination>* onlyFilterByDests = nullptr) const;
/**
* Shuffle and select coins until nTargetValue is reached while avoiding