diff options
| -rw-r--r-- | doc/release-notes.md | 1 | ||||
| -rw-r--r-- | src/wallet/wallet.cpp | 44 | ||||
| -rw-r--r-- | src/wallet/wallet.h | 1 | ||||
| -rwxr-xr-x | test/functional/wallet_balance.py | 48 |
4 files changed, 71 insertions, 23 deletions
diff --git a/doc/release-notes.md b/doc/release-notes.md index ea82962e75..a47c8802b0 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -85,6 +85,7 @@ Wallet ------ - The wallet now by default uses bech32 addresses when using RPC, and creates native segwit change outputs. +- The way that output trust was computed has been fixed in #16766, which impacts confirmed/unconfirmed balance status and coin selection. Low-level changes ================= diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b10a5deedc..0b7dc256ad 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1850,32 +1850,37 @@ bool CWalletTx::InMempool() const bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) const { + std::set<uint256> s; + return IsTrusted(locked_chain, s); +} + +bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const +{ // Quick answer in most cases - if (!locked_chain.checkFinalTx(*tx)) { - return false; - } + if (!locked_chain.checkFinalTx(*tx)) return false; int nDepth = GetDepthInMainChain(locked_chain); - if (nDepth >= 1) - return true; - if (nDepth < 0) - return false; - if (!pwallet->m_spend_zero_conf_change || !IsFromMe(ISMINE_ALL)) // using wtx's cached debit - return false; + if (nDepth >= 1) return true; + if (nDepth < 0) return false; + // using wtx's cached debit + if (!pwallet->m_spend_zero_conf_change || !IsFromMe(ISMINE_ALL)) return false; // Don't trust unconfirmed transactions from us unless they are in the mempool. - if (!InMempool()) - return false; + if (!InMempool()) return false; // Trusted if all inputs are from us and are in the mempool: for (const CTxIn& txin : tx->vin) { // Transactions not sent by us: not trusted const CWalletTx* parent = pwallet->GetWalletTx(txin.prevout.hash); - if (parent == nullptr) - return false; + if (parent == nullptr) return false; const CTxOut& parentOut = parent->tx->vout[txin.prevout.n]; - if (pwallet->IsMine(parentOut) != ISMINE_SPENDABLE) - return false; + // Check that this specific input being spent is trusted + if (pwallet->IsMine(parentOut) != ISMINE_SPENDABLE) return false; + // If we've already trusted this parent, continue + if (trusted_parents.count(parent->GetHash())) continue; + // Recurse to check that the parent is also trusted + if (!parent->IsTrusted(locked_chain, trusted_parents)) return false; + trusted_parents.insert(parent->GetHash()); } return true; } @@ -1961,10 +1966,11 @@ CWallet::Balance CWallet::GetBalance(const int min_depth, bool avoid_reuse) cons { auto locked_chain = chain().lock(); LOCK(cs_wallet); + std::set<uint256> trusted_parents; for (const auto& entry : mapWallet) { const CWalletTx& wtx = entry.second; - const bool is_trusted{wtx.IsTrusted(*locked_chain)}; + const bool is_trusted{wtx.IsTrusted(*locked_chain, trusted_parents)}; const int tx_depth{wtx.GetDepthInMainChain(*locked_chain)}; const CAmount tx_credit_mine{wtx.GetAvailableCredit(*locked_chain, /* fUseCache */ true, ISMINE_SPENDABLE | reuse_filter)}; const CAmount tx_credit_watchonly{wtx.GetAvailableCredit(*locked_chain, /* fUseCache */ true, ISMINE_WATCH_ONLY | reuse_filter)}; @@ -2011,6 +2017,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< const int min_depth = {coinControl ? coinControl->m_min_depth : DEFAULT_MIN_DEPTH}; const int max_depth = {coinControl ? coinControl->m_max_depth : DEFAULT_MAX_DEPTH}; + std::set<uint256> trusted_parents; for (const auto& entry : mapWallet) { const uint256& wtxid = entry.first; @@ -2032,7 +2039,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< if (nDepth == 0 && !wtx.InMempool()) continue; - bool safeTx = wtx.IsTrusted(locked_chain); + bool safeTx = wtx.IsTrusted(locked_chain, trusted_parents); // We should not consider coins from transactions that are replacing // other transactions. @@ -3094,11 +3101,12 @@ std::map<CTxDestination, CAmount> CWallet::GetAddressBalances(interfaces::Chain: { LOCK(cs_wallet); + std::set<uint256> trusted_parents; for (const auto& walletEntry : mapWallet) { const CWalletTx& wtx = walletEntry.second; - if (!wtx.IsTrusted(locked_chain)) + if (!wtx.IsTrusted(locked_chain, trusted_parents)) continue; if (wtx.IsImmatureCoinBase(locked_chain)) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 7d0fae0bc7..6c9b3f40ab 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -476,6 +476,7 @@ public: bool InMempool() const; bool IsTrusted(interfaces::Chain::Lock& locked_chain) const; + bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const; int64_t GetTxTime() const; diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py index c50dcd987a..a5f9a047ed 100755 --- a/test/functional/wallet_balance.py +++ b/test/functional/wallet_balance.py @@ -109,13 +109,51 @@ class WalletTest(BitcoinTestFramework): self.log.info("Test getbalance and getunconfirmedbalance with unconfirmed inputs") + # Before `test_balance()`, we have had two nodes with a balance of 50 + # each and then we: + # + # 1) Sent 40 from node A to node B with fee 0.01 + # 2) Sent 60 from node B to node A with fee 0.01 + # + # Then we check the balances: + # + # 1) As is + # 2) With transaction 2 from above with 2x the fee + # + # Prior to #16766, in this situation, the node would immediately report + # a balance of 30 on node B as unconfirmed and trusted. + # + # After #16766, we show that balance as unconfirmed. + # + # The balance is indeed "trusted" and "confirmed" insofar as removing + # the mempool transactions would return at least that much money. But + # the algorithm after #16766 marks it as unconfirmed because the 'taint' + # tracking of transaction trust for summing balances doesn't consider + # which inputs belong to a user. In this case, the change output in + # question could be "destroyed" by replace the 1st transaction above. + # + # The post #16766 behavior is correct; we shouldn't be treating those + # funds as confirmed. If you want to rely on that specific UTXO existing + # which has given you that balance, you cannot, as a third party + # spending the other input would destroy that unconfirmed. + # + # For example, if the test transactions were: + # + # 1) Sent 40 from node A to node B with fee 0.01 + # 2) Sent 10 from node B to node A with fee 0.01 + # + # Then our node would report a confirmed balance of 40 + 50 - 10 = 80 + # BTC, which is more than would be available if transaction 1 were + # replaced. + + def test_balances(*, fee_node_1=0): # getbalance without any arguments includes unconfirmed transactions, but not untrusted transactions assert_equal(self.nodes[0].getbalance(), Decimal('9.99')) # change from node 0's send - assert_equal(self.nodes[1].getbalance(), Decimal('30') - fee_node_1) # change from node 1's send + assert_equal(self.nodes[1].getbalance(), Decimal('0')) # node 1's send had an unsafe input # Same with minconf=0 assert_equal(self.nodes[0].getbalance(minconf=0), Decimal('9.99')) - assert_equal(self.nodes[1].getbalance(minconf=0), Decimal('30') - fee_node_1) + assert_equal(self.nodes[1].getbalance(minconf=0), Decimal('0')) # getbalance with a minconf incorrectly excludes coins that have been spent more recently than the minconf blocks ago # TODO: fix getbalance tracking of coin spentness depth assert_equal(self.nodes[0].getbalance(minconf=1), Decimal('0')) @@ -125,9 +163,9 @@ class WalletTest(BitcoinTestFramework): assert_equal(self.nodes[0].getbalances()['mine']['untrusted_pending'], Decimal('60')) assert_equal(self.nodes[0].getwalletinfo()["unconfirmed_balance"], Decimal('60')) - assert_equal(self.nodes[1].getunconfirmedbalance(), Decimal('0')) # Doesn't include output of node 0's send since it was spent - assert_equal(self.nodes[1].getbalances()['mine']['untrusted_pending'], Decimal('0')) - assert_equal(self.nodes[1].getwalletinfo()["unconfirmed_balance"], Decimal('0')) + assert_equal(self.nodes[1].getunconfirmedbalance(), Decimal('30') - fee_node_1) # Doesn't include output of node 0's send since it was spent + assert_equal(self.nodes[1].getbalances()['mine']['untrusted_pending'], Decimal('30') - fee_node_1) + assert_equal(self.nodes[1].getwalletinfo()["unconfirmed_balance"], Decimal('30') - fee_node_1) test_balances(fee_node_1=Decimal('0.01')) |