From dce032ce294fe0d531770f540b1de00dc1d13f4b Mon Sep 17 00:00:00 2001 From: Jeremy Rubin Date: Fri, 30 Aug 2019 12:39:41 -0700 Subject: [PATCH 1/9] Make IsTrusted scan parents recursively --- src/wallet/wallet.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6adcf15167b..5acd0dbf0c3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2319,8 +2319,12 @@ bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) const if (parent == nullptr) return false; const CTxOut& parentOut = parent->tx->vout[txin.prevout.n]; + // Check that this specific input being spent is trusted if (pwallet->IsMine(parentOut) != ISMINE_SPENDABLE) return false; + // Recurse to check that the parent is also trusted + if (!parent->IsTrusted(locked_chain)) + return false; } return true; } From 595f09d6de7f1b94428cdd1310777aa6a4c584e5 Mon Sep 17 00:00:00 2001 From: Jeremy Rubin Date: Fri, 30 Aug 2019 13:25:41 -0700 Subject: [PATCH 2/9] Cache tx Trust per-call to avoid DoS --- src/wallet/wallet.cpp | 12 +++++++++++- src/wallet/wallet.h | 1 + 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5acd0dbf0c3..ae6c268484c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2294,6 +2294,12 @@ bool CWalletTx::InMempool() const } 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& trustedParents) const { // Quick answer in most cases if (!locked_chain.checkFinalTx(*tx)) { @@ -2322,9 +2328,13 @@ bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) const // 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 (trustedParents.count(parent->GetHash())) + continue; // Recurse to check that the parent is also trusted - if (!parent->IsTrusted(locked_chain)) + if (!parent->IsTrusted(locked_chain, trustedParents)) return false; + trustedParents.insert(parent->GetHash()); } return true; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f9e2230a6ff..986baa89ee8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -616,6 +616,7 @@ public: bool InMempool() const; bool IsTrusted(interfaces::Chain::Lock& locked_chain) const; + bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set& trustedParents) const; int64_t GetTxTime() const; From 5dd7da4ccd1354f09e2d00bab29288db0d5665d0 Mon Sep 17 00:00:00 2001 From: Jeremy Rubin Date: Fri, 30 Aug 2019 13:31:11 -0700 Subject: [PATCH 3/9] Reuse trustedParents in looped calls to IsTrusted --- src/wallet/wallet.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ae6c268484c..a14d538fe6d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2420,10 +2420,11 @@ CWallet::Balance CWallet::GetBalance(const int min_depth, bool avoid_reuse) cons { auto locked_chain = chain().lock(); LOCK(cs_wallet); + std::set trustedParents; 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, trustedParents)}; 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)}; @@ -2470,6 +2471,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 trustedParents; for (const auto& entry : mapWallet) { const uint256& wtxid = entry.first; @@ -2491,7 +2493,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, trustedParents); // We should not consider coins from transactions that are replacing // other transactions. @@ -3776,11 +3778,12 @@ std::map CWallet::GetAddressBalances(interfaces::Chain: { LOCK(cs_wallet); + std::set trustedParents; for (const auto& walletEntry : mapWallet) { const CWalletTx& wtx = walletEntry.second; - if (!wtx.IsTrusted(locked_chain)) + if (!wtx.IsTrusted(locked_chain, trustedParents)) continue; if (wtx.IsImmatureCoinBase(locked_chain)) From a550c58267f50c59c2eea1d46edaa5019a8ad5d8 Mon Sep 17 00:00:00 2001 From: Jeremy Rubin Date: Tue, 3 Sep 2019 10:24:10 -0700 Subject: [PATCH 4/9] Update wallet_balance.py test to reflect new behavior --- test/functional/wallet_balance.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py index c50dcd987a8..9cf55dc65a0 100755 --- a/test/functional/wallet_balance.py +++ b/test/functional/wallet_balance.py @@ -112,10 +112,10 @@ class WalletTest(BitcoinTestFramework): 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')) # change from node 1's send # 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 +125,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')) From 5ffe0d144923f365cb1c2fad181eca15d1668692 Mon Sep 17 00:00:00 2001 From: Jeremy Rubin Date: Wed, 4 Sep 2019 11:40:53 -0700 Subject: [PATCH 5/9] Update comment in test/functional/wallet_balance.py Co-Authored-By: MarcoFalke --- test/functional/wallet_balance.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py index 9cf55dc65a0..1325681c9cf 100755 --- a/test/functional/wallet_balance.py +++ b/test/functional/wallet_balance.py @@ -112,7 +112,7 @@ class WalletTest(BitcoinTestFramework): 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('0')) # 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('0')) From b49dcbedf79613f0e0f61bfd742ed265213ed280 Mon Sep 17 00:00:00 2001 From: Jeremy Rubin Date: Fri, 6 Sep 2019 09:15:49 -0700 Subject: [PATCH 6/9] update variable naming conventions for IsTrusted --- src/wallet/wallet.cpp | 20 ++++++++++---------- src/wallet/wallet.h | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a14d538fe6d..22043810795 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2299,7 +2299,7 @@ bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) const return IsTrusted(locked_chain, s); } -bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::set& trustedParents) const +bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::set& trusted_parents) const { // Quick answer in most cases if (!locked_chain.checkFinalTx(*tx)) { @@ -2329,12 +2329,12 @@ bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::setIsMine(parentOut) != ISMINE_SPENDABLE) return false; // If we've already trusted this parent, continue - if (trustedParents.count(parent->GetHash())) + if (trusted_parents.count(parent->GetHash())) continue; // Recurse to check that the parent is also trusted - if (!parent->IsTrusted(locked_chain, trustedParents)) + if (!parent->IsTrusted(locked_chain, trusted_parents)) return false; - trustedParents.insert(parent->GetHash()); + trusted_parents.insert(parent->GetHash()); } return true; } @@ -2420,11 +2420,11 @@ CWallet::Balance CWallet::GetBalance(const int min_depth, bool avoid_reuse) cons { auto locked_chain = chain().lock(); LOCK(cs_wallet); - std::set trustedParents; + std::set trusted_parents; for (const auto& entry : mapWallet) { const CWalletTx& wtx = entry.second; - const bool is_trusted{wtx.IsTrusted(*locked_chain, trustedParents)}; + 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)}; @@ -2471,7 +2471,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 trustedParents; + std::set trusted_parents; for (const auto& entry : mapWallet) { const uint256& wtxid = entry.first; @@ -2493,7 +2493,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< if (nDepth == 0 && !wtx.InMempool()) continue; - bool safeTx = wtx.IsTrusted(locked_chain, trustedParents); + bool safeTx = wtx.IsTrusted(locked_chain, trusted_parents); // We should not consider coins from transactions that are replacing // other transactions. @@ -3778,12 +3778,12 @@ std::map CWallet::GetAddressBalances(interfaces::Chain: { LOCK(cs_wallet); - std::set trustedParents; + std::set trusted_parents; for (const auto& walletEntry : mapWallet) { const CWalletTx& wtx = walletEntry.second; - if (!wtx.IsTrusted(locked_chain, trustedParents)) + 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 986baa89ee8..3513ebb91c6 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -616,7 +616,7 @@ public: bool InMempool() const; bool IsTrusted(interfaces::Chain::Lock& locked_chain) const; - bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set& trustedParents) const; + bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set& trusted_parents) const; int64_t GetTxTime() const; From 8f174ef112199aa4e98d756039855cc561687c2e Mon Sep 17 00:00:00 2001 From: Jeremy Rubin Date: Fri, 6 Sep 2019 09:24:35 -0700 Subject: [PATCH 7/9] Systematize style of IsTrusted single line if --- src/wallet/wallet.cpp | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 22043810795..96ae7bbb1e5 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2302,38 +2302,29 @@ bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) const bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::set& 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]; // Check that this specific input being spent is trusted - if (pwallet->IsMine(parentOut) != ISMINE_SPENDABLE) - return false; + if (pwallet->IsMine(parentOut) != ISMINE_SPENDABLE) return false; // If we've already trusted this parent, continue - if (trusted_parents.count(parent->GetHash())) - 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; + if (!parent->IsTrusted(locked_chain, trusted_parents)) return false; trusted_parents.insert(parent->GetHash()); } return true; From 91f3073f08aff395dd813296bf99fd8ccc81bb27 Mon Sep 17 00:00:00 2001 From: Jeremy Rubin Date: Mon, 21 Oct 2019 13:22:56 -0700 Subject: [PATCH 8/9] Update release notes to mention changes to IsTrusted and impact on wallet --- doc/release-notes.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/release-notes.md b/doc/release-notes.md index ea82962e755..a47c8802b0c 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 ================= From 4671fc3d9e669da8b8781f0cbefee43cb9acd527 Mon Sep 17 00:00:00 2001 From: Jeremy Rubin Date: Mon, 21 Oct 2019 13:43:44 -0700 Subject: [PATCH 9/9] Expand on wallet_balance.py comment from https://github.com/bitcoin/bitcoin/pull/16766\#issuecomment-527563982 --- test/functional/wallet_balance.py | 38 +++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py index 1325681c9cf..a5f9a047edc 100755 --- a/test/functional/wallet_balance.py +++ b/test/functional/wallet_balance.py @@ -109,6 +109,44 @@ 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