Check foreign utxos are not local before inclusion

[nymius]

Description

Considering all foreign utxos must not be owned by wallet
The new check is added to ensure this invariant holds.

Before this change, foreign utxos could belong to the wallet, breaking the expected invariant, as exposed by this test at cc15e5d:

fn test_add_foreign_utxo_invalid_psbt_input() {
    let (mut wallet, _) = get_funded_wallet_wpkh();
    let outpoint = wallet.list_unspent().next().expect("must exist").outpoint; // The outpoint is extracted from the wallet itself!
    let foreign_utxo_satisfaction = wallet
        .public_descriptor(KeychainKind::External)
        .max_weight_to_satisfy()
        .unwrap();


    let mut builder = wallet.build_tx();
    let result =
        builder.add_foreign_utxo(outpoint, psbt::Input::default(), foreign_utxo_satisfaction);
    assert!(matches!(result, Err(AddForeignUtxoError::MissingUtxo)));
}

Fixes #1819

Changelog notice

No API changes.

Checklists

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing
• I've added tests for the new feature
• This pull request doesn't break 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

[nymius]

As part of BDK review club for #1798 we talked about #1819 and its fix candidate #1823 :

1. As Check foreign utxos are not local before inclusion #1823 introduces breaking changes, a compromise solution would be to add a silent check which works as a no-op in case transaction is found to be owned by the wallet. Also, one possible approach is to delay the introduction of the new NotForeignUtxo variant error (breaking change) until the release of the new major version, 2.0.0 with a TODO comment.
2. It wasn't clear the ownership of the transactions stored in TxGraph. The terms canonical and relevant were mentioned as a good source to get an idea of the topic from the documentation.
   Wallet::transactions docs have a good explanation of this.
   TxGraph's transactions don't have to belong to the wallet necessarily, and get_txout shouldn't be considered a good source of truth to establish if an Outpoint can be unlocked by Wallet or not. Wallet::is_mine is the correct method for this.

[nymius]

Next steps based on review club:

• Use Wallet::is_mine method to check if UTxO is local or not.
• Comment API breaking changes to avoid delaying the changes until next major version, but keep them around to be ready for it.
• Make failure to add local UTxO as foreign do nothing and return logic.

[nymius]

Applied suggestions in above comment checklist in commit 995df71.

995df71 will be squashed to 81a1cef once reviewed.

TODO:

• Squash 995df71 to 81a1cef

[evanlinjin]

I have no strong opinion about whether we should make this breaking/not-breaking.

Generally, I think it's easier for maintainers if we are allowed to break everything and tell everyone to upgrade. For BDK, I think as long as we make the persistence crates backwards-compatible we are good.

[ValuedMammal]

Style nit, but I'm inclined to write this as a match statement with the primary goal of locating the txout.

let txout = match (&psbt_input.witness_utxo, &psbt_input.non_witness_utxo) { ... };

[ValuedMammal]

I think you still need to provide the witness utxo if you want this to reach the is_mine check.

[ValuedMammal]

I have no strong opinion about whether we should make this breaking/not-breaking.

Generally, I think it's easier for maintainers if we are allowed to break everything and tell everyone to upgrade. For BDK, I think as long as we make the persistence crates backwards-compatible we are good.

I agree it's better to implement the appropriate fix than one that is potentially sub-optimal, so in that case I'd be in favor of adding the NotForeignUtxo error in this PR.