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 bitcoindevkit/bdk_wallet#29

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 bitcoindevkit/bdk_wallet#29 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]

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.

[nymius]

I have rebased, added some test to check all execution paths and addressed the comments.

[nymius]

Rebased again. CI errors are related to dependencies and not this PR code.

[nymius]

Rebased onto b26ff89

[nymius]

Rebased onto 362c3dc

[nymius]

Rebased onto 739b54f

[nymius]

Already commented on bitcoindevkit/bdk_wallet#29, but adding it here too for an easier context.

As discussed in today lib dev team call, the behavior explained above is something we should not constrain, and the issue mentioned in the discord chat should be controlled by the user, which makes this PR irrelevant for the issue.