8000 wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. by furszy · Pull Request #25005 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
8000

wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jun 17, 2022
Merged

wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4ce235e
wallet: return 'CoinsResult' struct in `AvailableCoins`
furszy Apr 22, 2022
9472ca0
wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times
furszy Apr 27, 2022
91902b7
wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index)
furszy Apr 27, 2022
a06fa94
wallet: IsSpent, 'COutPoint' arg instead of (hash, index)
furszy Apr 27, 2022
3d8a282
wallet: decouple IsSpentKey(scriptPubKey) from IsSpentKey(hash, n)
furszy Apr 27, 2022
4b83bf8
wallet: avoid extra IsSpentKey -> GetWalletTx lookups
furszy Apr 27, 2022
cdf185c
wallet: remove unused IsSpentKey(hash, index) method
furszy Apr 27, 2022
162d4ad
wallet: add 'only_spendable' filter to AvailableCoins
furszy Jun 2, 2022
fd5c996
wallet: GetAvailableBalance, remove double walk-through every availab…
furszy Apr 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/wallet/interfaces.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet,
result.txout = wtx.tx->vout[n];
result.time = wtx.GetTxTime();
result.depth_in_main_chain = depth;
result.is_spent = wallet.IsSpent(wtx.GetHash(), n);
result.is_spent = wallet.IsSpent(COutPoint(wtx.GetHash(), n));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's preferred to use {} list initialization - any reason not to? (Here and in quite a few other places)

Suggested change
result.is_spent = wallet.IsSpent(COutPoint(wtx.GetHash(), n));
result.is_spent = wallet.IsSpent(COutPoint{wtx.GetHash(), n});

return result;
}

Expand All @@ -121,7 +121,7 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet,
result.txout = output.txout;
result.time = output.time;
result.depth_in_main_chain = output.depth;
result.is_spent = wallet.IsSpent(output.outpoint.hash, output.outpoint.n);
result.is_spent = wallet.IsSpent(output.outpoint);
return result;
}

Expand Down Expand Up @@ -245,7 +245,7 @@ class WalletImpl : public Wallet
bool isLockedCoin(const COutPoint& output) override
{
LOCK(m_wallet->cs_wallet);
return m_wallet->IsLockedCoin(output.hash, output.n);
return m_wallet->IsLockedCoin(output);
}
void listLockedCoins(std::vector<COutPoint>& outputs) override
{
Expand Down
17 changes: 8 additions & 9 deletions src/wallet/receive.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,9 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx,
bool allow_used_addresses = (filter & ISMINE_USED) || !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);
CAmount nCredit = 0;
uint256 hashTx = wtx.GetHash();
for (unsigned int i = 0; i < wtx.tx->vout.size(); i++)
{
if (!wallet.IsSpent(hashTx, i) && (allow_used_addresses || !wallet.IsSpentKey(hashTx, i))) {
const CTxOut &txout = wtx.tx->vout[i];
for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
const CTxOut& txout = wtx.tx->vout[i];
if (!wallet.IsSpent(COutPoint(hashTx, i)) && (allow_used_addresses || !wallet.IsSpentKey(txout.scriptPubKey))) {
nCredit += OutputGetCredit(wallet, txout, filter);
if (!MoneyRange(nCredit))
throw std::runtime_error(std::string(__func__) + " : value out of range");
Expand Down Expand Up @@ -371,15 +370,15 @@ std::map<CTxDestination, CAmount> GetAddressBalances(const CWallet& wallet)
if (nDepth < (CachedTxIsFromMe(wallet, wtx, ISMINE_ALL) ? 0 : 1))
continue;

for (unsigned int i = 0; i < wtx.tx->vout.size(); i++)
{
for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
const auto& output = wtx.tx->vout[i];
CTxDestination addr;
if (!wallet.IsMine(wtx.tx->vout[i]))
if (!wallet.IsMine(output))
continue;
if(!ExtractDestination(wtx.tx->vout[i].scriptPubKey, addr))
if(!ExtractDestination(output.scriptPubKey, addr))
continue;

CAmount n = wallet.IsSpent(walletEntry.first, i) ? 0 : wtx.tx->vout[i].nValue;
CAmount n = wallet.IsSpent(COutPoint(walletEntry.first, i)) ? 0 : output.nValue;
balances[addr] += n;
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/wallet/rpc/coins.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
8000 Expand Up @@ -341,11 +341,11 @@ RPCHelpMan lockunspent()
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout index out of bounds");
}

if (pwallet->IsSpent(outpt.hash, outpt.n)) {
if (pwallet->IsSpent(outpt)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected unspent output");
}

const bool is_locked = pwallet->IsLockedCoin(outpt.hash, outpt.n);
const bool is_locked = pwallet->IsLockedCoin(outpt);

if (fUnlock && !is_locked) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected locked output");
Expand Down Expand Up @@ -638,7 +638,7 @@ RPCHelpMan listunspent()
cctl.m_max_depth = nMaxDepth;
cctl.m_include_unsafe_inputs = include_unsafe;
LOCK(pwallet->cs_wallet);
AvailableCoinsListUnspent(*pwallet, vecOutputs, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount);
vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount).coins;
}

LOCK(pwallet->cs_wallet);
Expand All @@ -649,7 +649,7 @@ RPCHelpMan listunspent()
CTxDestination address;
const CScript& scriptPubKey = out.txout.scriptPubKey;
bool fValidAddress = ExtractDestination(scriptPubKey, address);
bool reused = avoid_reuse && pwallet->IsSpentKey(out.outpoint.hash, out.outpoint.n);
bool reused = avoid_reuse && pwallet->IsSpentKey(scriptPubKey);

if (destinations.size() && (!fValidAddress || !destinations.count(address)))
continue;
Expand Down
6 changes: 2 additions & 4 deletions src/wallet/rpc/spend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1380,15 +1380,14 @@ RPCHelpMan sendall()

CMutableTransaction rawTx{ConstructTransaction(options["inputs"], recipient_key_value_pairs, options["locktime"], rbf)};
LOCK(pwallet->cs_wallet);
std::vector<COutput> all_the_utxos;

CAmount total_input_value(0);
bool send_max{options.exists("send_max") ? options["send_max"].get_bool() : false};
if (options.exists("inputs") && options.exists("send_max")) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot combine send_max with specific inputs.");
} else if (options.exists("inputs")) {
for (const CTxIn& input : rawTx.vin) {
if (pwallet->IsSpent(input.prevout.hash, input.prevout.n)) {
if (pwallet->IsSpent(input.prevout)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input not available. UTXO (%s:%d) was already spent.", input.prevout.hash.ToString(), input.prevout.n));
}
const CWalletTx* tx{pwallet->GetWalletTx(input.prevout.hash)};
Expand All @@ -1398,8 +1397,7 @@ RPCHelpMan sendall()
total_input_value += tx->tx->vout[input.prevout.n].nValue;
}
} else {
AvailableCoins(*pwallet, all_the_utxos, &coin_control, fee_rate, /*nMinimumAmount=*/0);
for (const COutput& output : all_the_utxos) {
for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).coins) {
CHECK_NONFATAL(output.input_bytes > 0);
if (send_max && fee_rate.GetFee(output.input_bytes) > output.txout.nValue) {
continue;
Expand Down
91 changes: 52 additions & 39 deletions src/wallet/spend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,18 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *walle
return CalculateMaximumSignedTxSize(tx, wallet, txouts, coin_control);
}

void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl, std::optional<CFeeRate> feerate, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount)
CoinsResult AvailableCoins(const CWallet& wallet,
const CCoinControl* coinControl,
std::optional<CFeeRate> feerate,
const CAmount& nMinimumAmount,
const CAmount& nMaximumAmount,
const CAmount& nMinimumSumAmount,
const uint64_t nMaximumCount,
bool only_spendable)
{
AssertLockHeld(wallet.cs_wallet);

vCoins.clear();
CAmount nTotal = 0;
CoinsResult result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, result is declared as closely as possible to where it's used.

// Either the WALLET_FLAG_AVOID_REUSE flag is not set (in which case we always allow), or we default to avoiding, and only in the case where
// a coin control object is provided, and has the avoid address reuse flag set to false, do we allow already used addresses
bool allow_used_addresses = !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE) || (coinControl && !coinControl->m_avoid_address_reuse);
Expand Down Expand Up @@ -159,76 +165,81 @@ void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const C
bool tx_from_me = CachedTxIsFromMe(wallet, wtx, ISMINE_ALL);

for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
const CTxOut& output = wtx.tx->vout[i];
const COutPoint outpoint(wtxid, i);

// Only consider selected coins if add_inputs is false
if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(COutPoint(entry.first, i))) {
if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(outpoint)) {
continue;
}

if (wtx.tx->vout[i].nValue < nMinimumAmount || wtx.tx->vout[i].nValue > nMaximumAmount)
if (output.nValue < nMinimumAmount || output.nValue > nMaximumAmount)
continue;

if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(COutPoint(entry.first, i)))
if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(outpoint))
continue;

if (wallet.IsLockedCoin(entry.first, i))
if (wallet.IsLockedCoin(outpoint))
continue;

if (wallet.IsSpent(wtxid, i))
if (wallet.IsSpent(outpoint))
continue;

isminetype mine = wallet.IsMine(wtx.tx->vout[i]);
isminetype mine = wallet.IsMine(output);

if (mine == ISMINE_NO) {
continue;
}

if (!allow_used_addresses && wallet.IsSpentKey(wtxid, i)) {
if (!allow_used_addresses && wallet.IsSpentKey(output.scriptPubKey)) {
continue;
}

std::unique_ptr<SigningProvider> provider = wallet.GetSolvingProvider(wtx.tx->vout[i].scriptPubKey);
std::unique_ptr<SigningProvider> provider = wallet.GetSolvingProvider(output.scriptPubKey);

bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false;
bool solvable = provider ? IsSolvable(*provider, output.scriptPubKey) : false;
bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable));
int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly));

vCoins.emplace_back(COutPoint(wtx.GetHash(), i), wtx.tx->vout.at(i), nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate);
// Filter by spendable outputs only
if (!spendable && only_spendable) continue;

int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly));
result.coins.emplace_back(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate);
result.total_amount += output.nValue;

// Checks the sum amount of all UTXO's.
if (nMinimumSumAmount != MAX_MONEY) {
nTotal += wtx.tx->vout[i].nValue;

if (nTotal >= nMinimumSumAmount) {
return;
if (result.total_amount >= nMinimumSumAmount) {
return result;
}
}

// Checks the maximum number of UTXO's.
if (nMaximumCount > 0 && vCoins.size() >= nMaximumCount) {
return;
if (nMaximumCount > 0 && result.coins.size() >= nMaximumCount) {
return result;
}
}
}

return result;
}

void AvailableCoinsListUnspent(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount)
CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* coinControl, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount)
{
AvailableCoins(wallet, vCoins, coinControl, /*feerate=*/ std::nullopt, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount);
return AvailableCoins(wallet, coinControl, /*feerate=*/ std::nullopt, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount, /*only_spendable=*/false);
}

CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl)
{
LOCK(wallet.cs_wallet);

CAmount balance = 0;
std::vector<COutput> vCoins;
AvailableCoinsListUnspent(wallet, vCoins, coinControl);
for (const COutput& out : vCoins) {
if (out.spendable) {
balance += out.txout.nValue;
}
}
return balance;
return AvailableCoins(wallet,
coinControl,
std::nullopt, /*feerate=*/
1, /*nMinimumAmount*/
MAX_MONEY, /*nMaximumAmount*/
MAX_MONEY, /*nMinimumSumAmount*/
0 /*nMaximumCount*/
).total_amount;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're explicitly relying on only_spendable to be true, I think it would be nice to make the argument explicit?

}

const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const CTransaction& tx, int output)
Expand Down Expand Up @@ -260,11 +271,8 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet)
AssertLockHeld(wallet.cs_wallet);

std::map<CTxDestination, std::vector<COutput>> result;
std::vector<COutput> availableCoins;

AvailableCoinsListUnspent(wallet, availableCoins);

for (const COutput& coin : availableCoins) {
for (const COutput& coin : AvailableCoinsListUnspent(wallet).coins) {
CTxDestination address;
if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable)) &&
ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) {
Expand Down Expand Up @@ -791,11 +799,16 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal(
CAmount selection_target = recipients_sum + not_input_fees;

// Get available coins
std::vector<COutput> vAvailableCoins;
AvailableCoins(wallet, vAvailableCoins, &coin_control, coin_selection_params.m_effective_feerate, 1, MAX_MONEY, MAX_MONEY, 0);
auto res_available_coins = AvailableCoins(wallet,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think the res_ prefix is not really necessary?

Suggested change
auto res_available_coins = AvailableCoins(wallet,
auto available_coins = AvailableCoins(wallet,

&coin_control,
coin_selection_params.m_effective_feerate,
1, /*nMinimumAmount*/
MAX_MONEY, /*nMaximumAmount*/
MAX_MONEY, /*nMinimumSumAmount*/
0); /*nMaximumCount*/

// Choose coins to use
std::optional<SelectionResult> result = SelectCoins(wallet, vAvailableCoins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params);
std::optional<SelectionResult> result = SelectCoins(wallet, res_available_coins.coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: since you're already touching this, maybe you could update result to something more helpful like selected_coins? It does affect a few other lines so would understand if out of scope.

if (!result) {
error = _("Insufficient funds");
return std::nullopt;
Expand Down
19 changes: 16 additions & 3 deletions src/wallet/spend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,29 @@ struct TxSize {
TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const std::vector<CTxOut>& txouts, const CCoinControl* coin_control = nullptr);
TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const CCoinControl* coin_control = nullptr) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet);

< CEB7 /td> struct CoinsResult {
std::vector<COutput> coins;
// Sum of all the coins amounts
CAmount total_amount{0};
};
/**
* populate vCoins with vector of available COutputs.
* Return vector of available COutputs.
Copy link
Contributor
@stickies-v stickies-v Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: CoinsResult is not technically a vector of COutputs, think this could be updated a little bit?

* By default, returns only the spendable coins.
*/
void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, std::optional<CFeeRate> feerate = std::nullopt, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
CoinsResult AvailableCoins(const CWallet& wallet,
const CCoinControl* coinControl = nullptr,
std::optional<CFeeRate> feerate = std::nullopt,
const CAmount& nMinimumAmount = 1,
const CAmount& nMaximumAmount = MAX_MONEY,
const CAmount& nMinimumSumAmount = MAX_MONEY,
const uint64_t nMaximumCount = 0,
bool only_spendable = true) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);

/**
* Wrapper function for AvailableCoins which skips the `feerate` parameter. Use this function
* to list all available coins (e.g. listunspent RPC) while not intending to fund a transaction.
*/
void AvailableCoinsListUnspent(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);

CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr);

Expand Down
8 changes: 2 additions & 6 deletions src/wallet/test/wallet_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,9 +583,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup)
// Lock both coins. Confirm number of available coins drops to 0.
{
LOCK(wallet->cs_wallet);
std::vector<COutput> available;
AvailableCoinsListUnspent(*wallet, available);
BOOST_CHECK_EQUAL(available.size(), 2U);
BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).coins.size(), 2U);
}
for (const auto& group : list) {
for (const auto& coin : group.second) {
Expand All @@ -595,9 +593,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup)
}
{
LOCK(wallet->cs_wallet);
std::vector<COutput> available;
AvailableCoinsListUnspent(*wallet, available);
BOOST_CHECK_EQUAL(available.size(), 0U);
BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).coins.size(), 0U);
}
// Confirm ListCoins still returns same result as before, despite coins
// being locked.
Expand Down
Loading
0