fix(wallet): categorize self-transfers to external addresses as trusted_pending#431
fix(wallet): categorize self-transfers to external addresses as trusted_pending#431Dmenec wants to merge 2 commits intobitcoindevkit:masterfrom
Conversation
…ed_pending Removes previous balance functionality. Now checks every outpoint owned by the wallet and searches for trusted transactions.
…pending Add tests to verify correct balance categorization for trusted/untrusted pending transactions. Covers: - owned inputs to external output - external inputs to own output - transitive trust through a chain of unconfirmed tx
Dmenec
changed the title
Dmenec
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #431 +/- ##
==========================================
+ Coverage 80.04% 80.16% +0.12%
==========================================
Files 24 24
Lines 5336 5365 +29
Branches 242 247 +5
==========================================
+ Hits 4271 4301 +30
+ Misses 987 986 -1
Partials 78 78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ValuedMammal
left a comment
ValuedMammal
left a comment
There was a problem hiding this comment.
Interesting approach. I'm not sure that I follow the transitive trust logic though - if Alice sends money to Bob, she might trust her own transaction but not Bob's transaction spending the output.
| .outpoints() | ||
| .iter() | ||
| .cloned() | ||
| .map(|((k, i), op)| ((k, i, op.txid), op)); |
There was a problem hiding this comment.
💯 This looks like an easier solution (including the txid with the output index) than rewriting the entire balance function on TxGraph.
| self.tx_graph.index.outpoints().iter().cloned(), | ||
| |&(k, _), _| k == KeychainKind::Internal, | ||
| outpoints_with_txid, | ||
| |&(k, _, txid), _| k == KeychainKind::Internal || trusted_txids.contains(&txid), |
There was a problem hiding this comment.
I think we should drop the KeychainKind::Internal check here entirely as there's no guarantee that foreign wallets don't pay to our change address by discovering it on chain, see #16 (comment).
There was a problem hiding this comment.
I agree with dropping the KeychainKind::Internal check. Another example (if I’m not mistaken) could be that, in a multisig wallet, outputs that belong to a shared descriptor can be marked trusted even if the wallet didn’t actually fund them. Dropping the check makes sure this doesn't happen.
|
I went through the code and ran the tests, everything looks good and all the tests pass aea97d4 |
3 participants
Fixes #273.
Description
Unconfirmed transactions spending owned UTXOs to external addresses were being incorrectly categorized as
untrusted_pending. This should not happen since all inputs are controlled by the wallet.Notes to the reviewers
The
balancefunction iterates over all transactions in the graph and marks a transaction as trusted if all its inputs spend from owned outpoints or from already-trusted transactions. This process repeats until no new trusted transactions are found, in order to handle chains of unconfirmed transactions transitively.Checklists
All Submissions:
just pbefore pushingBugfixes: