fix(wallet): categorize self-transfers to external addresses as trusted_pending

[Dmenec]

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 balance function iterates over all transactions in the graph and marks a transaction's owned outputs as trusted if all its inputs spend from owned outpoints.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran just p before pushing

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[codecov]

Codecov Report

❌ Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.16%. Comparing base (fb7681a) to head (3b2af6b).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
src/wallet/mod.rs	97.22%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
+ Coverage   80.04%   80.16%   +0.12%     
==========================================
  Files          24       24              
  Lines        5336     5369      +33     
  Branches      242      247       +5     
==========================================
+ Hits         4271     4304      +33     
+ Misses        987      986       -1     
- Partials       78       79       +1     

Flag	Coverage Δ
rust	80.16% <97.22%> (+0.12%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ValuedMammal]

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.

[qatkk]

I went through the code and ran the tests, everything looks good and all the tests pass aea97d4
I just wanted to mention one small point. The current implementation might process a transaction multiple times (depending on transaction dependencies and the graph structure), since it keeps going through the graph until no new transactions are added to the trusted transaction list.
For normal wallets with small transaction graphs, this shouldn’t be an issue, but it’s something to keep in mind for larger graphs or if performance ever becomes a concern. I also came across a discussion benchmarking transaction graph reconciliation and touching on transaction graph size Issue #1687.
Even so, I think it’s probably not worth adding extra complexity (like Kahn’s algorithm) just for efficiency.

[Dmenec]

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.

Yup, I think just verifying that all inputs of a transaction are owned by the wallet and the output belongs to it should be a better approach.

[edit]

Discussing with @qatkk we found there was still a problem with dependencies (addressed in 3b2af6b)

It now propagates untrust by iterating over all transactions and marks owned outputs as untrusted if any of their inputs are external or inherit untrust from a previous transaction, then excludes them from the trusted set. This should solve the untrusted problem if there's some untrusted transacition in the middle.

[nymius]

Although the code fixes the referenced issue, Isn't the code too much constrained by the TxGraph::balance signature?

The trust_predicate is missing key information that's available in the context of execution. What about bypassing the graph balance method and re implementing the logic directly in wallet?

[nymius]

I don't know if I'm missing an assumption over graph here, but if txin.previous_output is confirmed, wouldn't it mean that that output should be considered trusted?

This case:

#[test]
fn test_pay_to_internal_from_not_trusted_but_confirmed() {
    let (mut wallet, _) = get_funded_wallet_wpkh();

    // Build a tx whose input comes from an unknown (external) outpoint,
    // but whose output goes to our change (internal) keychain address.
    let external_txid = Txid::from_raw_hash(Hash::all_zeros());
    let tx = Transaction {
        input: vec![TxIn {
            previous_output: OutPoint {
                txid: external_txid,
                vout: 0,
            },
            ..Default::default()
        }],
        output: vec![TxOut {
            value: Amount::from_sat(500),
            script_pubkey: wallet
                .next_unused_address(KeychainKind::Internal)
                .address
                .script_pubkey(),
        }],
        version: transaction::Version::ONE,
        lock_time: absolute::LockTime::ZERO,
    };

    insert_tx(&mut wallet, tx);

    let anchor = ConfirmationBlockTime {
        block_id: wallet.latest_checkpoint().get(2000).unwrap().block_id(),
        confirmation_time: 0,
    };
    
    // The transaction is confirmed
    insert_anchor(&mut wallet, external_txid, anchor);

    let balance = wallet.balance();

    // The output is ours but the input is not owned, so it must be untrusted_pending.
    assert!(balance.trusted_pending > Amount::ZERO);
    assert_eq!(balance.untrusted_pending, Amount::ZERO);
}

[Dmenec]

Yup, I think there is another bug in there. Maybe another hashset of confirmed outpoints and checking if the hashet contains it on:

let is_untrusted = tx_node.tx.input.iter().any(|txin| {
    !owned_outpoints.contains(&txin.previous_output)
        || untrusted_outpoints.contains(&txin.previous_output)
});

Would solve that?

[nymius]

Sorry, did you intend to show something in the code? I'm not getting it.
Anyways, I wouldn't be using hash sets lightly here, not without testing against large cases.

[Dmenec]

Yes, I think I lost context just with this snippet. The point was to add another condition there to check if it's confirmed or not, maybe by storing all the confirmed transactions in a separate hash set.

Agreeing with benchmarking it first.

[nymius]

After discussing with @ValuedMammal, we conclude the definition of trust is: all inputs must be controlled by this wallet.
I don't know if it's already in the docs, but it would be worth adding it here.
In that case, my original comment is invalid, and you shouldn't make changes based on it.

[qatkk]

Regarding the definition of trust, are we deliberately not taking into account if a transaction is confirmed or not?

[nymius]

I would change this for loop { ... } and break if conditions are met. changed doesn't hold any meaning by itself outside of this particular context.

[nymius]

For normal wallets with small transaction graphs, this shouldn’t be an issue, but it’s something to keep in mind for larger graphs or if performance ever becomes a concern. I also came across a discussion benchmarking transaction graph reconciliation and touching on transaction graph size Issue #1687.

We are still behind on this, are you willing to add bench marking test for this PR?

[Dmenec]

Although the code fixes the referenced issue, Isn't the code too much constrained by the TxGraph::balance signature?

Yes!

The trust_predicate is missing key information that's available in the context of execution. What about bypassing the graph balance method and re implementing the logic directly in wallet?

Probably it's the best option, maybe tempting to deprecate it in the future on bdk_chain (if I'm not missing something on it's usages). Another option could be modify the signature to receive more context, but that would also be a breaking change, so bypassing it might be the easier path for incremental integration.

[qatkk]

We are still behind on this, are you willing to add bench marking test for this PR?

Sure just to make it a bit more clear, the benchmarking would be only to report the overhead of the balance computation regarding the graph size and its topology, true?

[nymius]

Sure just to make it a bit more clear, the benchmarking would be only to report the overhead of the balance computation regarding the graph size and its topology, true?

Yes, just benchmark the overhead of the balance computation in relation to the graph size. Adding topology without at least a simpler benchmark is an overkill for now.

[qatkk]

Yes, just benchmark the overhead of the balance computation in relation to the graph size. Adding topology without at least a simpler benchmark is an overkill for now.

I'm on it.

Meanwhile I wanted to propose a new approach to address the balance computation. Since the concept of Balance for a wallet only concerns the unspent outputs, I think it would be better to only process the transactions or the ancestors of a transaction from which the wallet owns unspent outputs. This approach effectively removes the need for traversing the transaction graph (which only grows in size with more usage of the wallet) multiple times. Can you please let me know if this approach makes sense to you?