feat(chain): Add verify_tx for TxGraph

[ValuedMammal]

Reintroduce a way to verify a transaction.

This adds a new feature to bdk_chain that uses feature bitcoin/bitcoinconsensus, and exposes the same through bdk wallet as bdk_chain/bitcoinconsensus.

closes bitcoindevkit/bdk_wallet#127

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

[evanlinjin]

I can't decide whether we need this in bdk_chain or even in bdk_wallet. On one hand, the bitcoinconsensus API is not difficult so the caller can just import it directly and use it without much difficulty...

Would love some input on this.

[ValuedMammal]

Right, I'm curious if anyone has specifically asked for this functionality in bdk.

[notmandatory]

How about leaving it as a draft until we are ready to scope out a 1.1 release. Maybe by then we'll have a use case, if not we can close it. There is a scenario described in #352 where this verify functionality could be used.

[notmandatory]

Ok per @LLFourn comments in bitcoindevkit/bdk_wallet#127 I think this should stay in the 1.0.0-alpha milestone.

[LLFourn]

@notmandatory I don't really have an opinion on this being in any milestone. My opinion is that the main issue is fixing RBF design. Being able to verify a transaction is also a nice to have but if no one needs it then it doesn't have to be in v1.0.0.

[f321x]

I currently use verify_tx in an onchain trading pipeline concept to verify bond transactions that are signed but not published (except cheating occurs). While it may is possible to use bitcoinconsensus the verify_tx function was convenient to use and the bdk docs were understandable as a new bdk user.

Also the old verify_tx was not compatible with the async Esplora backend feature of bdk, maybe it makes sense to consider this in the new implementation from the beginning.

[ValuedMammal]

Rebased on 4942cc6

[LLFourn]

Dunno if we should expand the scope of this PR but I'd expect there to be a way to construct a tx_graph such that every tx is verified in so far as the input spks are already available. Probably want to make this available for bdk_wallet too. We'd need some way to insert the transactions in topological order so that ancestors are inserted before children.

I'd guess this is actually what a user wants here to just have blanket protection against a malicious server trying to make it appear as though inputs that have not been spent are in fact spent.

[tnull]

Dunno if we should expand the scope of this PR but I'd expect there to be a way to construct a tx_graph such that every tx is verified in so far as the input spks are already available. Probably want to make this available for bdk_wallet too. We'd need some way to insert the transactions in topological order so that ancestors are inserted before children.

I'd guess this is actually what a user wants here to just have blanket protection against a malicious server trying to make it appear as though inputs that have not been spent are in fact spent.

Want to note that you probably don't want to lean to much on bitcoinconsensus as it unfortunately fails to build on WASM and mobile platforms (rust-bitcoin/rust-bitcoinconsensus#32). I don't think this is a blocker for this PR, but BDK's core functionalities probably can't make use of it for this reason unfortunately.

[ValuedMammal]

We'd need some way to insert the transactions in topological order so that ancestors are inserted before children.

For this we could implement a simplified version of Core's mining logic for package selection (miner.cpp) and leverage the existing features of TxGraph to walk ancestors and descendants. I think it would also be useful for sorting wallet transactions - for example using Wallet::transactions_sort_by and sorting by chain position may not be as effective at sorting same-block transactions.

[notmandatory]

Removing from 1.1 milestone since this is still in draft.

[notmandatory]

Reminder: the bdk_wallet parts of this PR will need to be removed and put in a new PR in the bdk_wallet repo.