Merge #16766: wallet: Make IsTrusted scan parents recursively
4671fc3d9e
Expand on wallet_balance.py comment from https://github.com/bitcoin/bitcoin/pull/16766\#issuecomment-527563982 (Jeremy Rubin)91f3073f08
Update release notes to mention changes to IsTrusted and impact on wallet (Jeremy Rubin)8f174ef112
Systematize style of IsTrusted single line if (Jeremy Rubin)b49dcbedf7
update variable naming conventions for IsTrusted (Jeremy Rubin)5ffe0d1449
Update comment in test/functional/wallet_balance.py (Jeremy Rubin)a550c58267
Update wallet_balance.py test to reflect new behavior (Jeremy Rubin)5dd7da4ccd
Reuse trustedParents in looped calls to IsTrusted (Jeremy Rubin)595f09d6de
Cache tx Trust per-call to avoid DoS (Jeremy Rubin)dce032ce29
Make IsTrusted scan parents recursively (Jeremy Rubin) Pull request description: This slightly modifies the behavior of IsTrusted to recursively check the parents of a transaction. Otherwise, it's possible that a parent is not IsTrusted but a child is. If a parent is not trusted, then a child should not be either. This recursive scan can be a little expensive, so ~it might be beneficial to have a way of caching IsTrusted state, but this is a little complex because various conditions can change between calls to IsTrusted (e.g., re-org).~ I added a cache which works per call/across calls, but does not store the results semi-permanently. Which reduces DoS risk of this change. There is no risk of untrusted parents causing a resource exploitation, as we immediately return once that is detected. This is a change that came up as a bug-fix esque change while working on OP_SECURETHEBAG. You can see the branch where this change is important here: https://github.com/bitcoin/bitcoin/compare/master...JeremyRubin:stb-with-rpc?expand=1. Essentially, without this change, we can be tricked into accepting an OP_SECURETHEBAG output because we don't properly check the parents. As this was a change which, on its own, was not dependent on OP_SECURETHEBAG, I broke it out as I felt the change stands on its own by fixing a long standing wallet bug. The test wallet_balance.py has been corrected to meet the new behavior. The below comment, reproduced, explains what the issue is and the edge cases that can arise before this change. # 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. The release notes have been updated to note the new behavior. ACKs for top commit: ariard: Code Review ACK4671fc3
, maybe extend DoS protection in a follow-up PR. fjahr: Code review ACK4671fc3d9e
ryanofsky: Code review ACK4671fc3d9e
. Changes since last review: 2 new commits adding suggested release note and python test comment, also a clean rebase with no changes to the earlier commits. The PR description is more comprehensive now, too. Looks good! promag: Code review ACK4671fc3d9e
. Tree-SHA512: 6b183ff425304fef49724290053514cb2770f4a2350dcb83660ef24af5c54f7c4c2c345b0f62bba60eb2d2f70625ee61a7fab76a7f491bb5a84be5c4cc86b92f
This commit is contained in:
commit
bdda137878
4 changed files with 71 additions and 23 deletions
|
@ -85,6 +85,7 @@ Wallet
|
||||||
------
|
------
|
||||||
|
|
||||||
- The wallet now by default uses bech32 addresses when using RPC, and creates native segwit change outputs.
|
- 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
|
Low-level changes
|
||||||
=================
|
=================
|
||||||
|
|
|
@ -1849,33 +1849,38 @@ bool CWalletTx::InMempool() const
|
||||||
}
|
}
|
||||||
|
|
||||||
bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) 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
|
// Quick answer in most cases
|
||||||
if (!locked_chain.checkFinalTx(*tx)) {
|
if (!locked_chain.checkFinalTx(*tx)) return false;
|
||||||
return false;
|
|
||||||
}
|
|
||||||
int nDepth = GetDepthInMainChain(locked_chain);
|
int nDepth = GetDepthInMainChain(locked_chain);
|
||||||
if (nDepth >= 1)
|
if (nDepth >= 1) return true;
|
||||||
return true;
|
if (nDepth < 0) return false;
|
||||||
if (nDepth < 0)
|
// using wtx's cached debit
|
||||||
return false;
|
if (!pwallet->m_spend_zero_conf_change || !IsFromMe(ISMINE_ALL)) return false;
|
||||||
if (!pwallet->m_spend_zero_conf_change || !IsFromMe(ISMINE_ALL)) // using wtx's cached debit
|
|
||||||
return false;
|
|
||||||
|
|
||||||
// Don't trust unconfirmed transactions from us unless they are in the mempool.
|
// Don't trust unconfirmed transactions from us unless they are in the mempool.
|
||||||
if (!InMempool())
|
if (!InMempool()) return false;
|
||||||
return false;
|
|
||||||
|
|
||||||
// Trusted if all inputs are from us and are in the mempool:
|
// Trusted if all inputs are from us and are in the mempool:
|
||||||
for (const CTxIn& txin : tx->vin)
|
for (const CTxIn& txin : tx->vin)
|
||||||
{
|
{
|
||||||
// Transactions not sent by us: not trusted
|
// Transactions not sent by us: not trusted
|
||||||
const CWalletTx* parent = pwallet->GetWalletTx(txin.prevout.hash);
|
const CWalletTx* parent = pwallet->GetWalletTx(txin.prevout.hash);
|
||||||
if (parent == nullptr)
|
if (parent == nullptr) return false;
|
||||||
return false;
|
|
||||||
const CTxOut& parentOut = parent->tx->vout[txin.prevout.n];
|
const CTxOut& parentOut = parent->tx->vout[txin.prevout.n];
|
||||||
if (pwallet->IsMine(parentOut) != ISMINE_SPENDABLE)
|
// Check that this specific input being spent is trusted
|
||||||
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;
|
||||||
|
// 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;
|
return true;
|
||||||
}
|
}
|
||||||
|
@ -1961,10 +1966,11 @@ CWallet::Balance CWallet::GetBalance(const int min_depth, bool avoid_reuse) cons
|
||||||
{
|
{
|
||||||
auto locked_chain = chain().lock();
|
auto locked_chain = chain().lock();
|
||||||
LOCK(cs_wallet);
|
LOCK(cs_wallet);
|
||||||
|
std::set<uint256> trusted_parents;
|
||||||
for (const auto& entry : mapWallet)
|
for (const auto& entry : mapWallet)
|
||||||
{
|
{
|
||||||
const CWalletTx& wtx = entry.second;
|
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 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_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)};
|
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 min_depth = {coinControl ? coinControl->m_min_depth : DEFAULT_MIN_DEPTH};
|
||||||
const int max_depth = {coinControl ? coinControl->m_max_depth : DEFAULT_MAX_DEPTH};
|
const int max_depth = {coinControl ? coinControl->m_max_depth : DEFAULT_MAX_DEPTH};
|
||||||
|
|
||||||
|
std::set<uint256> trusted_parents;
|
||||||
for (const auto& entry : mapWallet)
|
for (const auto& entry : mapWallet)
|
||||||
{
|
{
|
||||||
const uint256& wtxid = entry.first;
|
const uint256& wtxid = entry.first;
|
||||||
|
@ -2032,7 +2039,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector<
|
||||||
if (nDepth == 0 && !wtx.InMempool())
|
if (nDepth == 0 && !wtx.InMempool())
|
||||||
continue;
|
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
|
// We should not consider coins from transactions that are replacing
|
||||||
// other transactions.
|
// other transactions.
|
||||||
|
@ -3094,11 +3101,12 @@ std::map<CTxDestination, CAmount> CWallet::GetAddressBalances(interfaces::Chain:
|
||||||
|
|
||||||
{
|
{
|
||||||
LOCK(cs_wallet);
|
LOCK(cs_wallet);
|
||||||
|
std::set<uint256> trusted_parents;
|
||||||
for (const auto& walletEntry : mapWallet)
|
for (const auto& walletEntry : mapWallet)
|
||||||
{
|
{
|
||||||
const CWalletTx& wtx = walletEntry.second;
|
const CWalletTx& wtx = walletEntry.second;
|
||||||
|
|
||||||
if (!wtx.IsTrusted(locked_chain))
|
if (!wtx.IsTrusted(locked_chain, trusted_parents))
|
||||||
continue;
|
continue;
|
||||||
|
|
||||||
if (wtx.IsImmatureCoinBase(locked_chain))
|
if (wtx.IsImmatureCoinBase(locked_chain))
|
||||||
|
|
|
@ -476,6 +476,7 @@ public:
|
||||||
|
|
||||||
bool InMempool() const;
|
bool InMempool() const;
|
||||||
bool IsTrusted(interfaces::Chain::Lock& locked_chain) 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;
|
int64_t GetTxTime() const;
|
||||||
|
|
||||||
|
|
|
@ -109,13 +109,51 @@ class WalletTest(BitcoinTestFramework):
|
||||||
|
|
||||||
self.log.info("Test getbalance and getunconfirmedbalance with unconfirmed inputs")
|
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):
|
def test_balances(*, fee_node_1=0):
|
||||||
# getbalance without any arguments includes unconfirmed transactions, but not untrusted transactions
|
# 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[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
|
# Same with minconf=0
|
||||||
assert_equal(self.nodes[0].getbalance(minconf=0), Decimal('9.99'))
|
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
|
# 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
|
# TODO: fix getbalance tracking of coin spentness depth
|
||||||
assert_equal(self.nodes[0].getbalance(minconf=1), Decimal('0'))
|
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].getbalances()['mine']['untrusted_pending'], Decimal('60'))
|
||||||
assert_equal(self.nodes[0].getwalletinfo()["unconfirmed_balance"], 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].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('0'))
|
assert_equal(self.nodes[1].getbalances()['mine']['untrusted_pending'], Decimal('30') - fee_node_1)
|
||||||
assert_equal(self.nodes[1].getwalletinfo()["unconfirmed_balance"], Decimal('0'))
|
assert_equal(self.nodes[1].getwalletinfo()["unconfirmed_balance"], Decimal('30') - fee_node_1)
|
||||||
|
|
||||||
test_balances(fee_node_1=Decimal('0.01'))
|
test_balances(fee_node_1=Decimal('0.01'))
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue