diff --git a/doc/release-notes.md b/doc/release-notes.md
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -5,3 +5,8 @@
This release includes the following features and fixes:
+
+Wallet
+------
+
+- The way that output trust was computed has been fixed, which impacts confirmed/unconfirmed balance status and coin selection.
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -490,6 +490,8 @@
bool InMempool() const;
bool IsTrusted(interfaces::Chain::Lock &locked_chain) const;
+ bool IsTrusted(interfaces::Chain::Lock &locked_chain,
+ std::set &trusted_parents) const;
int64_t GetTxTime() const;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2053,6 +2053,12 @@
}
bool CWalletTx::IsTrusted(interfaces::Chain::Lock &locked_chain) const {
+ std::set s;
+ return IsTrusted(locked_chain, s);
+}
+
+bool CWalletTx::IsTrusted(interfaces::Chain::Lock &locked_chain,
+ std::set &trusted_parents) const {
// Quick answer in most cases
TxValidationState state;
if (!locked_chain.contextualCheckTransactionForCurrentBlock(
@@ -2089,9 +2095,19 @@
}
const CTxOut &parentOut = parent->tx->vout[txin.prevout.GetN()];
+ // 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->GetId())) {
+ continue;
+ }
+ // Recurse to check that the parent is also trusted
+ if (!parent->IsTrusted(locked_chain, trusted_parents)) {
+ return false;
+ }
+ trusted_parents.insert(parent->GetId());
}
return true;
@@ -2193,9 +2209,10 @@
isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED;
auto locked_chain = chain().lock();
LOCK(cs_wallet);
+ std::set 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()};
const Amount tx_credit_mine{wtx.GetAvailableCredit(
/* fUseCache */ true, ISMINE_SPENDABLE | reuse_filter)};
@@ -2255,6 +2272,7 @@
const Consensus::Params params = Params().GetConsensus();
+ std::set trusted_parents;
for (const auto &entry : mapWallet) {
const TxId &wtxid = entry.first;
const CWalletTx &wtx = entry.second;
@@ -2281,7 +2299,7 @@
continue;
}
- bool safeTx = wtx.IsTrusted(locked_chain);
+ bool safeTx = wtx.IsTrusted(locked_chain, trusted_parents);
// Bitcoin-ABC: Removed check that prevents consideration of coins from
// transactions that are replacing other transactions. This check based
@@ -3489,10 +3507,11 @@
std::map balances;
LOCK(cs_wallet);
+ std::set 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;
}
diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py
--- a/test/functional/wallet_balance.py
+++ b/test/functional/wallet_balance.py
@@ -127,21 +127,52 @@
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
# change from node 0's send
assert_equal(self.nodes[0].getbalance(), Decimal('9.99'))
- # change from node 1's send
- assert_equal(
- self.nodes[1].getbalance(),
- Decimal('30') - fee_node_1)
+ # node 1's send had an unsafe input
+ assert_equal(self.nodes[1].getbalance(), Decimal('0'))
# 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'))
@@ -155,11 +186,15 @@
"unconfirmed_balance"], Decimal('60'))
# Doesn't include output of node 0's send since it was spent
- assert_equal(self.nodes[1].getunconfirmedbalance(), Decimal('0'))
- 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)
+ 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'))